Skip to content

Reorder phone confirmation and review page#1530

Merged
jmhooper merged 1 commit intomasterfrom
jmhooper-verify-phone-password-flow
Aug 1, 2017
Merged

Reorder phone confirmation and review page#1530
jmhooper merged 1 commit intomasterfrom
jmhooper-verify-phone-password-flow

Conversation

@jmhooper
Copy link
Contributor

@jmhooper jmhooper commented Jul 6, 2017

Why: It is confusing for users to verify their phone number after
entering confirming their profile by entering their password. This
commit changes the order so the user verifies their phone immediately
after entering it

@jmhooper jmhooper changed the title Reorder phone confirmation and review page [WIP] Reorder phone confirmation and review page Jul 6, 2017
@jmhooper
Copy link
Contributor Author

jmhooper commented Jul 6, 2017

I've re-ordered the steps, but there's some follow on things I still need to do:

  • Verify that I didn't do something that would allow users to short-circuit the phone verification process and get a verified account without verifying their phone
  • There's a failing spec where it expects users signing out / back in in the middle of confirming a phone number on record to be redirected to the OTP entry screen

@zachmargolis
Copy link
Contributor

Does this preserve the "verify by phone or by mail" choice screen?

@jmhooper
Copy link
Contributor Author

jmhooper commented Jul 6, 2017

Yes, it should. I can to be sure though.

@jmhooper
Copy link
Contributor Author

jmhooper commented Jul 6, 2017

Just ran through the mail flow. It is still there and appears to work 👍

@jmhooper
Copy link
Contributor Author

Some updates:

  • I added a spec to make sure the Idv::Session service won't activate users profile unless their phone is confirmed
  • I modified the failing IdV flow test to test that the cancel link redirects to /verify/cancel like the rest of the cancel links through the IdV process

@jmhooper
Copy link
Contributor Author

I still need to modify the confirmations controller so it redirects to the otp verification if the phone number is unconfirmed. Also, there's some dead code on the account page that tells the user to verify their account by phone which will need to be removed.

@jmhooper
Copy link
Contributor Author

I changed up the review controller so it redirects to phone confirmation if the user needs to confirm their phone. The confirmations controller checks that an active profile has been created before allowing any actions. The profile is created in the review controller so no further work should need to be done to the confirmations controller.

There is still a wrinkle where a user can bail out of the process during the confirmations step and end up with a profile that is not activated. Because of that I'm leaving some of the code to allow a user to return to confirmation from the account page. I'd prefer to break that work off into another PR.

@jmhooper jmhooper changed the title [WIP] Reorder phone confirmation and review page Reorder phone confirmation and review page Jul 31, 2017
@monfresh
Copy link
Contributor

The code looks good overall, but I found the following bug:

  1. Make LOA3 request from SP
  2. Sign in or create an account
  3. Go through IdV flow and choose to activate by phone and enter a different phone than the one you setup 2FA with
  4. Once you are prompted for the OTP, click on "Use another phone number"
    Expected: You are able to enter a new number
    Actual: You get redirected to the OTP prompt

Could we make sure to add a test for this? The test should perform the flow in 3 different scenarios: via SAML, via OIDC, and when visiting the site directly.

@jmhooper
Copy link
Contributor Author

jmhooper commented Aug 1, 2017

Hmm, I don't think this bug is specific to my branch. I'm seeing something similar on master. There the difference is instead of redirecting to the OTP entry screen, it is redirecting me to the password screen.

It looks like the problem is that once you've entered your phone of record, we make the assumption that you wouldn't want to go back and change it. So, we redirect to the next step if you have entered your phone. On master, that is the password entry, here it is phone confirmation.

Corollary: The same is true for finance and profile info. It is impossible to back and change anything. We may want to think that through.

This behavior appears to occur regardless of whether you use OIDC or SAML.

I'm happy to fix, but I think it is out of scope here. This probably needs it's own issue and PR.

@jmhooper
Copy link
Contributor Author

jmhooper commented Aug 1, 2017

@monfresh: I have opened an issue for the above bug here: https://github.com/18F/identity-private/issues/2268

Copy link
Contributor

@monfresh monfresh left a comment

Choose a reason for hiding this comment

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

LGTM

@monfresh
Copy link
Contributor

monfresh commented Aug 1, 2017

Fair enough. We can address the bug in a separate PR.

**Why**: It is confusing for users to verify their phone number after
entering confirming their profile by entering their password. This
commit changes the order so the user verifies their phone immediately
after entering it

Add spec for session#complete_session

**Why**: While working on the verify by phone flow for the IdV process,
I want to add a test to make sure that I don't create a situation where
a profile can be activated without a confirmed phone number of record.

Link to cancel path for idv phone OTP cancel

**Why**: When a user cancels during identity verification, we don't want
to sign them out. They were being signed out because we were sharing
code with the authentication 2FA flow.

Check in review controller if phone confirmed

**Why**: If the user has not confirmed their phone during the IdV flow,
we want to redirect to the phone confirmation instead of allowing them
to continue with the creation of their profile.
@jmhooper jmhooper force-pushed the jmhooper-verify-phone-password-flow branch from ae400e6 to f2845b7 Compare August 1, 2017 15:21
@jmhooper jmhooper merged commit a9526ea into master Aug 1, 2017
@jmhooper jmhooper deleted the jmhooper-verify-phone-password-flow branch August 1, 2017 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants