Logout user when their activated status is switched to off #10876
Conversation
Signed-off-by: snipe <snipe@snipe.net>
There was a problem hiding this comment.
This looks really solid! I know it sucks to be messing with Auth because it's so scary, but it's also awesome to know that our stuff is buttoned-up really well. And I think because all the other login methods check the Activated flag, that I'm comfortable that this change will probably not start locking out users or causing any kind of ruckus. Nice work and really clean way of handling the problem, love it :)
| @@ -3,7 +3,7 @@ | |||
| return array( | |||
|
|
|||
| 'account_already_exists' => 'An account with the this email already exists.', | |||
| 'account_not_found' => 'The username or password is incorrect.', | |||
| 'account_not_found' => 'The username or password is incorrect or this user is not approved to login.', | |||
There was a problem hiding this comment.
UGH - I hate to have to re-do a string this way, but I can totally see why we would.
| // to inactive (aka unable to login) | ||
| if (($request->user()) && (!$request->user()->isActivated())) { | ||
| Auth::logout(); | ||
| return redirect()->guest('login'); |
There was a problem hiding this comment.
I actually don't know this ->guest() method but so long as that takes you to /login that's good enough for me!
There was a problem hiding this comment.
It's a built-in laravel-ism that still applies middleware (like CSRF protection, etc) but doesn't apply the auth routes.
Hey nerds,
This PR fixes a potential issue in business logic where when a user's status is toggled from activated to deactivated (and therefore should not be able to be logged in anymore). While we would disallow the user from ever logging in again if that flag is toggled, we weren't checking on their activated status on page load (via middleware for example), so a user who had an existing active session could still potentially see things that should be gated behind an activated user account.
This could normally be mitigated by using the
KillAllSessionsconsole command, clearing the contents of thestorage/framework/sessionsdirectory, or changing the cookie name, but all of those options logout ALL users, which could be kind of annoying.This fixes https://huntr.dev/bounties/ebc26354-2414-4f72-88aa-f044aec2b2e1/ (which may not be visible until the CVE is publicly issued.)
We've tested this locally and I think it should be good to go, but always appreciate extra eyeballs. TY!