LG-10918 Cancel option visible on OTP screen by kevinsmaster5 · Pull Request #9165 · 18F/identity-idp

kevinsmaster5 · 2023-09-07T20:10:09Z

🎫 Ticket

🛠 Summary of changes

Modify the bottom of the Phone/Text confirmation screen so it uses the common cancel_or_back_to_options partial that is used by the other auth method confirmation views.

📜 Testing Plan

On http://localhost:3000/ create a new account and pick Face or touch unlock as the first MFA
Add another method then do Text or voice
On Cancel link can click back to add second MFA screen

aduth

I would not expect this to be specific to "Face or Touch Unlock", but rather any phone OTP confirmation stemming from the MFA selection screen. Maybe we can use MfaSetupConcern#in_multi_mfa_selection_flow? for this?

aduth

Can we add some test coverage for the expected behavior under these conditions?

aduth · 2023-09-15T12:28:27Z

app/views/two_factor_authentication/otp_verification/show.html.erb

I don't think the screenshot in the pull request is accurate (anymore), since the partial being removed also rendered the footer with the border, which is no longer present as expected.

aduth · 2023-09-15T12:33:26Z

app/views/two_factor_authentication/otp_verification/show.html.erb

I hadn't necessarily expected that the "Cancel" text needed to change as part of the ticket, more the link destination. In that regard, I don't know that we need to actually update the view here, and instead look at fixing the logic of PhoneDeliveryPresenter#cancel_link to handle the scenario where someone in the multi-MFA selection flow should be returned to the MFA selection screen.

identity-idp/app/presenters/two_factor_auth_code/phone_delivery_presenter.rb

Lines 59 to 66 in a7ed678

def cancel_link

locale = LinkLocaleResolver.locale

if confirmation_for_add_phone || reauthn

account_path(locale: locale)

else

sign_out_path(locale: locale)

end

end

I see. I took the title too literally. I'll work it back from the presenter instead.

