Added users-delete API endpoint

- Refactored some delete checks into repo.
- Added tests to cover.
- Moved some translations to align with activity/logging system.

diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php
index 1341e70bc95274438f79cc275bdd189eca480a90..41cdc1c70b35e33b180a0d6f68b2c0965a4e583b 100644 (file)
--- a/app/Auth/UserRepo.php
+++ b/app/Auth/UserRepo.php
@@ -2,13 +2,16 @@
 
 namespace BookStack\Auth;
 
 
 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\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\Exceptions\UserUpdateException;

+use BookStack\Facades\Activity;

 use BookStack\Uploads\UserAvatars;
 use Exception;
 use Illuminate\Database\Eloquent\Builder;
 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)
     {
      */
     public function destroy(User $user, ?int $newOwnerId = null)
     {

+        $this->ensureDeletable($user);
+

         $user->socialAccounts()->delete();
         $user->apiTokens()->delete();
         $user->favourites()->delete();
         $user->socialAccounts()->delete();
         $user->apiTokens()->delete();
         $user->favourites()->delete();


@@ -204,6 +209,22 @@ class UserRepo
                 $this->migrateOwnership($user, $newOwner);
             }
         }
                 $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 5627dd1f80456a7066110833642ed131db4d2d11..bc7263c774228b13cd7c625c10782baee0ed3df0 100644 (file)
--- 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 $signature = 'bookstack:delete-users';
 

-    protected $user;
-

     protected $userRepo;
 
     /**
     protected $userRepo;
 
     /**


@@ -26,9 +24,8 @@ class DeleteUsers extends Command
      */
     protected $description = 'Delete users that are not "admin" or system users';
 
      */
     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();
     }
         $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') {
         $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
             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 ed1a4b13d78170470d93d851d430fe1c1ba56fce..6ca31f0fda440e1b67633a3a9167e1ed5f89c1ae 100644 (file)
--- 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 BookStack\Auth\User;
 use BookStack\Auth\UserRepo;
 use Closure;

+use Illuminate\Http\Request;

 
 class UserApiController extends ApiController
 {
 
 class UserApiController extends ApiController
 {


@@ -19,6 +20,9 @@ class UserApiController extends ApiController
         ],
         'update' => [
         ],
         ],
         'update' => [
         ],

+        'delete' => [
+            'migrate_ownership_id' => ['integer', 'exists:users,id'],
+        ],

     ];
 
     public function __construct(UserRepo $userRepo)
     ];
 
     public function __construct(UserRepo $userRepo)


@@ -56,6 +60,24 @@ class UserApiController extends ApiController
         return response()->json($singleUser);
     }
 
         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.
      */
     /**
      * Format the given user model for single-result display.
      */

diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php
index 3903682eb41a2f11179a89b6abcdff9bcfc572a5..511b4d33ce3fba1b256fdc67d3e79d332b4c97ff 100644 (file)
--- 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);
 
         $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->userRepo->destroy($user, $newOwnerId);

-        $this->showSuccessNotification(trans('settings.users_delete_success'));
-        $this->logActivity(ActivityType::USER_DELETE, $user);

 
         return redirect('/settings/users');
     }
 
         return redirect('/settings/users');
     }

diff --git a/dev/api/requests/users-delete.json b/dev/api/requests/users-delete.json
new file mode 100644 (file)
index 0000000..8a94934
--- /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 83a374d66a083f3f25fc41523f87c0a58deab214..b0d180298886ae0fef2f66f16851019f45d9e29e 100644 (file)
--- 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',
 
     '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',
     // Other
     'commented_on'                => 'commented on',
     'permissions_update'          => 'updated permissions',

diff --git a/resources/lang/en/settings.php b/resources/lang/en/settings.php
index 65e2e5264a93cce73e7a520d14acd3645a9be5bb..d6a35650803304a468c72ac2b72c1a7b746dc399 100755 (executable)
--- 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_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',
     '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 2adc3f7754c8495d4a8b6bd527308bae7245c1d6..01564c7d3f7c9c3cc937150b7cd7794f46b02e89 100644 (file)
--- 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::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 24c825f8f348dfd3ddb43edc565b80681151e133..4a3c4724af731a9505f683218199cd0f5686e2eb 100644 (file)
--- a/tests/Api/UsersApiTest.php
+++ b/tests/Api/UsersApiTest.php
@@ -17,6 +17,14 @@ class UsersApiTest extends TestCase
         // TODO
     }
 
         // 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();
     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));
+    }

 }
 }