Added check for last admin on role change

Will show error message if last admin and admin role is removed.
Closes #1124
Also cleaned up user controller a little.

diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php
index d436ab8eb0e72f2eeee6f5f04198ae6a57f2a4a0..31b91108d2b11e1e28dbb233f4f249dc3092ba38 100644 (file)
--- a/app/Auth/UserRepo.php
+++ b/app/Auth/UserRepo.php
@@ -3,6 +3,7 @@
 use Activity;
 use BookStack\Entities\Repos\EntityRepo;
 use BookStack\Exceptions\NotFoundException;
 use Activity;
 use BookStack\Entities\Repos\EntityRepo;
 use BookStack\Exceptions\NotFoundException;

+use BookStack\Exceptions\UserUpdateException;

 use BookStack\Uploads\Image;
 use Exception;
 use Images;
 use BookStack\Uploads\Image;
 use Exception;
 use Images;


@@ -42,7 +43,7 @@ class UserRepo
      */
     public function getById($id)
     {
      */
     public function getById($id)
     {

-        return $this->user->findOrFail($id);
+        return $this->user->newQuery()->findOrFail($id);

     }
 
     /**
     }
 
     /**


@@ -135,6 +136,40 @@ class UserRepo
         return true;
     }
 
         return true;
     }
 

+    /**
+     * Set the assigned user roles via an array of role IDs.
+     * @param User $user
+     * @param array $roles
+     * @throws UserUpdateException
+     */
+    public function setUserRoles(User $user, array $roles)
+    {
+        if ($this->demotingLastAdmin($user, $roles)) {
+            throw new UserUpdateException(trans('errors.role_cannot_remove_only_admin'), $user->getEditUrl());
+        }
+
+        $user->roles()->sync($roles);
+    }
+
+    /**
+     * Check if the given user is the last admin and their new roles no longer
+     * contains the admin role.
+     * @param User $user
+     * @param array $newRoles
+     * @return bool
+     */
+    protected function demotingLastAdmin(User $user, array $newRoles) : bool
+    {
+        if ($this->isOnlyAdmin($user)) {
+            $adminRole = $this->role->getSystemRole('admin');
+            if (!in_array(strval($adminRole->id), $newRoles)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+

     /**
      * Create a new basic instance of user.
      * @param array $data
     /**
      * Create a new basic instance of user.
      * @param array $data


@@ -143,7 +178,6 @@ class UserRepo
      */
     public function create(array $data, $verifyEmail = false)
     {
      */
     public function create(array $data, $verifyEmail = false)
     {

-

         return $this->user->forceCreate([
             'name'     => $data['name'],
             'email'    => $data['email'],
         return $this->user->forceCreate([
             'name'     => $data['name'],
             'email'    => $data['email'],

diff --git a/app/Exceptions/NotifyException.php b/app/Exceptions/NotifyException.php
index df96b5d12d204c660fc06b36bbe7cd2bf7baaeaf..78ffde05c07e28655ad2cb25ca5a4f04553d2dfc 100644 (file)
--- a/app/Exceptions/NotifyException.php
+++ b/app/Exceptions/NotifyException.php
@@ -11,7 +11,7 @@ class NotifyException extends \Exception
      * @param string $message
      * @param string    $redirectLocation
      */
      * @param string $message
      * @param string    $redirectLocation
      */

-    public function __construct($message, $redirectLocation)
+    public function __construct(string $message, string $redirectLocation = "/")

     {
         $this->message = $message;
         $this->redirectLocation = $redirectLocation;
     {
         $this->message = $message;
         $this->redirectLocation = $redirectLocation;

diff --git a/app/Exceptions/UserUpdateException.php b/app/Exceptions/UserUpdateException.php
new file mode 100644 (file)
index 0000000..eb41dec
--- /dev/null
+++ b/app/Exceptions/UserUpdateException.php
@@ -0,0 +1,3 @@
+<?php namespace BookStack\Exceptions;
+
+class UserUpdateException extends NotifyException {}

diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php
index 24f8b67cbc4663a83e6777d585326b7759bc9759..cc5ada3f283d2bd4e24f01cc9235e637b0ffa9a1 100644 (file)
--- a/app/Http/Controllers/UserController.php
+++ b/app/Http/Controllers/UserController.php
@@ -3,6 +3,7 @@
 use BookStack\Auth\Access\SocialAuthService;
 use BookStack\Auth\User;
 use BookStack\Auth\UserRepo;
 use BookStack\Auth\Access\SocialAuthService;
 use BookStack\Auth\User;
 use BookStack\Auth\UserRepo;

+use BookStack\Exceptions\UserUpdateException;

 use Illuminate\Http\Request;
 use Illuminate\Http\Response;
 
 use Illuminate\Http\Request;
 use Illuminate\Http\Response;
 


@@ -15,7 +16,7 @@ class UserController extends Controller
     /**
      * UserController constructor.
      * @param User     $user
     /**
      * UserController constructor.
      * @param User     $user

-     * @param \BookStack\Auth\UserRepo $userRepo
+     * @param UserRepo $userRepo

      */
     public function __construct(User $user, UserRepo $userRepo)
     {
      */
     public function __construct(User $user, UserRepo $userRepo)
     {


@@ -59,6 +60,7 @@ class UserController extends Controller
      * Store a newly created user in storage.
      * @param  Request $request
      * @return Response
      * Store a newly created user in storage.
      * @param  Request $request
      * @return Response

+     * @throws UserUpdateException

      */
     public function store(Request $request)
     {
      */
     public function store(Request $request)
     {


@@ -89,7 +91,7 @@ class UserController extends Controller
 
         if ($request->filled('roles')) {
             $roles = $request->get('roles');
 
         if ($request->filled('roles')) {
             $roles = $request->get('roles');

-            $user->roles()->sync($roles);
+            $this->userRepo->setUserRoles($user, $roles);

         }
 
         $this->userRepo->downloadAndAssignUserAvatar($user);
         }
 
         $this->userRepo->downloadAndAssignUserAvatar($user);


@@ -122,8 +124,9 @@ class UserController extends Controller
     /**
      * Update the specified user in storage.
      * @param  Request $request
     /**
      * Update the specified user in storage.
      * @param  Request $request

-     * @param  int     $id
+     * @param  int $id

      * @return Response
      * @return Response

+     * @throws UserUpdateException

      */
     public function update(Request $request, $id)
     {
      */
     public function update(Request $request, $id)
     {


@@ -140,13 +143,13 @@ class UserController extends Controller
             'setting'          => 'array'
         ]);
 
             'setting'          => 'array'
         ]);
 

-        $user = $this->user->findOrFail($id);
+        $user = $this->userRepo->getById($id);

         $user->fill($request->all());
 
         // Role updates
         if (userCan('users-manage') && $request->filled('roles')) {
             $roles = $request->get('roles');
         $user->fill($request->all());
 
         // Role updates
         if (userCan('users-manage') && $request->filled('roles')) {
             $roles = $request->get('roles');

-            $user->roles()->sync($roles);
+            $this->userRepo->setUserRoles($user, $roles);

         }
 
         // Password updates
         }
 
         // Password updates


@@ -185,7 +188,7 @@ class UserController extends Controller
             return $this->currentUser->id == $id;
         });
 
             return $this->currentUser->id == $id;
         });
 

-        $user = $this->user->findOrFail($id);
+        $user = $this->userRepo->getById($id);

         $this->setPageTitle(trans('settings.users_delete_named', ['userName' => $user->name]));
         return view('users/delete', ['user' => $user]);
     }
         $this->setPageTitle(trans('settings.users_delete_named', ['userName' => $user->name]));
         return view('users/delete', ['user' => $user]);
     }


@@ -194,6 +197,7 @@ class UserController extends Controller
      * Remove the specified user from storage.
      * @param  int $id
      * @return Response
      * Remove the specified user from storage.
      * @param  int $id
      * @return Response

+     * @throws \Exception

      */
     public function destroy($id)
     {
      */
     public function destroy($id)
     {


@@ -279,7 +283,7 @@ class UserController extends Controller
             $viewType = 'list';
         }
 
             $viewType = 'list';
         }
 

-        $user = $this->user->findOrFail($id);
+        $user = $this->userRepo->getById($id);

         setting()->putUser($user, 'bookshelves_view_type', $viewType);
 
         return redirect()->back(302, [], "/settings/users/$id");
         setting()->putUser($user, 'bookshelves_view_type', $viewType);
 
         return redirect()->back(302, [], "/settings/users/$id");

diff --git a/resources/lang/en/errors.php b/resources/lang/en/errors.php
index 7a881e021a70ba94db76e380d7f71e8f4612c460..b91a0c3e11c96cfdc6f85d4f9940c472476610b2 100644 (file)
--- a/resources/lang/en/errors.php
+++ b/resources/lang/en/errors.php
@@ -64,6 +64,7 @@ return [
     'role_cannot_be_edited' => 'This role cannot be edited',
     'role_system_cannot_be_deleted' => 'This role is a system role and cannot be deleted',
     'role_registration_default_cannot_delete' => 'This role cannot be deleted while set as the default registration role',
     'role_cannot_be_edited' => 'This role cannot be edited',
     'role_system_cannot_be_deleted' => 'This role is a system role and cannot be deleted',
     'role_registration_default_cannot_delete' => 'This role cannot be deleted while set as the default registration role',

+    'role_cannot_remove_only_admin' => 'This user is the only user assigned to the administrator role. Assign the administrator role to another user before attempting to remove it here.',

 
     // Comments
     'comment_list' => 'An error occurred while fetching the comments.',
 
     // Comments
     'comment_list' => 'An error occurred while fetching the comments.',

diff --git a/tests/Permissions/RolesTest.php b/tests/Permissions/RolesTest.php
index 95cb7cd5726ec7e54bd31902a86511f6a745386b..d2294679936c18f72fcc27d20107e1e4c4039d5d 100644 (file)
--- a/tests/Permissions/RolesTest.php
+++ b/tests/Permissions/RolesTest.php
@@ -78,6 +78,28 @@ class RolesTest extends BrowserKitTest
             ->dontSee($testRoleUpdateName);
     }
 
             ->dontSee($testRoleUpdateName);
     }
 

+    public function test_admin_role_cannot_be_removed_if_last_admin()
+    {
+        $adminRole = Role::where('system_name', '=', 'admin')->first();
+        $adminUser = $this->getAdmin();
+        $adminRole->users()->where('id', '!=', $adminUser->id)->delete();
+        $this->assertEquals($adminRole->users()->count(), 1);
+
+        $viewerRole = $this->getViewer()->roles()->first();
+
+        $editUrl = '/settings/users/' . $adminUser->id;
+        $this->actingAs($adminUser)->put($editUrl, [
+            'name' => $adminUser->name,
+            'email' => $adminUser->email,
+            'roles' => [
+                'viewer' => strval($viewerRole->id),
+            ]
+        ])->followRedirects();
+
+        $this->seePageIs($editUrl);
+        $this->see('This user is the only user assigned to the administrator role');
+    }
+

     public function test_manage_user_permission()
     {
         $this->actingAs($this->user)->visit('/settings/users')
     public function test_manage_user_permission()
     {
         $this->actingAs($this->user)->visit('/settings/users')