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

Fix regression due to warden session scope usage #85

Merged
merged 7 commits into from
Jun 5, 2024

Conversation

strouptl
Copy link
Collaborator

@strouptl strouptl commented Jun 5, 2024

@strzibny, I discovered that there was an issue with my usage of warden session for the refresh credentials functionality: you have to pass the scope explicitly to session (e.g. warden.session(:user)), or else it resorts to the default scope. This causes problems when there is more than one model which utilizes devise-otp (e.g. User and Admin).

I have resolved this issue, and created a CHANGELOG with the details (excerpted below). All tests are passing.

Details:

  • Correct warden session usage for refresh_credentials hook and helper methods (requires scope to be specified)
  • Add Admin model and AdminPosts controller to dummy app for testing;
  • Add tests to confirm resolution;

@strouptl strouptl force-pushed the warden_session_fix branch from 88cb49f to b521515 Compare June 5, 2024 09:19
@strzibny
Copy link
Collaborator

strzibny commented Jun 5, 2024

Great you realized this as I only use one scope

@strzibny
Copy link
Collaborator

strzibny commented Jun 5, 2024

You don't need to update changelog for unreleased things. I'll rewrite it to be a single version announcement.

@strzibny strzibny merged commit 181d788 into wmlele:master Jun 5, 2024
1 check passed
@strouptl
Copy link
Collaborator Author

strouptl commented Jun 5, 2024

Cool. I'll rebase the other branches.

@strouptl strouptl deleted the warden_session_fix branch June 5, 2024 09:44
@strouptl
Copy link
Collaborator Author

strouptl commented Jun 5, 2024

Done.

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