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

Cognito Triggers - PreSignUp, PostConfirmation, PostAuthentication, CustomMessage #57

Merged
merged 6 commits into from
Jun 12, 2024

Conversation

jsonfry
Copy link
Contributor

@jsonfry jsonfry commented Jun 6, 2024

Added Cognito events for PostConfirmation, PostAuthentication, CustomMessage and handled the other PreSignUp trigger sources: PreSignUp_ExternalProvider and PreSignUp_AdminCreateUser

Motivation:

Continuing the work done #27, I've added support for some more Cognito trigger events.

Modifications:

I'm not sure of the original author's intentions for how to handle triggers that are of the same "shape". E.g. there are three different trigger sources that follow the "PreSignUp" shape (PreSignUp_SignUp, PreSignUp_ExternalProvider and PreSignUp_AdminCreateUser). The original author had named a single enum case .preSignUpSignUp, but the other two trigger sources would have been exactly the same.

I have therefore changed the name to preSignUp and implemented three new ones (PostConfirmation, PostAuthentication, CustomMessage) in that same way, and treating the params.triggerSource as the way you tell which exact preSignUp/CustomMessage etc you're dealing with.

Happy for feedback on this.

I've also made the fields on the common parameters public so they can be read.

Result:

When a Cognito User Pool has the PreSignUp, PostConfirmation, PostAuthentication, and/or CustomMessage triggers setup, they can now be fully handled.

@sebsto sebsto self-assigned this Jun 10, 2024
@sebsto sebsto self-requested a review June 10, 2024 09:51
@sebsto sebsto added the enhancement New feature or request label Jun 10, 2024
@sebsto
Copy link
Contributor

sebsto commented Jun 10, 2024

@swift-server-bot test this please

Copy link
Contributor

@sebsto sebsto left a comment

Choose a reason for hiding this comment

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

Thank you for having written this. It looks good to me and it looks like it can be merged. I would add a one line comment to make the large TriggerSource enum easier to read and understand, nut beside this, I don't have other comments

Sources/AWSLambdaEvents/Cognito.swift Show resolved Hide resolved
@sebsto
Copy link
Contributor

sebsto commented Jun 10, 2024

Thank you @jsonfry
I was giving a second thought about this

  1. What do you think about this issue filed a few weeks ago ?

  2. The CloudWatch event solved a similar challenge : having one common enveloppe for many possible subtypes, depending on the message type. They solved it differently than you, using generics and dedicated sub structs. See Is CognitoEventResponse necessary? #56
    Is there a valid reason to use to different approach (the enum with associated types as you did, and the approach using generics like they did).

It's a truly open question - I have not made my mind yet. @adam-fowler what do you think ?

@jsonfry
Copy link
Contributor Author

jsonfry commented Jun 10, 2024

  1. I did see that issue when I was working on adding these events, I think you'd have to make all the fields nullable on any response struct to decode the request (which has just an empty json object for response), which then wouldn't be a proper representation of the type.

  2. I did initially wonder why the original author of the cognito events here hadn't used generics too. But then I realised that as my single deployed lambda can be triggered by any of these cognito events, dealing with runtime generics would have been more of a pain and not as easily achievable as enums with assoicated types. It's easier to let the decoder tell you what type it is rather than have to know beforehand and try and hook in somewhere.

@adam-fowler
Copy link
Member

It's a truly open question - I have not made my mind yet. @adam-fowler what do you think ?

  • If you asking should we have separate CognitoEvent and CognitoEventResponse structs, then the answer is yes. They are used for different things.
  • Also I prefer using an enum with associated values

@sebsto
Copy link
Contributor

sebsto commented Jun 12, 2024

@swift-server-bot test this please

Copy link
Contributor

@sebsto sebsto left a comment

Choose a reason for hiding this comment

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

LGTM

@sebsto sebsto merged commit a0b64e9 into swift-server:main Jun 12, 2024
5 checks passed
@Buratti
Copy link
Contributor

Buratti commented Jun 12, 2024

Just saw your mention on the other issue @sebsto. Since this has been merged, I'll keep this short.

IMHO, this approach is more verbose than necessary. Additionally, according to AWS documentation, CognitoEvent and CognitoEventResponse don't appear to be distinct entities.

Input event edited in place:
Define Auth Challenge
Create Auth Challenge
Custom Message
Pre Sign-Up

Input event returned as is:
Post Authentication

AWS Samples:
Cognito Custom Authentication

I can see the proposed solution as a more Swift-like approach compared to in-place editing. However, considering that it introduces a pattern completely different from the examples in the AWS documentation, I suggest we provide documentation with a few examples of our own.

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

Successfully merging this pull request may close these issues.

4 participants