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.4k stars 2.11k forks source link

OAuth login flow replaces location after "signedIn" event is dispatched #13530

Open mkalam-alami opened 3 weeks ago

mkalam-alami commented 3 weeks ago

Before opening, please confirm:

JavaScript Framework

Angular

Amplify APIs

Authentication

Amplify Version

v6

Amplify Categories

auth

Backend

None

Environment information

Describe the bug

When OAuth sign in flow completes and the "signedIn" event is dispatched, the flow has actually not totally completed, and the library replaces the browser URL which may clash with application behavior.

Expected behavior

That the "signedIn" event is dispatched when the OAuth flow is fully completed, with no further actions from the Amplify library. This behavior allows applications to take control on the redirection, in my case overriding the static redirect URL with a dynamic route.

Reproduction steps

  1. Trigger an OAuth authentication workflow with signInWithRedirect()
  2. When signed in, the library will:

See completeOAuthFlow.ts behavior : https://github.com/aws-amplify/amplify-js/blob/e6c5f60225c6ba0beeb4d5d2ae4cd55615062152/packages/auth/src/providers/cognito/utils/oauth/completeOAuthFlow.ts#L251

Code Snippet

// ....

      Hub.listen('auth', async (data) => {
        console.log("auth event", data)
      });

      const replaceStateVanilla = history.replaceState;
      history.replaceState = function () {
        console.log("replaceState", arguments)
        replaceStateVanilla.apply(history, arguments as any);
      }

      await signInWithRedirect({
        provider: {
          custom: '...',
        },
        customState: "/a/custom/url/i/want/to/eventually/view/instead/of/the/cognito/configured/one"
      });

Log output

``` 'auth event' {channel: 'auth', payload: {event: 'signedIn', data: {…} // .... replaceState Arguments(3) [{…}, '', 'http://localhost:4200/index.html', callee: (...), Symbol(Symbol.iterator): ƒ] ```

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

israx commented 3 weeks ago

Hello @mkalam-alami. Amplify will clear any code or token query parameters to avoid exposing them on the current URL. Ideally, the library should provide an additional API that allows the completion of the OAuth flow, hence giving the developer the ability to change the current window history.

We are tracking this issue here.

mkalam-alami commented 3 weeks ago

Hi @israx, thanks for your answer. I confirm having explicit control over completion of the OAuth flow would resolve my problem.

Just to clarify, my workflow is actually started by calling signInWithRedirect() unlike #13343. A workaround to my issue was to use a setTimeout() to delay my replaceState by one tick, which makes it executed after Amplify clears the URL itself. Example:

    Hub.listen('auth', (data) => {
      if (data.payload.event === 'customOAuthState') {
        this.customState = data.payload.data;
      } else if (data.payload.event === 'signedIn') {
        setTimeout(() => { // ensure callback is triggered after Amplify cleanup
          history.replaceState({}, '', this.customState); // restore the requested URL
        })
      }
    });

I suspect users typically do not encounter this behavior because of relying on await getCurrentUser() rather than the signedIn event to finalize their flow, which possibly delays the callback by a tick or two. I managed to make my app work that way as well, although the code felt more convoluted (ex. below) and exposed to this possible bug.

    Hub.listen('core', async (data) => {
      if (data.payload.event === 'configure') {
        try {
          await getCurrentUser();
          history.replaceState({}, '', this.customState); // restore the requested URL
        } catch (error) {
          // ... signInWithRedirect()
        }
      }
    });

    Hub.listen('auth', (data) => {
      if (data.payload.event === 'customOAuthState') {
        this.customState = data.payload.data;
      }
    });
israx commented 3 weeks ago

Thank you for providing all the context. I think we can change the order of operations and clear query params before dispatching the events. Let me bring this issue to the team.

cwomack commented 3 weeks ago

Marking this as a bug in v6, as v5 of Amplify does not have this issue and will clear the window history before dispatching the hub events.