Closed sihu closed 1 year ago
Hi @sihu,
Thank you for your comprehensive report on this issue. Would it be possible for you to share an example repository where the issue can be reproduced? I'm currently quite pressed for time, and having access to such an example would definitely expedite the process of fixing it. If the issue is confirmed, I agree that your first choice is the best solution, and I'd be more than happy to accept a pull request that addresses it.
Hi @waiting-for-dev
I did some further investigation and found the real origin of all evil - turns out it was me messing up things 🙈 Nevertheless I think my learnings could be valuable for others to find similar problems since it was hard to debug:
devise
only calls the set_user
callback once and stores the user in the Proxy and is also able to fetch it again from there without calling the hook again (see warden/lib/warden/proxy.rb#L217)before_actions
that made usage of current_user
and i also had protect_from_forgery
-callsprotect_from_forgery with: :null_session
for api calls. Here is what happened:
protect_from_forgery
failed via verify_authenticity_token
which i found out by seeing a console message Can't verify CSRF token authenticity
. The behaviour null_session
cleared my session via handle_unverified_request
(see devise/controllers/helpers.rb#L259, warden/lib/warden/proxy.rb#L266)before_action
call. thus it had to call set_user
again on warden/lib/warden/proxy.rb#L229 (which caused the second call to the hook), since it will just re-authenticate with the params if it does not find a session => this is a bit unexpected and quite hard to debug. If i wouldn't have used the Allowlist-Strategy of devise-jwt
, this would not matter and i would never have found out, but like this it created a duplicate entry.handle_unverified_request
or the before-hooks simply behaved differently), but i did not further follow this problem.To sum up: I am OK if we close this issue since it's not a bug. Maybe it would still valuable to avoid the second call to a hook in case of a re-authentification in the same request - maybe it would also hide this behaviour even more. Turns out it's just very hard to debug if you mess up with before-hooks. An option could be to add a warning to the logs if you call the hook again. I could still prepare a repo to showcase this scenario but i think it's quite specific to my use case.
Thx so much for your work!
Many thanks for your investigation and your follow-up, @sihu. It's always a pleasure to get comprehensive reports when an issue is open.
it will just re-authenticate with the params if it does not find a session => this is a bit unexpected and quite hard to debug
That's because of how warden works. It'll try to fetch the user from the session and, if not present, cascade through the configured strategies until one or none succeeds. See https://github.com/wardencommunity/warden/wiki/Strategies for details.
I think it doesn't make sense to add anything from our end, as the library is doing what it should do: add a token at every authentication request. Anyway, I'm sure other people in the future will find your investigation here relevant, and it will save them a good amount of time.
The behaviour of calling the hook every time wardens'
after_set_user
is called lead to problems using this library together with https://github.com/waiting-for-dev/devise-jwt since with theDevise::JWT::RevocationStrategies::Allowlist
always two entries where created.Version: 0.8.1
To Reproduce
Devise::JWT::RevocationStrategies::Allowlist
Explanation
In order to support
current_user
in before-hooks warden authenticates the user even before creating the session. (e.g. https://github.com/heartcombo/devise/issues/4951)The control flow causes that two JWTs created (and thus stored to the database):
current_user
callswarden.authenticate
(see devise/lib/devise/controllers/helpers.rb#L127)warden.authenticate
triggersafter_set_user
(see warden/lib/warden/proxy.rb#L110)devise-jwt
registers onafter_set_user
and triggersuser.on_jwt_dispatch
(see warden-jwt_auth/lib/warden/jwt_auth/hooks.rb#L62user.on_jwt_dispatch
creates an AllowlistedJWT entry (see devise-jwt/lib/devise/jwt/revocation_strategies/allowlist.rb#L42)Current behavior
Two JWTs are created since
on_jwt_dispatch
is called twice. Once because ofcurrent_user
callswarden.authenticate
and once becauseSessionsController#create
callsauthenticate!
(see devise/app/controllers/devise/sessions_controller.rb#L19)Expected behavior
Create only one JWT.
Possible solutions
I came up with a Monkey patch:
This would avoid that
prepare_token
is called if it's already created within that request.Another solution would be to modify the
on_jwt_dispatch?
in order to be able to decide on an implementation level, what you want to do on a second dispatch of the jwt:This would not be my favourite solution but allow the user to handle both cases.
Maybe i have overseen something and there is an easier fix for my issue. I would be in favour of my first suggested solution. This could lead to side effects and maybe there is a reason someone wants to listen to all
after_set_user
-events? Happy to contribute after discussing here what might be the best solution to this problem?