amplify-js icon indicating copy to clipboard operation
amplify-js copied to clipboard

feat(auth): Add identityPoolEndpoint config to allow using a custom CognitoIdentity endpoint

Open whummer opened this issue 1 year ago • 13 comments
trafficstars

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: https://github.com/aws-amplify/amplify-js/pull/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: https://github.com/aws-amplify/amplify-js/blob/b6de5f9127f5382bb880becbcbc29748d8cf9d60/packages/auth/src/providers/cognito/utils/clients/CognitoIdentityProvider/base.ts#L32-L42

Description of how you validated changes

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

Checklist

  • [x] PR description included
  • [x] 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 avatar Jun 30 '24 15:06 whummer

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

whummer avatar Jul 05 '24 14:07 whummer

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.

Neuroforge avatar Jul 31 '24 03:07 Neuroforge

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

any updates on this PR?

dmytzo avatar Jul 31 '24 21:07 dmytzo

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 🙌

whummer avatar Jul 31 '24 23:07 whummer

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!

HuiSF avatar Aug 01 '24 01:08 HuiSF

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!

whummer avatar Aug 15 '24 14:08 whummer

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

Neuroforge avatar Aug 17 '24 05:08 Neuroforge

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 avatar Aug 21 '24 16:08 HuiSF

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 https://github.com/aws-amplify/amplify-js/pull/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.

HuiSF avatar Sep 12 '24 16:09 HuiSF

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! 🙏

whummer avatar Sep 14 '24 08:09 whummer

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

Neuroforge avatar Sep 14 '24 08:09 Neuroforge

Any news on this one?

Neuroforge avatar Oct 02 '24 01:10 Neuroforge

Hello, any update on this?

Rohan0135 avatar Oct 14 '24 21:10 Rohan0135

What's happening with this? Last update was in September.

Neuroforge avatar Oct 22 '24 00:10 Neuroforge

Hi all, sorry about the delay. I've made a tag (local-stack) release that includes the changes in this PR. LocalStack folks do you mind to install this tag and test the custom endpoints on your side, and let us know if you encounter and issues? You can gain the package by running the following

npm install aws-amplify@local-stack

HuiSF avatar Oct 22 '24 15:10 HuiSF

Thanks @HuiSF , appreciate the update! 🚀 We'll look into this, and report back here asap. Just to confirm - is the required logic already contained in the local-stack, in other words is - this PR obsolete now, or would we still need the changes in this PR? Thanks for your help!

whummer avatar Oct 24 '24 11:10 whummer

Hi @whummer the tag release for testing is based on this PR, we will get this PR merged if all the tests are going well for a formal release. Thanks for your patience!

HuiSF avatar Oct 24 '24 16:10 HuiSF

Was able to perform a login to LocalStack with AmplifyJS V6 by setting DISABLE_CORS_CHECKS=1 when starting LocalStack.

So, i've had a crack at this...

First step was to upgrade to AmplifyJs V6. So it's very likely that my config is wrong. But i have my cdk stack deployed to LocalStack and the services appear to be running.

export const getAmplifyConfiguration = () => {
  return {
    API: {
      REST: {
        api: {
          authMode: "userPool",
          endpoint: "http://localhost:4566/restapis/bloop/dev",
          name: "api",
        },
      },
    },
    Auth: {
      Cognito: {
        identityPoolEndpoint: "http://localhost:4566",
        identityPoolId: "ap-southeast-floop",
        userPoolClientId: "userPoolClientId",
        userPoolEndpoint: "http://localhost:4566/auth",
        userPoolId: "userPoolId",
      },
    },
    Storage: {
      S3: {
        bucket: "storagestack-dev",
        endpoint: "http://localhost:4566",
        region: "ap-southeast-2",
        s3ForcePathStyle: true, // Required for LocalStack
      },
      customPrefix: {
        private: "",
        protected: "",
        public: "",
      },
    },
  };
};

Neuroforge avatar Oct 27 '24 15:10 Neuroforge

Looks like we have another one that could make this tricky. S3 has a similar issue.

https://github.com/aws-amplify/amplify-js/issues/13085

Neuroforge avatar Oct 27 '24 16:10 Neuroforge

