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

[Amplify Gen2] please add a custom sender lambda trigger as a defineAuth's attribute #1607

Open
ggj0418 opened this issue Jun 4, 2024 · 5 comments
Labels
auth Issue pertaining to Amplify Auth feature-request New feature or request

Comments

@ggj0418
Copy link

ggj0418 commented Jun 4, 2024

Environment information

System:
  OS: Windows 11 10.0.22631
  CPU: (22) x64 Intel(R) Core(TM) Ultra 7 155H
  Memory: 11.66 GB / 31.53 GB
Binaries:
  Node: 20.11.0 - C:\Program Files\nodejs\node.EXE
  Yarn: 1.22.21 - ~\AppData\Roaming\npm\yarn.CMD
  npm: 10.8.0 - ~\WebstormProjects\amplify-next-pages-template\node_modules\.bin\npm.CMD
  pnpm: undefined - undefined
NPM Packages:
  @aws-amplify/backend: 1.0.2
  @aws-amplify/backend-cli: 1.0.3
  aws-amplify: 6.2.0
  aws-cdk: 2.140.0
  aws-cdk-lib: 2.140.0
  typescript: 5.4.5
AWS environment variables:
  AWS_NODEJS_CONNECTION_REUSE_ENABLED = 1
  AWS_SDK_LOAD_CONFIG = 1
  AWS_STS_REGIONAL_ENDPOINTS = regional
No CDK environment variables

Description

these values are the defineAuth method's option attributes.

triggers?: Partial<Record<'createAuthChallenge' | 'customMessage' | 'defineAuthChallenge' | 'postAuthentication' | 'postConfirmation' | 'preAuthentication' | 'preSignUp' | 'preTokenGeneration' | 'userMigration' | 'verifyAuthChallengeResponse', ConstructFactory<...>>> | undefined

there is no custom sender lambda value.

To reach that requirement, i had to custom the userPool on a phase of amplify backend cdk.

The sample code is here.

// backend.ts
const backend = defineBackend({
  sendVerificationCodeFunction,
  auth,
  data,
  storage
})

const { cfnUserPool } = backend.auth.resources.cfnResources
const existedSendVerificationCodeFunction = backend.sendVerificationCodeFunction.resources.lambda
existedSendVerificationCodeFunction.grantInvoke(
  new aws_iam.ServicePrincipal('cognito-idp.amazonaws.com')
)
const key = aws_kms.Key.fromKeyArn(
  cfnUserPool,
  `${KeyId}`,
  `${KeyArn}`
)
key.grantDecrypt(existedSendVerificationCodeFunction)

cfnUserPool.addPropertyOverride('LambdaConfig', {
  CustomSMSSender: {
    LambdaArn: existedSendVerificationCodeFunction.functionArn,
    LambdaVersion: 'V1_0'
  },
  KMSKeyID: key.keyArn
})

and this is the function's handler code

// handler.ts
export const handler = async (event: CustomSMSSenderTriggerEvent) => {
  try {
    const { decrypt } = buildClient(CommitmentPolicy.REQUIRE_ENCRYPT_ALLOW_DECRYPT)
    const generatorKeyId: string = `${KeyAlias}`
    const keyIds: string[] = [`${KeyArn}`]

    const keyringInput: KmsKeyringNodeInput = { generatorKeyId, keyIds }
    const keyring = new KmsKeyringNode(keyringInput)

    const request = parseEvent(event)
    console.log('Request:', JSON.stringify(request, null, 2))

    let plainTextCode: string | undefined
    if (request.code) {
      const { plaintext, messageHeader } = await decrypt(keyring, b64.toByteArray(request.code))
      plainTextCode = plaintext.toString('utf-8')
    }
    
    // do something with that plainTextCode

    return {
      statusCode: 200,
      headers: {
        'Access-Control-Allow-Origin': '*',
        'Access-Control-Allow-Headers': '*'
      },
      body: JSON.stringify({ success: true })
    }
  } catch (error) {
    console.error('Error sending message:', error)
    return {
      statusCode: 500,
      headers: {
        'Access-Control-Allow-Origin': '*',
        'Access-Control-Allow-Headers': '*'
      },
      body: JSON.stringify({ success: false })
    }
  }
}

