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.41k stars 2.12k forks source link

TypeError: Cannot read properties of undefined (reading 'length') for getting the authorized user #11106

Closed asp3 closed 3 months ago

asp3 commented 1 year ago

Before opening, please confirm:

JavaScript Framework

React

Amplify APIs

Authentication

Amplify Categories

auth

Environment information

    "aws-amplify": "^5.0.18",

Describe the bug

This happens sproadically sometimes, but is heavily reproducable when reloading the page very quickly in a short time span (maybe 5-10 times within a few seconds). Then, we get the errors listed below, in currentUserPoolUser

Screenshot 2023-03-20 at 10 21 25 AM Screenshot 2023-03-20 at 10 19 55 AM

Expected behavior

The user is still logged in, or I would assume there is a concise error if they have been logged out.

Reproduction steps

This happens sproadically sometimes, but is heavily reproducable when reloading the page very quickly in a short time span (maybe 5-10 times within a few seconds).

Code Snippet

e.prototype.currentUserPoolUser = function(e) {
                var t = this;
                return this.userPool ? new Promise(function(n, r) {
                    t._storageSync.then(function() {
                        return h(t, void 0, void 0, function() {
                            var t, i, o, a, s, u, c, l = this;
                            return m(this, function(u) {
                                switch (u.label) {
                                case 0:
                                    if (!this.isOAuthInProgress())
                                        return [3, 2];
                                    return eY.debug("OAuth signIn in progress, waiting for resolution..."),
                                    [4, new Promise(function(e) {
                                        var t = setTimeout(function() {
                                            eY.debug("OAuth signIn in progress timeout"),
                                            w.X.remove("auth", n),
                                            e()
                                        }, 1e4);
                                        function n(r) {
                                            var i = r.payload.event;
                                            ("cognitoHostedUI" === i || "cognitoHostedUI_failure" === i) && (eY.debug("OAuth signIn resolved: " + i),
                                            clearTimeout(t),
                                            w.X.remove("auth", n),
                                            e())
                                        }
                                        w.X.listen("auth", n)
                                    }
                                    )];
                                case 1:
                                    u.sent(),
                                    u.label = 2;
                                case 2:
                                    if (!(t = this.userPool.getCurrentUser()))
                                        return eY.debug("Failed to get user from user pool"),
                                        r("No current user"),
                                        [2];
                                    u.label = 3;
                                case 3:
                                    return u.trys.push([3, 7, , 8]),
                                    [4, this._userSession(t)];
                                case 4:
                                    if (i = u.sent(),
                                    !(o = !!e && e.bypassCache))
                                        return [3, 6];
                                    return [4, this.Credentials.clear()];
                                case 5:
                                    u.sent(),
                                    u.label = 6;
                                case 6:
                                    if (a = this._config.clientMetadata,
                                    !(void 0 === (s = i.getAccessToken().decodePayload().scope) ? "" : s).split(" ").includes(eW))
                                        return eY.debug("Unable to get the user data because the " + eW + " is not in the scopes of the access token"),
                                        [2, n(t)];
                                    return t.getUserData(function(e, i) {
                                        return h(l, void 0, void 0, function() {
                                            var o, a, s, u, c, l, d;
                                            return m(this, function(d) {
                                                switch (d.label) {
                                                case 0:
                                                    if (!e)
                                                        return [3, 7];
                                                    if (eY.debug("getting user data failed", e),
                                                    !this.isSessionInvalid(e))
                                                        return [3, 5];
                                                    d.label = 1;
                                                case 1:
                                                    return d.trys.push([1, 3, , 4]),
                                                    [4, this.cleanUpInvalidSession(t)];
                                                case 2:
                                                    return d.sent(),
                                                    [3, 4];
                                                case 3:
                                                    return o = d.sent(),
                                                    r(Error("Session is invalid due to: " + e.message + " and failed to clean up invalid session: " + o.message)),
                                                    [2];
                                                case 4:
                                                    return r(e),
                                                    [3, 6];
                                                case 5:
                                                    n(t),
                                                    d.label = 6;
                                                case 6:
                                                    return [2];
                                                case 7:
                                                    for (u = 0,
                                                    a = i.PreferredMfaSetting || "NOMFA",
                                                    s = []; u < i.UserAttributes.length; u++) // Error here

Log output

``` // Put your logs below this line ```

aws-exports.js

No response

Manual configuration

No response

Additional configuration

No response

Mobile Device

No response

Mobile Operating System

No response

Mobile Browser

No response

Mobile Browser Version

No response

Additional information and screenshots

No response

cwomack commented 1 year ago

Hello, @asp3 đź‘‹ and thanks for opening this issue. I've been able to reproduce the error reliably tied to calling Auth.currentUserPoolUser(), but have a few questions to clarify what's happening to cause the page to reload so many times and result in the error you're seeing.

Are you able to share what the frontend code looks like that is making these calls? If this only happens sporadically and is reproducible only when a multitude of page reloads are forced, it's possible that if there's lots of Auth methods being called consecutively on each page AND multiple refreshes calling them... then you might be getting throttled based on the Cognito User Pool request rate quotes. This could explain why the UserAttributes.length could be coming back as null or undefined if the API call is being throttled.

This can be confirmed (or ruled out) it out by looking into your Amazon CloudWatch metrics for the API calls and looking for ThrottleCount. First you'd want to view available metrics in CloudWatch and then search for the proper "namespace" as described here.

Any additional context or code you can share on what's making these calls on the frontend would be appreciated!

charles01pd2018 commented 1 year ago

Hey @cwomack - thanks for the quick reply on this issue! I work with @asp3 and I can confirm that I see this issue in my dev environment. Can't give too much context for code snippets since I don't work too heavily with the Auth side of our codebase - however I seem to get this bug even without performing multiple fast refreshes so not sure if it's 100% to do with Cognito User Pool request rate quotes.

Seems to happen to me every so often when I switch accounts and then restart my dev server. Not sure why - but our call to getCurrentUser returns an undefined user once the dev server is rebooted.

To bypass - I manually reset the application cookies to logout and re-login. Until I do that, I am completely blocked out of using the application.

I'll provide more context if I'm able to pinpoint what's going on!

EDIT: The switch involves going from a Google account to a regular sign-in account (username and password). Browser is Google Chrome.

asp3 commented 1 year ago

bump on this issue, @cwomack since we are seeing it happen fairly often in our prod environment. I think it makes sense that it has something to do with the rates, but theres a very low chance we're hitting those limits. Let me know what thoughts/ideas you have for this issue, thanks in advance!

The code that we're calling is just Auth.currentAuthenticatedUser, which seems to lead to this. we pass in {bypassCache: true} as the parameters each time - maybe that could be the reason?

In Cloudwatch metrics, I only see categories for successes, which I'm assuming means that we dont have the throttle events.

Screenshot 2023-03-27 at 10 53 42 AM
cwomack commented 1 year ago

@asp3 and @charles01pd2018, appreciate the responses and additional clarity! Thanks for checking the CloudWatch metrics to rule out the quota limit possibility.

The UserId returned from Cognito will be different when going from a Google Federated Sign in to the username/password flow. Are you able to check what UserId is associated with the Auth.currentAuthenticatedUser call when the TypeError is thrown? It sounds like this Cognito possibly has a mismatch between what the session information for the current logged in user is, and what it expected. Got a few additional questions:

asp3 commented 1 year ago

@cwomack

We will test with bypass cache false, as our use case doesnt really need the most up to date data, just checking if the user is authenticated or not, and to refresh the session.

const { data, error, mutate } = useSWR("getCurrentUser", fetchUserInfo);
export const refreshSession = async () => {
    const cachedUser = await LocalKeyValueStore.get(CACHED_USER_KEY);
    try {
        await Auth.currentSession();
        return JSON.parse(cachedUser);
    } catch (e) {
        if (cachedUser) {
            await signOut();
            toast.error("Sorry! For security reasons, you must log back in!");
            reportAndThrow("refreshSession", { user: cachedUser })(e);
        }
        return null;
    }
};
export const fetchUserInfo = async (): Promise<LocalUser> => {
    try {
        const cachedUser = await refreshSession();
        if (cachedUser) {
            fetchServerUserInfo().catch();
            return cachedUser;
        }
    } catch {
        return { user: null };
    }

    const user = await fetchServerUserInfo();

    if (!user) {
        return {
            user: null,
        };
    }
export const fetchServerUserInfo = async (): Promise<LocalUser> => {
    try {
        const userInfo = await Auth.currentAuthenticatedUser({ bypassCache: false });
        // TODO: what should we return? cuz we're using the return value in `fetchUserInfo`
        if (!userInfo) return;

Its a pretty simple setup, but the getCurrentUser swr can get updated from a lot of places, so it could be that we are making lots of calls to the currentAuthenticatedUser, and since we pass bypassCache: true, maybe there were some issues there. We are testing with that field as false and will get back to you with the info we find!

asp3 commented 1 year ago

@cwomack @stocaaro I've done some more debugging in our code, and I noticed something interesting.

import { AUTH_TYPE, AWSAppSyncClient } from "aws-appsync";

const _client = new AWSAppSyncClient({
    url: awsConfig.aws_appsync_graphqlEndpoint,
    region: awsConfig.aws_appsync_region,
    disableOffline: true,
    auth: {
        type: AUTH_TYPE.AMAZON_COGNITO_USER_POOLS,
        jwtToken: async () => {
            console.log("FETCHING USER JWT");
            return (await Auth.currentSession()).getIdToken().getJwtToken();
        },
    },
});

const _noAuthClient = new AWSAppSyncClient({
    url: awsConfig.aws_appsync_graphqlEndpoint,
    region: awsConfig.aws_appsync_region,
    disableOffline: true,
    auth: {
        type: AUTH_TYPE.AWS_IAM,
        credentials: async () => {
            console.log("FETCHING USER CRED");
            return await Auth.currentCredentials();
        },
    },
});

const makeClient = __client => ({
    query: <ResolveType>(...args) => {
        return new Promise<ResolveType>((resolve, reject) => {
            __client
                .hydrated()
                .then(() => {
                    if (DEBUG_MODE) {
                        const queryName = args[0].query.definitions[0].name.value;
                        console.log(
                            `[FETCHING] - [${queryName.toLowerCase().includes("list") ? "LIST" : "QUERY"}]`,
                            queryName,
                            args[0].variables
                        );
                    }
                    __client
                        .query(...args)
                        .then(resolve)
                        .catch(reject);
                })
                .catch(reject);
        });
    },
    mutate: <ResolveType>(arg1, ...args) => {
        if (DEBUG_MODE) {
            console.log("[MUTATING]", arg1.mutation.definitions[0].name.value, arg1.variables);
        }

        // Filter out the __typename attribute from the input to any mutations
        const input = arg1?.variables?.input;
        if (input) {
            arg1 = {
                ...arg1,
                variables: {
                    ...arg1.variables,
                    input: deepClearTypenames(input),
                },
            };
        }
        return new Promise<ResolveType>((resolve, reject) => {
            __client
                .hydrated()
                .then(() => {
                    __client
                        .mutate(arg1, ...args)
                        .then(resolve)
                        .catch(reject);
                })
                .catch(reject);
        });
    },
});

export const client = makeClient(_client);
export const noAuthClient = makeClient(_noAuthClient);
Screenshot 2023-04-14 at 11 56 48 AM

It seems that before our graphql calls (which are made through noAuthClient.query or client.query. This seems like our setup is fundamentally incorrect, I was wondering if you guys have some pointers on how to make this more efficient.

Additionally, we seem to be seeing some memory leaks in our application as well, and I was wondering if the _client.hydrated could be leading to that (since we do that each time we query or mutate.

asp3 commented 1 year ago

Doing more research into it, it seems that the actual userData key has a value of {} in the storage, so the following lines don't work as expected.

getUserData(callback, params) {
        if (!(this.signInUserSession != null && this.signInUserSession.isValid())) {
            this.clearCachedUserData();
            return callback(new Error('User is not authenticated'), null);
        }

        const userData = this.getUserDataFromCache();

        if (!userData) {
            this.fetchUserData()
                .then(data => {
                    callback(null, data);
                })
                .catch(callback);
            return;
        }

Since userData is technically still present, the fetchUserData call is skipped, which seems to lead to this null error

@cwomack @stocaaro

asp3 commented 1 year ago

removed all calls of anything Auth. from a page, and the error still occurs. the SSR call return the correct user, and then something on client side (I assume the Amplify configure call?).

export const fetchUser = async (SSR) => {
    try {
        return (await SSR.Auth.currentAuthenticatedUser()).signInUserSession.idToken.payload
    } catch (e) {
        console.log("e", e);
        return null;
    }
};

const LandingPage = ({ serverUser }) => {
    return (
       <div />
    );
};

export const getServerSideProps = async context => {
    const SSR = withSSRContext(context);
    const user = await fetchUser(SSR);
    const { code } = context.query;

    return {
        props: {
                serverUser: user
            },
        },
};

still throws the current error. This is a pretty big bug from our end, as it happens to a good amount of our users, and since its not a catchable error (since its not anything we call), we are not able to properly fix it for the useres that run into it. @cwomack @stocaaro @nadetastic

Screenshot 2023-04-16 at 6 15 09 PM
asp3 commented 1 year ago

bump on this issue, still causing lots of user sign in errors (and its not a catchable error, as its not from any specific call that I can tell, since it seems to be coming from the init) @cwomack :)

sgnix commented 1 year ago

Bump. Running into the same issue in production.

cwomack commented 1 year ago

@asp3 and @sgnix, appreciate your patience on this issue. I'm currently reviewing this and some similar issues tied to the TypeError: Cannot read properties of undefined (reading 'length') with the team internally. I'll report back with an update soon!

asp3 commented 1 year ago

hi @cwomack, any update on this? still running into this same issue.

lukasburns commented 1 year ago

Hi, any update on this! Its happening as well for our users

gabrielmanara commented 1 year ago

Hi, it's happening often with our users too. Any updates @cwomack?

cwomack commented 1 year ago

Appreciate everyone's patience on this issue. It is still being reviewed by our team internally and we are determining the best path forward regarding this error. I'll update this issue as soon as I can!

cwomack commented 1 year ago

@asp3, @gabrielmanara, and @lukasburns, can any of you share a simple repo that can easily reproduce this issue? After further reviews and attempts at reproduction internally, we cannot get this error to be thrown on a basic User Pool configuration. To be clear, I mistakenly reproduced a different TypeError in my initial comment on this issue.

Based on the stack trace shared in this comment, it indeed appears to be coming from a call to Auth.currentAuthenticatedUser() but it's odd that it would be sporadic. What type of attribute settings do you have configured for you Cognito User Pool? And are there multiple sign in flows (i.e. standard sign-in, federated sign-in, etc.)?

florianjeanmart commented 11 months ago

Hello, I have the same issue from yesterday. Just after a reload of the page, I have the same error : image image For a unknown reason, the userData are empty : image

Every time I refresh the screen, I do a call to the currentAuthenticatedUser to check if the user is logged :

isLogged(): Observable<boolean> {
    return defer(() => Auth.currentAuthenticatedUser()
      .then(() => true)
      .catch((err: any) => {
        console.log(err);
        return false;
      }))
      .pipe(
        tap((logged) => this._loggedSbj.next(logged))
      );
  }

and the error is thrown.

This is the first time, so not sure I can reproduce this error.

micah-redwood commented 9 months ago

Ran into this today as well, similar stack trace as others, happened exactly 6 times after I'd reload the page, presumably trigged by the useAuthenticator hook?

Auth.ts:1742 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'length')
    at AuthClass2.<anonymous> (Auth.ts:1742:50)
    at step (tslib.es6.js:100:23)
    at Object.next (tslib.es6.js:81:53)
    at tslib.es6.js:74:71
    at new Promise (<anonymous>)
    at __awaiter40 (tslib.es6.js:70:12)
    at user.getUserData.bypassCache.bypassCache (Auth.ts:1719:25)
    at CognitoUser2.getUserData (CognitoUser.js:1001:7)
    at AuthClass2.<anonymous> (Auth.ts:1718:13)
    at step (tslib.es6.js:100:23)

We have both standard sign-in and federated sign-in via Okta (two different user pools). Had to clear LocalStorage to get out of this error state. Refreshing didn't fix it.

cwomack commented 4 months ago

@florianjeanmart and @micah-redwood, can you share any additional frontend code or reproduction steps for this?

cwomack commented 3 months ago

If anyone is still experiencing this issue in v5 on the latest version, please let us know. We'd recommend upgrading to the newest major version, v6 of Amplify, and seeing if this issue persists. We improved out TypeScript support and also updated the Auth API's to be functional API's with slightly different behavior (see here for more info).

micah-redwood commented 3 months ago

We've been using v6 in prod now for many months and haven't seen this show up in Sentry, so hopefully it's fixed.

That being said, we now intentionally lazy-load all @aws-amplify/ui-react components and no longer use useAuthenticator() to track the current user's state, so useAuthenticator() is only used on a tiny fraction of the pages that it used to be (due to performance concerns, both in terms of bundle size and re-renders).

If anyone else is interested in this approach, it involves calling fetchAuthSession() initially and then again whenever Hub fires an auth event. Has been working in production and resulted in massive bundle size savings and much fewer auth-related re-renders (since we expose a subset of user attributes and diff those before re-rendering).

cwomack commented 3 months ago

Thanks for the reply @micah-redwood. Upgrading to v6 of Amplify would be our recommendation for anyone still experiencing this or coming across this issue. Please feel free to reply back if there's further issues with this error or additional context that was missed.