search

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.44k stars 2.13k forks source link

Race condition in Amplify login flow #13566

Closed Lirontal1 closed 4 months ago

Lirontal1 commented 4 months ago

Before opening, please confirm:

JavaScript Framework

React

Amplify APIs

Authentication

Amplify Version

v6

Amplify Categories

auth

Backend

None

Environment information

``` # Put output below this line System: OS: macOS 14.4.1 CPU: (10) arm64 Apple M1 Max Memory: 75.66 MB / 32.00 GB Shell: 5.9 - /bin/zsh Binaries: Node: 21.7.2 - /opt/homebrew/bin/node npm: 10.5.0 - /opt/homebrew/bin/npm Browsers: Chrome: 126.0.6478.127 Safari: 17.4.1 npmPackages: @apollo/client: ^3.8.8 => 3.8.8 @apollo/client/cache: undefined () @apollo/client/core: undefined () @apollo/client/dev: undefined () @apollo/client/errors: undefined () @apollo/client/link/batch: undefined () @apollo/client/link/batch-http: undefined () @apollo/client/link/context: undefined () @apollo/client/link/core: undefined () @apollo/client/link/error: undefined () @apollo/client/link/http: undefined () @apollo/client/link/persisted-queries: undefined () @apollo/client/link/remove-typename: undefined () @apollo/client/link/retry: undefined () @apollo/client/link/schema: undefined () @apollo/client/link/subscriptions: undefined () @apollo/client/link/utils: undefined () @apollo/client/link/ws: undefined () @apollo/client/react: undefined () @apollo/client/react/components: undefined () @apollo/client/react/context: undefined () @apollo/client/react/hoc: undefined () @apollo/client/react/hooks: undefined () @apollo/client/react/parser: undefined () @apollo/client/react/ssr: undefined () @apollo/client/testing: undefined () @apollo/client/testing/core: undefined () @apollo/client/utilities: undefined () @apollo/client/utilities/globals: undefined () @babel/preset-react: ^7.23.3 => 7.23.3 @cypress/angular: 0.0.0-development @cypress/mount-utils: 0.0.0-development @cypress/react: 0.0.0-development @cypress/react18: 0.0.0-development @cypress/svelte: 0.0.0-development @cypress/vue: 0.0.0-development @cypress/vue2: 0.0.0-development @datadog/browser-rum: ^5.4.0 => 5.6.0 @datadog/datadog-ci: ^2.27.0 => 2.36.0 @emotion/react: ^11.11.1 => 11.11.3 @emotion/styled: ^11.11.0 => 11.11.0 @ianvs/prettier-plugin-sort-imports: ^4.1.1 => 4.1.1 @monaco-editor/react: 4.6.0 => 4.6.0 @mui/lab: ^5.0.0-alpha.155 => 5.0.0-alpha.158 @mui/material: ^5.14.20 => 5.15.2 @mui/system: ^5.14.20 => 5.15.2 @mui/utils: ^5.14.20 => 5.15.2 @mui/x-date-pickers: ^6.18.3 => 6.18.6 @mui/x-tree-view: ^6.17.0 => 6.17.0 @storybook/addon-actions: ^7.6.17 => 7.6.17 @storybook/addon-essentials: ^7.6.17 => 7.6.17 @storybook/addon-links: ^7.6.17 => 7.6.17 @storybook/addon-mdx-gfm: ^7.6.17 => 7.6.17 @storybook/node-logger: ^7.6.17 => 7.6.17 (8.1.2) @storybook/react: ^7.6.17 => 7.6.17 (8.1.2) @storybook/react-vite: 8.1.2 => 8.1.2 @svgr/rollup: ^8.1.0 => 8.1.0 @testing-library/dom: ^9.3.3 => 9.3.3 @testing-library/jest-dom: ^6.1.5 => 6.1.6 @testing-library/react: ^14.1.2 => 14.1.2 @testing-library/user-event: ^14.5.1 => 14.5.2 @types/react-beautiful-dnd: ^13.1.7 => 13.1.8 @types/react-dom: ^18.2.17 => 18.2.18 @typescript-eslint/eslint-plugin: ^6.12.0 => 6.16.0 @typescript-eslint/parser: ^6.12.0 => 6.16.0 @vitejs/plugin-basic-ssl: ^1.0.2 => 1.0.2 @vitejs/plugin-react: 4.3.0 => 4.3.0 @vitest/coverage-v8: ^1.2.0 => 1.2.0 @vitest/ui: ^1.2.0 => 1.2.0 aws-amplify: 6.0.12 => 6.0.12 aws-amplify/adapter-core: undefined () aws-amplify/analytics: undefined () aws-amplify/analytics/kinesis: undefined () aws-amplify/analytics/kinesis-firehose: undefined () aws-amplify/analytics/personalize: undefined () aws-amplify/analytics/pinpoint: undefined () aws-amplify/api: undefined () aws-amplify/api/server: undefined () aws-amplify/auth: undefined () aws-amplify/auth/cognito: undefined () aws-amplify/auth/cognito/server: undefined () aws-amplify/auth/enable-oauth-listener: undefined () aws-amplify/auth/server: undefined () aws-amplify/datastore: undefined () aws-amplify/in-app-messaging: undefined () aws-amplify/in-app-messaging/pinpoint: undefined () aws-amplify/push-notifications: undefined () aws-amplify/push-notifications/pinpoint: undefined () aws-amplify/storage: undefined () aws-amplify/storage/s3: undefined () aws-amplify/storage/s3/server: undefined () aws-amplify/storage/server: undefined () aws-amplify/utils: undefined () cronstrue: ^2.47.0 => 2.47.0 cypress: ^13.6.1 => 13.6.2 d3: ^7.8.5 => 7.8.5 date-fns: ^2.30.0 => 2.30.0 dompurify: ^3.0.8 => 3.0.8 env-cmd: ^10.1.0 => 10.1.0 eslint: 8.55.0 => 8.55.0 eslint-config-airbnb: ^19.0.4 => 19.0.4 eslint-config-airbnb-typescript: ^17.1.0 => 17.1.0 eslint-config-prettier: ^9.1.0 => 9.1.0 eslint-plugin-import: ^2.29.0 => 2.29.1 eslint-plugin-jest: ^27.6.0 => 27.6.0 eslint-plugin-jsx-a11y: ^6.8.0 => 6.8.0 eslint-plugin-react: ^7.33.2 => 7.33.2 eslint-plugin-react-hooks: ^4.6.0 => 4.6.0 eslint-plugin-react-refresh: ^0.4.4 => 0.4.5 eslint-plugin-storybook: ^0.6.15 => 0.6.15 graphql: ^16.8.1 => 16.8.1 (15.8.0) husky: 8.0.3 => 8.0.3 jsdom: ^23.0.1 => 23.0.1 lint-staged: ^15.2.4 => 15.2.4 monaco-editor: ^0.44.0 => 0.44.0 msw: 2.0.10 => 2.0.10 node-sql-parser: ^4.18.0 => 4.18.0 notistack: ^2.0.8 => 2.0.8 prettier: ^3.1.0 => 3.1.1 (2.8.8) prop-types: ^15.8.1 => 15.8.1 react: ^18.2.0 => 18.2.0 react-beautiful-dnd: ^13.1.1 => 13.1.1 react-day-picker: ^8.10.1 => 8.10.1 react-diff-viewer: ^3.1.1 => 3.1.1 react-dom: ^18.2.0 => 18.2.0 react-grid-layout: ^1.4.4 => 1.4.4 react-idle-timer: ^5.7.2 => 5.7.2 react-json-view: ^1.21.3 => 1.21.3 react-query: ^3.39.3 => 3.39.3 react-quill: ^2.0.0 => 2.0.0 react-router-dom: ^6.20.1 => 6.21.1 react-transition-group: ^4.4.5 => 4.4.5 (2.9.0) react-transition-group/CSSTransition: undefined () react-transition-group/ReplaceTransition: undefined () react-transition-group/SwitchTransition: undefined () react-transition-group/Transition: undefined () react-transition-group/TransitionGroup: undefined () react-transition-group/TransitionGroupContext: undefined () react-transition-group/config: undefined () react-viewport-list: ^7.1.1 => 7.1.2 recharts: ^2.10.3 => 2.10.3 storybook: ^7.6.17 => 7.6.17 typescript: ^5.3.2 => 5.3.3 vite: 5.2.11 => 5.2.11 vite-plugin-babel: ^1.1.3 => 1.2.0 vite-plugin-eslint: ^1.8.1 => 1.8.1 vite-plugin-mkcert: ^1.17.1 => 1.17.1 vite-plugin-monaco-editor: ^1.1.0 => 1.1.0 vitest: 1.6.0 => 1.6.0 web-vitals: ^3.5.0 => 3.5.1 npmGlobalPackages: @nestjs/cli: 10.2.1 npm: 10.5.0 protobufjs-cli: 1.1.2 typescript: 5.3.3 ```

