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

Add expiring, databaseless password reset tokens (alternative solution) #823

Closed
wants to merge 4 commits into from
Closed

Add expiring, databaseless password reset tokens (alternative solution) #823

wants to merge 4 commits into from

Conversation

ignatiusreza
Copy link

Hi guys! I saw that #682 got stale and with #817 on the way, I thought that it's a good time to try to revive this effort..

This PR is a rebased of #682, with extra changes on top to address the concern related to the risk of exposing the existing encrypted password..

In summary, the differences between this PR and #682 is, in this PR reset token are generated using ActiveSupport::MessageEncryptor instead of ActiveSupport::MessageVerifier..

As can be seen from the module documentation, ActiveSupport::MessageEncryptor is designed as a simple way to encrypt values which get stored somewhere you don't trust., which perfectly fit our needs..

As part of the changes, I introduces a new class Clearance::Tokenizer to wrap the logic around ActiveSupport::MessageEncryptor and changes the configuration option from configuration.message_verifier to configuration.tokenizer.. I think, in the long term, we could somehow merge Clearance::Token and Clearance::Tokenizer (or delegate token generation logic inside Clearance::Token to Clearance::Tokenizer)..


put :update, params: update_parameters(
user,
new_password: "",
new_password: ""

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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"

Choose a reason for hiding this comment

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

Style/TrailingCommaInHashLiteral: Put a comma after the last item of a multiline hash.

get :edit, params: {
user_id: user,
token: token_for(user)

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [83/80]

tokenizer = double

allow(Clearance.configuration.tokenizer).to receive(:new).and_return(tokenizer)
allow(tokenizer).to receive(:generate).with(1, expires_in: time_limit).and_return("SEKRET")

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [99/80]

time_limit = Clearance.configuration.password_reset_time_limit
tokenizer = double

allow(Clearance.configuration.tokenizer).to receive(:new).and_return(tokenizer)

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [87/80]

module Generators
class UpgradeGenerator < Rails::Generators::Base
include Rails::Generators::Migration
source_root File.expand_path("../templates", __FILE__)

Choose a reason for hiding this comment

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

Style/ExpandPathArguments: Use expand_path('templates', dir) instead of expand_path('../templates', FILE).

@ignatiusreza
Copy link
Author

btw, rubocop is complaining quite a lot even for existing codes.. so i'm a little bit confused as to which style/guideline to follow?

attr_reader :encryptor

def decrypt(token)
begin

Choose a reason for hiding this comment

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

Style/RedundantBegin: Redundant begin block detected.

@@ -1,3 +1,5 @@
require 'clearance/tokenizer'

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

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)

Choose a reason for hiding this comment

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

Naming/MemoizedInstanceVariableName: Memoized variable @user does not match method name find_user_by_password_reset_token. Use @find_user_by_password_reset_token instead.

Derek Prior & Melissa Xie and others added 4 commits April 17, 2019 22:50
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.
replace default tokenizer from ActiveSupport::MessageVerifier
with ActiveSupport::MessageEncryptor, since the later can
potentially leaks user private information

ActiveSupport::MessageEncryptor are setup 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..
@mjankowski mjankowski closed this Apr 23, 2019
@ignatiusreza
Copy link
Author

@mjankowski I'm sorry, do you mind sharing the reason why this PR is closed? I hope i didn't step on anyone's toes.. 🙇‍♂️

@mjankowski
Copy link
Contributor

I have no idea how that happened.

@ignatiusreza
Copy link
Author

Okay, that's odd.. 🤔 is it okay to get this reopened then? Or is there no longer any interest for this feature?

@mjankowski
Copy link
Contributor

I figured it out ... while I did not actively close this PR, I did delete the branch that this PR had been against, which had the side effect of closing this PR! Sorry about that.

I'm not actually able to re-open this ... but if you want to re-open against master we can review it again.

@ignatiusreza
Copy link
Author

Sure, no problem.. i'll rebase against master and will open a new PR.. thanks for investigating!

@ignatiusreza
Copy link
Author

PR reopened at #834

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.

4 participants