search

jeremyevans / rodauth

Ruby's Most Advanced Authentication Framework
http://rodauth.jeremyevans.net
MIT License
1.65k stars 95 forks source link

Skip default session clearing in JWT mode #404

Closed janko closed 3 months ago

janko commented 3 months ago

Originally, #clear_session in JWT feature was calling super to clear the session hash from the instance variable. However, in https://github.com/jeremyevans/rodauth/pull/140 an explicit session.clear was added, because super didn't handle sessions plugin being loaded.

This means it's not necessary to call super anymore. This behavior caused an error when rodauth-omniauth is used with rodauth-rails, where super being called when clearing JWT session called Rails implementation. The root cause was ultimately in rodauth-omniauth, but I thought it would be useful to have this change as a safeguard for any plugin extending #clear_session.

jeremyevans commented 3 months ago

Thanks for the patch!

Not calling super seems undesirable at first glance. Overriding clear_session in the Rodauth config would be fine as that would be called before the jwt feature implementation, and no other feature seems to override the default clear_session in the base feature (except the internal_request feature, which always overrides without calling super). Therefore, I don't think this should cause problems with core Rodauth, even if it seems initially undesirable.

Do you think that external Rodauth features that implement clear_session would expect their implementation to be ignored if jwt feature was used? If so, at the very least, we should document this behavior in the jwt feature documentation.

janko commented 3 months ago

Thanks for sharing your thoughts about it. I was thinking that web framework integrations with Rodauth might count with their clear_session override not being called for JWT requests. However, at least in Rails, when there is no session middleware loaded, it falls back to Rack's default session (which is an empty hash), and rodauth-omniauth had a bug which prevented this fallback (causing the session to be nil).

I don't feel strongly that JWT feature should skip calling the original implementation, so maybe this can be revisited only if there are problems. I believe I fixed the bug in rodauth-omniauth, so I think we should be good there.