aws-amplify / amplify-js

A declarative JavaScript library for application development using cloud services.
https://docs.amplify.aws/lib/q/platform/js
Apache License 2.0
9.39k stars 2.11k forks source link

Deduplicate GetId and GetCredentialsForIdentity made by fetchAuthSession #13499

Open OrmEmbaar opened 1 week ago

OrmEmbaar commented 1 week ago

Is this related to a new or existing framework?

No response

Is this related to a new or existing API?

Authentication, PubSub

Is this related to another service?

Cognito

Describe the feature you'd like to request

The fetchAuthSession singleton should deduplicate requests to fetch credentials from the server.

We are in the process of upgrading from aws-amplify v4 to v6. On initial page load, we fetch data from our API using signed Authorization headers and set-up subscriptions using the Amplify PubSub library. For requests to our own server we are calling fetchAuthSession manually to get the credentials to create the signature. For subscriptions, fetchAuthSession is being called internally by the Amplify PubSub library.

As this all happens concurrently on initial page load, the fetchAuthSession singleton has not yet populated its internal cache with any credentials. We are therefore hitting the Cognito server 20 to 30 times on page load, which is unnecessary network load and causes us to be rate limited by the Cognito server (this pattern was working fine in v4).

We therefore would like to see the fetchAuthSession singleton de-duplicate concurrent in-flight requests to fetch credentials from the Cognito server.

Describe the solution you'd like

The aws-amplify library should await any in-flight requests to the Cognito server instead of making duplicate concurrent requests.

Describe alternatives you've considered

Our present solution is to wrap fetchAuthSession inside a useQuery hook from the ReactQuery library. ReactQuery will de-duplicate requests for us, so we can pass the returned refetch method around our application without worrying about sending redundant, duplicate requests. A similar approach could possibly be achieved by wrapping it in a debounce.

To make that work for the PubSub library, we have been forced to extend the PubSub class and inject a custom endpoint method that uses our de-duplicated fetchAuthSession method.

This is all messy. It adds a lot of code and boilerplate which shouldn't be necessary.

Additional context

No response

Is this something that you'd be interested in working on?

cwomack commented 1 week ago

Hello, @OrmEmbaar and thanks for creating this feature request. I'll review it with the team internally and follow up with any additional questions we have.

mkolbusz commented 1 week ago

@cwomack I think I have encountered the same problem while using Tanstack Query with NextJs. After session tokens have expired and Tanstack Query is trying to refetch the data the server multiplies the cookies and tokens as presented below: Selection_083

Minimum repository with reproduction: https://github.com/mkolbusz/nextjs-amplify-v6-issues

Steps to reproduce:

  1. Login.
  2. Go to the other tab in the browser.
  3. Wait for the session to expire.
  4. Enter the tab of the application (refetching data and refreshing the session at the same time).
  5. The auth cookies are multiplicated.

This problem does not allow to sign out properly.

OrmEmbaar commented 1 week ago

It looks like the library already has tools needed to make this work. This deDupeAsyncFunction function is used to wrap the refreshToken method.

https://github.com/aws-amplify/amplify-js/blob/de6b06db80b02687e597aceaa2c33f65171a6389/packages/core/src/utils/deDupeAsyncFunction.ts#L10

But that is only called when refreshing tokens, not when fetching credentials. To de-duplicate initial credential fetch, the getId function would also need to be wrapped by deDupeAsyncFunction:

https://github.com/aws-amplify/amplify-js/blob/de6b06db80b02687e597aceaa2c33f65171a6389/packages/core/src/awsClients/cognitoIdentity/getId.ts#L56

It seems like an easy change. Can we make this happen?

jimblanc commented 1 week ago

@mkolbusz This appears to be a bug on the Amplify side unrelated to deduping due to setting different "path" values on refresh, we're working on a fix. In the mean time, would you mind creating a new ticket to track the issue?

mkolbusz commented 1 week ago

@jimblanc issue created: https://github.com/aws-amplify/amplify-js/issues/13509