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

Add Cognito User Pool Triggers #3657

Merged
merged 16 commits into from Jun 6, 2017

Conversation

hassankhan
Copy link
Contributor

@hassankhan hassankhan commented May 22, 2017

What did you implement:

Closes #3637. Adds support for Cognito User Pool Triggers, and includes integration tests.

How did you implement it:

Created new plugin for the package:compileEvents hook that checks events in serverless.yml for a cognitoUserPool property.

How can we verify it:

By running the integration tests in tests/integration/aws/cognito-user-pool-trigger/:

$ node_modules/.bin/jest cognito-user-pool-trigger

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

This includes unit tests and documentation. A simple integration test
has been added.
Now includes test cases for single/multiple user pools with single/multiple
event definitions on single/multiple functions.
@pmuens pmuens self-requested a review May 22, 2017 06:14
@pmuens pmuens added this to the 1.15 milestone May 22, 2017
@HeroesDieYoung
Copy link

Any word on the priority of this? I'm looking to have lambda's using cognito triggers in my project and would very much like to be able to configure that directly in serverless rather than having to go to the console for it.

@pmuens
Copy link
Contributor

pmuens commented May 24, 2017

Any word on the priority of this?

@HeroesDieYoung sure!

We've added it to the v1.15 milestone (today we're releasing v1.14). We'll review this as soon as v1.14 is out the door so that it can be merged ASAP.

Once merged you might want to work with master / the commit hash after merging so that you can already benefit w/o having to wait for v1.15.

@HeroesDieYoung
Copy link

Fantastic, thanks for the prompt reply! I'll keep an eye on this PR then.

@hassankhan
Copy link
Contributor Author

hassankhan commented May 24, 2017

The milestone says v1.15 should be done by June 7th, so I'd check back then @HeroesDieYoung. Good to know I'm not the only person who needs this 😄

Copy link
Contributor

@pmuens pmuens left a comment

Choose a reason for hiding this comment

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

This is super nice! Well done @hassankhan 👍 🥇

Nice code and big +1 for adding integration tests. 💯

I just read through the code and added some comments (most of the time some minor things).

Really excited about this feature!

I ran the integration tests. Unfortunately they failed with the message: Cannot read property 'Id' of undefined

Could you provide a simple serverless.yml file (or quick step by step guide) so that we can test it manually?

Thanks in advance!

@@ -0,0 +1,139 @@
<!--
title: Serverless Framework - AWS Lambda Events - CloudWatch Log
Copy link
Contributor

Choose a reason for hiding this comment

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

