Skip to content

Log if password matches current for password reset event#11423

Merged
aduth merged 10 commits intomainfrom
aduth-password-changed
Oct 31, 2024
Merged

Log if password matches current for password reset event#11423
aduth merged 10 commits intomainfrom
aduth-password-changed

Conversation

@aduth
Copy link
Copy Markdown
Contributor

@aduth aduth commented Oct 29, 2024

🛠 Summary of changes

Adds an event property indicating whether the password was the same as the user's current password when successfully resetting a user password.

This will be used to understand how common this behavior is, in case there's improvements we could make to this workflow, particularly for users who might be impacted by identity verification profile deactivation due to password reset.

Related Slack discussion: https://gsa-tts.slack.com/archives/C01710KMYUB/p1730231046429359

📜 Testing Plan

  1. Go to http://localhost:3000
  2. Create an account. Remember your password
  3. Once you reach account dashboard, go to http://localhost:3000/verify
  4. Complete identity verification
  5. Sign out
  6. Click "Forgot your password?
  7. Submit the email address associated with your account
  8. Click "Reset your password" in the email you received
  9. Run make watch_events in a separate terminal process
  10. When prompted to choose a new password, enter the same or a different password from what you know to be the user's current password
  11. Observe that password_matches_existing is false if you entered a different password, or true if you entered the same password

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@aduth aduth changed the title Log if password was different for password reset event Log if password matches current for password reset event Oct 29, 2024
end

def update_user
@password_matches_existing = user.valid_password?(password)
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.

This is a relatively expensive comparison, I'm not sure if we'd want to do this on every password reset going forward. It may even be desirable to feature flag it?

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.

Looks like we do roughly 100k password resets a day. If we are concerned we could capture this for a subset (5%? 10%?) and still get a meaningful result?

Copy link
Copy Markdown
Contributor Author

@aduth aduth Oct 30, 2024

Choose a reason for hiding this comment

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

Yeah I could set this up as an A/B test to limit the performance impact, and to allow us to quickly turn it off.

However, that being said, if we ever did want to do any sort of follow-up work here (e.g. avoid deactivating profiles for unchanged password), I'm not sure there's a way around doing the check.

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.

Although, in that hypothetical future, I suppose the cost of checking whether the password has changed would be offset slightly by avoiding hashing the new password when it's unchanged.

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@aduth aduth merged commit a419d8c into main Oct 31, 2024
@aduth aduth deleted the aduth-password-changed branch October 31, 2024 11:40
aduth added a commit that referenced this pull request Nov 20, 2024
aduth added a commit that referenced this pull request Nov 21, 2024
…ent (#11457)

* Revert "Log if password matches current for password reset event (#11423)"

This reverts commit a419d8c.

* Remove inaccurate route verb

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

* Refactor form specs for updated expectations

* Remove spec expectations for password_matches_existing

---------

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
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