Skip to content

Conversation

@tillprochaska
Copy link
Contributor

@tillprochaska tillprochaska commented Nov 19, 2021

Right now, users are automatically signed in after resetting their password. However, for some use cases, it might make sense that they have to sign in explicitly.

For example, in an app we’re building, we have implemented 2FA on top of Clearance. After a password reset, we want users to sign in themselves using the second factor. Otherwise, password resets could be used to circumvent 2FA. (While we could ask for the second factor before resetting the password, we decided to allow password resets without the second factor, but then force users to sign in.)

This PR adds a new configuration option to Clearance, sign_in_on_password_reset. It defaults to true, i.e. the default behavior does not change.

Closes #949.


expect(response).to redirect_to(Clearance.configuration.redirect_url)
expect(cookies["remember_token"]).to be_present
it "redirects, but does not sign in the user" do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn’t sure wether this should go into the controller spec for PasswordsController or into this request spec, but decided to add it to the request spec as this one tests the current behavior.

config.sign_in_guards = []
config.user_model = "User"
config.parent_controller = "ApplicationController"
config.sign_in_on_password_reset = false
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be sufficient as the configuration option is quiet self-explanatory and there is additional information in the respective lib/clerance/configuration.rb.

@secure_cookie = false
@signed_cookie = false
@sign_in_guards = []
@sign_in_on_password_reset = true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any preference regarding the order of instance variables and method definitions in this class? I simply added the new configuration option at the end. They seem to be grouped somewhat semantically.

@tillprochaska tillprochaska marked this pull request as ready for review November 19, 2021 15:56
@dorianmariecom
Copy link
Contributor

I've made some changes and credited you in #969

I think it's better for now to stick to controller specs. Migrating to requests specs would be nice though but the tests are already organized around controller specs.

@tillprochaska
Copy link
Contributor Author

@dorianmariefr That's great, thanks a lot!

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.

Disable auto-login after password reset

2 participants