Create "You've created your [x] authentication method!" page by jmdembe · Pull Request #6111 · 18F/identity-idp

jmdembe · 2022-03-24T18:15:21Z

This PR creates the confirmation page that will inform the user that they have successfully set up one of their selected MFA methods.

Notes:

Currently, there is no path to go from the "Choose your method page" to "You've created your first authentication method". This is expected because routing is the next piece of the functionality that we are adding for the MFA feature.
Another next step is to add "You've added your second method", "You've added your third method", etc. The logic is not here because the routing has not been set up. A presenter will be added to facilitate that logic once routing and the data structure is in place
"You've added your first authentication method", "Add phone", and "Face ID has been added to your account" is dummy data for the purpose of this ticket
We are implementing small commits for this feature for ease of testing and to make sure that we have not broken the functionality of two-factor authentication setup.
EDIT: The button and the "Skip for now" links do not lead the user to the correct page at this time as we still need to set up the router.

aduth · 2022-03-24T20:36:24Z

config/routes.rb

For consistency with other routes, can we replace - with _ ?

Suggested change

get '/auth-method-confirmation' => 'mfa_confirmation#index'

get '/auth_method_confirmation' => 'mfa_confirmation#index'

aduth · 2022-03-24T20:37:21Z

app/views/mfa_confirmation/index.html.erb

Not sure if it's just because draft, but noting that we should probably want a unique title here from "Confirm the password for your account" ?

zachmargolis

Left some ideas to increase code coverage to get around the error

zachmargolis · 2022-03-30T18:01:21Z

app/controllers/application_controller.rb

this method is marked as not covered by code coverage, and I'm not seeing it used anywhere in the diff, can we remove it?

zachmargolis · 2022-03-30T18:03:44Z

config/routes.rb

Looks like this method was added but here and so was the template, but I'm not seeing it in the diff. Can we add a stub for it like this?

identity-idp/app/controllers/users/backup_code_setup_controller.rb

Line 13 in 5968911

def index; end

so add that stub to the mfa_confirmation_controller.rb file? If so, that makes sense.

This would also be where we could add a check to not render in production if we want to prevent it from being available there

zachmargolis · 2022-03-30T18:04:11Z

app/views/mfa_confirmation/index.html.erb

this template was added but I'm not seeing any tests that call it. WDYT of adding a very simple view spec for it?

zachmargolis · 2022-03-30T19:31:55Z

app/controllers/mfa_confirmation_controller.rb

+    if IdentityConfig.store.select_multiple_mfa_options
+      auth_method_confirmation_url
+    else
+      render :file => "views/mfa_confirmation/new.html.erb", :layout => true, :status => 302


How come this is a 302 status (usually a redirect?) but it's rendering instead of redirecting?

Also can we use new-style hashes render file: "abc/def", layout: true if possible?

yeah, I thought redirecting would cause it not render, but I was wrong. Instead, we are going to put a flag on that route.

aduth · 2022-04-01T12:29:45Z

app/views/users/two_factor_authentication_setup/index.html.erb


 <%= render 'shared/cancel', link: destroy_user_session_path %>

+<%= link_to t('account_reset.cancel_request.cancel'), root_url %>


Are we intending to add a second link to the live MFA setup screen?

aduth

👍

aduth · 2022-04-05T15:20:11Z

config/routes.rb

A few thoughts on this route and associated controller:

MfaConfirmationController seems to be primarily concerned with the "reauthn" (password reentry) behavior, not with MFA setup, where MFA setup is split across TwoFactorAuthenticationSetupController and ad-hoc controllers. I might wonder if this should be a separate controller, or part of TwoFactorAuthenticationSetupController?

While we're not entirely consistent about it, I usually prefer when the route aligns to the controller name, since it makes it easier for me to backtrack from a URL to its associated controller.

To me, #index implies some sort of listing, or at least an entry-point of a controller, where this is more like a confirmation screen.

Depending on how this feature evolves, I suppose this view would probably be the rendered result of, for example, TwoFactorAuthenticationSetupController#create ? With that in mind, it could be fine if it's expected to be temporary, though we may be able to save some future effort by implementing it closer to where we expect it to be eventually used.

I think we are going in the direction of using the TwoFactorAuthenticationSetupController to set up the MFA prototype. In fact, work for setup in that controller is being done in a separate ticket

I think the plan is to have the route and controller to reflect its new functionality. This might be in the form of changing the name of TwoFactorAuthenticationSetupController and any routes associated with it.

By the look of it, it can be changed to a #show. I can address that part now. There is no logic associated with this page right now, so that will be easy.

I anticipate that we will have additional changes to routes and controllers. There are a few things to think through to make it sound, but I think that is a little further down the road.

That all sounds good to me! 👍

changelog: Improvements, Feature, Add interstitial confirmation page

aduth reviewed Mar 24, 2022

View reviewed changes

jmdembe force-pushed the jd-auth-method-confirmation-page branch 4 times, most recently from 590592c to 2502e5b Compare March 30, 2022 17:34

jmdembe marked this pull request as ready for review March 30, 2022 17:41

zachmargolis approved these changes Mar 30, 2022

View reviewed changes

jmdembe force-pushed the jd-auth-method-confirmation-page branch from 2502e5b to 8a649ef Compare March 30, 2022 18:11

zachmargolis reviewed Mar 30, 2022

View reviewed changes

jmdembe force-pushed the jd-auth-method-confirmation-page branch from 6deeeeb to f678d19 Compare March 30, 2022 21:20

aduth reviewed Apr 1, 2022

View reviewed changes

jmdembe force-pushed the jd-auth-method-confirmation-page branch 2 times, most recently from ef1c809 to 0d12668 Compare April 5, 2022 14:01

aduth approved these changes Apr 5, 2022

View reviewed changes

For MFA: Add page to confirm that the user has added their first method

1fdcc60

changelog: Improvements, Feature, Add interstitial confirmation page

jmdembe force-pushed the jd-auth-method-confirmation-page branch from 0d12668 to 1fdcc60 Compare April 5, 2022 16:39

jmdembe merged commit 5e10029 into main Apr 5, 2022

jmdembe deleted the jd-auth-method-confirmation-page branch April 5, 2022 18:23

zachmargolis mentioned this pull request Apr 12, 2022

Deploy RC 186 to Production #6189

Merged

	get '/auth-method-confirmation' => 'mfa_confirmation#index'
	get '/auth_method_confirmation' => 'mfa_confirmation#index'


		<%= render 'shared/cancel', link: destroy_user_session_path %>

		<%= link_to t('account_reset.cancel_request.cancel'), root_url %>

Conversation

jmdembe commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis 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!

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!

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!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmdembe Apr 5, 2022 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmdembe commented Mar 24, 2022 •

edited

Loading

jmdembe Apr 5, 2022 •

edited

Loading