Hi @whummer what's the possible urls that LocalStack can generate? Is http://localhost:<port> the only format/pattern?

HuiSF avatar Oct 29 '24 18:10 HuiSF

Any updates on getting this merged?

Neuroforge avatar Dec 18 '24 23:12 Neuroforge

Hi, just wondering what the ETA is for this one, looks like just some small code review comments need addressed?

theflyingcodr avatar Feb 10 '25 11:02 theflyingcodr

This PR should fulfill https://github.com/aws-amplify/amplify-js/issues/13445

HuiSF avatar Apr 03 '25 21:04 HuiSF

Hi folks, really sorry for the delay here. Thanks for all the feedback, and for supporting to get this change over the line. 🙌

I have now rebased the PR onto latest main, and resolved the merge conflicts.

@HuiSF I've noticed that you went ahead and added a couple more changes and tests in the identity-endpoint branch in this repo: https://github.com/aws-amplify/amplify-js/compare/main...identity-endpoint 🚀 Do you want me to cherry-pick your changes and update this PR, or do you want to take over and push this over the line in a separate PR, based on your version of the identity-endpoint branch? (either way is fine for me.. 👍 )

Btw, regarding this question:

Hi @whummer what's the possible urls that LocalStack can generate? Is http://localhost:<port> the only format/pattern?

There's actually different patterns for URLs that LocalStack can generate. First of all, it could be either http:// or https:// protocol URLs. Also, the target host could be localhost:<port> or localhost.localstack.cloud:<port>, or in fact also other custom hostnames (depending on how users are deploying LocalStack, e.g., within Docker, or Kubernetes). Can you please share some context - do we need to define the pattern for allowed custom endpoints, or could we simply allow users to set any arbitrary custom domain?

Thanks a lot for your help!

whummer avatar Apr 05 '25 12:04 whummer

Not to be rude, but will it be merged soon? 🙏

IdoBloch4 avatar Apr 09 '25 19:04 IdoBloch4

@whummer @IdoBloch4 yes. we are actively working on this. Appreciate your patience here 🙏

ashika112 avatar Apr 10 '25 06:04 ashika112

Thanks for the follow up @whummer for clarifying the url that localstack generates. I will follow up on this.

Can you please share some context - do we need to define the pattern for allowed custom endpoints, or could we simply allow users to set any arbitrary custom domain?

We've received a requirement internally that we would need to validate the custom endpoint at some level to prevent the consumer doing something wrong accidentally. Since you are saying with LocalStack the endpoint can be with any random domain, I think we would need to remove the restriction.

But hey why would you force push to this feature branch 😅 it removed all my changes for properly supporting this feature, the diff you linked above seems incomplete and missing a few commits. Could you confirm are you still going to work on this feature, or Amplify team should take the PR over?

HuiSF avatar Apr 11 '25 17:04 HuiSF

Thanks @HuiSF for the update!

We've received a requirement internally that we would need to validate the custom endpoint at some level to prevent the consumer doing something wrong accidentally. Since you are saying with LocalStack the endpoint can be with any random domain, I think we would need to remove the restriction.

Thanks for clarifying. I think it would be awesome if we could remove that requirement, allowing users to configure arbitrary endpoint URLs! 🙌

But hey why would you force push to this feature branch 😅 it removed all my changes for properly supporting this feature, the diff you linked above seems incomplete and missing a few commits. Could you confirm are you still going to work on this feature, or Amplify team should take the PR over?

Really sorry about that - please feel free to take over from here, I will refrain from making any changes to the branch from here onwards (unless you ask me to). 🙏 Please let me know if I can help! 👍

whummer avatar Apr 13 '25 11:04 whummer

Hi @HuiSF , 👋 wanted to briefly follow up on this PR - do you have any updates for us, anything we can help out with? Do you want me to restore the previous state of the PR (before I accidentally force-pushed it), or are you able to restore it on your end..? 🙌

whummer avatar May 09 '25 19:05 whummer

Hi @whummer sorry for the delay again, needed to spend time on something else.

I will open a new PR for merging this feature.

HuiSF avatar May 12 '25 17:05 HuiSF