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 CustomAuthDomain parameter to template.yaml #44

Closed
wants to merge 1 commit into from
Closed

add CustomAuthDomain parameter to template.yaml #44

wants to merge 1 commit into from

Conversation

opaquejacob
Copy link

Description of changes:
Cognito allows you to configure a User Pool with a custom domain (which is usually the {auth subdomain}.{your domain} of the SPA). This change gives the option for consumers of the application to specify their own domain, and have the Lambda@Edge functions use that auth domain (for redirects, etc.) instead of the default Cognito provided domain.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@opaquejacob
Copy link
Author

opaquejacob commented Mar 13, 2020

Please note this does not add the custom auth domain config to the User Pool for the consumer. A valid custom domain config needs a valid AWS managed certificate that covers the custom auth domain. The config can be done in the AWS console, CLI, or with a CloudFormation template.

@ottokruse
Copy link
Collaborator

First off, thank you for your work!

I have been reviewing this PR, and I like how clean it is.

Going through it, here's a couple of review remarks:

  • I can imagine users also expecting the parameter "CustomAuthDomain" to entail that the user pool would be created with that domain name. The description clears this up, but "ExistingAuthDomainName" (or so) would I think be a better name for the parameter.
  • The Custom::UserPoolDomain resource should become conditional now right?
  • It would be great if we could also have an example on the use of the new "CustomAuthDomain" in the folder "example-serverless-app-reuse"

But right now, I'm doubting if we should actually abandon this approach ... (sorry) ... and go for an alternate approach only: make it possible to use an existing user pool (and domain and client) (there is an enhancement request for this already: #19 ). That would solve for even more usage scenario's:

  • Have the User Pool be in another region (often requested)
  • Ability to customize the user pool (e.g. attributes, MFA, advanced security, domain).

Also, that would keep the serverless application more simple. You added a parameter today, but it is conceivable that there will be more PR's for other similar customizations of the user pool, domain and/or client. It will become many parameters, and then it would surely be better to just let users setup their own user pool just the way they want it, and use that.

So TBH I am leaning towards implementing #19 instead of going forward with this PR.

I would appreciate hearing your thoughts on this.

@opaquejacob
Copy link
Author

Sorry for the delay.

I agree with all of your first set of points. And no worries if you want to abandon this approach. My opinion on #19 is the difficulty it would be to ensure the consumer configures the user pool correctly for use in the edge Lambdas. I believe in giving the consumer a very specific set of parameters that can be customized, and reserving the rest to guarantee a correct configuration. So I am in favor of future PR's for similar customizations as they are needed.

@ottokruse
Copy link
Collaborator

the difficulty it would be to ensure the consumer configures the user pool correctly for use in the edge Lambdas

Can you elaborate on that? What difficulties do you perceive? The User Pool client with the callback URL's and such?

It may be an option for #19 to create the client in our code anyway.

@ottokruse
Copy link
Collaborator

Closing in favor of #75

Also: CloudFormation now supports natively the creation of User Pool Domain and User Pool Client with OAuth config.

@ottokruse ottokruse closed this Aug 20, 2020
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.

2 participants