Describe the bug

Hi! We have the following login configuration:

  useEffect(() => {
    const unsubscribe = Hub.listen('auth', ({ payload }) => {
      switch (payload.event) {
        case 'customOAuthState':
          setTimeout(() => navigate(decodeURIComponent(payload.data)));
          break;
        case 'signInWithRedirect':
          getUser();
          break;
        case 'signInWithRedirect_failure':
          logout(...)
          document.location.reload();
          break;
        case 'tokenRefresh_failure':
          logout(...)
          document.location.reload();
          break;
        default:
          break;
      }
    });
    getUser();

    return unsubscribe;
  }, []);

    const getUser = async () => {
    try {
      const shouldLogin = !localStorage.getItem('hasPassedLoginFlow');
      const result =  await Auth.fetchAuthSession({ forceRefresh: false });
      if (result.tokens?.idToken?.payload) {
        const { payload } = result.tokens.idToken;
        setAuthState({
          isAuthenticated: true
        });
        setIsLoading(false);
      } else {
        setAuthState({ isAuthenticated: false });
        if (shouldLogin) {
          localStorage.setItem('hasPassedLoginFlow', 'true');
          signIn();
          // if there is nothing cognito related, we need to logout and reload the page
        } else if (
          // check if any key related to Cognito exists in local storage
          !localStorage.getItem(Object.keys(localStorage).find(key => /^CognitoIdentityServiceProvider(.*)$/.test(key)) as string)
        ) {
          console.error(`[authentication error]: No Token received after redirect to login: ${JSON.stringify(result)}`);
          logout(...);
          document.location.reload();
        }
      }
    } catch (error) {
      setAuthState({isAuthenticated: false });
      setIsLoading(false);
    }
  };

