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

Remove default authenticate_session middleware #547

Open
wants to merge 1 commit into
base: 4.x
Choose a base branch
from

Conversation

owenconti
Copy link

This PR removes the default authenticate_session middleware that is set by Sanctum.

We ran into a hard to debug issue in our application where the user would be logged out immediately after impersonating some users via Nova. As it turns out the "some" ended up being based on if the users shared the same password or not. So locally, everything worked fine as all seeded users get the same password. But in production, this is not the case.

I think another potential update here would be to keep the default config as-is, but update the Sanctum docs to make it clear what the AuthenticateSession middleware does, and that it is enabled by default. However, it seems like all other areas of the Laravel ecosystem make the "log out other devices" feature opt-in, instead of opt-out.

test: Fix test to include AuthenticateSession middleware during test
@taylorotwell
Copy link
Member

Hey @owenconti, we probably don't want to remove this from here as it could be a security issue, but instead we should maybe fix it in Nova's impersonation logic.

Do you have an issue if you use Nova's "Stop Impertonating" button? Can you give us a step-by-step of how to recreate?

@SRWieZ
Copy link

SRWieZ commented Nov 21, 2024

I have a similar issue with Nova.

I had to change auth:sanctum to auth:web to make it work.

Route::middleware([
    'auth:web',
    config('jetstream.auth_session'),
    EnsureEmailIsVerified::class,
])->group(
// ..

I didn't fully understand the problem at the time, but I’m sharing this in case it helps fixing the issue.

@owenconti
Copy link
Author

@taylorotwell definitely understand not wanting to remove the default value for security reasons, no problem there.

I think this isn't on Nova, though. Any other package (or custom code) that does impersonation in combination with using Sanctum (and more specifically Sanctum's EnsureFrontendRequestsAreStateful middleware) will run into the issue.

Maybe a docs update to the Sanctum Middleware that explains that the AuthenticateSession middleware will be applied by default which results in changes need for impersonation to work correctly would be a safer choice? I'd be happy to draft those doc updates if that's the route you want to go.

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.

3 participants