This title needs to be updates (still refers to CloudWatch log

-->

<!-- DOCS-SITE-LINK:START automatically generated -->
### [Read this on the main serverless docs site](https://www.serverless.com/framework/docs/providers/aws/events/)
Copy link
Contributor

Choose a reason for hiding this comment

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

This link should point to the event readme (cognito-user-pool-trigger) in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I thought it auto-generated that so I left it alone 😄

Copy link
Contributor

@pmuens pmuens May 26, 2017

Choose a reason for hiding this comment

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

😄 Yes, for some reason it's not auto-generated. Don't know why though 😄

- cognitoUserPool:
pool: MyUserPoolFromResources
trigger: PostConfirmation
resources:
Copy link
Contributor

Choose a reason for hiding this comment

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

A newline between the end of the functions definition and the resources section would make this easier to digest.

```

[aws-triggers-list]: http://docs.aws.amazon.com/cognito/latest/developerguide/cognito-user-identity-pools-working-with-aws-lambda-triggers.html#cognito-user-pools-lambda-trigger-syntax-shared
[logical-resource-names]: https://serverless.com/framework/docs/providers/aws/guide/resources#aws-cloudformation-resource-reference
Copy link
Contributor

Choose a reason for hiding this comment

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

This link should be relative so that we can change the URL / structure later on.

'functionName',
'poolId',
'TriggerSource'
)).to.equal('FunctionNameLambdaPermissionCognitoUserPoolTriggerSourcePoolIdTriggerSource');
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick question: Do you know if there's a limit for the resource logical Id length in CloudFormation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't seem to find any information regarding length limits, the only thing I can find is this.

};

// Check if pool name has been declared in `Resources`
if (_.has(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe check for the Type as well? Might be pretty unlikely but I'm thinking about the case when one has defined another Resource (e.g. an S3 bucket) with the name of a pool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea 👍 A little extra validation won't hurt

As requested in PR review
There were several resource names that were incorrectly specified
that were causing the tests to fail. `Utils.getCognitoUserPoolId`
was only returning a single item, which was also causing failures.
If multiple pools were specified that had the same trigger i.e.
`PreSignUp` then the last function would incorrectly be attached to
each pool. Also added a test case.
@hassankhan
Copy link
Contributor Author

hassankhan commented May 25, 2017

Hi @pmuens, I think I've covered everything you requested. The integration tests were indeed failing, my fault for making assumptions. I thought Travis would have failed the build 😕 .

Also fixed a bug found during the multiple-pools-multiple-events-multiple-functions integration test where the wrong Lambda ARN was being added to the pools.

Please let me know if it's all up to scratch 👍

@hassankhan
Copy link
Contributor Author

hassankhan commented May 25, 2017

I've just found what could be an annoying error - occasionally the Cognito User Pool gets created before the Lambdas do. This causes a problem because the User Pool CF template has a LambdaConfig property which maps the trigger sources to each Lambda's ARN.

I think the best way to deal with it might be to add something like: DependsOn: [{firstLambdaLogicalId}, {secondLambdaLogicalId}] to a User Pool's CF template, what do you guys think? @pmuens @HyperBrain

Copy link
Contributor

@pmuens pmuens left a comment

Choose a reason for hiding this comment

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

Thanks for updating @hassankhan 👍

I just ran the integration tests again. 7 passed and 2 failed. Here are the error messages:

  ● AWS - Cognito User Pool Trigger: Single User Pool with single event with single function › should call the specified function when PreSignUp event is triggered

    TypeError: Cannot read property 'Id' of undefined

and

● AWS - Cognito User Pool Trigger: Single User Pool with multiple events with multiple functions › should call the specified function when CustomMessage event is triggered

    Command failed: /app/bin/serverless logs --function customMessage1 --noGreeting true

Adding a DependsOn to the corresponding resource (e.g. Lambda) sounds reasonable.

We do this with other events as well 👍

@HyperBrain
Copy link
Member

@hassankhan @pmuens Regarding the dependency declaration I think this is a good solution to ensure that everything works on deployment.

@pmuens
Copy link
Contributor

pmuens commented May 30, 2017

@hassankhan just wanted to do a quick follow-up on this PR.

Do you need any help here? Can we do smth. so that it's ready to be merged in v1.15?

@hassankhan
Copy link
Contributor Author

@pmuens Sorry I haven't had time to update it. Weirdly the integration tests seemed to run fine on my machine (I ran them with this: node_modules/.bin/jest cognito-user-pool-trigger). I also haven't added the DependsOn attribute either.

I'm not sure how much time I'll have this week as I have work commitments, I'll try my best 👍

@pmuens
Copy link
Contributor

pmuens commented May 30, 2017

Thanks for the update @hassankhan 👍

I'll debug this integration test issue again this week and will also look into the DependsOn property.

I'm not sure how much time I'll have this week as I have work commitments, I'll try my best 👍

No worries! Don't want to hurry you. Just wanted to make sure that we can get this into the next release since it's a super valuable feature.

@@ -103,12 +103,16 @@ class AwsCompileCognitoUserPoolEvents {

const userPoolLogicalId = this.provider.naming.getCognitoUserPoolLogicalId(poolName);

const DependsOn = _.map(currentPoolTriggerFunctions, (value) => this
.provider.naming.getLambdaLogicalId(value.functionName));

const userPoolTemplate = {
Type: 'AWS::Cognito::UserPool',
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it is wise to create the user pool for the user (if he did not declare it in the resources) at all. There are lots of other User Pool settings that have to be made available in that case (e.g. password complexity, MFA, any many more). The standard settings are not usable in a production environment.

This comment is generic, not scoped to this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the feedback @HyperBrain 👍

I'm not too familiar with Cognito User Pools. Should we keep it here or remove it? I'm always in favor of having a default (even if it's super rudimentary).

Copy link
Contributor

@pmuens pmuens left a comment

Choose a reason for hiding this comment

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

Just did a final code review and tested it thoroughly.

Works like a charm! Thanks @hassankhan 👍 💯

Merging this now :shipit:

@pmuens pmuens merged commit f1df11a into serverless:master Jun 6, 2017
@hassankhan hassankhan deleted the add-cognito-user-pool-triggers branch June 6, 2017 12:18
@keshavkaul
Copy link

keshavkaul commented Jun 9, 2017

@hassankhan @pmuens I have already updated sls to 1.5.1, and I can't seem to successfully test cognitoUserPool event. When deploying, I don't get any errors. And going in cognito console, I don't see the trigger link setup with the lambda.

Also, it might help to clarify what MyUserPool exactly is - pool Id or pool name. I have tried both but no success.

@pmuens
Copy link
Contributor

pmuens commented Jun 10, 2017

@keshavkaul thanks for the comment 👍 .

This feature was added to Serverless v1.15.0 and is not available in Serverless v1.5.x

Could you open up a new issue which shows your serverless.yml and gists of your generated CloudFormation files if you're on v1.15.0?

Thanks in advance!

@keshavkaul
Copy link

keshavkaul commented Jun 10, 2017

@pmuens Sorry I have v1.15.1. I will update to v1.15.2 and again test it. thanks

@jamesladd
Copy link

Is there some example code to go with this?

@dbligh
Copy link

dbligh commented Jan 14, 2018

I am struggling trying to get this to work correctly.

I've got a UserPool defined in my resources, and have the lambda trigger defined under functions. But when I view the user pool in the console, it has no triggers assigned to the user pool. Both are created separately, but unlinked.

Here's a sample project I'm testing this with (latest version of serverless 1.25.0):


provider:
  name: aws
  runtime: nodejs6.10

functions:
  hello:
    handler: handler.hello
    events:
      - cognitoUserPool:
        pool: SampleUserPool
        trigger: CustomMessage
resources:
  Resources:
    CognitoUserPoolSampleUserPool:
      Type: AWS::Cognito::UserPool

@jamesladd
Copy link

jamesladd commented Jan 14, 2018 via email

@dbligh
Copy link

dbligh commented Jan 14, 2018

I think I finally resolved this, 2 days of hair pulling :)


provider:
  name: aws
  runtime: nodejs6.10

functions:
  hello:
    handler: handler.hello
resources:
  Resources:
    CognitoUserPoolSampleUserPool:
      Type: AWS::Cognito::UserPool
      DependsOn: HelloLambdaFunction
      Properties:
        LambdaConfig:
          CustomMessage: 
            Fn::GetAtt: [ HelloLambdaFunction, Arn ]

The 'hello' function gets turned into 'HelloLambdaFunction' (case sensitive) in the Cfn script, so that's how you need to reference it. I can now see both the UserPool and the Trigger properly configured.

Hope this helps anyone else that has been struggling on this.

@robfapadmi
Copy link

@dbligh your solution certainly works, but that was possible before this PR! I am also unable to get the new event trigger working. My serverless.yml looks like this:

functions:
  customCognitoMessages:
    handler: src/CustomCognitoMessages.handler
    events:
      - cognitoUserPool:
        pool: [my-user-pool-id]
        trigger: CustomMessage


resources:
  Resources:
    cognitoIdentityPool:
      Type: AWS::Cognito::IdentityPool
      Properties:
        AllowUnauthenticatedIdentities: false
        IdentityPoolName:
          [my-identity-pool-id]

I'm then checking the Triggers section in the Cognito AWS console, but it remains empty. Does anyone have any ideas as to why this is not working?

@hassankhan
Copy link
Contributor Author

hassankhan commented Jan 18, 2018

Man I really messed up when writing the docs 😞

@robfapadmi:

functions:
  customCognitoMessages:
    handler: src/CustomCognitoMessages.handler
    events:
      - cognitoUserPool:
        pool: [my-user-pool-id]
        trigger: CustomMessage


resources:
  Resources:
    CognitoIdentityPool[my-identity-pool-id]:
      Type: AWS::Cognito::IdentityPool
      Properties:
        AllowUnauthenticatedIdentities: false

@jamesladd
Copy link

jamesladd commented Jan 18, 2018 via email

@hassankhan
Copy link
Contributor Author

If anyone would be so kind as to make a PR that explains this perhaps in a clearer way?

@jamesladd My pleasure

@dbligh
Copy link

dbligh commented Jan 20, 2018

It turns out that the cause of most of my headaches was because of a plugin I was using to split stacks (serverless-nested-stack). Was causing a bunch of dependency issues for me that I couldn't work around.

My resolution was to isolate all my cognito components into a separate project, and create stack outputs, and refer to them in my main project. The other benefit to this being that it keeps my resource count down in my main project.

@janoist1
Copy link

janoist1 commented Aug 22, 2018

@hassankhan Unfortunately your example config doesn't work for me, it won't set the PostConfirmation trigger.
I have tried using pool ID as well as pool name from pool props (UserPoolName: my-${self:custom.stage}-user-pool). None of them worked.

@JaderCM
Copy link
Contributor

JaderCM commented Oct 15, 2018

Same here. Following this document the lambda function and cognito pool are created but not linked (the combo to this trigger is empty).

service: "CognitoTest"
provider:
  name: aws
  runtime: nodejs6.10

functions:
  CognitoTriggerPostConfirmation:
    handler: Cognito/Triggers.PostConfirmation
    events:
      - cognitoUserPool:
        pool: TestUserPool
        trigger: PostConfirmation

resources:
  Resources:
    CognitoUserPoolTestUserPool:
      Type: AWS::Cognito::UserPool

serverless 1.32.0

@jamesladd
Copy link

jamesladd commented Oct 15, 2018 via email

@jamesladd
Copy link

jamesladd commented Oct 15, 2018 via email

@JaderCM
Copy link
Contributor

JaderCM commented Oct 15, 2018

Thank you for reply @jamesladd, but your configuration not have a trigger of Cognito Pool.

I see some messages customized like "EmailVerificationMessage" or "EmailVerificationSubject", polices to invoke lambda functions.

My and @janoist1 case is a trigger when the user confirm the registration process. The combo that we are talking about is this:

screenshot_1

@jamesladd
Copy link

jamesladd commented Oct 15, 2018 via email

@JaderCM
Copy link
Contributor

JaderCM commented Oct 15, 2018

What is the configuration of your role "UserRegistrationRole"? I'm trying reproduce here your example.

@JaderCM
Copy link
Contributor

JaderCM commented Oct 16, 2018

Thank you @jamesladd. I compared my yaml file with your and I found the problem with mine.

This yaml file not works:

functions:
  CognitoTriggerPostConfirmation:
    handler: Cognito/Triggers.PostConfirmation
    private: true
    events:
      - cognitoUserPool:
        pool: TestUserPool
        trigger: PostConfirmation

resources:
  Resources:
    CognitoUserPoolTestUserPool:
      Type: AWS::Cognito::UserPool

But this code works:

functions:
  CognitoTriggerPostConfirmation:
    handler: Cognito/Triggers.PostConfirmation
    private: true
    events:
      - cognitoUserPool:
          pool: TestUserPool
          trigger: PostConfirmation

resources:
  Resources:
    CognitoUserPoolTestUserPool:
      Type: AWS::Cognito::UserPool

The difference is 2 spaces before the section inside of the property cognitoUserPool. The documentation show this and I lost. My mistake.

@janoist1 maybe is the same for you.

@JaderCM
Copy link
Contributor

JaderCM commented Oct 16, 2018

@hassankhan it's possible to reference the cognito user pool with another way than with prefix "CognitoUserPool"?

My current stage have a CognitoUserPool with users and if I change the logical id, another cognito user pool will be created.

So, how can I reference my current CognitoUserPool in this lambda?

My case:

functions:
  CognitoTriggerPostConfirmation:
    handler: Cognito/Triggers.PostConfirmation
    private: true
    events:
      - cognitoUserPool:
          pool: [how-to-get-MyExistentUserPool?]
          trigger: PostConfirmation

resources:
  Resources:
    MyExistentUserPool:
      Type: AWS::Cognito::UserPool

@JaderCM
Copy link
Contributor

JaderCM commented Oct 16, 2018

@hassankhan I see here the function that you are using to retrieve the Cognito User Pool by the Logical Id (getCognitoUserPoolLogicalId) and I ask you for what reason you did this?

My question is related with the cases of existent Cognito User Pool and we can not change the Logical Id to use this feature to link a Lambda Function to a Cognito User Pool trigger.

In this case, if we use the same values of "User Pool Logical Id" and property "cognitoUserPool.pool" the link will works fine.

@jamesladd
Copy link

jamesladd commented Oct 16, 2018 via email

@hassankhan
Copy link
Contributor Author

Hi @JaderCM, sorry for the delayed response.

it's possible to reference the cognito user pool with another way than with prefix "CognitoUserPool"?

Unfortunately no, since this is following the same Serverless naming patterns that all other resources do.

So, how can I reference my current CognitoUserPool in this lambda?

As I understand it, you cannot reference a pre-existing user pool. You would have to use the one generated by the framework.

I see here the function that you are using to retrieve the Cognito User Pool by the Logical Id (getCognitoUserPoolLogicalId) and I ask you for what reason you did this?

Not entirely sure what you're referring to...

For any other questions, I'd strongly suggest opening a new issue instead of commenting on this PR 😄. That way, there'd be more visibility rather than on this PR which only has a few eyes on it 👍

@JaderCM
Copy link
Contributor

JaderCM commented Oct 19, 2018

Hi @hassankhan, thanks for reply.

As I understand it, you cannot reference a pre-existing user pool. You would have to use the one generated by the framework.

My current (pre-existing) user pool was created by the serverless framework. The problem is that now I need to link this pool with my lambda by trigger and I cannot.

Not entirely sure what you're referring to...

My question is about if is possible to reference user pool using just the logical id without prefix.

I opened the issue #5401 to discuss about.

@iamdavidmartin
Copy link

iamdavidmartin commented Nov 27, 2019

this pull requests has caused me a lot of pain. in hindsight, i wish i would have never touched this.

  1. silent failures. this code silently fails when you do not indent cognitoUserPool properties like pool and trigger. i am not the only person who has strugged with this. you ought to get an error message that tells you that you've misconfigured the parameters.
  1. circular dependencies. this code creates them. maybe that's just a problem with cognito triggers, but it's simple to create triggers without the dependencies.
  1. can't reference the UserPool by the actual name yml name of the user pool. It's undocumented, but you have to figure out through extensive googling that if your user pool is called "CognitoUserPoolUsers", the pool property should get "Users".

i was able to implement the correct trigger without any of these headaches with a single line of code here, found here: #5401 (comment)

🤦🏻‍♂️🤦🏻‍♂️🤦🏻‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS Cognito User Pool triggers support