IdentityModel / oidc-client-js

OpenID Connect (OIDC) and OAuth2 protocol support for browser-based JavaScript applications
Apache License 2.0
2.43k stars 842 forks source link

UserManager.signinRedirect() returns a resolved promise even when the redirection isn't finalized #1363

Open Lautaro-Garcia opened 3 years ago

Lautaro-Garcia commented 3 years ago

Hello! Thank you for mantaining this library, I've been using it for some time now, and yesterday I found a case that I wasn't expecting. I don't know if it's the desired use case for that API. If it is, ignore this issue.

So, if I use the UserManager#signinRedirect() method to start the sign-in process it returns a resolved promise, allowing code that's hooked up in a .then() method to run even when the page will eventually change to the login page:

async foo() {
  await userManager.signinRedirect();
  console.log('something');  // <- This may run, depending on how quickly the browser changes pages
}

I expected that signinRedirect() would stop the execution flow (because the user will be redirected to a new URL). Wouldn't it be better if it returned a pending promise, so if the library user awaits the redirection it won't keep executing things? I wrote this as a question, because maybe you implemented this in that way to support another feature that I'm not using :thinking:

egbertn commented 3 years ago

Hey I am not the author, but signinRedirect make the browser navigate to a new page. That means, any current scripts would stop anyway. The place where the flow continues would be at signinRedirectCallback() which you need to implement.

Lautaro-Garcia commented 3 years ago

That's correct, I would expect that the things "below" the signinRedirect() wouldn't be executed, but that's not what I found in practice. Maybe it's because my browser doesn't change pages that quickly (I tested it with both Firefox and Chrome).