Skip to content

Always confirm phone number with OTP during unsupervised identity verification#6545

Merged
mitchellhenke merged 3 commits intomainfrom
mitchellhenke/always-confirm-idv-phone
Jul 14, 2022
Merged

Always confirm phone number with OTP during unsupervised identity verification#6545
mitchellhenke merged 3 commits intomainfrom
mitchellhenke/always-confirm-idv-phone

Conversation

@mitchellhenke
Copy link
Contributor

@mitchellhenke mitchellhenke commented Jul 1, 2022

Currently, we send phone OTP codes as a random six character alphanumeric for both authentication and identity verification to meet the 800-63A enrollment code requirements. Phone OTPs as a second factor requires the OTP code to have 20 bits of entropy when generating it, and typically this means sending a six digit numeric code (as we have in the past).

Sending six character alphanumeric codes allows us to skip sending and confirming the enrollment code for a phone number during proofing if the user has previously confirmed that phone number in their account. The alphanumeric code has caused some issues though, so we are intending to revert to sending six digit numeric codes for authentication.

We are still required to meet the higher level of entropy for proofing enrollment codes, so this pull request includes changes to always require sending the proofing OTP and not allow skipping it, even if you're confirmed the phone previously. Doing this will allow us to send different kinds of codes for both authentication and proofing rather than always sending 6 alphanumeric.


Most of the changes in this PR are to the tests since they previously defaulted to not having to receive and enter a phone OTP. This included switching some specs to use TOTP as MFA instead of phone because the OTP rate limit is relatively low (at two) in test and I didn't want to increase it.

Copy link
Contributor Author

@mitchellhenke mitchellhenke Jul 1, 2022

Choose a reason for hiding this comment

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

This is the crux and only thing needed in this PR to change the behavior, everything else is to support changing the specs

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there are other things we could simplify if we can always assume OTP confirmation will be required. At first I wondered if we could get rid of this variable altogether, though it appears it's necessary to push people to the review step if they're already done with this one.

Maybe we could still clean up some of the OTP-related redirect logic?

e.g.

if phone_confirmation_required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Potentially? I'm not sure though, the current flow is:

  1. PhoneFinder
  2. Confirm Phone via OTP if needed

so the stuff we can delete in the app might be more limited. The tests are a different story though.

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/always-confirm-idv-phone branch 5 times, most recently from 9d7bcd1 to f0976ae Compare July 5, 2022 17:59
@mitchellhenke mitchellhenke marked this pull request as ready for review July 5, 2022 19:26
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

Copy link
Contributor

@orenyk orenyk left a comment

Choose a reason for hiding this comment

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

I don't know this code well enough to approve but just wanted to say that I'm SO glad that we're moving back towards 6-digit OTP for MFA 🙌 . Nice work @mitchellhenke!

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/always-confirm-idv-phone branch from f0976ae to f794ef9 Compare July 8, 2022 13:56
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/always-confirm-idv-phone branch from f794ef9 to f794a3c Compare July 11, 2022 17:59
Mitchell Henke added 3 commits July 14, 2022 10:29
changelog: Improvements, Identity Verification, Always require confirming phone with OTP during proofing
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/always-confirm-idv-phone branch from f794a3c to 621b6c5 Compare July 14, 2022 15:29
@mitchellhenke mitchellhenke merged commit ab979fe into main Jul 14, 2022
@mitchellhenke mitchellhenke deleted the mitchellhenke/always-confirm-idv-phone branch July 14, 2022 18: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.

5 participants