…instead (#9165) (#8522)

jmdembe · 2023-09-22T13:28:37Z

spec/features/multi_factor_authentication/mfa_cta_spec.rb

We need to create an account at this point before we do mock_webauthn_setup_challenge

jmdembe · 2023-09-22T18:33:17Z

spec/features/multi_factor_authentication/mfa_cta_spec.rb

sign_up_and_set_password does not need to be assigned to a user because it also creates a password

Suggested change

user = sign_up_and_set_password

user.password = Features::SessionHelper::VALID_PASSWORD

sign_up_and_set_password

jmdembe

Everything here LGTM once the comments are addressed. I would say that we should put the feature spec that was created into a new folder and file in spec/features so that it's clearer to what we are testing.

jmdembe · 2023-09-22T18:55:04Z

spec/views/two_factor_authentication/otp_verification/show.html.erb_spec.rb

We don't need this line anymore

Thank you. That's removed.

kevinsmaster5 · 2023-09-22T20:14:53Z

Everything here LGTM once the comments are addressed. I would say that we should put the feature spec that was created into a new folder and file in spec/features so that it's clearer to what we are testing.

So there's a /spec/features/phone folder - should I start a new xyz_spec.rb file there specific to the new test?

jmdembe · 2023-09-22T20:42:40Z

Everything here LGTM once the comments are addressed. I would say that we should put the feature spec that was created into a new folder and file in spec/features so that it's clearer to what we are testing.

So there's a /spec/features/phone folder - should I start a new xyz_spec.rb file there specific to the new test?

yeah, that's how I would approach it

aduth · 2023-09-25T14:38:52Z

app/presenters/two_factor_auth_code/phone_delivery_presenter.rb

I'm not expecting data to be here, should it be?

Suggested change

:in_multi_mfa_selection_flow,

:data

:in_multi_mfa_selection_flow

You are correct. That's removed.

… cancel

aduth

One question, but works well in my testing 👍

aduth · 2023-09-25T19:10:46Z

spec/features/phone/mfa_setup_add_phone_spec.rb

I might wonder if we should put this in spec/features/two_factor_authentication/multiple_mfa_sign_up_spec.rb as part of the multi-MFA-during-setup test suite, rather than a new standalone file. Thoughts?

I agree, especially if it's not likely that in the near future more tests would need to be added to that spec.

aduth reviewed Sep 12, 2023

View reviewed changes

kevinsmaster5 marked this pull request as ready for review September 13, 2023 12:37

aduth mentioned this pull request Sep 15, 2023

LG-10769: Diffferentiate between account creation and account and account management of adding MFA #9133

Merged

aduth reviewed Sep 15, 2023

View reviewed changes

kevinsmaster5 marked this pull request as draft September 15, 2023 19:01

kevinsmaster5 force-pushed the kmas-lg-10918-cancel-option-visible branch from 507f2b4 to 2df5cc1 Compare September 19, 2023 13:39

kevinsmaster5 marked this pull request as ready for review September 20, 2023 14:06

solipet pushed a commit that referenced this pull request Sep 21, 2023

changelog: Internal, In-Person Proofing, Use Rails-like i18n wrapper …

0a8ab97

…instead (#9165) (#8522)

jmdembe reviewed Sep 22, 2023

View reviewed changes

jmdembe approved these changes Sep 22, 2023

View reviewed changes

jmdembe reviewed Sep 22, 2023

View reviewed changes

kevinsmaster5 force-pushed the kmas-lg-10918-cancel-option-visible branch from 695d41f to 7f2a343 Compare September 25, 2023 12:53

aduth reviewed Sep 25, 2023

View reviewed changes

kevinsmaster5 added 14 commits September 25, 2023 12:48

changelog: Bug Fixes, MFA, removes Cancel option from Phone/Text screen

397b4b5

add a face or touch unlock check for cancel or return type footer link

13fff41

simplify Choose another option

e8c7471

fix openid bug regression

503b554

make otp test more ubiquitous for other mfa not just ft unlock

aba8a93

adds in_multi_mfa_selection_flow to context

53b6849

add test coverage for not-yet signed up

89ac9de

refactor to put Cancel logic in presenter

98e2881

temporarily remove sign in test

6a8f652

adds test case for setup and phone access code validation redirect on…

1bee97a

… cancel

fix missing click

495bfd6

moves account setup before mock_webauthn...

d35265d

lint fix

1625a95

remove unneeded statements

a8f3d14

kevinsmaster5 added 4 commits September 25, 2023 12:48

move add phone at setup spec to its own file

db9ead0

lint fix

ab2107d

added includes fix pinpoint-check

e3f3312

removes unneeded :data and includes from spec

e9dc25e

kevinsmaster5 force-pushed the kmas-lg-10918-cancel-option-visible branch from e3d62e8 to e9dc25e Compare September 25, 2023 16:48

aduth approved these changes Sep 25, 2023

View reviewed changes

move adding phone test to multiple_mfa_sign_up

bf14af5

kevinsmaster5 merged commit cfbf165 into main Sep 26, 2023

kevinsmaster5 deleted the kmas-lg-10918-cancel-option-visible branch September 26, 2023 11:49

solipet mentioned this pull request Sep 28, 2023

Deploy idp RC 318 to Prod #9279

Merged

	def cancel_link
	locale = LinkLocaleResolver.locale
	if confirmation_for_add_phone \|\| reauthn
	account_path(locale: locale)
	else
	sign_out_path(locale: locale)
	end
	end

	user = sign_up_and_set_password
	user.password = Features::SessionHelper::VALID_PASSWORD
	sign_up_and_set_password

	:in_multi_mfa_selection_flow,
	:data
	:in_multi_mfa_selection_flow

Conversation

kevinsmaster5 commented Sep 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmdembe Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmdembe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinsmaster5 commented Sep 22, 2023

Uh oh!

jmdembe commented Sep 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kevinsmaster5 commented Sep 7, 2023 •

edited

Loading

aduth Sep 15, 2023 •

edited

Loading

jmdembe Sep 22, 2023 •

edited

Loading

jmdembe left a comment •

edited

Loading