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

Incorrect middleware ordering #414

Closed
6 tasks done
marijnvanderhorst opened this issue Jun 21, 2023 · 9 comments
Closed
6 tasks done

Incorrect middleware ordering #414

marijnvanderhorst opened this issue Jun 21, 2023 · 9 comments
Assignees
Labels
Scope: Bug Confirmed report of unexpected problems or unintended behavior.

Comments

@marijnvanderhorst
Copy link

Checklist

Laravel Version

10

SDK Version

7.8

PHP Version

PHP 8.2

Description

Assume we have an API-only application. Hence, we only need to authorize requests. No authentication has to be performed whatsoever.

From the sample repository, it is clear that with the latest version of this SDK we should be able to use the auth middleware that is provided by the Laravel framework for authorization of API requests. For this, we should only have to define the routes in the routes/api.php file and include said middleware in the route.

With the latest version of the SDK, this does not seem to work since the AuthorizerMiddleware has a lower priority than the auth middleware. Hence, the auth guard is only set to the API version after the Laravel framework tries (and fails) to authenticate the request using the default AuthenticationGuard.

How can we reproduce this issue?

  1. composer create-project auth0-samples/laravel auth0-laravel-quickstart.
  2. Create .env file based on the provided .env.example.
  3. php artisan key:generate.
  4. Set the following variables in the .env file. Note that we don't set any client ID and secret since we will only need to authorize requests, no authentication is required.
AUTH0_DOMAIN=...
AUTH0_CUSTOM_DOMAIN=...
AUTH0_AUDIENCE=...
  1. Serve application and send a GET request to the /api/private endpoint.
  2. Observe an internal server error orginating from vendor/auth0/login/src/Guards/AuthenticationGuard.php. I strongly believe the authentication guard should not be used for API requests, rather I would expect the Authorization Guard to be used instead.
  3. Add the following line to the boot() method of the app/Providers/AppServiceProvider.php.
$this->app->make(Kernel::class)->prependToMiddlewarePriority(AuthorizerMiddleware::class);
  1. Send a GET request to the /api/private endpoint again (with appropriate Bearer token).
  2. Observe a 200 OK response with message Your token is valid; you are authorized..

IMO, this shows that the middleware priority is indeed incorrectly configured within the SDK. This, as ensuring that the AuthorizerMiddleware is executed before the auth middleware resolves the issue immediately. IMO the SDK should take care of this.

@evansims
Copy link
Member

evansims commented Jun 21, 2023

Hi @marijnvanderhorst,

I suspect this might be related to a bug I just pushed a fix for, #415. You should not be pre-pending or prioritizing that middleware, it isn't necessary.

Beyond that, are you issuing the GET request using an Accept: application/json header? Laravel's api middleware group will only return appropriate responses if that is supplied, otherwise it will fall back to serving from the web middleware group.

Under intended circumstances, the AuthorizationGuard will never run from api middleware group-served routes, as it isn't inserted into that group.

@evansims
Copy link
Member

evansims commented Jun 21, 2023

Hey @marijnvanderhorst, could you please try the new 7.9.1 release and check if that resolves the issue for you? I suspect it should address the problem.

@evansims evansims self-assigned this Jun 21, 2023
@evansims evansims added the Scope: Bug Confirmed report of unexpected problems or unintended behavior. label Jun 21, 2023
@squpshift
Copy link

I am experiencing an issue that I suspect is also related to this. I have a laravel project using https://github.com/inertiajs/inertia-laravel/

With auth0 installed, inertia's HandleInertiaRequests middleware share method is no longer able to access āuth()->user()or $request->user()- they both return null.

This also means that my application's Laravel Nova package is broken as it is built on Inertia.

Note that I am able to access the logged in user via āuth()->user()inside my controller (after the middleware has run).

@evansims
Copy link
Member

evansims commented Jun 22, 2023

@squpshift That doesn't sound related to the reported issue here, but I'll be happy to look into it separately. (Perhaps you meant to reply to #412?)

@squpshift
Copy link

@squpshift That doesn't sound related to the reported issue here, but I'll be happy to look into it separately. (Perhaps you meant to reply to #412?)

It seemed to be an issue of middleware order to me, I'm happy to open a separate issue though.

I'm having the issue without telescope installed, so not related to #412.

@marijnvanderhorst
Copy link
Author

Hey @marijnvanderhorst, could you please try the new 7.9.1 release and check if that resolves the issue for you? I suspect it should address the problem.

Unfortunately, it still doesn't work. I'm getting a 401 Unauthorized now, even though I'm sending a valid token. This, as when I change the auth.defaults.guard value manually to auth0-api, it does work as expected.

It seems to be using Laravel's SessionGuard now to authorize the /api/private endpoint. Perhaps the default guard is not updated appropriately?

Were you not able to reproduce the issue using the steps that I provided in the description?

@mikebrandl
Copy link
Contributor

I also have this issue.

Likewise: if I change the auth.defaults.guard value manually to auth0-api, it does work as expected.

@squpshift
Copy link

squpshift commented Jun 25, 2023

Changing the auth.defaults.guard did not resolve my version of this issue, but @marijnvanderhorst's suggestion did, which I have included in PR #419

@evansims
Copy link
Member

evansims commented Jul 5, 2023

Thank you all for your help in troubleshooting this issue. I have a fix merged, and will ship a new release ASAP that includes it.

@evansims evansims closed this as completed Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Bug Confirmed report of unexpected problems or unintended behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants