diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php
index d436ab8eb..31b91108d 100644
--- 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 BookStack\Exceptions\UserUpdateException;
use BookStack\Uploads\Image;
use Exception;
use Images;
@@ -42,7 +43,7 @@ class UserRepo
*/
public function getById($id)
{
- return $this->user->findOrFail($id);
+ return $this->user->newQuery()->findOrFail($id);
}
/**
@@ -135,6 +136,40 @@ class UserRepo
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
@@ -143,7 +178,6 @@ class UserRepo
*/
public function create(array $data, $verifyEmail = false)
{
-
return $this->user->forceCreate([
'name' => $data['name'],
'email' => $data['email'],
diff --git a/app/Exceptions/NotifyException.php b/app/Exceptions/NotifyException.php
index df96b5d12..78ffde05c 100644
--- a/app/Exceptions/NotifyException.php
+++ b/app/Exceptions/NotifyException.php
@@ -11,7 +11,7 @@ class NotifyException extends \Exception
* @param string $message
* @param string $redirectLocation
*/
- public function __construct($message, $redirectLocation)
+ public function __construct(string $message, string $redirectLocation = "/")
{
$this->message = $message;
$this->redirectLocation = $redirectLocation;
diff --git a/app/Exceptions/UserUpdateException.php b/app/Exceptions/UserUpdateException.php
new file mode 100644
index 000000000..eb41dece6
--- /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 24f8b67cb..cc5ada3f2 100644
--- 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\Exceptions\UserUpdateException;
use Illuminate\Http\Request;
use Illuminate\Http\Response;
@@ -15,7 +16,7 @@ class UserController extends Controller
/**
* UserController constructor.
* @param User $user
- * @param \BookStack\Auth\UserRepo $userRepo
+ * @param 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
+ * @throws UserUpdateException
*/
public function store(Request $request)
{
@@ -89,7 +91,7 @@ class UserController extends Controller
if ($request->filled('roles')) {
$roles = $request->get('roles');
- $user->roles()->sync($roles);
+ $this->userRepo->setUserRoles($user, $roles);
}
$this->userRepo->downloadAndAssignUserAvatar($user);
@@ -122,8 +124,9 @@ class UserController extends Controller
/**
* Update the specified user in storage.
* @param Request $request
- * @param int $id
+ * @param int $id
* @return Response
+ * @throws UserUpdateException
*/
public function update(Request $request, $id)
{
@@ -140,13 +143,13 @@ class UserController extends Controller
'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->roles()->sync($roles);
+ $this->userRepo->setUserRoles($user, $roles);
}
// Password updates
@@ -185,7 +188,7 @@ class UserController extends Controller
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]);
}
@@ -194,6 +197,7 @@ class UserController extends Controller
* Remove the specified user from storage.
* @param int $id
* @return Response
+ * @throws \Exception
*/
public function destroy($id)
{
@@ -279,7 +283,7 @@ class UserController extends Controller
$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");
diff --git a/resources/lang/en/errors.php b/resources/lang/en/errors.php
index 7a881e021..b91a0c3e1 100644
--- 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_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.',
diff --git a/tests/Permissions/RolesTest.php b/tests/Permissions/RolesTest.php
index 95cb7cd57..d22946799 100644
--- a/tests/Permissions/RolesTest.php
+++ b/tests/Permissions/RolesTest.php
@@ -78,6 +78,28 @@ class RolesTest extends BrowserKitTest
->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')