Kick other sessions when changing password (#13194)

* Kick other session when changing password
Invalidate other sessions when a user password gets changed

* Don't logout admin users when they change passwords.
Cleanup phpstan exceptions

* only restore user if needed

* comment odd behavior

* $current_user typehint

LibreNMS/Authentication/AuthorizerBase.php

@@ -39,12 +39,6 @@ abstract class AuthorizerBase implements Authorizer
         return static::$CAN_UPDATE_PASSWORDS;
     }
 
-    public function changePassword($username, $newpassword)
-    {
-        //not supported by default
-        return false;
-    }
-
     public function canManageUsers()
     {
         return static::$HAS_AUTH_USERMANAGEMENT;

LibreNMS/Authentication/MysqlAuthorizer.php

@@ -18,7 +18,7 @@ class MysqlAuthorizer extends AuthorizerBase
         $username = $credentials['username'] ?? null;
         $password = $credentials['password'] ?? null;
 
-        $user_data = User::thisAuth()->where(['username' => $username])->select('password', 'enabled')->first();
+        $user_data = User::thisAuth()->firstWhere(['username' => $username]);
         $hash = $user_data->password;
         $enabled = $user_data->enabled;
 
@@ -27,8 +27,10 @@ class MysqlAuthorizer extends AuthorizerBase
         }
 
         if (Hash::check($password, $hash)) {
+            // Check if hash algorithm is current and update it if it is not
             if (Hash::needsRehash($hash)) {
-                $this->changePassword($username, $password);
+                $user_data->setPassword($password);
+                $user_data->save();
             }
 
             return true;
@@ -53,25 +55,6 @@ class MysqlAuthorizer extends AuthorizerBase
         }
     }
 
-    public function changePassword($username, $password)
-    {
-        // check if updating passwords is allowed (mostly for classes that extend this)
-        if (! static::$CAN_UPDATE_PASSWORDS) {
-            return false;
-        }
-
-        /** @var User $user */
-        $user = User::thisAuth()->where('username', $username)->first();
-
-        if ($user) {
-            $user->setPassword($password);
-
-            return $user->save();
-        }
-
-        return false;
-    }
-
     public function addUser($username, $password, $level = 0, $email = '', $realname = '', $can_modify_passwd = 1, $descr = '')
     {
         $user_array = get_defined_vars();

LibreNMS/Interfaces/Authentication/Authorizer.php

@@ -85,13 +85,6 @@ interface Authorizer
      */
     public function updateUser($user_id, $realname, $level, $can_modify_passwd, $email);
 
-    /**
-     * @param  string  $username  The $username to update
-     * @param  string  $newpassword
-     * @return bool If the update was successful
-     */
-    public function changePassword($username, $newpassword);
-
     /**
      * Delete a user.
      *

app/Http/Controllers/UserController.php

@@ -31,6 +31,7 @@ use App\Models\AuthLog;
 use App\Models\Dashboard;
 use App\Models\User;
 use App\Models\UserPref;
+use Auth;
 use Illuminate\Support\Str;
 use LibreNMS\Authentication\LegacyAuth;
 use LibreNMS\Config;
@@ -168,6 +169,15 @@ class UserController extends Controller
     {
         if ($request->get('new_password') && $user->canSetPassword($request->user())) {
             $user->setPassword($request->new_password);
+            /** @var User $current_user */
+            $current_user = Auth::user();
+            Auth::setUser($user); // make sure new password is loaded, can only logout other sessions for the active user
+            Auth::logoutOtherDevices($request->new_password);
+
+            // when setting the password on another account, restore back to the user's account.
+            if ($current_user->user_id !== $user->user_id) {
+                Auth::setUser($current_user);
+            }
         }
 
         $user->fill($request->all());
@@ -176,17 +186,15 @@ class UserController extends Controller
             Toastr::success(__('Updated dashboard for :username', ['username' => $user->username]));
         }
 
