Kmas lg 9961 more mfa same type

[kevinsmaster5]

🎫 Ticket

LG-9961

🛠 Summary of changes

Enables a user to select more than one of the same MFA type at account setup (except for Backup Codes and PIV). The confirmation view is removed and after the first option is set the user is redirected back to the secondary option view. When additional options are added they also return to the secondary option view until they click continue on to the account view.

Some style changes were added to give clearer feedback to the user about what options have been selected.

Figma

📜 Testing Plan

Steps to confirm the changes.

• Create a new account, confirm email, set password
• On the Two Factor Authentication view pick a MFA option and click Continue
• You should return to the Two Factor Authentication Setup view but now it will display your first option above the checkbox fields as in the updated design. The selected checkbox from step 2 should still be selectable again unless it is Backup Codes or PIV/CAC
• Clicking continue again after selecting another MFA option and completing its setup should return you to the Two Factor Authentication Setup view.

👀 Screenshots

[aduth]

Looking really good overall! Left a couple comments, but this is working well 👍

[aduth]

Before merging, can we also rebase or merge main, so we don't run any risk of issues with the branch being out-of-date? It looks like the merge base is pretty old (a5aa1bc).

[aduth]

Looks great! And thanks for the patience with all the reviews 😅

[aduth]

Maybe for a follow-up, but it's rather unexpected to me that we need to be nil-tolerant on the users all of a sudden. It feels to me like maybe it's an issue with how we initialize the specs, but the setup presenters should ideally enforce that a user always exists.

[kevinsmaster5]

I'm troubleshooting an error that seemed to indicate GitLab wasn't happy about something to do with webauthn_configurations
https://gitlab.login.gov/lg/identity-idp/-/jobs/659011

--- Caused by: ---

 # NoMethodError:
 #   undefined method `webauthn_configurations' for nil:NilClass
 #   ./app/presenters/two_factor_authentication/webauthn_platform_selection_presenter.rb:25:in `mfa_configuration_count'

I'll take a look from the spec angle and see if I can find the problem.

[aduth]

For posterity: @kevinsmaster5 and I paired on the last few commits, related to some build failures with user sometimes being nil. We discovered that since these presenter classes are used in both set-up as well as sign-in, the user would not always be present -- only during set-up.

We addressed this by:

• Adding an additional condition to disabled? to only apply the default logic for set-up, to rely on short-circuit evaluation to prevent mfa_configuration_count being called during sign-in (981b926)
• Making user a required argument of the presenter classes to avoid potential confusion around user being present or not, since it's reasonably available during sign-in as well as set-up (b76e7c7, f0c2147)

Separately, we want to propose a refactor which would split up the presenter classes between set-up and sign-in, since:

1. There's almost no shared logic between the two flows, almost everything in the base class already branches based on the configuration.present? (i.e. between sign-in and set-up)
2. A lot of the methods are only relevant for set-up (including those added and revised here), but it's not entirely clear this is the case

[aduth]

I would not expect to see attr_reader in specs.

[kevinsmaster5]

refactored that at cf44f91

[aduth]

Couple minor notes on spec changes, but otherwise still LGTM 👍

[aduth]

I'd suggest adding test coverage for the new logic branch we added in 981b926

Suggested change

	end
	end
	context 'with configuration' do
	let(:single_configuration_only) { true }
	let(:mfa_configuration_count) { 1 }
	let(:configuration) { create(:phone_configuration, user: user) }
	before do
	allow(presenter).to receive(:configuration).and_return(configuration)
	end
	it { expect(presenter.disabled?).to eq(false) }
	end

[aduth]

We could probably move this up to the top scope so that we only have to define it once (after line 3).

I'd also suggest making it create an actual value, since this is effectively setting user to nil, which we're not expecting to happen in any real-world usage.

let(:user) { build(:user) }

Same note would apply to other changes where we define let(:user) {} this way.