Skip to content

LG-6274: Allow users to select phone as their sole second option#6356

Merged
jmdembe merged 9 commits intomainfrom
LG-6274-second-mfa-option
May 20, 2022
Merged

LG-6274: Allow users to select phone as their sole second option#6356
jmdembe merged 9 commits intomainfrom
LG-6274-second-mfa-option

Conversation

@jmdembe
Copy link
Contributor

@jmdembe jmdembe commented May 16, 2022

This ticket fixes a bug that emerged after #6278 and #6276 were completed

Current state: In setting up an account, the user would select one 2FA method (except for phone due to Kantara requirements). Once setup is complete, if the user selects the option to add a second MFA method, they will not be proceed with phone setup due to the validation added for

Future state: If the user selects phone as their sole additional authentication method, the user will be able to select the phone option and proceed with setup.

After
Screen Recording 2022-05-18 at 1 35 34 PM

@jmdembe jmdembe force-pushed the LG-6274-second-mfa-option branch 3 times, most recently from 66bbe95 to 0222a76 Compare May 18, 2022 17:25
@jmdembe jmdembe marked this pull request as ready for review May 18, 2022 17:33
@jmdembe jmdembe force-pushed the LG-6274-second-mfa-option branch from 67a9e08 to 3d17fc4 Compare May 18, 2022 18:23
Comment on lines 56 to 60
Copy link
Contributor

Choose a reason for hiding this comment

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

For mixed operators like || and &&, can we add parentheses around the parts which are meant to be tested together? I don't know the order of operations off the top of my head, so this is hard for me to interpret.

Also, we could return the boolean result directly, rather than extra return statement.

Suggested change
if count >= 2 || count == 1 && MfaContext.new(user).phone_configurations.none?
return true
else
return false
end
count >= 2 || (count == 1 && MfaContext.new(user).phone_configurations.none?)

Comment on lines 64 to 62
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the parentheses meaningful here?

Suggested change
(IdentityConfig.store.select_multiple_mfa_options &&
phone_selected? && phone_only_mfa_method?) &&
!phone_alternative_enabled?
IdentityConfig.store.select_multiple_mfa_options &&
phone_selected? &&
phone_only_mfa_method? &&
!phone_alternative_enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The parenthesis stayed around as I was refactoring

@jmdembe jmdembe force-pushed the LG-6274-second-mfa-option branch from 39fbe51 to 023ceb8 Compare May 19, 2022 16:39
@jmdembe jmdembe requested a review from a team May 19, 2022 16:41
@jmdembe jmdembe force-pushed the LG-6274-second-mfa-option branch from 75ee7ee to 352edad Compare May 19, 2022 20:07
@jmdembe jmdembe force-pushed the LG-6274-second-mfa-option branch from 352edad to 65c1de0 Compare May 20, 2022 14:13
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Comment on lines 72 to 91
%w[phone].each do |selection|
result = subject.submit(selection: selection)

expect(result.success?).to eq false
end
end
end

context 'when a user wants to select phone as their second authentication method' do
let(:user) { create(:user, :with_authentication_app) }
before do
allow(IdentityConfig.store).to receive(:select_multiple_mfa_options).and_return(true)
end

it 'submits the form' do
%w[phone].each do |selection|
result = subject.submit(selection: ['phone'])

expect(result.success?).to eq true
end
Copy link
Contributor

Choose a reason for hiding this comment

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

The loops here don't seem to be necessary for a single item, and in the second case we're already not using the loop parameter.

Suggested change
%w[phone].each do |selection|
result = subject.submit(selection: selection)
expect(result.success?).to eq false
end
end
end
context 'when a user wants to select phone as their second authentication method' do
let(:user) { create(:user, :with_authentication_app) }
before do
allow(IdentityConfig.store).to receive(:select_multiple_mfa_options).and_return(true)
end
it 'submits the form' do
%w[phone].each do |selection|
result = subject.submit(selection: ['phone'])
expect(result.success?).to eq true
end
result = subject.submit(selection: ['phone'])
expect(result.success?).to eq false
end
end
context 'when a user wants to select phone as their second authentication method' do
let(:user) { create(:user, :with_authentication_app) }
before do
allow(IdentityConfig.store).to receive(:select_multiple_mfa_options).and_return(true)
end
it 'submits the form' do
result = subject.submit(selection: ['phone'])
expect(result.success?).to eq true

@jmdembe jmdembe merged commit 13a2d28 into main May 20, 2022
@jmdembe jmdembe deleted the LG-6274-second-mfa-option branch May 20, 2022 20:27
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