Add expiring, databaseless password reset tokens #682
Conversation
- Remove support for Ruby 1.9, 2.0, and 2.1 - Remove support for Rails 3.1, 3.2, 4.0, and 4.1
These messages are used to tell users they can't access a page without signing in, that their password is incorrect, etc. These are much closer to error or alert states than notice.
`PasswordsController#url_after_create` was never called by our code.
These `respond_to` checks were added to support changing Rails API as new versions of Rails were released. Now that we support 4.2 or newer, we don't need them.
* Remove deprecated parameter handling * Order actions more logically * Update to double quotes
| expect(email.html_part.body).to include(link) | ||
| expect(email.html_part.body).to have_css( | ||
| "a", | ||
| text: I18n.t("clearance_mailer.change_password.link_text") |
There was a problem hiding this comment.
Put a comma after the last parameter of a multiline method call.
derekprior
force-pushed
the
dp-password-resets
branch
3 times, most recently
from
9a6185f
to
45b3607
Compare
| user_model = Clearance.configuration.user_model | ||
|
|
||
| begin | ||
| user_id, encrypted_password, expiration = verifier.verify(token) |
There was a problem hiding this comment.
Maybe encrypted_password shouldn't be part of what we sign here? It allows us to invalidate any tokens as soon as the password is changed but it also means the encrypted password is part of the payload (signed). I dont know that the tradeoff is worth it. Curious for additional thoughts here...
There was a problem hiding this comment.
What about storing a hash of the encrypted password? The token would still change with the password without leaking anything.
I can't think of reason you wouldn't provide a user with their encrypted password, but it makes me nervous anyway.
There was a problem hiding this comment.
I think it makes me nervous because I'd be providing anyone that can access that email access to their encrypted password. I'll play with it some to see what I can come up with.
Hashing the encrypted password is a decent idea. Complicates the lookup a tiny bit, but it may be worth it.
|
This is a great idea. |
| @@ -56,6 +56,8 @@ Clearance.configure do |config| | |||
| config.routes = true | |||
| config.httponly = false | |||
| config.mailer_sender = "[email protected]" | |||
| config.message_verifier = ActiveSupport::MessageVerifier.new(secret_key_base) | |||
| config.password_reset_time_limit = 15.minutes | |||
|
I had to implement expiring reset tokens in Clearance on a project last week. Very excited to see this become something upstream (and to remove dependency on the database). |
|
This also fixes #612. Thank you! |
It is a best security practice for password reset token to expire after some amount of time. In Clearance 1.x, this was not the case. A password reset email could be used months after it was originally sent so long as no other password reset was ever completed. In this change, password resets expire after 15 minutes (configurable) or after the user successfully changes their password in any manner (whichever comes first). The token, confusingly called `confirmation_token`, is no longer stored in the database. Instead, we use `ActiveSupport::MessageVerifier` to sign the token and validate it when it is used. The message verifier is configurable in case developers want to use something else. In a future refactoring, I'd like to introduce a layer between Clearance and `ActiveSupport::MessageVerifier` to make the API a bit more pleasant to use, but this is an exercise for a future PR. For instance, I'd prefer that the Clearance abstraction generate and validate tokens only by taking a user object (and using the Clearance configuration).
derekprior
force-pushed
the
dp-password-resets
branch
from
45b3607
to
13b31ac
Compare
This cleans up some of the duplication of knowledge for how password reset tokens are generated and allows us to move tests for the various ways a reset token can be invalid into unit tests.
We were previously using the encrypted password as part of the signed password reset token. Theoretically, emailing this token out could expose the encrypted password to some adversary who would then be able to do offline attacks against it. This would likely not be very successful, but in an abundance of caution, this change exposes an MD5'd version of the encrypted password instead.
| token = Clearance.configuration.message_verifier.generate([ | ||
| user.id, | ||
| Digest::MD5.hexdigest(user.encrypted_password), | ||
| Clearance.configuration.password_reset_time_limit.from_now |
There was a problem hiding this comment.
Put a comma after the last item of a multiline array.
|
Is there still any effort to get this into Clearance? |
It is a best security practice for password reset token to expire after
some amount of time. In Clearance 1.x, this was not the case. A password
reset email could be used months after it was originally sent so long as
no other password reset was ever completed.
In this change, password resets expire after 15 minutes (configurable)
or after the user successfully changes their password in any manner
(whichever comes first).
The token, confusingly called
confirmation_token, is no longer storedin the database. Instead, we use
ActiveSupport::MessageVerifiertosign the token and validate it when it is used. The message verifier is
configurable in case developers want to use something else.
In a future refactoring, I'd like to introduce a layer between Clearance
and
ActiveSupport::MessageVerifierto make the API a bit more pleasantto use, but this is an exercise for a future PR. For instance, I'd
prefer that the Clearance abstraction generate and validate tokens only
by taking a user object (and using the Clearance configuration).
Closes #465