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

withAuthenticationRequired invokes loginWithRedirect multiple times #309

Closed
kweiberth opened this issue Dec 21, 2021 · 8 comments · Fixed by #311
Closed

withAuthenticationRequired invokes loginWithRedirect multiple times #309

kweiberth opened this issue Dec 21, 2021 · 8 comments · Fixed by #311

Comments

@kweiberth
Copy link
Contributor

Explanation:

  • I had an intermittent/inconsistent login issue on my site. It was happening mostly in Google Chrome incognito, but was also being reported by users using regular Chrome.
  • I reached out to Auth0 support, and we figured out that there was a mismatch happening b/w code_verifier and code_challenge and the call to /authorize would 403.
  • Auth0 support agent figured out that my application was setting code_verifier multiple times due to multiple invocations of loginWithRedirect().
  • My application is using withAuthenticationRequired method from @auth0/auth0-react. To help myself debug, I pulled out this method into my own application so I could debug/log stuff. I realized that the useEffect here is being invoked multiple times, and the culprit is loginOptions as a dependency. I'm not passing in loginOptions, so it seems that on each render we're defaulting it to a new instance of an empty object (here), and this is triggering the useEffect (bc they are different empty objects and they are !==).
  • To fix the issue, I defined const emptyObject = {} and then used it as the default: loginOptions = emptyObject.

Expected behavior:

  • Should invoke loginWithRedirect only once.

Reproduction:

  • Not sure if this is specific to my application or how I've set things up. Unfortunately I haven't had a chance to try and build a simple example that reproduces. I'm using Next.js v11.0.0.

Environment:

  • Version of auth0-react used: 1.8.0
  • Which browsers have you tested in? Chrome, esp incognito
  • Which framework are you using, if applicable (Angular, React, etc): Next.js v11.0.0
  • Other modules/plugins/libraries that might be involved: N/a
@adamjmcgrath
Copy link
Contributor

Hi @kweiberth - thanks for raising this

Expected behavior:
Should invoke loginWithRedirect only once.

withAuthenticationRequired only invokes loginWithRedirect once (see https://github.com/auth0/auth0-react/blob/master/src/with-authentication-required.tsx). If you're invoking loginWithRedirect multiple times, it means you're trying to render the component that is wrapped by withAuthenticationRequired multiple times.

I'd be happy to debug what's happening with your app, if you can provide me with a repo that demonstrates the issue.

@adamjmcgrath adamjmcgrath added the question Further information is requested label Dec 21, 2021
@kweiberth
Copy link
Contributor Author

kweiberth commented Dec 21, 2021

Hi @adamjmcgrath, thanks for the quick response. I'll see if I can get a repo together. In the meantime, can you describe a little more why we are so certain that withAuthenticationRequired invokes loginWithRedirect only once?

From what I can tell, it's being invoked inside a useEffect here, and the useEffect will be invoked anytime any one of isLoading, routeIsAuthenticated, loginWithRedirect, loginOptions or returnTo changes b/w renders. In my case, it's loginOptions that is causing multiple invocations. It does seem like some parent/wrapped component of mine is rendering more than once. However, I'm not passing in a new loginOptions; instead, withAuthenticationRequired is defaulting it to an empty object, but on each render that's a new instance of an empty object, and it triggers useEffect.

@kweiberth
Copy link
Contributor Author

@adamjmcgrath I've created a repo that reproduces this issue: https://github.com/kweiberth/multiple-Auth0-logins. State is set in _app.tsx here, which causes an additional render, which causes the useEffect in withAuthenticationRequired here to be invoked twice. Comments here show how we can avoid/fix this issue.

Let me know if you need any more information!

@adamjmcgrath
Copy link
Contributor

Thanks for sharing the repo @kweiberth

but on each render that's a new instance of an empty object, and it triggers useEffect.

You make a good point - I'll take a look at this today or tomorrow and get back to you.

@adamjmcgrath adamjmcgrath added needs investigation and removed question Further information is requested labels Dec 22, 2021
@adamjmcgrath
Copy link
Contributor

@kweiberth - I've had a look at your repo and while I think there's some merit to your suggestion about login options (or something similar). I'll add something to our backlog, but this will probably have to wait until the new year.

As an aside, you're also creating a new instance of Auth0Provider for every route change/App render (see https://github.com/kweiberth/multiple-Auth0-logins/blob/master/utils/with-auth0.tsx#L104) which will have other undesired effects. You should take a look at the nextjs example app in the repo, specifically https://github.com/auth0/auth0-react/blob/master/examples/nextjs-app/pages/_app.js

@kweiberth
Copy link
Contributor Author

kweiberth commented Dec 22, 2021

Hi @adamjmcgrath, thanks for looking into it. Will it help if I make a PR? It's a pretty simple change. Edit: just saw the link to the change you made. 👍 No worry on timing... I've pulled this logic out into my app for now, and whenever it gets released I can migrate back to official library.

Re: instances on each route: I'm doing this bc auth0 JS SPA is a huge bundle size (like 40kb I think?), so I can't add it to every route. There are profile-type pages on our site that need to load as fast as possible, and don't require auth. I've been meaning to see if I can lazy load it w/ Next.js's dynamic imports, but haven't had a chance to explore.

@adamjmcgrath
Copy link
Contributor

Sure - happy to look at a PR @kweiberth thanks. But either way, it wont get released until shortly after the beginning of January

Re: multiple instances on each route: I'm doing this bc auth0 JS SPA is a huge bundle size (like 40kb I think?), so I can't add it to every route.

Ok, I would just make sure that https://github.com/kweiberth/multiple-Auth0-logins/blob/master/utils/with-auth0.tsx#L104 doesn't get invoked more than once per page load

@kweiberth
Copy link
Contributor Author

Okay yeah, makes sense. I don't think it is, but I guess it could be happening transiently/inconsistently to my users (similar to this one that we're discussing/fixing here).

I've made a PR to address this issue: #311

adamjmcgrath pushed a commit that referenced this issue Dec 23, 2021
* Avoid potential multiple invocations of loginWithRedirect

Resolves #309

* add test to assert single call

* remove comment
hrrytech92 added a commit to hrrytech92/auth0-react that referenced this issue May 31, 2022
* Avoid potential multiple invocations of loginWithRedirect

Resolves auth0/auth0-react#309

* add test to assert single call

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

Successfully merging a pull request may close this issue.

2 participants