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

fix: avoid reinitiating partial authentication flow when user is already authenticated #962

Merged
merged 3 commits into from
Mar 8, 2023

Conversation

jamesdh
Copy link
Contributor

@jamesdh jamesdh commented Mar 8, 2023

Fixes: #922, and possibly some others. Influenced by authts/oidc-client-ts#402.

TLDR:

The problem here is that the stored state is removed immediately after the redirection from login. So if the user presses "back", then logs again, there is no more state available, even tho the redirect URL contains a state parameter. In other words, oidc-client-ts tries to load the stored state, but none is available because it removed it the first time the user logged in.

Personally what I discovered was that upon redirect from login, we still had the auth params in our query string. So if one were to reload the page or browse forward a page then hit the back button, we'd encounter the same thing. hasCodeInUrl() would pass which would trigger an invalid auth flow again. Therefore, I simply add a check that not only does hasCodeInUrl check out, but also that userManager doesn't actively have a user already authenticated.

I feel like I might be missing something here since I'm not very aware of all the possible authentication flows that are being used with oidc-react. If I could get some additional eyes on this I'd greatly appreciate it.

@jamesdh jamesdh requested a review from simenandre March 8, 2023 04:27
@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@3eaf620). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             main      #962   +/-   ##
========================================
  Coverage        ?   100.00%           
========================================
  Files           ?         1           
  Lines           ?        83           
  Branches        ?        31           
========================================
  Hits            ?        83           
  Misses          ?         0           
  Partials        ?         0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@simenandre simenandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's land this and get it out there to people. From what I can understand, it wouldn't hurt anything and its just logical that the user object should be falsy together with having the code in the URL.

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

Successfully merging this pull request may close these issues.

Add a check that oidc.{state} exists in localStorage. Bug with reactStrictMode (<StrictMode/>)
2 participants