Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip default session clearing in JWT mode #404

Closed
wants to merge 1 commit into from

Conversation

janko
Copy link
Contributor

@janko janko commented Apr 4, 2024

Originally, #clear_session in JWT feature was calling super to clear the session hash from the instance variable. However, in #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
Copy link
Owner

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
Copy link
Contributor Author

janko commented Apr 8, 2024

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.

@janko janko closed this Apr 8, 2024
@janko janko deleted the simplify-jwt-clear-session branch April 8, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants