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

Deduplicate GetId and GetCredentialsForIdentity made by fetchAuthSession #13499

Open
1 of 2 tasks
OrmEmbaar opened this issue Jun 15, 2024 · 13 comments
Open
1 of 2 tasks
Labels
Auth Related to Auth components/category feature-request Request a new feature

Comments

@OrmEmbaar
Copy link

OrmEmbaar commented Jun 15, 2024

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?

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change
@OrmEmbaar OrmEmbaar added the pending-triage Issue is pending triage label Jun 15, 2024
@cwomack
Copy link
Member

cwomack commented Jun 17, 2024

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.

@cwomack cwomack added feature-request Request a new feature Auth Related to Auth components/category and removed pending-triage Issue is pending triage labels Jun 17, 2024
@mkolbusz
Copy link

@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
Copy link
Author

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

export const deDupeAsyncFunction = <A extends any[], R>(

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:

export const getId = composeServiceApi(

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

@OrmEmbaar OrmEmbaar changed the title Deduplicate in-flight fetchAuthSession token refresh requests Deduplicate fetchAuthSession credential requests Jun 18, 2024
@jimblanc
Copy link
Contributor

jimblanc commented Jun 18, 2024

@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
Copy link

@jimblanc issue created: #13509

@HuiSF HuiSF changed the title Deduplicate fetchAuthSession credential requests Deduplicate GetId and GetCredentialsForIdentity made by fetchAuthSession Jun 26, 2024
@OrmEmbaar
Copy link
Author

A workaround

import { AuthSession, fetchAuthSession } from 'aws-amplify/auth';

/**
 * This function creates a singleton that fetches the current session. This is necessary
 * because the Amplify fetchAuthSession method does not de-duplicate credential requests to
 * the Cognito server. That means that duplicate requests will be made to the server if
 * multiple components call fetchAuthSession concurrently before the internal credential
 * cache has been populated. This singleton ensures that only one request is made at a time.
 *
 * A feature request has been made to the Amplify team to add de-duplication
 * to the fetchAuthSession method. If that feature is added, this layer of
 * abstraction can be removed.
 *
 * @see https://github.com/aws-amplify/amplify-js/issues/13499
 */
function createFetchAuthSessionDedupeSingleton() {
  let pendingRequest: Promise<AuthSession> | null = null;

  return () => {
    if (!pendingRequest) {
      pendingRequest = new Promise(async (resolve, reject) => {
        try {
          const response = await fetchAuthSession();
          resolve(response);
        } catch (error) {
          reject(error);
        } finally {
          pendingRequest = null;
        }
      });
    }

    return pendingRequest;
  };
}

export const fetchAuthSessionDedupe = createFetchAuthSessionDedupeSingleton();

@didemkkaslan
Copy link

@OrmEmbaar how about the serverside fetchAuthSession?
import { fetchAuthSession } from 'aws-amplify/auth/server'
Did you do the same deduplication for it too?

@github-actions github-actions bot added the pending-maintainer-response Issue is pending a response from the Amplify team. label Oct 22, 2024
@OrmEmbaar
Copy link
Author

@didemkkaslan No, I was concerned that a singleton on the server would leak credentials across requests. Also, I don't have any concurrent authentication requests happening on the server during page load, so it wasn't necessary.

@didemkkaslan
Copy link

Got it thanks @OrmEmbaar, do you think this will also fix the tokens getting multiplied after session tokens expired problem mkolbusz mentions.

#13499 (comment)

@OrmEmbaar
Copy link
Author

OrmEmbaar commented Oct 22, 2024

@didemkkaslan That issue should have been resolved in a previous update #13509 (comment). Are you on the latest version?

@didemkkaslan
Copy link

Yes I'am but this still is the case for me.

  "aws-amplify": "^6.6.4",
 "@aws-amplify/adapter-nextjs": "^1.2.21",
Screenshot 2024-10-22 at 16 07 31

Maybe its because of my custom cookie storage can't really find the problem btw env variable is prod so its {} for the cookie options. Tokens get multiplied for the same path /

const cookieOptions: OptionsType =
  process.env.NEXT_PUBLIC_ENV === 'msteams'
    ? {
        domain: 'tab.app.spiky.ai' as string,
        sameSite: 'none' as 'lax' | 'strict' | 'none',
        secure: true,
      }
    : {};

const keyValueStorage = createKeyValueStorageFromCookieStorageAdapter({
  get(name) {
    const value = getCookie(name, cookieOptions);
    return { name, value };
  },
  getAll() {
    const cookies = getCookies(cookieOptions);
    return Object.keys(cookies).map((name) => ({ name, value: cookies[name] }));
  },
  set(name, value) {
    setCookie(name, value, cookieOptions);
  },
  delete(name) {
    deleteCookie(name, cookieOptions);
  },
});

@OrmEmbaar
Copy link
Author

@didemkkaslan From the cookie names it looks like you have two sets of cookies from two different users, which is different to the issue reported by mkolbusz above. Is it causing issues in your application? Perhaps your storage adapter is not clearing them properly.

@HuiSF
Copy link
Member

HuiSF commented Oct 22, 2024

Hi @didemkkaslan, echo to what @OrmEmbaar it seems you had an end user signed in before and didn't sign out before signing in a different user. It's unrelated to the issue described in the OP. And it's an different issue from #13509 that has been fixed.

@github-actions github-actions bot removed the pending-maintainer-response Issue is pending a response from the Amplify team. label Oct 22, 2024
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

No branches or pull requests

6 participants