feat(authenticator): state machine updates for email mfa #6317
Conversation
|
jjarvisp
force-pushed
the
james/feat-email-mfa/update-state-machine
branch
from
cda33cd to
6497bd2
Compare
There was a problem hiding this comment.
Great work overall @jjarvisp. Left some feedback around state duplication that should be pretty straightforward to address, and some nits/questions
| @@ -26,6 +27,7 @@ export const defaultTexts = { | |||
| CREATE_ACCOUNT: 'Create Account', | |||
| CREATING_ACCOUNT: 'Creating Account', | |||
| EMAIL_ADDRESS: 'Email', | |||
| EMAIL_OTP: 'Email Message (EMAIL)', | |||
There was a problem hiding this comment.
"(EMAIL)" reads kind of weird here, can you provide context?
There was a problem hiding this comment.
Removed this. Radio labels will be:
- Email Message
- Authenticator App (TOTP)
- Text Message (SMS)
packages/ui/src/i18n/dictionaries/authenticator/defaultTexts.ts
Outdated
Show resolved
Hide resolved
| handleSetupEmail({ formValues }) { | ||
| return services.handleConfirmSignIn({ | ||
| challengeResponse: formValues.email, | ||
| }); | ||
| }, | ||
| handleSelectMfaType({ formValues }) { | ||
| return services.handleConfirmSignIn({ | ||
| challengeResponse: formValues.mfa_type, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Any reason not to simplify so only services.confirmSignIn is used for all challengeResponse handling?
| @@ -28,6 +29,9 @@ export type ChallengeName = | |||
| | 'ADMIN_NO_SRP_AUTH' | |||
| | 'NEW_PASSWORD_REQUIRED'; | |||
|
|
|||
| // JS v6 Mfa Types | |||
| export type AuthMFAType = 'SMS' | 'TOTP' | 'EMAIL'; | |||
There was a problem hiding this comment.
Is AuthMFAType not exposed from aws-amplify/auth? Think it should be if not
There was a problem hiding this comment.
It's never been exposed, not sure why. I will look into this.
| setupEmail: { | ||
| initial: 'edit', | ||
| exit: ['clearFormValues', 'clearError', 'clearTouched'], | ||
| states: { | ||
| edit: { | ||
| entry: 'sendUpdate', | ||
| on: { | ||
| SUBMIT: { actions: 'handleSubmit', target: 'submit' }, | ||
| SIGN_IN: '#signInActor.signIn', | ||
| CHANGE: { actions: 'handleInput' }, | ||
| }, | ||
| }, | ||
| submit: { | ||
| tags: 'pending', | ||
| entry: ['sendUpdate', 'clearError'], | ||
| invoke: { src: 'handleSetupEmail', ...handleSignInResponse }, | ||
| }, | ||
| }, | ||
| }, | ||
| selectMfaType: { | ||
| initial: 'edit', | ||
| exit: ['clearFormValues', 'clearError', 'clearTouched'], | ||
| states: { | ||
| edit: { | ||
| entry: 'sendUpdate', | ||
| on: { | ||
| SUBMIT: { actions: 'handleSubmit', target: 'submit' }, | ||
| SIGN_IN: '#signInActor.signIn', | ||
| CHANGE: { actions: 'handleInput' }, | ||
| }, | ||
| }, | ||
| submit: { | ||
| tags: 'pending', | ||
| entry: ['sendUpdate', 'clearError'], | ||
| invoke: { src: 'handleSelectMfaType', ...handleSignInResponse }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
AFAICT these states are the same as the existing state.confirmSignIn with minor exceptions (exit includes "clearChallengeName", the values of states.submit.invoke.src). Would be ideal to use one state (similar comment on ln 365) for less maintenance overhead/avoid further bundle size bloat)
There was a problem hiding this comment.
This is a good callout, while I agree these additional states could be crammed into the confirmSignIn state if necessary, the split utilized here will provide easier maintainability, consistency with existing patterns, and lower complexity (at the cost of increased bundle size).
There is precedent for splitting states out that also call confirmSignIn (see setupTotp). The established pattern seems to be State -> Route -> Screen. Deviating from this pattern will introduce additional complexity at multiple points within the lifecycle including determining which form fields to populate and adding conditional rendering to the presentational layer.
jjarvisp
dismissed stale reviews from tiffanynwyeung and jordanvn
via
9f70329
Description of changes
The purpose of this pull request is to add state machine updates required to support Email MFA.
Two new states are added:
CONTINUE_SIGN_IN_WITH_EMAIL_SETUPCONTINUE_SIGN_IN_WITH_MFA_SETUP_SELECTIONandCONTINUE_SIGN_IN_WITH_MFA_SELECTIONIssue #, if available
Description of how you validated changes
Checklist
yarn testpasses and tests are updated/addeddocs,e2e,examples, or other private packages.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.