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

Uncaught Exception from Auth0 controller in vendor #141

Closed
eruvio opened this issue Sep 25, 2019 · 12 comments
Closed

Uncaught Exception from Auth0 controller in vendor #141

eruvio opened this issue Sep 25, 2019 · 12 comments
Assignees

Comments

@eruvio
Copy link

eruvio commented Sep 25, 2019

Description

src/controllers/Auth0Controller.php should wrap $service->getUser() in a try/catch block. When \Exception is caught, the $profile should be set null, and $service->logout() called. The controller action throws exceptions and does not provide a way to handle them.

Reproduction

I have encountered an Auth0 CoreException due to my VM's date/time being slightly out of sync. When this occurs, the getUser() method invoked in the callback controller-action throws an uncaught exception. I am proposing to catch this \Exception and set the $profile null, as well as invoking logout() on the Auth0 service class. Additionally, a redirect should be issued to Auth0's logout URL to terminate the session there.

Environment

  • auth0/login 5.2.0
@eruvio
Copy link
Author

eruvio commented Sep 25, 2019

If in agreement, I'd be happy to PR.

@joshcanhelp
Copy link
Contributor

@eruvio - Thank you for the report here!

I agree, that call should be wrapped and handled by the framework. It looks like a few things could go wrong:

  • Invalid state
  • User already has an active session in the SDK
  • HTTP or other error in the code exchange
  • Invalid ID token
  • HTTP or other error in the userinfo call (if that's called)

... so, a lot that could go wrong :)

I definitely think calling $service->logout() is a good idea as that would address one of these sources. I'm not so sure about ending the Auth0 session, though. If you're on the callback then you've authenticated successfully and something happened in the application itself. If we end the Auth0 session then we can't leverage SSO if the user needs to log in again (which, really, is the only action they could take).

One other thing to keep in mind ... the callback URL could receive an error and error_description parameter as a response if something went wrong on the server. Right now, this package doesn't handle that.

If you have the time to create and work a PR, I'd be more than happy to review it, appreciate the offer! We're close to releasing on this library (today or tomorrow) but happy to get this one right and release after.

@eruvio
Copy link
Author

eruvio commented Sep 25, 2019

Cool! I will work on the PR today. Thanks for the advise regarding errors

@eruvio
Copy link
Author

eruvio commented Sep 26, 2019

Hi Josh,

Regarding ending the Auth0 central session - while I agree with your point that this is error is on the user's application, the reason I propose this is because for my application (and others, I assume) do not have a '/' route. For example, I am building a logged-in administrative application where / redirects to /login, which redirects to Auth0 via calling the Auth0 service class login(). Therefore, I encounter an infinite redirect loop if I do not end the Auth0 session, because:

  1. Exception is caught, $service->logout() is called, and then a redirect to / occurs
  2. / redirects to /login , which redirects to Auth0
  3. Auth0 redirects back to the callback route of the application, which rethrows the Exception from step 1

Any thoughts on how to circumvent this? Logging the user out of Auth0 via redirect was my initial thought.

@joshcanhelp
Copy link
Contributor

OK, I see what you're saying here, and I'm guessing you're not the only one with that issue. The problem I see here with logging out of Auth0 is that the user will:

  1. Get to a protected route (any route, in your app's scenario)
  2. Get redirected to Auth0 to login
  3. Log in successfully
  4. After some period of time, end up back on the Auth0 login page requiring credentials

We should, at some point during that, be able to tell the user something went wrong and they need to try again. Same if the Auth0 server returns an error response in the URL query.

I looked briefly at the error handling docs for Laravel:

https://laravel.com/docs/5.8/errors

Would it make sense to provide a default rendered error page to inform users that something went wrong and they should try again? Is there a way to do that so developers can override or extend it? Or, at least provide a path or handler via the Auth0 config file to redirect on error (or after logging in, if it should be different than the homepage)?

I'm sorry I don't know more about Laravel's error handling to provide a more specific solution.

@xaoseric
Copy link

This also affects version 5.3.0, with Laravel 6, I am getting a CoreException of "Can't initialize a new session while there is one active session already"

@joshcanhelp joshcanhelp self-assigned this Oct 21, 2019
@xaoseric
Copy link

xaoseric commented Oct 21, 2019

I modified the SessionStateHandler class and added the following code. after adding the following code to the issue method, it works.

$state = $this->store->get(self::STATE_NAME);
if ($state === null) {
     $state = uniqid('', true);
     $this->store($state);
}

@joshcanhelp
Copy link
Contributor

@xaoseric - I'm not totally convinced that the right thing to do is to change the SDK in this case. The new/active session error is not related to state here so that would still bubble up.

I'm curious, though, if you're able to walk me through what's happening to get a state mismatch in your application. It looks like state is getting generated and sent but not stored? Or something else? Could be curious to understand why this fixes your issue.

I'll leave this open to track how the callback should be addressed.

@joshcanhelp joshcanhelp added this to the 7.0.0 milestone Oct 21, 2019
@xaoseric
Copy link

@joshcanhelp I haven't seen the new/active session error since the change in the sdk. Only time i saw it was when I deleted a user still logged in and had to remove laravel session files to fix it.

@philliphartin
Copy link

philliphartin commented Feb 14, 2020

We've been having many issues when working in our local development environments with the can't initialize a new session while there is one active error.

if ($this->user) {
    throw new CoreException('Can\'t initialize a new session while there is one active session already');
}

I'm on auth0/login v5.3.0 and laravel 6.14. It's a major source of frustration.

It's a major pain.

@joshcanhelp
Copy link
Contributor

@pjhartin - Sorry for the trouble here. How a lot of this is handled is changing in the next major version of this library. Would you be able to give the change-user-model branch a try and see if it works for you?

https://github.com/auth0/laravel-auth0/tree/change-user-model

Make sure to composer update as the PHP SDK was updated. PHP 7.1 required.

@joshcanhelp joshcanhelp removed this from the 6.0.0 milestone Apr 3, 2020
@evansims evansims assigned evansims and unassigned joshcanhelp Nov 30, 2020
@evansims
Copy link
Member

Closing as this issue appears to have gone stale, and appears to have been addressed in later releases.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants