Feat/mfa ts samples #5624
Feat/mfa ts samples #5624
Conversation
Initial commit
chore:address PR feedback
Co-authored-by: idevk <idevk@amazon.com>
This reverts commit ef0305f.
katieklein
added
external-contributor
product-owner-review-needed
israx
left a comment
israx
left a comment
There was a problem hiding this comment.
Hey @devnotfound . Thanks so much for putting this together.
PR looks good, just a few minor changes about not having imports and references from scoped packages, user passed on authenticated APIs should come from Auth.currentAuthenticatedUser, and finally about keeping the current format for user attributes.
src/fragments/lib/auth/js/mfa.mdx
Outdated
| type SetupTOTPAuthParameters = { | ||
| user: string; | ||
| challengeAnswer: string; | ||
| mfaType?: 'SMS_MFA' | 'SOFTWARE_TOKEN_MFA' | null; |
There was a problem hiding this comment.
mfaType param is already nullable, we can avoid the null type here
src/fragments/lib/auth/js/mfa.mdx
Outdated
| export async function setupTOTPAuth({ user, challengeAnswer, mfaType }: SetupTOTPAuthParameters) { | ||
| // To setup TOTP, first you need to get a `authorization code` from Amazon Cognito | ||
| // `user` is the current Authenticated user | ||
| const tOTPCode = await Auth.setupTOTP(user); |
There was a problem hiding this comment.
Technically, Auth.setupTOTP(user) doesn't return the OTP code yet. It returns a secretCode that is used on authenticator Apps to generate the OTP code.
src/fragments/lib/auth/js/mfa.mdx
Outdated
|
|
||
| // Finally, when sign-in with MFA is enabled, use the confirmSignIn method | ||
| // to pass the TOTP code and MFA type. | ||
| await Auth.confirmSignIn(user, tOTPCode, mfaType); // Optional, MFA Type e.g. SMS_MFA || SOFTWARE_TOKEN_MFA |
There was a problem hiding this comment.
the OTP code used here should come from an authenticator app, we can make that reference above
src/fragments/lib/auth/js/mfa.mdx
Outdated
| // and then trigger the following function with a button click | ||
| // For example, the email and phone_number are required attributes | ||
| const { username, email, phone_number } = getInfoFromUserInput(); | ||
| const { inputUsername, inputEmail, inputPhoneNumber } = await getInfoFromUserInput(); |
There was a problem hiding this comment.
user attributes format should be based on the standard claims of OpenId Connect
src/fragments/lib/auth/js/mfa.mdx
Outdated
| inputEmail, | ||
| inputPhoneNumber, |
There was a problem hiding this comment.
should be on the standard format as mentioned above
src/fragments/lib/auth/js/mfa.mdx
Outdated
| import { CognitoUser } from '@aws-amplify/auth'; | ||
| import { ClientMetaData } from '@aws-amplify/auth/lib-esm/types'; |
There was a problem hiding this comment.
trying to avoid this import pattern and all the reference as suggested above
Batch commit of all initial review changes Co-authored-by: israx <70438514+israx@users.noreply.github.com>
line replacement test
add try/catch for TS and JS blocks
update location of Auth.currentAuthenticatedUser() call
Final changes
abdallahshaban557
added
ready-to-merge
6 participants
Description of changes:
Related GitHub issue #, if available:
N/A
Instructions
If this PR should not be merged upon approval for any reason, please submit as a DRAFT
Which product(s) are affected by this PR (if applicable)?
Which platform(s) are affected by this PR (if applicable)?
Please add the product(s)/platform(s) affected to the PR title
Checks
Does this PR conform to the styleguide?
Does this PR include filetypes other than markdown or images? Please add or update unit tests accordingly.
Are any files being deleted with this PR? If so, have the needed redirects been created?
Are all links in MDX files using the MDX link syntax rather than HTML link syntax?
ref: MDX:
[link](https://link.com)HTML:
<a href="https://link.com">link</a>When this PR is ready to merge, please check the box below
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.