Skip to content
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

feat: Add support for custom Lambda function email senders in Auth construct #2087

Merged
merged 124 commits into from
Oct 31, 2024

Conversation

vigy02
Copy link
Contributor

@vigy02 vigy02 commented Oct 7, 2024

Problem

This PR addresses the need for more flexible email sender configuration in the Auth construct, specifically allowing the use of custom Lambda functions for email sending capabilities.

Changes

  • Added support for using Lambda functions as email senders in the Auth construct
  • Implemented KMS key management for custom email senders
  • Updated types and interfaces to accommodate new email sender options
  • Added new test case for custom email sender functionality
  • Modified existing code to handle Lambda function email senders, including permission setup
  • Introduced functionality for attaching a user's KMS key to the customEmailSender for message encryption and decryption.
  • Added integration tests to verify customEmailSender functionality.

Validation

A new test case was added to verify the behavior when a function is provided as an email sender. This test ensures that the custom email sender is properly set up when a Lambda function is provided.

** Manual Validation for KMS-ARN pass check **
Screenshot 2024-10-24 at 6 19 55 PM
Screenshot 2024-10-24 at 6 19 45 PM
Screenshot 2024-10-24 at 6 19 36 PM

Checklist

  • If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.

@vigy02 vigy02 requested a review from awsluja October 25, 2024 01:23
Comment on lines 196 to 198
"zoneinfo",
"SKey",
"decrypt"
Copy link
Member

Choose a reason for hiding this comment

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

please insert those in alphabetical order.

@@ -54,6 +54,7 @@ type ConversationHandlerFunctionProps = {
modelId: string;
region?: string;
}>;
memoryMB?: number;
Copy link
Member

Choose a reason for hiding this comment

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

you might need to merge latest main. this change shouldn't be on the diff.

Comment on lines 51 to 52
email: Pick<UserPoolSESOptions, 'fromEmail' | 'fromName' | 'replyTo'> | IFunction;
kmsKeyArn?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Is kmsKeyArn applicable to any of Pick<UserPoolSESOptions, 'fromEmail' | 'fromName' | 'replyTo'> ?

If not then we should introduce type for custom sender.
Something like:
type CustomEmailSender = { handler: IFunction, kmsKeyArn?: string } and then use that instead of IFunction in the union.

Or perhaps have email: Pick<UserPoolSESOptions, 'fromEmail' | 'fromName' | 'replyTo'> | IFunction | CustomEmailSender so that passing function without kmskey is still easy.

The bottom line is that we should find typing that doesn't let you specify kmsKeyArn if IFunction is not used.

@josefaidt wdyt ?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is a good point, i think this would be enough:
email: Pick<UserPoolSESOptions, 'fromEmail' | 'fromName' | 'replyTo'> | CustomEmailSender

Copy link
Contributor

Choose a reason for hiding this comment

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

feedback makes sense. I like this take

email: Pick<UserPoolSESOptions, 'fromEmail' | 'fromName' | 'replyTo'> | IFunction | CustomEmailSender

this is also simple, and easier to understand that you only have two valid configuration options (ses or a sender function with/without a custom kms key)

email: Pick<UserPoolSESOptions, 'fromEmail' | 'fromName' | 'replyTo'> | CustomEmailSender

Comment on lines 46 to 47
email: Pick<UserPoolSESOptions, 'fromEmail' | 'fromName' | 'replyTo'> | ConstructFactory<AmplifyFunction>;
kmsKeyArn?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Similar feedback here. We should find better typing.

Comment on lines 269 to 270
(name) => name.includes('say-hello')
); // TODO - add func name
Copy link
Member

Choose a reason for hiding this comment

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

yes, we should name this function something different.

@@ -280,6 +287,7 @@ class DataStorageAuthWithTriggerTestProject extends TestProjectBase {
await this.checkLambdaResponse(node16Lambda[0], expectedResponse);
await this.checkLambdaResponse(funcWithSsm[0], 'It is working');
await this.checkLambdaResponse(funcWithAwsSdk[0], 'It is working');
await this.checkLambdaResponse(funcCustomSender[0], 'Hello, World!');
Copy link
Member

Choose a reason for hiding this comment

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

This is not good assertion for this feature. It just sends request directly to lambda. We should be asserting that communication between cognito and lambda works.

How to make this happen?:

  1. We need to execute an operation on user pool that triggers email sending.
  2. We need a function that can signal a fact that it was called by something and await that.
    1. Please take a look at solution we have for "scheduled functions". We use SQS queue to capture the fact that function was called.
      1. await sqsClient.send(
        new SendMessageCommand({
        QueueUrl: queueUrl,
        MessageBody: messageBody,
        })
        );
      2. // wait for schedule to invoke the function one time for it to send a message
      3. And fail with timeout (if sqs message doesn't arrive),

@@ -84,6 +85,12 @@ export type CustomAttributeString = CustomAttributeBase & StringAttributeConstra
dataType: 'String';
};

// @public
export type CustomEmailSenderConstruct = {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export type CustomEmailSenderConstruct = {
export type CustomEmailSender = {

or CustomEmailSenderOptions

This is not a construct. It has a reference to IFunction inside but this type is not implementing Construct super class.

Comment on lines 662 to 666
const queue = await this.resourceFinder.findByBackendIdentifier(
backendId,
'AWS::SQS::Queue',
(name) => name.includes('testFuncQueue')
);
Copy link
Member

Choose a reason for hiding this comment

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

We should add separate queue for this scenario.

Otherwise messages from email and scheduled functions will land in the same queue and we can't tell which one is which.

Copy link
Member

Choose a reason for hiding this comment

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

It seems that sequencing of post deployment assertions help. But it's very implicit assumption. Using separate resources for both scenarios will remove that trap.

UserPoolId: userPoolId[0],
Username: username,
TemporaryPassword: password,
MessageAction: 'SUPPRESS',
Copy link
Member

Choose a reason for hiding this comment

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

Does Cognito try to send email if we suppress messaging ?

Comment on lines 4 to 6
const customEmailSenderFunction = {
handler: funcWithSchedule,
};
Copy link
Member

Choose a reason for hiding this comment

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

Related to my previous comments.

We shouldn't reuse funcWithSchedule here. Because we'll be blending together two independent scenarios.

We should create a function similar to funcWithSchedule with it's own associated queue. I.e. create similar but isolated setup.

/**
* CustomEmailSender type for configuring a custom Lambda function for email sending
*/
export type CustomEmailSenderConstruct = {
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, this should not end with *Construct in the name

import { defaultNodeFunc, funcWithSchedule } from '../function.js';

const customEmailSenderFunction = {
handler: funcWithSchedule,
Copy link
Contributor

@rtpascual rtpascual Oct 31, 2024

Choose a reason for hiding this comment

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

I agree with what Kamil said about this, funcWithSchedule also has a cron job to invoke it every minute which may make testing this inaccurate.

Copy link
Contributor

Choose a reason for hiding this comment

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

We would probably want a new/different function with same SQS setup for a different queue and no schedule set. That way only message in the queue will be from when you create a new user in triggerEmailSending and not from scheduled invocations.

@vigy02 vigy02 merged commit 11d62fe into main Oct 31, 2024
66 of 69 checks passed
@vigy02 vigy02 deleted the e2e/vigy/feature/custom-email-function branch October 31, 2024 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-e2e Label that will include e2e tests in PR checks workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants