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(auth): Add identityPoolEndpoint config to allow using a custom CognitoIdentity endpoint #13552

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

whummer
Copy link

@whummer whummer commented Jun 30, 2024

Add identityPoolEndpoint config to allow using a custom CognitoIdentity endpoint.

Motivation

We already have the ability to specify a custom endpoint for CognitoIdentityProvider, which was added by @jimblanc in this PR: #12236 (and then later renamed to userPoolEndpoint in this PR)

In addition to specifying a custom endpoint for user pools (CognitoIdentityProvider), it would be awesome if we can do the same thing for identity pools (CognitoIdentity) as well.

Description of changes

This PR introduces a userPoolEndpoint field for the Amplify configuration. The field is optional, and if specified, then the endpointResolver in cognitoIdentity/base.ts uses its value as the endpoint for the CognitoIdentity SDK client.

The implementation of the new endpointResolver function is analogous to the existing function for CognitoIdentityProvider here:

const endpointResolver = ({ region }: EndpointResolverOptions) => {
const authConfig = Amplify.getConfig().Auth?.Cognito;
const customURL = authConfig?.userPoolEndpoint;
const defaultURL = new AmplifyUrl(
`https://${SERVICE_NAME}.${region}.${getDnsSuffix(region)}`,
);
return {
url: customURL ? new AmplifyUrl(customURL) : defaultURL,
};
};

Description of how you validated changes

Local testing of various Amplify sample apps (including the Lambda Powertools workshop repo /cc @heitorlessa ).

Checklist

  • PR description included
  • yarn test passes
  • Unit Tests are changed or added. <-- would be grateful for some inputs here 🙌 - are tests required for this change, and what would be a good place to add them?
  • Relevant documentation is changed or added (and PR referenced) <-- Would be happy for some pointers here as well - tried generating the API docs via yarn docs, but that seems to have generated lots of changes that are unrelated to this PR. 🤔

Checklist for repo maintainers

  • Verify E2E tests for existing workflows are working as expected or add E2E tests for newly added workflows
  • New source file paths included in this PR have been added to CODEOWNERS, if appropriate

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@whummer whummer force-pushed the identity-endpoint branch 2 times, most recently from 3016c58 to 83b58dc Compare June 30, 2024 15:23
@whummer whummer changed the title Add identityPoolEndpoint config to allow using a custom Cognito-Identity endpoint feat(auth): Add identityPoolEndpoint config to allow using a custom Cognito-Identity endpointL Jun 30, 2024
@whummer whummer marked this pull request as ready for review June 30, 2024 15:41
@whummer whummer changed the title feat(auth): Add identityPoolEndpoint config to allow using a custom Cognito-Identity endpointL feat(auth): Add identityPoolEndpoint config to allow using a custom CognitoIdentity endpoint Jun 30, 2024
@whummer
Copy link
Author

whummer commented Jul 5, 2024

Hi folks @ukhan-amazon @haverchuck @cshfang @jimblanc @HuiSF, just wanted to briefly follow up on this - any feedback on this PR? Please let me know if you have any suggestion regarding implementation, testing, etc.

Happy to iterate on the implementation, but would be awesome to get this functionality in, to allow more flexible configuration and easier local testing. Looking forward to getting your feedback! 🙌 Thank you

@Neuroforge
Copy link

Neuroforge commented Jul 31, 2024

This PR is really needed for anyone who wants to use AWS amplify js with Localstack.

@whummer Are you able to fix the branch to be up to date with the base branch?

@ukhan-amazon @haverchuck @cshfang @jimblanc @HuiSF The changes are not substantial, but creates the exact fix needed that is stopping anyone using this with LocalStack. It seems off that a custom endpoint can be set for everything except the identity pool.

This PR is a game changer when it comes to using the awesomeness of AWS Amplify and the amazingness of LocalStack.

@dmytzo
Copy link

dmytzo commented Jul 31, 2024

hi @ukhan-amazon @haverchuck @cshfang @jimblanc @HuiSF

any updates on this PR?

@whummer
Copy link
Author

whummer commented Jul 31, 2024

Thanks for the feedback and chiming in here @Neuroforge @dmytzo - updated the branch to be up to date with the base branch. 👍 Hoping to get some feedback from the maintainers soon 🙌

@HuiSF
Copy link
Member

HuiSF commented Aug 1, 2024

Hi all! Thank you very much for the contribution from LocalStack! My apologies for the delayed attention.

We are currently testing the code change. We noticed that the existing user pool endpoint resolver implementation (which the code change in this PR referred to) does not work on the server side of a SSR-enabled app. We are exploring a solution to fix it and will suggest corresponding changes here.

Thanks again for the contribution, and your patience!

@whummer
Copy link
Author

whummer commented Aug 15, 2024

Hi @HuiSF , thanks for getting back to us and providing some context. Just wanted to briefly check in - is there anything we can do to help here?

I understand that you are running some more tests for SSR-enabled apps. Could it be an option to get these changes in now, and then solve the SSR issue holistically for CognitoIdentity and CognitoIdentityProvider in a future iteration?

I'm just thinking of ways to unblock us here - our users would love to see this feature and test their Amplify apps with LocalStack! 🙌 Thanks a bunch for your help - please let us know how we can help!

@Neuroforge
Copy link

I'm willing to give some time to help this if there's anything i can do to unblock this.

@cwomack cwomack added feature-request Request a new feature Auth Related to Auth components/category labels Aug 20, 2024
@HuiSF
Copy link
Member

HuiSF commented Aug 21, 2024

Hi @whummer I opened the PR for fixing the userPoolEndpoint cannot be applied on the server side, and it's currently under reviews. Sorry about the delay, it turned out to be a quite substantial restructuring.

In the meantime, I hope to learn more about the workflows of using Amplify with LocalStack so we can review the details internally. Could you clarify the following?

  1. The endpoint of the local service emulator managed by LocalStack is different from the Amazon service endpoint, correct?
  2. What's the workflow for a developer who is using Amplify with LocalStack when they need to deploy their project to a production environment? (A flowchart would be very helpful.)

@HuiSF
Copy link
Member

HuiSF commented Sep 12, 2024

Hi all, the fix PR around the custom user pool endpoint has merged. We've reviewed this proposal internally and we can proceed. Do you want to keep working on this PR with the following?

  1. Update the implementation to adapt the pattern we introduced in fix(auth): custom userPoolEndpoint cannot be applied on the server-side #13739.
  2. Add corresponding unit tests.

If you are feeling it would be overwhelming, please let us know. We are happy to update this PR and get it merged. Thanks again for your patience.

@whummer
Copy link
Author

whummer commented Sep 14, 2024

Hi @HuiSF, thanks so much for the update, this is great progress! 🚀 If we could get some help from you to get this PR over the line, that would actually be fantastic 🙌 - please feel free to push to this branch directly (should technically work, but please let us know if there are permission issues, happy to invite you to the forked repo). Thanks a ton for your help on this! 🙏

@Neuroforge
Copy link

@whummer do you think we can get an localstack docs/article/example on how to set this up properly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auth Related to Auth components/category feature-request Request a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants