"token_expired" error handling uniformisation between VerifyCookie and VerifySession
quaresc opened this issue · 8 comments
Hello !
I’ve encountered an incoherence between VerifyCookie and VerifySession for a token_expired error.
VerifyCookie ignores it whereas VerifySession does not and I think it complexifies some pipelines.
This incoherence has been raised here #517
VerifyCookie:
VerifySession:
An exemple where this difference is problematic:
I implemented the remember_me function in order to create a refresh token alongside a session cookie with my access token in it.
If my access token is expired, I would like to check if a refresh exists/valid to exchange it for an access token.
To handle such case, I have to ignore the halt in Guardian.Plug.VerifySession which will allow Guardian.Plug.VerifyCookie to be called but also handle this error directly in my error_handler (could be handled in ensure_authenticated plug in some cases).
plug(Guardian.Plug.VerifySession, claims: @claims, halt: false)
plug(Guardian.Plug.VerifyCookie)def auth_error(conn, {:invalid_token, :token_expired}, _opts) do
conn
end
Wouldn’t be best to uniformise both plugs or at least provide an option ?
Maybe my pipeline isn't coherent, feel free to tell me :) !
So the VerifyCookie should look if your access_token has expired and if so use the refresh_token in the cookie to get a new access_token. If you swap your two plugs I think everything should work as excepted. Is there something I am missing?
plug(Guardian.Plug.VerifyCookie)
plug(Guardian.Plug.VerifySession, claims: @claims)
Maybe we should make this more obvious in the docs
If I swap my two plugs, I believe that it will first check my refresh_token instead of my access_token
Yea, I guess it will always do the exchange. I am wondering how other people have set up their pipelines.
I guess it would make sense if VerifyCookie checked if the current session token was valid and then just used it?
Exactly and I couldn't really find other pipelines with a similar use case with refresh and access tokens (expect in the linked issue)
I guess it would work but it would do exactly the same job of VerifySession which might be a little confusing + if you don't use session tokens, wouldn't it be an unnecessary check ?
Yea, so I think the problem comes down to that you could have a pipeline looking like this
plug(Guardian.Plug.VerifyHeader, claims: @claims, halt: false)
plug(Guardian.Plug.VerifySession, claims: @claims, halt: false)
plug(Guardian.Plug.VerifyCookie)
Where you want to get token from the header if is there, else
Get it from the session, else
Get it from the cookie
So I guess we could let the VerifyHeader and VerifySession parse through if the token has expired.
But then we will not be able to handle this case in the error handler, but maybe this is okay since it will get handled in ensure_autenticated
As previously mentioned this would be a breaking change. As some could rely on the VerifySession throwing the Token expired error. What do you think @ueberauth/developers
VerifyCookie being not really useful in this pipeline on its own, one alternative could be to implement an option to VerifyHeader and VerifySession like verify_cookie: true which will check and exchange the cookie directly (and avoid a breaking change)
Well the verify cookie as of now actually just uses the refresh_token to generate the access_token on every request if you don't have the VerifySession in front.
I think it would actually make sense to have an option to both verify header and session to refresh from a cookie, instead of it being a plug itself. I have always been confused about the verify cookie anyways since it sounds like it will look up an access token there.
My current suggestion would be to deprecate VerifyCookie and add options to verify header and session to reauth from a refresh token in a cookie.