mysociety / alaveteli

Provide a Freedom of Information request system for your jurisdiction
https://alaveteli.org
Other
389 stars 195 forks source link

Entering an invalid 2FA OTP on password reset causes problems #5710

Open mdeuk opened 4 years ago

mdeuk commented 4 years ago

When a user has 2FA enabled their OTP is required in order to carry out a password reset (as per #2697); however, we don't handle this cleanly if the user has forgotten their 2FA OTP.

At the password reset screen, we ask for the password/confirmation and the OTP, however, if the user enters incorrect OTP we don't give them the opportunity to re-enter.

In this scenario, we display an error and ask for the password combination again - however, the user cannot proceed from this screen as the OTP field is missing. This creates a circuitous infinite loop, as the only way to resolve the issue is to either go back to the original reset screen, or request another password reset link.

image

Steps to reproduce

  1. Request a password reset for any 2FA enabled account
  2. Click the password reset link in the system generated email
  3. Note that you are prompted for a password, confirmation, and the 2FA OTP. Enter an incorrect OTP code. Click "Change your password on $sitename"
  4. Note that you receive an error "Invalid one time password", and that the "Change your password on $sitename" screen is re-generated without the "Two factor one time passcode" field (password_change_user_otp_code)

This is always reproducible.

Expected result

If the user enters an invalid OTP code the resultant screen should either offer the opportunity to re-enter the code; or provide relevant advice on what the next step could be (e.g. re-setting again, or invoking any recovery mechanism [if defined]).

garethrees commented 4 years ago

Looks like we didn't set @otp_enabled for the update action, so in the error condition where we re-render the form we don't render the OTP field https://github.com/mysociety/alaveteli/blob/develop/app/controllers/password_changes_controller.rb#L114

The dumb fix is:

       else
+        @otp_enabled = otp_enabled?(@password_change_user)
         render :edit

But we should set it a bit further up for the whole action https://github.com/mysociety/alaveteli/blob/develop/app/controllers/password_changes_controller.rb#L82