I hope the amplify backend supply this feature officially

@ggj0418 ggj0418 added the pending-triage Incoming issues that need categorization label Jun 4, 2024
@edwardfoyle edwardfoyle added feature-request New feature or request auth Issue pertaining to Amplify Auth and removed pending-triage Incoming issues that need categorization labels Jun 4, 2024
@jamilsaadeh97
Copy link

I was looking into contributing this feature into the repo and I saw #2087 merged yesterday, so I would like to figure some things out before diving deeper if that's okay.

The feature introduced this new type:

export type CustomEmailSender = {
     handler: IFunction;
     kmsKeyArn?: string;
 };

If I want to implement CustomSmsSender using the same strategy as CustomEmailSender, it would have the same fields(a handler and a KMS key) but CloudFormation expects the KMS key to be inside the LambdaConfig, not a specific sender.
My question is, should we have two KMS keys or the current structure needs to be refactored in order to also have CustomSmsSender trigger?

Thanks for any information

@jamilsaadeh97
Copy link

I also find it a bit confusing that CustomEmailSender is to be defined in senders:{} instead of triggers:{} since this and CustomSmsSender are lambda triggers and defined as such in all the documentation in AWS

@ggj0418
Copy link
Author

ggj0418 commented Nov 18, 2024

@jamilsaadeh97

Thank you for raising this point! After reviewing the current structure and CloudFormation's requirements, I believe refactoring the KMS key configuration to reside at the LambdaConfig level would be a more scalable and maintainable solution. Here's my reasoning:

Why refactor to a shared kmsKeyArn in LambdaConfig?

  1. Alignment with CloudFormation:

    • CloudFormation already expects the kmsKeyArn to be part of the LambdaConfig, not specific to each sender. By following this convention, we can avoid potential misconfigurations and ensure compatibility with CloudFormation's structure.
  2. Simplified Configuration:

    • A shared KMS key in LambdaConfig reduces redundancy and makes it easier to manage configurations. This is especially beneficial if the key is meant to serve both CustomEmailSender and CustomSmsSender use cases.
  3. Flexibility for Future Extensions:

    • By placing the kmsKeyArn in LambdaConfig, we create a centralized configuration point. If new senders or triggers are added in the future, they can reuse the same structure without additional changes.

Proposed Changes:

  • Refactor the current structure so that the kmsKeyArn is moved from CustomEmailSender to LambdaConfig.
  • Update CustomSmsSender to follow the same pattern, utilizing the shared kmsKeyArn from LambdaConfig.

Example:

Here’s an example of how the updated types might look:

export type LambdaConfig = {
    handler: IFunction;
    kmsKeyArn?: string; // Shared KMS key for all senders
};

export type CustomEmailSender = {
    handler: IFunction;
};

export type CustomSmsSender = {
    handler: IFunction;
};

@Amplifiyer
Copy link
Contributor

@ggj0418 , thank you for the detailed proposal. I agree that it's an ideal way to handle both custom sms and email.

However, the challenge is that it will be a braking change and we can't do this refactor until we are ready to ship a Major version of the library. Until then I see us supporting this feature with

export type CustomEmailSender = {
    handler: IFunction;
    kmsKeyArn?: string;
};

export type CustomSmsSender = {
    handler: IFunction;
    kmsKeyArn?: string;
};

And validate and fail if the kmsKeyArn provided in both custom senders are different, nudging customers to use the same key. @josefaidt thoughts?

@josefaidt
Copy link
Contributor

Hey @ggj0418 I also agree that it makes the most sense to refactor it out -- and thank you for the detailed proposal!

@Amplifiyer 's proposal makes sense as an intermediate solution until we're ready to ship a breaking change, despite the need to specify the same value twice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth Issue pertaining to Amplify Auth feature-request New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants