FIX: Ensure disabling 2FA works as expected (PR #10485)

This will fix the bug reported here on Meta

The second factor management page has previously included a “Disable” button. Selecting this button brings up a confirmation:

old

Instead of disabling all registered TOTP Authenticators and Security Keys, confirming only disables all TOTP Authenticators.


This PR will change the “Disable” button on the second factor management page to say “Disable All,” and make a few styling/copy improvements to the confirmation:

Screen Shot 2020-08-13 at 6 45 51 PM

Screen Shot 2020-08-19 at 3 31 25 PM

It also fixes the behavior so that all second factor methods are removed when using the “Disable All” button.

Notes

  • The test was tricky and takes heavy inspirations from other tests. I imagine there may be a more elegant or complete implementation, but I’m pretty sure I have the most important functionality tested. If I need to make any changes, let me know.

  • Both second factor methods can be disabled individually instead of using the “Disable All” button using the pencil icon buttons showing in the above screenshot. What I came to realize is that the implementation for individual disabling is quite different than the disable all method. The “Disable All” button deletes the relevant user_second_factors and user_security_keys records from the database, whereas disabling the second factors individually sets enabled: false in the relevant record and keeps it around. This is not new behavior and nothing is technically broken, I just wanted to bring it up in case we might want to have consistency. If anything needs to change, I imagine it can be addressed in a follow-up commit.

GitHub

          expect(user.reload.user_second_factors).to be_empty
          expect(user.security_keys).to be_empty
          expect(user.reload.user_second_factors.totps.first).to eq(totp_second_factor)
          expect(user.security_keys.first).to eq(security_key_second_factor)
          expect(user.user_second_factors).to be_empty
          expect(user.security_keys).to be_empty

Let’s use the helpers in https://github.com/discourse/discourse/blob/0967ce478d85ad5053522944b36ed9212f867457/spec/support/sidekiq_helpers.rb#L19

          label: `${iconHTML("ban")}${I18n.t("user.second_factor.disable")}`

All tests start from a clean slate so we don’t need to check for this.

The use of the fabricator ensures that the object is successfully created so we don’t need these assertions too.

@tshenry Just to confirm, do we need to disable backup codes too?

do we need to disable backup codes too?

Good question, @tgxworld! I can’t think of any reason why backup codes should hang around after someone uses the “Delete All” option, so I’ll ensure they are destroyed as well.

I really appreciate all of the feedback and explanations surrounding the tests. I’m still getting the hang of writing them, so everything you’ve mentioned is extremely helpful :heart:

@tgxworld I checked on the backup code situation. Using the “Disable All” button triggers:

current_user.user_second_factors.destroy_all

Because of this, any current_user.user_second_factors.backup_codes are inherently destroyed.

One note: when we remove every TOTP method and security key individually, the backup codes do remain in database, but the codes are not available to regenerate/disable through the UI until at least one second factor is added. I think this is probably fine, but figured it’s worth mentioning the behavior.