Ensure authentication method is stored when setting up backup codes or PIV/CAC

[mitchellhenke]

🛠 Summary of changes

The auth_method value in the session is used for doing re-authentication checks and for checking whether an account's authentication method meets a service provider's authentication level (AAL2, AAL2-phishing-resistant, AAL2-HSPD12). The setting is currently inconsistent, and is not set when registering backup codes or PIV/CAC.

OTP-based 2FA methods (TOTP, phone) call handle_valid_otp during setup since they have to be confirmed which sets auth_method in the session during account registration. Webauthn does it directly in the setup controller since it does not use the separate verification controller when registering new webauthn authenticators.

Since we do not set auth_method when registering backup codes and PIV/CAC, this leads two a couple of issues:

1. If you are arriving from an SP that requires HSPD12 (PIV/CAC), and need to create a new account, you will have to verify your PIV/CAC twice. It is required the first time to register the PIV/CAC in your account, and this should be sufficient. Since we do not set auth_method, the auth_method check fails and you are required to go through PIV/CAC authentication again.
2. If you sign up for an account with only backup codes and/or PIV/CAC, you will not be considered "recently authenticated" since you do not have an auth_method. This means if you attempt to take account management actions like adding 2FA, emails, etc., you will be asked to re-authenticate (potentially with one of the backup codes you just received). This issue does not affect production currently.

To address this, this PR sets the auth_method after successfully setting up backup codes and PIV/CAC.

[aduth]

LGTM

OTP-based 2FA methods (TOTP, phone) call handle_valid_otp during setup since they have to be confirmed which sets auth_method in the session during account registration. Webauthn does it directly in the setup controller since it does not use the separate verification controller when registering new webauthn authenticators.

I wonder if we could consolidate this better, or at least have some more consistency.

Couple ideas:

• Move webauthn assignment to the controller's mark_user_as_fully_authenticated for consistency
  • Aside: To double-check, does this need to be broken down webauthn / webauthn_platform? 🤔
• Move mark_user_as_fully_authenticated to TwoFactorAuthenticatableMethods so that it can be used similar to (and used by!) handle_valid_otp_for_authentication_context

[aduth]

Why is this conditional? (Same note for PIV)

[mitchellhenke]

Ah oops, forgot this was in there. I was experimenting with whether we always want to set it

e.g. if I already have authenticated with my PIV or something, do we want to "downgrade" my auth_method to backup codes. If I then attempt to authenticate to a phishing-resistant or PIV-required SP, I'll be asked to verify again. This is already how it works now for other methods, so I don't think it's adding any new problems, but it's something we could sorta address for backup codes since they'll typically be configured last on account setup?

[aduth]

Hm, maybe we need a way to track multiple authentication methods?

One concern I might have with this is that it can refresh authn_at for a different auth_method. Is that a problem?

[mitchellhenke]

That's a good point. I don't think it would necessarily cause any problems today, but it's better to be consistent while we have these updates scattered around for now.

authn_at is only used for re-authentication checks currently (which is entirely internal functionality at the moment).
We may want to keep track of it more comprehensively as an array or something with whether it was confirmation/setup or an authentication, timestamp, etc. It could help us protect against edge cases where a weaker method overwrites a stronger one that would meet phishing-resistant or HSPD12 and prevent a user from having to authenticate again. I'm hazarding a guess that it's not particularly frequent, but worth addressing in another PR either way I think.

I've removed the if user_session[:auth_method].blank? check to keep the behavior consistent across controllers.

[mitchellhenke]

Couple ideas:

• Move webauthn assignment to the controller's mark_user_as_fully_authenticated for consistency

Agreed and applied that in the most recent commits

• Aside: To double-check, does this need to be broken down webauthn / webauthn_platform? 🤔

I had tried that as well, I wasn't sure why we wouldn't, so I brought that change in with da1fafaf1

• Move mark_user_as_fully_authenticated to TwoFactorAuthenticatableMethods so that it can be used similar to (and used by!) handle_valid_otp_for_authentication_context

I agree. I started to try to unify them, but it got a little complex and I'm not sure it fits well into this PR. It kind of intersects with both handle_valid_otp_for_authentication_context and mark_user_session_authenticated.

[aduth]

LGTM