Add expiring, databaseless password reset tokens (with encrypted tokens) #834
Conversation
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).
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.
|
|
||
| put :update, params: update_parameters( | ||
| user, | ||
| new_password: "", | ||
| new_password: "" |
There was a problem hiding this comment.
Style/TrailingCommaInArguments: Put a comma after the last parameter of a multiline method call.
| user_id: user.id, | ||
| token: user.reload.confirmation_token, | ||
| user_id: 1, | ||
| token: token |
There was a problem hiding this comment.
Style/TrailingCommaInHashLiteral: Put a comma after the last item of a multiline hash.
| Timecop.freeze(1.day.from_now) do | ||
| get :edit, params: { | ||
| user_id: 1, | ||
| token: token |
There was a problem hiding this comment.
Style/TrailingCommaInHashLiteral: Put a comma after the last item of a multiline hash.
| get :edit, params: { | ||
| user_id: 1, | ||
| token: user.confirmation_token + "a", | ||
| token: "a" |
There was a problem hiding this comment.
Style/TrailingCommaInHashLiteral: Put a comma after the last item of a multiline hash.
| get :edit, params: { | ||
| user_id: user, | ||
| token: token_for(user) |
There was a problem hiding this comment.
Style/TrailingCommaInHashLiteral: Put a comma after the last item of a multiline hash.
| token = Clearance::PasswordResetToken.generate_for(user) | ||
|
|
||
| Timecop.freeze(1.day.from_now) do | ||
| expect(Clearance::PasswordResetToken.find_user(user.id, token)).to be nil |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [81/80]
| user = create(:user) | ||
| token = Clearance::PasswordResetToken.generate_for(user) | ||
|
|
||
| expect(Clearance::PasswordResetToken.find_user(user.id + 1, token)).to be nil |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [83/80]
| module Generators | ||
| class UpgradeGenerator < Rails::Generators::Base | ||
| include Rails::Generators::Migration | ||
| source_root File.expand_path("../templates", __FILE__) |
There was a problem hiding this comment.
Style/ExpandPathArguments: Use expand_path('templates', dir) instead of expand_path('../templates', FILE).
| attr_reader :encryptor | ||
|
|
||
| def decrypt(token) | ||
| begin |
There was a problem hiding this comment.
Style/RedundantBegin: Redundant begin block detected.
| def find_user_for_update | ||
| find_user_by_id_and_confirmation_token | ||
| def find_user_by_password_reset_token(user_id, token) | ||
| @user ||= Clearance::PasswordResetToken.find_user(user_id, token) |
There was a problem hiding this comment.
Naming/MemoizedInstanceVariableName: Memoized variable @user does not match method name find_user_by_password_reset_token. Use @find_user_by_password_reset_token instead.
replace default tokenizer from ActiveSupport::MessageVerifier with ActiveSupport::MessageEncryptor, since the former can potentially leaks user private information ActiveSupport::MessageEncryptor are configured to work using key generated from user encrypted_password and the application secret_key_base.. this ensure that the token automatically get invalidated when user changed their password..
Reopened against master from #823