fix(authenticator): migrate totpSecretCode generation to state machine #3333
Conversation
🦋 Changeset detectedLatest commit: 61882f5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| const serviceSnapshot = service.getSnapshot() as AuthMachineState; | ||
|
|
||
| // legacy `QRFields` values only used for SetupTOTP page to retrieve issuer information, will be removed in future | ||
| const QRFields = useMemo( |
There was a problem hiding this comment.
Removed the useMemo wrappers for QRFields and fields as serviceSnapshot does not maintain a stable reference, leading to to re-evaluation on every to call to useAuthenticator.
Additionally added logic to ensure that getQRFields is only called while route is equal to setupTOTP since QRFields is not relevant to other routes.
| totpSecretCode, | ||
| unverifiedContactMethods, | ||
| user, | ||
| /** @deprecated For internal use only */ |
There was a problem hiding this comment.
Everything in the return below this line inherits this JSDoc comment.
| }, | ||
| onError: { | ||
| target: 'edit', | ||
| actions: 'setRemoteError', |
There was a problem hiding this comment.
We may want to consider adding error handling logic that allows the UI to render a "try again" button if TOTP code generation fails, but am considering that out of scope for this PR
| 'plugin:vue/vue3-essential', | ||
| 'eslint:recommended', | ||
| '@vue/eslint-config-typescript/recommended', | ||
| '@vue/eslint-config-prettier', |
There was a problem hiding this comment.
Removed as this was causing prettier eslint warnings that do not align with our prettier config
| const actorState = computed(() => | ||
| getActorState(state.value) | ||
| ) as ComputedRef<SignInState>; | ||
| const { totpSecretCode, user } = actorState.value.context; |
There was a problem hiding this comment.
Reading from the actor context here rather than useAuthenticator since it is not typed. Probably should be addressed in the future
|
|
||
| <base-wrapper class="amplify-flex amplify-authenticator__column"> | ||
| <base-wrapper class="amplify-flex amplify-authenticator__column"> | ||
| <template v-if="qrCode.isLoading"> |
There was a problem hiding this comment.
Updated the conditional rendering logic to align with other implementations. Previously the entire template was prevented from rendering while the QR code was loading, now just the QR code image itself has that behavior
| formOverrides?.['QR'] ?? {}; | ||
| const totpCode = | ||
| typeof totpSecretCode === 'string' && typeof totpUsername === 'string' |
There was a problem hiding this comment.
Had to add these conditionals to appease TS (same with line 58)
| type FormFieldsProps = { | ||
| isPending: AuthenticatorMachineContext['isPending']; | ||
| validationErrors?: AuthenticatorMachineContext['validationErrors']; | ||
| isPending: UseAuthenticator['isPending']; |
There was a problem hiding this comment.
Replaced usage of AuthenticatorMachineContext with UseAuthenticator for these downstream types since they are ultimately returned by useAuthenticator
calebpollman
changed the title
|
|
calebpollman
changed the title
packages/react-core/src/Authenticator/hooks/useAuthenticator/useAuthenticator.ts
Show resolved
Hide resolved
ErikCH
left a comment
ErikCH
left a comment
There was a problem hiding this comment.
I looked over the Vue changes and that will work. The useAuthenticator composable is a little tricky to add Types too. That could certainly be something that we can add in the future.
Co-authored-by: William Lee <[email protected]>
3 participants
Description of changes
Migrate
AuthenticatorTOTP secret code generation from UI components to state machine:sign-in-totp-mfainteg test by ensuring thatAuth.setupTOTPis only called onceAuthenticatorsetupTOTPUX with otherAuthenticatorimplementationstotpSecretCodeinuseAuthenticator(code is generated only forsetupTOTProute, value isnullfor all other routes)getQRFieldsfrom being called for non-setupTOTProutes inuseAuthenticatoruseMemoinuseAuthenticatorIssue #, if available
NA
Description of how you validated changes
Manually tested, integ tests
Checklist
yarn testpassessideEffectsfield updatedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.