Skip to content

LG-7483: Remove feature flag select_multiple_mfa_options#7505

Merged
aduth merged 2 commits intomainfrom
aduth-lg-7483-select-multiple-mfa
Dec 20, 2022
Merged

LG-7483: Remove feature flag select_multiple_mfa_options#7505
aduth merged 2 commits intomainfrom
aduth-lg-7483-select-multiple-mfa

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Dec 16, 2022

🎫 Ticket

LG-7483

🛠 Summary of changes

Removes the select_multiple_mfa_options feature flag, which is now live in all environments.

Also fixes an issue where MFA "Delete" links would not be shown on the account dashboard in some circumstances, since that was intended to apply only for Kantara restrictions (not live in production), but instead had been keying off the select_multiple_mfa_options feature (live in production).

📜 Testing Plan

  1. Check that you can configure multiple MFA for account creation
  2. Check that you can delete MFA methods from your account when you have multiple active, regardless of whether the MFA method is "restricted"

The second test procedure requires overrides in local development config/application.yml (until LG-7586):

development:
  kantara_2fa_phone_restricted: false

@aduth aduth requested a review from a team December 16, 2022 16:48
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is unreachable, since PhoneSelectionPresenter overrides the info method and therefore setup_info would not be called for those methods. The code here is incorrect anyways, since it's showing the Kantara messaging based on select_multiple_mfa_options (should only be shown with kantara_2fa_phone_restricted).

def info
IdentityConfig.store.kantara_2fa_phone_restricted &&
MfaContext.new(user).enabled_mfa_methods_count == 0 ?
t('two_factor_authentication.two_factor_choice_options.phone_info_html') :
t('two_factor_authentication.two_factor_choice_options.phone_info')
end

Copy link
Contributor

@jmdembe jmdembe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense. LGTM 👍🏾

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused at needing to remove some of these SAML-specific handling, though as I understand why they were introduced in #6894 to trigger the post-auth redirect that's typically handled in JavaScript, I feel like it would be unexpected that a SAML auth would be "complete" at this point. In main, the URL here is the same as what's being asserted below (UriService.add_params(@saml_authn_request, locale: :es)), which feels very unexpected. My guess is that based on the timing of the merge of #6894 being after multiple MFA setup was already live in most environments, this was buggy in situations where the config was turned off (as it had been in test environments), and that the changes here are the correct, expected behavior.

changelog: Bug Fixes, Account Management, Fix missing links to delete MFA methods when phone method is present
@aduth aduth force-pushed the aduth-lg-7483-select-multiple-mfa branch from 964f56e to a1ba73b Compare December 20, 2022 18:05
@aduth aduth merged commit d05fbcc into main Dec 20, 2022
@aduth aduth deleted the aduth-lg-7483-select-multiple-mfa branch December 20, 2022 18:49
@aduth aduth mentioned this pull request Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants