-
Notifications
You must be signed in to change notification settings - Fork 102
feat(auth): Add email MFA support to Amplify Auth construct #3036
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 0b7171c The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good, have some questions and a change you should make.
| * @param mfa - MFA settings | ||
| * @param cfnUserPool - CloudFormation UserPool resource | ||
| */ | ||
| private configureEmailMFA = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there additional configurations needed for Email MFA? Since there is not an equivalent configuration function for SMS or TOTP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not needed I removed this redundant code.
| email: { | ||
| fromEmail: '[email protected]', | ||
| fromName: 'Example.com', | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is senders with email (custom email sending) required to enabled email MFA?
(not really a blocker, just curious about why senders with email is enabled in both of the tests where we want email MFA to be enabled)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This property already exists and does not require enable email MFA as it can be used for other email purposes 1) email verification upon user registration and forget password
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not necessary for email MFA, can you write a test that doesn't use it (you can just alter one of you existing tests) so when people look at these tests in the future they are not under the impression that enabling senders with email is necessary to set up in order to use email MFA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it is required, if we did not pass it we will get the following error.
[ERROR] [BackendBuildError] Unable to deploy due to CDK Assembly Error
∟ Caused by: [AssemblyError] Assembly builder failed
∟ Caused by: [AmplifyAuthConstructInitializationError] Failed to instantiate auth construct
∟ Caused by: [ValidationError] To enable email-based MFA, set `email` property to the Amazon SES email-sending configuration.
Resolution: See the underlying error message for more details.
Resolution: Check the Caused by error and fix any issues in your backend code
12:14:54 PM Stack Trace for BackendBuildError
12:14:54 PM BackendBuildError: Unable to deploy d
Apologies for the confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good, make sure to put that information in the docs if it is not already there 👍
| unauthenticated_identities_enabled?: boolean; | ||
| mfa_configuration?: 'NONE' | 'OPTIONAL' | 'REQUIRED'; | ||
| mfa_methods?: ('SMS' | 'TOTP')[]; | ||
| mfa_methods?: ('SMS' | 'TOTP' | 'EMAIL')[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client_config_vX files are autogenerated files and should not be edited by hand. If you want to make updates to then, update their related schema file and generate a new client_config file using json-schema-to-typescript.
As a note, with schema_v1.4, you will want to run json-schema-to-typescript with --unreachableDefinitions to ensure all the types are properly generated (leave some kind of comment behind in the newly generated config file to warn the next person -- leaving the comment behind should be the only time you are manually editing the client_config files).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran the json-schema-to-typescript generation command, but it appears the generated files haven't been updated in a while. The output includes changes beyond just the MFA modifications.
Command used:
npx json-schema-to-typescript -i schema_v1.json -o client_config_v1.ts --no-format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for making the updates from the last review! I just have one more alteration I want you to make, then this PR should be good to go 👍
| /** | ||
| * JSON schema | ||
| */ | ||
| $schema?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make manual changes to this file so the $schema property and the name property under AmazonLocationServiceConfig are the reverted back to their states before this version of client_config_v1.4 was generated? (and regenerate the Api.md file after you make the changes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ✅
|
This is a useful feature! When senders.email is not configured for the user pool, Cognito falls back on default email configuration (which doesnt seem to support email mfa). email mfa needs SES configured Maybe a check to ensure that sender.email is configured correctly would help. The way I understand it is, what could currently happen is, the app would deploy but prevent users from actually using mfa if the user pool uses the default email config |
No, it will not deploy it will throw an error for example (see the error message below). Additionally we have a call out in our documentation about Amazon SES configuring |
|
Okay great. I missed the previous comments, seems this has already been addressed. This should be good to go, I don't see any other issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Problem
This PR adds support for email-based multi-factor authentication (MFA) to the Amplify Auth construct. Previously, the Auth construct only supported SMS and TOTP MFA methods, limiting authentication options for applications that prefer
email-based verification.
Issue number, if available:
Changes
• Auth Construct: Added email MFA configuration support in construct.ts with new type definitions in types.ts
• API Updates: Extended public API to include email MFA options with TypeScript types
• Client Configuration: Updated all client config schema versions (v1, v1.1-v1.4) to support email MFA settings
• Documentation: Updated API.md and README.md with email MFA usage examples
• Test Coverage: Added comprehensive unit tests for email MFA functionality
Corresponding docs PR, if applicable:
Validation
• Unit Tests: Added 56 lines of test coverage in construct.test.ts validating email MFA configuration and behavior
• Manual testing: I tested on a amplify sample app which is linked locally with amplify-backend. I set the email MFA to true and false and checked it updates correctly in the Cognito user pool
Checklist
run-e2elabel set.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.