LG-13872: MFA Setup Constraint removal for restricting SP on second MFA addition. #11029
LG-13872: MFA Setup Constraint removal for restricting SP on second MFA addition. #11029
Conversation
…fa lista all options regardless of SP
app/controllers/users/two_factor_authentication_setup_controller.rb
Outdated
Show resolved
Hide resolved
spec/features/two_factor_authentication/multiple_mfa_sign_up_spec.rb
Outdated
Show resolved
Hide resolved
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
Suggestion for code organization, but tests well and otherwise LGTM 👍
| def phishing_resistant? | ||
| service_provider_mfa_policy.phishing_resistant_required? && | ||
| !mfa_context.phishing_resistant_configurations.present? | ||
| end | ||
|
|
||
| def piv_cac_required? | ||
| service_provider_mfa_policy.piv_cac_required? && | ||
| !mfa_context.piv_cac_configurations.present? | ||
| end |
There was a problem hiding this comment.
I kinda feel like it might be better for this logic to live inside the presenter class, since that helps keep the controller leaner, and we can more easily add tests for it there. We also already have mfa_policy inside TwoFactorOptionsPresenter, which we can use phishing_resistant_mfa_enabled? from.
MfaPolicy doesn't have an equivalent to checking if PIV/CAC configuration exists, but maybe we could add a method, or create mfa_context inside TwoFactorOptionsPresenter.
There was a problem hiding this comment.
hmm yea I can update to address,
kevinsmaster5
left a comment
kevinsmaster5
left a comment
There was a problem hiding this comment.
LG & checks out with local testing 👍
3 participants
🎫 Ticket
Link to the relevant ticket:
LG-13872
🛠 Summary of changes
This fixes it so that users that have a piv or phishing resistance requirement by SP are not hampered from adding a secondary MFA method after selecting those methods.
📜 Testing Plan
Provide a checklist of steps to confirm the changes.