From 2cd7a48044ede60c487bbe575fc20ed99c702571 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 3 Feb 2022 15:12:50 +0000 Subject: [PATCH] Added users-delete API endpoint - Refactored some delete checks into repo. - Added tests to cover. - Moved some translations to align with activity/logging system. --- app/Auth/UserRepo.php | 21 +++++++++ app/Console/Commands/DeleteUsers.php | 9 ++-- .../Controllers/Api/UserApiController.php | 22 +++++++++ app/Http/Controllers/UserController.php | 14 ------ dev/api/requests/users-delete.json | 3 ++ resources/lang/en/activities.php | 3 ++ resources/lang/en/settings.php | 1 - routes/api.php | 3 +- tests/Api/UsersApiTest.php | 47 +++++++++++++++++++ 9 files changed, 101 insertions(+), 22 deletions(-) create mode 100644 dev/api/requests/users-delete.json diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php index 1341e70bc..41cdc1c70 100644 --- a/app/Auth/UserRepo.php +++ b/app/Auth/UserRepo.php @@ -2,13 +2,16 @@ namespace BookStack\Auth; +use BookStack\Actions\ActivityType; use BookStack\Entities\EntityProvider; use BookStack\Entities\Models\Book; use BookStack\Entities\Models\Bookshelf; use BookStack\Entities\Models\Chapter; use BookStack\Entities\Models\Page; use BookStack\Exceptions\NotFoundException; +use BookStack\Exceptions\NotifyException; use BookStack\Exceptions\UserUpdateException; +use BookStack\Facades\Activity; use BookStack\Uploads\UserAvatars; use Exception; use Illuminate\Database\Eloquent\Builder; @@ -189,6 +192,8 @@ class UserRepo */ public function destroy(User $user, ?int $newOwnerId = null) { + $this->ensureDeletable($user); + $user->socialAccounts()->delete(); $user->apiTokens()->delete(); $user->favourites()->delete(); @@ -204,6 +209,22 @@ class UserRepo $this->migrateOwnership($user, $newOwner); } } + + Activity::add(ActivityType::USER_DELETE, $user); + } + + /** + * @throws NotifyException + */ + protected function ensureDeletable(User $user): void + { + if ($this->isOnlyAdmin($user)) { + throw new NotifyException(trans('errors.users_cannot_delete_only_admin'), $user->getEditUrl()); + } + + if ($user->system_name === 'public') { + throw new NotifyException(trans('errors.users_cannot_delete_guest'), $user->getEditUrl()); + } } /** diff --git a/app/Console/Commands/DeleteUsers.php b/app/Console/Commands/DeleteUsers.php index 5627dd1f8..bc7263c77 100644 --- a/app/Console/Commands/DeleteUsers.php +++ b/app/Console/Commands/DeleteUsers.php @@ -15,8 +15,6 @@ class DeleteUsers extends Command */ protected $signature = 'bookstack:delete-users'; - protected $user; - protected $userRepo; /** @@ -26,9 +24,8 @@ class DeleteUsers extends Command */ protected $description = 'Delete users that are not "admin" or system users'; - public function __construct(User $user, UserRepo $userRepo) + public function __construct(UserRepo $userRepo) { - $this->user = $user; $this->userRepo = $userRepo; parent::__construct(); } @@ -38,8 +35,8 @@ class DeleteUsers extends Command $confirm = $this->ask('This will delete all users from the system that are not "admin" or system users. Are you sure you want to continue? (Type "yes" to continue)'); $numDeleted = 0; if (strtolower(trim($confirm)) === 'yes') { - $totalUsers = $this->user->count(); - $users = $this->user->where('system_name', '=', null)->with('roles')->get(); + $totalUsers = User::query()->count(); + $users = User::query()->whereNull('system_name')->with('roles')->get(); foreach ($users as $user) { if ($user->hasSystemRole('admin')) { // don't delete users with "admin" role diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php index ed1a4b13d..6ca31f0fd 100644 --- a/app/Http/Controllers/Api/UserApiController.php +++ b/app/Http/Controllers/Api/UserApiController.php @@ -5,6 +5,7 @@ namespace BookStack\Http\Controllers\Api; use BookStack\Auth\User; use BookStack\Auth\UserRepo; use Closure; +use Illuminate\Http\Request; class UserApiController extends ApiController { @@ -19,6 +20,9 @@ class UserApiController extends ApiController ], 'update' => [ ], + 'delete' => [ + 'migrate_ownership_id' => ['integer', 'exists:users,id'], + ], ]; public function __construct(UserRepo $userRepo) @@ -56,6 +60,24 @@ class UserApiController extends ApiController return response()->json($singleUser); } + /** + * Delete a user from the system. + * Can optionally accept a user id via `migrate_ownership_id` to indicate + * who should be the new owner of their related content. + * Requires permission to manage users. + */ + public function delete(Request $request, string $id) + { + $this->checkPermission('users-manage'); + + $user = $this->userRepo->getById($id); + $newOwnerId = $request->get('migrate_ownership_id', null); + + $this->userRepo->destroy($user, $newOwnerId); + + return response('', 204); + } + /** * Format the given user model for single-result display. */ diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 3903682eb..511b4d33c 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -262,21 +262,7 @@ class UserController extends Controller $user = $this->userRepo->getById($id); $newOwnerId = $request->get('new_owner_id', null); - if ($this->userRepo->isOnlyAdmin($user)) { - $this->showErrorNotification(trans('errors.users_cannot_delete_only_admin')); - - return redirect($user->getEditUrl()); - } - - if ($user->system_name === 'public') { - $this->showErrorNotification(trans('errors.users_cannot_delete_guest')); - - return redirect($user->getEditUrl()); - } - $this->userRepo->destroy($user, $newOwnerId); - $this->showSuccessNotification(trans('settings.users_delete_success')); - $this->logActivity(ActivityType::USER_DELETE, $user); return redirect('/settings/users'); } diff --git a/dev/api/requests/users-delete.json b/dev/api/requests/users-delete.json new file mode 100644 index 000000000..8a94934e0 --- /dev/null +++ b/dev/api/requests/users-delete.json @@ -0,0 +1,3 @@ +{ + "migrate_ownership_id": 5 +} \ No newline at end of file diff --git a/resources/lang/en/activities.php b/resources/lang/en/activities.php index 83a374d66..b0d180298 100644 --- a/resources/lang/en/activities.php +++ b/resources/lang/en/activities.php @@ -59,6 +59,9 @@ return [ 'webhook_delete' => 'deleted webhook', 'webhook_delete_notification' => 'Webhook successfully deleted', + // Users + 'user_delete_notification' => 'User successfully removed', + // Other 'commented_on' => 'commented on', 'permissions_update' => 'updated permissions', diff --git a/resources/lang/en/settings.php b/resources/lang/en/settings.php index 65e2e5264..d6a356508 100755 --- a/resources/lang/en/settings.php +++ b/resources/lang/en/settings.php @@ -188,7 +188,6 @@ return [ 'users_migrate_ownership' => 'Migrate Ownership', 'users_migrate_ownership_desc' => 'Select a user here if you want another user to become the owner of all items currently owned by this user.', 'users_none_selected' => 'No user selected', - 'users_delete_success' => 'User successfully removed', 'users_edit' => 'Edit User', 'users_edit_profile' => 'Edit Profile', 'users_edit_success' => 'User successfully updated', diff --git a/routes/api.php b/routes/api.php index 2adc3f775..01564c7d3 100644 --- a/routes/api.php +++ b/routes/api.php @@ -68,4 +68,5 @@ Route::put('shelves/{id}', [BookshelfApiController::class, 'update']); Route::delete('shelves/{id}', [BookshelfApiController::class, 'delete']); Route::get('users', [UserApiController::class, 'list']); -Route::get('users/{id}', [UserApiController::class, 'read']); \ No newline at end of file +Route::get('users/{id}', [UserApiController::class, 'read']); +Route::delete('users/{id}', [UserApiController::class, 'delete']); \ No newline at end of file diff --git a/tests/Api/UsersApiTest.php b/tests/Api/UsersApiTest.php index 24c825f8f..4a3c4724a 100644 --- a/tests/Api/UsersApiTest.php +++ b/tests/Api/UsersApiTest.php @@ -17,6 +17,14 @@ class UsersApiTest extends TestCase // TODO } + public function test_no_endpoints_accessible_in_demo_mode() + { + // TODO + // $this->preventAccessInDemoMode(); + // Can't use directly in constructor as blocks access to docs + // Maybe via route middleware + } + public function test_index_endpoint_returns_expected_shelf() { $this->actingAsApiAdmin(); @@ -61,4 +69,43 @@ class UsersApiTest extends TestCase ], ]); } + + public function test_delete_endpoint() + { + $this->actingAsApiAdmin(); + /** @var User $user */ + $user = User::query()->where('id', '!=', $this->getAdmin()->id) + ->whereNull('system_name') + ->first(); + + $resp = $this->deleteJson($this->baseEndpoint . "/{$user->id}"); + + $resp->assertStatus(204); + $this->assertActivityExists('user_delete', null, $user->logDescriptor()); + } + + public function test_delete_endpoint_fails_deleting_only_admin() + { + $this->actingAsApiAdmin(); + $adminRole = Role::getSystemRole('admin'); + $adminToDelete = $adminRole->users()->first(); + $adminRole->users()->where('id', '!=', $adminToDelete->id)->delete(); + + $resp = $this->deleteJson($this->baseEndpoint . "/{$adminToDelete->id}"); + + $resp->assertStatus(500); + $resp->assertJson($this->errorResponse('You cannot delete the only admin', 500)); + } + + public function test_delete_endpoint_fails_deleting_public_user() + { + $this->actingAsApiAdmin(); + /** @var User $publicUser */ + $publicUser = User::query()->where('system_name', '=', 'public')->first(); + + $resp = $this->deleteJson($this->baseEndpoint . "/{$publicUser->id}"); + + $resp->assertStatus(500); + $resp->assertJson($this->errorResponse('You cannot delete the guest user', 500)); + } }