We would like to know if this code is well written and if there are any ways to improve this, as we have some race condition in which users successfully pass the IDP login, but still have no session in await Auth.fetchAuthSession. those flows are successfully saved as we call again getUser function when we receive a signInWithRedirect event

Expected behavior

Users will be able to login without a race condition

Reproduction steps

No real way to reproduce.

Code Snippet

// Put your code below this line.

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 4 months ago

Hello, @Lirontal1 and thanks for opening this issue. It looks like you are trying to call the v5 Auth.fetchAuthSession API rather than the v6 fetchAuthSession as seen here. That's likely one reason you don't have anything being returned by the await Auth.fetchAuthSession.

It looks like you're trying to check for a session on your user after sign-in, so you may want to do this by calling fetchAuthSession just after any sign-in events that you're listening for via the Hub Auth channel. Let me know if this clears things up or if you have further questions!

israx commented 4 months ago

hello @Lirontal1. Can you upgrade to the latest version of the library and see if you still face any issue?

Also something that I notice. The above code might lead to a race condition. The library will dispatch the customOAuthState event first — I imagine that's why the setTimeout is used. And then the signInWithRedirect event. The issue that I see with this approach is that setTimeout will be called sync and delegate the navigate(decodeURIComponent(payload.data)) code to the event queue. Then the signInWithRedirect event will resolve and call the getUser function, so any async code inside that method will also be pushed to the event queue.

So the order of async operations should resolve in that way:

1.navigate(decodeURIComponent(payload.data))

  1. fetchAuthSession

A potential solution is to move the navigate logic to the signInWithRedirect event — You can assign whatever data is returned from the customOAuthState paylaod to a variable and then access it when the signInWithRedirect is dispatched.

matheusgrandi commented 4 months ago

I'm having a similar issue, but just happening in production.

In my case, this private-route works as expected in localhost, but when in production the authStatus is coming as unauthenticated. If I go to the login page after signed-in (which is a Authenticator component) and try to login again it says: There is already a signed in user. but still getting the unauthenticated

I'm stuck in this more than a week and don't know what is the issue, and why it's just happening in prod.

// PrivateRoute.tsx
import { Loader } from '@platform/patterns';
import { useAuthenticator } from '@aws-amplify/ui-react';
import { PropsWithChildren } from 'react';
import { Navigate } from 'react-router-dom';

type PrivateRouteProps = {
  redirectTo: string;
};

const PrivateRoute: React.FC<PropsWithChildren<PrivateRouteProps>> = ({
  children,
  redirectTo,
}) => {
  const { authStatus } = useAuthenticator();

  return authStatus === 'configuring' ? (
    <Loader isLoading />
  ) : authStatus !== 'authenticated' ? (
    <Navigate to={redirectTo} />
  ) : (
    children
  );
};

export default PrivateRoute;
// SignIn.tsx
import { Authenticator, useAuthenticator } from '@aws-amplify/ui-react';
import '@aws-amplify/ui-react/styles.css';
import { Box } from '@mui/material';

const SignInPage: React.FC = () => {
  const { authStatus } = useAuthenticator();

  return (
    <Box display={'flex'} alignItems={'center'} justifyContent={'center'}>
      <Authenticator hideSignUp>
        {({ signOut, user }) => (
          <main>
            <h3>Status {authStatus}</h3>
            <h1>Hello {user?.username}</h1>
            <button onClick={signOut}>Sign out</button>
          </main>
        )}
      </Authenticator>
    </Box>
  );
};

export default SignInPage;
israx commented 4 months ago

Hello @matheusgrandi. Your issue is with the useAuthenticator hook not returning the right state. Is your app and private route wrapped around the <Authenticator.Provider> component?

If you still have issues with the authenticator please open a ticket on the Amplify-ui repo

cwomack commented 4 months ago

Closing this issue as we have not heard back from you. If you are still experiencing this, please feel free to reply back and provide any information previously requested and we'd be happy to re-open the issue.

Thank you!