LG-10487 Inconsistent enforcement of phishing-required second MFA setup

[kevinsmaster5]

🎫 Ticket

LG-10487

🛠 Summary of changes

• Added another method specific to phishing resistant required accounts and included it into the selection validation method
• Add spec to cover changes

📜 Testing Plan

Locally test with Sinatra running.

• At http://localhost:9292/ set AAL setting to Phishing-resistant AAL2, click Sign-in
• Create an account and select any of the offered methods.
• When prompted to add another method click the button to add another
• At the next setup screen instead of selecting a method click Continue

You should expect to see the Continue to Example Sinatra App screen and the Agree and continue button.
You should not expect to see the Add MFA form again with the error "Select an additional authentication method."

[aduth]

We already have a memoized instance of MfaPolicy.new(user) available through the mfa_user method, so I don't think we should add a new one.

[aduth]

I'm kinda thinking rather than adding to the logic here, a fix might be as simple as removing line 76 below (the check for in_phishing_resistant_or_piv_cac_required_flow? in has_no_configured_mfa?). We're already limiting the options to phishing-resistant methods on the first MFA setup, and I feel like we should be able to expect that as long as they've already got one configured MFA and they're in the phishing-resistant flow, we can assume that they have a phishing-resistant MFA method.

I'd like to see some feature spec coverage verifying that:

• A user who comes through phishing-resistant request has to set up a phishing-resistant method as their first MFA method
  • A spec for this probably already exists, but we should confirm
• A user who comes through phishing-resistant request and opts to add another method can Continue or Cancel without setting up a second method

[kevinsmaster5]

My first run through on this I got an error at /spec/features/openid_connect/phishing_resistant_required_spec.rb. On further troubleshooting seemed to indicate a scenario of a user setting up their account on a non-phishing-resistant-required workflow but then for whatever reason they tried logging into a phishing-resistant-required workflow. I don't know how likely that is to happen TBH. I could replicate that failure and it consisted of reloading the Additional authentication required page but no error message.
I'm interested in finding a more optimal way of resolving that issue and refactor what I implemented.

[aduth]

Interesting, I wondered about cases like that. So in that scenario I expect that the user would see the filtered set of phishing-resistant MFA options, but the test is checking to make sure they'd see the expected error message upon trying to bypass / skip by clicking "Continue".

Maybe what we could do as a compromise is adapt the "no configured MFA" logic to something which is more broadly checking that the user has the minimum required?

Thinking something like this, perhaps:

diff --git a/app/forms/two_factor_options_form.rb b/app/forms/two_factor_options_form.rb
index 5d85c4cb0..8f0333fa1 100644
--- a/app/forms/two_factor_options_form.rb
+++ b/app/forms/two_factor_options_form.rb
@@ -27,7 +27,7 @@ class TwoFactorOptionsForm
   private
 
   def validate_selection_present
-    return if !has_no_mfa_or_in_required_flow? || selection.present? || phishing_resistant_and_mfa?
+    return if selection.present? || has_minimum_required_mfa_methods?
     errors.add(:selection, missing_selection_error_message, type: :missing_selection)
   end
 
@@ -43,10 +43,6 @@ class TwoFactorOptionsForm
     }
   end
 
-  def in_phishing_resistant_or_piv_cac_required_flow?
-    phishing_resistant_required || piv_cac_required
-  end
-
   def user_needs_updating?
     (%w[voice sms] & selection).present? &&
       !selection.include?(user.otp_delivery_preference)
@@ -62,8 +58,16 @@ class TwoFactorOptionsForm
     selection.include?('phone') || selection.include?('voice') || selection.include?('sms')
   end
 
-  def has_no_configured_mfa?
-    mfa_user.enabled_mfa_methods_count == 0
+  def has_minimum_required_mfa_methods?
+    if piv_cac_required
+      mfa_user.piv_cac_configurations.count > 0
+    elsif mfa_user.webauthn_platform_configurations.any?
+      !platform_auth_only_option?
+    elsif phishing_resistant_required
+      mfa_user.phishing_resistant_configurations.count > 0
+    else
+      mfa_user.enabled_mfa_methods_count > 0
+    end
   end
 
   def platform_auth_only_option?
@@ -71,22 +75,11 @@ class TwoFactorOptionsForm
       mfa_user.webauthn_platform_configurations.count == 1
   end
 
-  def has_no_mfa_or_in_required_flow?
-    has_no_configured_mfa? ||
-      in_phishing_resistant_or_piv_cac_required_flow? ||
-      platform_auth_only_option?
-  end
-
-  def phishing_resistant_and_mfa?
-    MfaPolicy.new(user).unphishable? && in_phishing_resistant_or_piv_cac_required_flow?
-  end
-
   def missing_selection_error_message
-    if has_no_configured_mfa? || in_phishing_resistant_or_piv_cac_required_flow? ||
-       phishing_resistant_and_mfa?
-      t('errors.two_factor_auth_setup.must_select_option')
-    elsif platform_auth_only_option?
+    if platform_auth_only_option?
       t('errors.two_factor_auth_setup.must_select_additional_option')
+    else
+      t('errors.two_factor_auth_setup.must_select_option')
     end
   end
 end

[kevinsmaster5]

That works great. Thanks!

[aduth]

LGTM 👍

[aduth]

I kinda snuck the rearranging of the selection.present? check into the diff in #9119 (comment), but the nice side-effect is that it should prevent a potential handful of database queries for the majority case where a user makes a selection before submitting the form, thanks to short-circuiting logic. 🎉

[aduth]

Why was this change necessary?

[kevinsmaster5]

I think I was stuck on something and removing with_authentication_app relieved it. It doesn't appear to be causing issues now with it added back.

[aduth]

I suspect it was copied from above context block, but I don't see that this variable has any relevance or impact on the specs in this block.

Suggested change

let(:enabled_mfa_methods_count) { 1 }

[kevinsmaster5]

I see that. Thanks!