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

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

Open
3 tasks done
mkalam-alami opened this issue Jun 21, 2024 · 4 comments
Open
3 tasks done
Labels
Auth Related to Auth components/category bug Something isn't working Hub Related to Hub category VP Version parity issues between v5 and v6

Comments

@mkalam-alami
Copy link

mkalam-alami commented Jun 21, 2024

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:
  • Dispatch the "signedIn", "signedInWithRedirect", and "customOAuthState" events
  • THEN it will override the location URL, with prevents the application from regaining control on the browser location by handling these events

See completeOAuthFlow.ts behavior :

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

@mkalam-alami mkalam-alami added the pending-triage Issue is pending triage label Jun 21, 2024
@israx
Copy link
Member

israx commented Jun 21, 2024

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.

@cwomack cwomack self-assigned this Jun 21, 2024
@cwomack cwomack added the Auth Related to Auth components/category label Jun 21, 2024
@mkalam-alami
Copy link
Author

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

israx commented Jun 21, 2024

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

cwomack commented Jun 21, 2024

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.

@cwomack cwomack added bug Something isn't working VP Version parity issues between v5 and v6 and removed pending-triage Issue is pending triage labels Jun 21, 2024
@cwomack cwomack removed their assignment Jul 11, 2024
@cwomack cwomack added the Hub Related to Hub category label Dec 11, 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 bug Something isn't working Hub Related to Hub category VP Version parity issues between v5 and v6
Projects
None yet
Development

No branches or pull requests

3 participants