-        if ($user->isDirty()) {
+        if ($user->save()) {
-            if ($user->save()) {
+            Toastr::success(__('User :username updated', ['username' => $user->username]));
-                Toastr::success(__('User :username updated', ['username' => $user->username]));
-            } else {
-                Toastr::error(__('Failed to update user :username', ['username' => $user->username]));
 
-                return redirect()->back();
+            return redirect(route(Str::contains(URL::previous(), 'preferences') ? 'preferences.index' : 'users.index'));
-            }
         }
 
-        return redirect(route(Str::contains(URL::previous(), 'preferences') ? 'preferences.index' : 'users.index'));
+        Toastr::error(__('Failed to update user :username', ['username' => $user->username]));
+
+        return redirect()->back();
     }
 
     /**

app/Http/Kernel.php

@@ -34,7 +34,7 @@ class Kernel extends HttpKernel
             \App\Http\Middleware\EncryptCookies::class,
             \Illuminate\Cookie\Middleware\AddQueuedCookiesToResponse::class,
             \Illuminate\Session\Middleware\StartSession::class,
-            // \Illuminate\Session\Middleware\AuthenticateSession::class,
+            \Illuminate\Session\Middleware\AuthenticateSession::class,
             \Illuminate\View\Middleware\ShareErrorsFromSession::class,
             \App\Http\Middleware\VerifyCsrfToken::class,
             \Illuminate\Routing\Middleware\SubstituteBindings::class,

phpstan-baseline.neon

@@ -30,16 +30,6 @@ parameters:
 			count: 1
 			path: LibreNMS/Authentication/MysqlAuthorizer.php
 
-		-
-			message: "#^If condition is always true\\.$#"
-			count: 1
-			path: LibreNMS/Authentication/MysqlAuthorizer.php
-
-		-
-			message: "#^Unreachable statement \\- code above always terminates\\.$#"
-			count: 1
-			path: LibreNMS/Authentication/MysqlAuthorizer.php
-
 		-
 			message: "#^Parameter \\#2 \\$password of method LibreNMS\\\\Authentication\\\\MysqlAuthorizer\\:\\:addUser\\(\\) expects string, null given\\.$#"
 			count: 1
@@ -260,16 +250,6 @@ parameters:
 			count: 4
 			path: tests/AddHostTest.php
 
-		-
-			message: "#^Parameter \\#1 \\$username of method LibreNMS\\\\Interfaces\\\\Authentication\\\\Authorizer\\:\\:changePassword\\(\\) expects string, null given\\.$#"
-			count: 1
-			path: tests/AuthHTTPTest.php
-
-		-
-			message: "#^Parameter \\#2 \\$newpassword of method LibreNMS\\\\Interfaces\\\\Authentication\\\\Authorizer\\:\\:changePassword\\(\\) expects string, null given\\.$#"
-			count: 1
-			path: tests/AuthHTTPTest.php
-
 		-
 			message: "#^Parameter \\#1 \\$attr of method LibreNMS\\\\Authentication\\\\SSOAuthorizer\\:\\:authSSOGetAttr\\(\\) expects string, int given\\.$#"
 			count: 2
@@ -280,16 +260,6 @@ parameters:
 			count: 2
 			path: tests/AuthSSOTest.php
 
-		-
-			message: "#^Parameter \\#1 \\$username of method LibreNMS\\\\Interfaces\\\\Authentication\\\\Authorizer\\:\\:changePassword\\(\\) expects string, null given\\.$#"
-			count: 1
-			path: tests/AuthSSOTest.php
-
-		-
-			message: "#^Parameter \\#2 \\$newpassword of method LibreNMS\\\\Interfaces\\\\Authentication\\\\Authorizer\\:\\:changePassword\\(\\) expects string, null given\\.$#"
-			count: 1
-			path: tests/AuthSSOTest.php
-
 		-
 			message: "#^Parameter \\#2 \\$default of function set_null expects null, int given\\.$#"
 			count: 2

tests/AuthHTTPTest.php

@@ -57,7 +57,6 @@ class AuthHTTPTest extends TestCase
         $a = LegacyAuth::reset();
 
         $this->assertFalse($a->canUpdatePasswords());
-        $this->assertFalse($a->changePassword(null, null));
         $this->assertTrue($a->canManageUsers());
         $this->assertTrue($a->canUpdateUsers());
         $this->assertTrue($a->authIsExternal());

tests/AuthSSOTest.php

@@ -227,7 +227,6 @@ class AuthSSOTest extends DBTestCase
         $a = LegacyAuth::reset();
 
         $this->assertFalse($a->canUpdatePasswords());
-        $this->assertFalse($a->changePassword(null, null));
         $this->assertTrue($a->canManageUsers());
         $this->assertTrue($a->canUpdateUsers());
         $this->assertTrue($a->authIsExternal());