Do not 500 when resetting password for account with an unconfirmed email address that has since been confirmed by another account#6042
Conversation
mitchellhenke
force-pushed
the
mitchellhenke/reset-password-500
branch
from
8f7927b to
e3bae65
Compare
mitchellhenke
changed the title
mitchellhenke
force-pushed
the
mitchellhenke/reset-password-500
branch
2 times, most recently
from
0e2ee75 to
5609e02
Compare
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM! nice find!
This reminds me of another duplicate error we'd seen in our logs: https://cm-jira.usa.gov/browse/LG-5171, I wonder if a similar case of multiple accounts is what's causing that bug
app/forms/reset_password_form.rb
Outdated
There was a problem hiding this comment.
should we add a unit test for this method/failure?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
mitchellhenke
force-pushed
the
mitchellhenke/reset-password-500
branch
from
0a9a28b to
123a5d2
Compare
changelog: Bug Fixes, Reset Password, Fix 500 error when resetting password for account with email confirmed by another account
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
mitchellhenke
force-pushed
the
mitchellhenke/reset-password-500
branch
from
123a5d2 to
bbd9bb7
Compare
| # 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? |
There was a problem hiding this comment.
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.
2 participants
Kind of circuitous, but very possible way to get a 500 (NewRelic)
The index collision happens because we only allow one record with a given email address to be confirmed. The reset password token is generated for the first account which hasn't yet confirmed the email address. In the meantime, a second account is created that adds and confirms the same email address. When the first account opens the password reset email, and enters a password, we attempt to update the email address as being confirmed (and correctly fail).