Skip to content

Commit

Permalink
fix middleware sorting for authenticating sessions
Browse files Browse the repository at this point in the history
  • Loading branch information
@taylorotwell
taylorotwell committed Mar 4, 2022
1 parent ee2958b commit 50b46db
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 3 deletions.
8 changes: 8 additions & 0 deletions src/Illuminate/Contracts/Session/Middleware/AuthenticatesSessions.php
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php

namespace Illuminate\Contracts\Session\Middleware;

interface AuthenticatesSessions
{
//
}
2 changes: 1 addition & 1 deletion src/Illuminate/Foundation/Http/Kernel.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class Kernel implements KernelContract
\Illuminate\Contracts\Auth\Middleware\AuthenticatesRequests::class,
\Illuminate\Routing\Middleware\ThrottleRequests::class,
\Illuminate\Routing\Middleware\ThrottleRequestsWithRedis::class,
\Illuminate\Session\Middleware\AuthenticateSession::class,
\Illuminate\Contracts\Session\Middleware\AuthenticatesSessions::class,
\Illuminate\Routing\Middleware\SubstituteBindings::class,
\Illuminate\Auth\Middleware\Authorize::class,
];
Expand Down
3 changes: 2 additions & 1 deletion src/Illuminate/Session/Middleware/AuthenticateSession.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@
use Closure;
use Illuminate\Auth\AuthenticationException;
use Illuminate\Contracts\Auth\Factory as AuthFactory;
use Illuminate\Contracts\Session\Middleware\AuthenticatesSessions;

class AuthenticateSession
class AuthenticateSession implements AuthenticatesSessions
{
/**
* The authentication factory implementation.
Expand Down
2 changes: 1 addition & 1 deletion tests/Foundation/Http/KernelTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public function testGetMiddlewarePriority()
\Illuminate\Contracts\Auth\Middleware\AuthenticatesRequests::class,
\Illuminate\Routing\Middleware\ThrottleRequests::class,
\Illuminate\Routing\Middleware\ThrottleRequestsWithRedis::class,
\Illuminate\Session\Middleware\AuthenticateSession::class,
\Illuminate\Contracts\Session\Middleware\AuthenticatesSessions::class,
\Illuminate\Routing\Middleware\SubstituteBindings::class,
\Illuminate\Auth\Middleware\Authorize::class,
], $kernel->getMiddlewarePriority());
Expand Down

6 comments on commit 50b46db

@relaypilot
Copy link

@relaypilot relaypilot commented on 50b46db Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This commit seems to have brought up an issue with the laravel-impersonate package.

Linking for reference: 404labfr/laravel-impersonate#154

@program-mia
Copy link

Choose a reason for hiding this comment

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

Yup, this one seems to prevent any attempt to try and log in as a different user while being logged in (at least in Jetstream afaik). Worked well in 9.3.1. Maybe it's a jetstream Auth middleware that should also extend the new contract?

@coatesjonathan
Copy link

@coatesjonathan coatesjonathan commented on 50b46db Mar 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I have just installed a fresh installation of Laravel 9.5, with Jetstream installed and I was getting an error when logging in/registering a new user.

I believe it's related to this.

Method Illuminate\Auth\SessionGuard::getDefaultDriver does not exist.

I have been forced to downgrade to laravel 9.3

@simonmaass
Copy link

Choose a reason for hiding this comment

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

yes same here!

@taylorotwell
Copy link
Member Author

@taylorotwell taylorotwell commented on 50b46db Mar 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This commit is correct - I'm not reverting it. If it breaks that impersonate package that is something they likely need to fix on their end.

It is likely broken because when they impersonate another user the session can no longer be authenticated because the password in the session no longer matches the impersonated user's password. In short, they would need to update the session with the hash of the impersonated user's password.

It was only working before because AuthenticateSession was totally broken and being run in the wrong order. Now that it is actually working correctly it has surfaced this oversight in the impersonation package.

@fylzero
Copy link
Contributor

@fylzero fylzero commented on 50b46db Mar 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@taylorotwell I don't know if I would say this is an oversight in an impersonation package... I have an app doing impersonation just via Auth::loginUsingId() and it causes the user to be logged out for the reason you specified.

...but yes, when using loginUsingId with Sanctum/Inertia, this is essentially trying to compare password_hash_sanctum against password_hash_web and logs the user out.

if ($request->session()->get('password_hash_'.$this->auth->getDefaultDriver()) !== $request->user()->getAuthPassword()) {
    $this->logout($request);
}

Justing noting: Taylor responded in this thread (laravel/jetstream#997). This just is something that shouldn't have been allowed before and requires adding the current user's password hash to the session (as he stated above).

Please sign in to comment.