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

Session#login could return a Promise resolving to redirectUrl (or an object containing it) #2066

Open
2 of 4 tasks
elf-pavlik opened this issue Apr 6, 2022 · 6 comments
Open
2 of 4 tasks

Comments

@elf-pavlik
Copy link

elf-pavlik commented Apr 6, 2022

Search terms you've used

login

Impacted environment

In which environment would the proposed feature apply ?

  • The browser
  • Node.js
  • Other (please specify): ...
  • I'm not sure.

Feature suggestion

Session#login could return a Promise resolving to the redirectUrl (or an object containing it), which gets passed to handleRedirect callback.

Actual functionality/enhancement

Currently Session#login returns Promise<void> which is not very useful.

Having the redirectUrl available once Promise resolves would make some of our code a little simpler. It would also simplify mocking the login in tests.

@elf-pavlik elf-pavlik changed the title Session#login could return a Promise resolving to redirectUrl (or an objects containing it) Session#login could return a Promise resolving to redirectUrl (or an object containing it) Apr 6, 2022
@NSeydoux
Copy link
Contributor

NSeydoux commented Apr 6, 2022

The reason why the Session#login function returns a Promise<void> is because logging in redirects the user away from the page: between the login and the handleIncomingRedirect, the context is lost because of the redirect and ensuing page reload. Note that because of this, the promise returned by login never resolves: when window.location is set, the browser is eventually redirected, but asynchronously and in a way which isn't under the control of the app. In order to avoid race conditions, the login function is effectively blocking.

This design will need to evolve when we add support for popup-based redirect, and then login could eventually return. I'll consider your suggestion then.

@elf-pavlik
Copy link
Author

handleRedirect

If a function is provided, the browser will not auto-redirect and will instead trigger that function to redirect. Required in non-browser environments, ignored in the browser.

Actually, currently, we only use this client in node.
It would be really nice to at least in the node have the option not to provide handleRedirect and just get redirectUrl from the resolved promise.

Looking at the description, the behavior in the browser doesn't seem clear. First, it says if provided, the browser will not auto-redirect just to later state ignored in the browser.

To keep the interface consistent across platforms, the following approach might work:

  1. If handleRedirect is provided just use it as a callback
  2. ILoginInputOptions could be extended in the browser with autoRedirect: boolean, which could default to true to keep the current behavior.
  3. If the promise resolves it resoves to redirectUrl (or an object containing it), sometimes in the browser redirect will happen before it resolves but that seems fine.

@NSeydoux
Copy link
Contributor

NSeydoux commented Apr 7, 2022

Oh I see, I didn't realize this was about the Node behaviour. I'll see what can be done, but consider the following:

  1. The user initiates login in the app, and they are redirected to their OIDC Provider.
  2. They log into their OIDC Provider, and get redirected to the client
  3. The client gets the auth code from the redirect IRI, and performs the backchannel exchange to get the ID token

The handleRedirect callback given to the login function is used in step 1 to redirect the user to the OIDC provider. The incoming redirect happens in a separate request, which is a different context from the server point of view, and only a correlation id held in the state and shared storage allow to complete the flow. There is no reference from step 3 to step 1, so resolving the promise from step 1 with data obtained in step 3 would require to use an async communication mechanism, such as the storage, and for step 1 promise to poll the storage until the expected result is found. That would add some complexity to the code that I don't think brings that much value. Am I missing something?

However, what you're pointing out about the handleRedirect documentation is a good point, I'll fix that up.

@elf-pavlik
Copy link
Author

I'm only talking about step 1, and the redirectUrl which currently only gets passed to the handleRedirect callback. What we would like is that login returns a Promise which resolves to that very redirectUrl, this way we can handle redirecting to the OIDC Provider after the promise resolves. This would be an alternative to passing a handleRedirect function to login and using it as a callback.

@NSeydoux
Copy link
Contributor

NSeydoux commented Apr 7, 2022

Ok I see what you mean now. We are considering making a split between a low-level and a high-level library, the low-level one giving a finer control to application developers with specific auth requirements. Returning the redirect IRI, and leaving it up to the developer to handle the redirection, could be a feature of such an API. It is unlikely that we add this to the higher-level one though, which we want to keep as simple as possible, which sometimes means limiting the number of options.

To make sure I'm approaching this at the right angle: this isn't blocking you, right? It would be a nice optimization, but it isn't preventing you from implementing your use case, does it?

@elf-pavlik
Copy link
Author

To make sure I'm approaching this at the right angle: this isn't blocking you, right? It would be a nice optimization, but it isn't preventing you from implementing your use case, does it?

Yes, it's mostly about the ergonomy of the interface, as well as a slightly more convenient way to mock it in tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants