Closed nmbrone closed 4 years ago
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?
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 formax_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?https://github.com/ueberauth/guardian/blob/62744fb61fd62afe8bbb49b75b377be14aae65e6/lib/guardian/plug.ex#L318-L321
The existing behavior brakes the following use case:
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.