ueberauth/guardian

remember_me ignores token's TTL

nmbrone opened this issue · 2 comments

Here we're trying to get max_age for a cookie from the token itself but because of the reason described in #570 (list concatenation) we ignoring the value and always using the value from config or defaults. Do we really need to have a default value for max_age (or allow it to be configured)? Why not simply rely on token's TTL itself? Or there is a reason for keeping a cookie alive longer than token's TTL?

defp cookie_options(mod, %{"exp" => timestamp}) do
max_age = timestamp - Guardian.timestamp()
[max_age: max_age] ++ cookie_options(mod, %{})
end

The existing behavior brakes the following use case:

MyGuardian.Plug.remember_me(conn, resourse, claims, ttl: {90, :days})

Then one will expect a client get a refresh token for ~3 months but instead, the cookie will be expired after 28 days (which is current default max_age).

Of course, you can work around this and place into config cookie_options: [max_age: 60 * 60 * 24 * 90] but it feels wrong.

Good catch, I see a few problems with this code. First of all, I don't think we need default cookie_options anymore, since we can never pass claims to cookie_options that doesn't have an exp
So we could simplify the code like this

defp cookie_options(mod, %{"exp" => timestamp}) do 
   max_age = timestamp - Guardian.timestamp() 
   Keyword.merge([max_age: max_age], mod.config(:cookie_options, []))
end

This will also means that going forward all cookies will expire at the same time as the token they hold, which sounds reasonable to me

Side note I prefer to use a token type instead of a custom ttl, that way you make sure you dont have multiple different ttl set around your code.

@ueberauth/developers Do you see any problems with the above?