Skip to content

Complete path for users to setup an additional method#6276

Merged
jmdembe merged 27 commits intomainfrom
jd-finish-sad-path
May 10, 2022
Merged

Complete path for users to setup an additional method#6276
jmdembe merged 27 commits intomainfrom
jd-finish-sad-path

Conversation

@jmdembe
Copy link
Contributor

@jmdembe jmdembe commented Apr 29, 2022

This PR closes the loop with regards to completing multi factor authentication setup.

Previously, once the user returned to the to the MFA setup page and selected an additional option, they would be re-directed to the account page leaving them unable to set up their account. We are now allowing for additional method setup that will lead to a confirmation that setup is complete.

new MFA setup screen
Notes:

@jmdembe jmdembe force-pushed the jd-finish-sad-path branch from 15c96a0 to da42ea6 Compare April 29, 2022 16:14
changelog: Upcoming feature, multi-factor-authentication, complete sad path
@jmdembe jmdembe force-pushed the jd-finish-sad-path branch from da42ea6 to 4a5a48f Compare April 29, 2022 16:21
@jmdembe jmdembe force-pushed the jd-finish-sad-path branch 2 times, most recently from cd9656c to 01ab424 Compare April 29, 2022 17:59
@jmdembe jmdembe force-pushed the jd-finish-sad-path branch from 01ab424 to 9393561 Compare April 29, 2022 18:22
@jmdembe jmdembe requested a review from a team May 5, 2022 17:10
class MfaSelectionController < ApplicationController
include UserAuthenticator
include MfaSetupConcern

Copy link
Contributor

Choose a reason for hiding this comment

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

can we ensure user is authenticated.
We also want to ensure that users arrive at this page properly.

before_action :authenticate_user before_action :confirm_user_authenticated_for_2fa_setup

Copy link
Contributor

Choose a reason for hiding this comment

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

can we ensure user is authenticated. We also want to ensure that users arrive at this page properly.

before_action :authenticate_user before_action :confirm_user_authenticated_for_2fa_setup

Was this suggestion implemented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it had been originally, but I believe what happened is that with a merge that I had to make with another PR that it got lost in a merge somewhere. I created a patch in LG-6236

@mdiarra3 mdiarra3 requested a review from zachmargolis May 5, 2022 19:01
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM, a few comments

end
let(:current_sp) { create(:service_provider) }

describe 'GET index' do
Copy link
Contributor

Choose a reason for hiding this comment

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

style, I'd name it like this:

Suggested change
describe 'GET index' do
describe '#index' do

jmdembe and others added 2 commits May 6, 2022 14:11
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@jmdembe jmdembe force-pushed the jd-finish-sad-path branch from 7398b50 to 25fb7bb Compare May 6, 2022 18:37
@jmdembe jmdembe force-pushed the jd-finish-sad-path branch from 236981d to 0f9cee5 Compare May 10, 2022 14:29
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.

4 participants