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?
Lines 318 to 321 in 62744fb
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?