Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion app/forms/reset_password_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def valid_token
# If the user is not saved in the database, that means looking them up by
# their token failed
errors.add(:reset_password_token, 'invalid_token', type: :invalid_token)
elsif !user.reset_password_period_valid?
elsif !user.reset_password_period_valid? || invalid_account?
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also worth noting that this should resolve the vast vast majority of cases, but the chance still exists that the two accounts could end up in a race condition where they both try to confirm the email address at the same time in the window of time between the SELECT here and the UPDATE that follows.

errors.add(:reset_password_token, 'token_expired', type: :token_expired)
end
end
Expand All @@ -56,6 +56,21 @@ def mark_profile_inactive
user.proofing_component&.destroy
end

# It is possible for an account that is resetting their password to be "invalid".
# If an unconfirmed account (which must have one unconfirmed email address) resets their
# password and a different account then adds and confirms that same email address,
# the initial account is no longer able to confirm their email address and is effectively invalid.
#
# They may still have a valid forgot password link for the initial account, which would normally
# mark their email as confirmed when they set a new password, but we do not want to allow it
# because we only allow an email address to be confirmed on one account.
def invalid_account?
!user.confirmed? &&
EmailAddress.confirmed.exists?(
email_fingerprint: user.email_addresses.map(&:email_fingerprint),
)
end
Comment on lines 67 to 72
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we add a unit test for this method/failure?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm comfortable with the test in https://github.com/18F/identity-idp/pull/6042/files#diff-6169030eddc977e1ae39fe9c01f51f38bf4c8dfbec994c3539a8c10d5340b8caR154-R169 covering the expected behavior. Is there an aspect or edge case missing?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think for odd model joins like this, that doing a unit tests is clearer than an acceptance test, but I don't feel that strongly, the overall behavior is covered by the one you linked

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If it helps, there isn't a join and could probably be simplified to just be user.email_addresses.first since an unconfirmed account should only ever have one email address


def extra_analytics_attributes
{
user_id: user.uuid,
Expand Down
19 changes: 19 additions & 0 deletions spec/forms/reset_password_form_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,25 @@
end
end

context 'when the unconfirmed email address has been confirmed by another account' do
it 'does not raise an error and is not successful' do
user = create(:user, :unconfirmed)
user.update(reset_password_sent_at: Time.zone.now)
user2 = create(:user)
create(
:email_address, email: user.email_addresses.first.email, user_id: user2.id,
confirmed_at: Time.zone.now
)

form = ResetPasswordForm.new(user)

result = form.submit(password: 'a good and powerful password')

expect(result.success?).to eq(false)
expect(result.errors).to eq({ reset_password_token: ['token_expired'] })
end
end

it_behaves_like 'strong password', 'ResetPasswordForm'
end
end