Added user-update API endpoint

- Required changing the docs generator to handle more complex
  object-style rules. Bit of a hack for some types (password).
- Extracted core update logic to repo for sharing with API.
- Moved user update language string to align with activity/logging
  system.
- Added tests to cover.

diff --git a/app/Api/ApiDocsGenerator.php b/app/Api/ApiDocsGenerator.php
index 4cba7900b7942adca030e193647b99f0622dc6c5..76157c9a5c8ad60924fc8907d598526bb05c6f95 100644 (file)
--- a/app/Api/ApiDocsGenerator.php
+++ b/app/Api/ApiDocsGenerator.php
@@ -3,11 +3,13 @@
 namespace BookStack\Api;
 
 use BookStack\Http\Controllers\Api\ApiController;
 namespace BookStack\Api;
 
 use BookStack\Http\Controllers\Api\ApiController;

+use Exception;

 use Illuminate\Contracts\Container\BindingResolutionException;
 use Illuminate\Support\Collection;
 use Illuminate\Support\Facades\Cache;
 use Illuminate\Support\Facades\Route;
 use Illuminate\Support\Str;
 use Illuminate\Contracts\Container\BindingResolutionException;
 use Illuminate\Support\Collection;
 use Illuminate\Support\Facades\Cache;
 use Illuminate\Support\Facades\Route;
 use Illuminate\Support\Str;

+use Illuminate\Validation\Rules\Password;

 use ReflectionClass;
 use ReflectionException;
 use ReflectionMethod;
 use ReflectionClass;
 use ReflectionException;
 use ReflectionMethod;


@@ -100,11 +102,36 @@ class ApiDocsGenerator
             $this->controllerClasses[$className] = $class;
         }
 
             $this->controllerClasses[$className] = $class;
         }
 

-        $rules = $class->getValdationRules()[$methodName] ?? [];
+        $rules = collect($class->getValidationRules()[$methodName] ?? [])->map(function($validations) {
+            return array_map(function($validation) {
+                return $this->getValidationAsString($validation);
+            }, $validations);
+        })->toArray();

 
         return empty($rules) ? null : $rules;
     }
 
 
         return empty($rules) ? null : $rules;
     }
 

+    /**
+     * Convert the given validation message to a readable string.
+     */
+    protected function getValidationAsString($validation): string
+    {
+        if (is_string($validation)) {
+            return $validation;
+        }
+
+        if (is_object($validation) && method_exists($validation, '__toString')) {
+            return strval($validation);
+        }
+
+        if ($validation instanceof Password) {
+            return 'min:8';
+        }
+
+        $class = get_class($validation);
+        throw new Exception("Cannot provide string representation of rule for class: {$class}");
+    }
+

     /**
      * Parse out the description text from a class method comment.
      */
     /**
      * Parse out the description text from a class method comment.
      */

diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php
index 41cdc1c70b35e33b180a0d6f68b2c0965a4e583b..cb0c0d2fad2113ab4272ad69abec4d9f3c4f6981 100644 (file)
--- a/app/Auth/UserRepo.php
+++ b/app/Auth/UserRepo.php
@@ -58,12 +58,13 @@ class UserRepo
     /**
      * Get all users as Builder for API
      */
     /**
      * Get all users as Builder for API
      */

-    public function getApiUsersBuilder() : Builder
+    public function getApiUsersBuilder(): Builder

     {
         return User::query()->select(['*'])
             ->scopes('withLastActivityAt')
             ->with(['avatar']);
     }
     {
         return User::query()->select(['*'])
             ->scopes('withLastActivityAt')
             ->with(['avatar']);
     }

+

     /**
      * Get all the users with their permissions in a paginated format.
      * Note: Due to the use of email search this should only be used when
     /**
      * Get all the users with their permissions in a paginated format.
      * Note: Due to the use of email search this should only be used when


@@ -170,10 +171,10 @@ class UserRepo
     public function create(array $data, bool $emailConfirmed = false): User
     {
         $details = [
     public function create(array $data, bool $emailConfirmed = false): User
     {
         $details = [

-            'name'             => $data['name'],
-            'email'            => $data['email'],
-            'password'         => bcrypt($data['password']),
-            'email_confirmed'  => $emailConfirmed,
+            'name' => $data['name'],
+            'email' => $data['email'],
+            'password' => bcrypt($data['password']),
+            'email_confirmed' => $emailConfirmed,

             'external_auth_id' => $data['external_auth_id'] ?? '',
         ];
 
             'external_auth_id' => $data['external_auth_id'] ?? '',
         ];
 


@@ -185,6 +186,44 @@ class UserRepo
         return $user;
     }
 
         return $user;
     }
 

+    /**
+     * Update the given user with the given data.
+     * @param array{name: ?string, email: ?string, external_auth_id: ?string, password: ?string, roles: ?array<int>, language: ?string} $data
+     * @throws UserUpdateException
+     */
+    public function update(User $user, array $data, bool $manageUsersAllowed): User
+    {
+        if (!empty($data['name'])) {
+            $user->name = $data['name'];
+            $user->refreshSlug();
+        }
+
+        if (!empty($data['email']) && $manageUsersAllowed) {
+            $user->email = $data['email'];
+        }
+
+        if (!empty($data['external_auth_id']) && $manageUsersAllowed) {
+            $user->external_auth_id = $data['external_auth_id'];
+        }
+
+        if (isset($data['roles']) && $manageUsersAllowed) {
+            $this->setUserRoles($user, $data['roles']);
+        }
+
+        if (!empty($data['password'])) {
+            $user->password = bcrypt($data['password']);
+        }
+
+        if (!empty($data['language'])) {
+            setting()->putUser($user, 'language', $data['language']);
+        }
+
+        $user->save();
+        Activity::add(ActivityType::USER_UPDATE, $user);
+
+        return $user;
+    }
+

     /**
      * Remove the given user from storage, Delete all related content.
      *
     /**
      * Remove the given user from storage, Delete all related content.
      *


@@ -252,10 +291,10 @@ class UserRepo
         };
 
         return [
         };
 
         return [

-            'pages'    => $query(Page::visible()->where('draft', '=', false)),
+            'pages' => $query(Page::visible()->where('draft', '=', false)),

             'chapters' => $query(Chapter::visible()),
             'chapters' => $query(Chapter::visible()),

-            'books'    => $query(Book::visible()),
-            'shelves'  => $query(Bookshelf::visible()),
+            'books' => $query(Book::visible()),
+            'shelves' => $query(Bookshelf::visible()),

         ];
     }
 
         ];
     }
 


@@ -267,10 +306,10 @@ class UserRepo
         $createdBy = ['created_by' => $user->id];
 
         return [
         $createdBy = ['created_by' => $user->id];
 
         return [

-            'pages'       => Page::visible()->where($createdBy)->count(),
-            'chapters'    => Chapter::visible()->where($createdBy)->count(),
-            'books'       => Book::visible()->where($createdBy)->count(),
-            'shelves'     => Bookshelf::visible()->where($createdBy)->count(),
+            'pages' => Page::visible()->where($createdBy)->count(),
+            'chapters' => Chapter::visible()->where($createdBy)->count(),
+            'books' => Book::visible()->where($createdBy)->count(),
+            'shelves' => Bookshelf::visible()->where($createdBy)->count(),

         ];
     }
 
         ];
     }
 

diff --git a/app/Http/Controllers/Api/ApiController.php b/app/Http/Controllers/Api/ApiController.php
index 63f942412c577834cdcc244901c4c05fd9d9887e..9652654be4d79a364d956149943cc266af7a6a18 100644 (file)
--- a/app/Http/Controllers/Api/ApiController.php
+++ b/app/Http/Controllers/Api/ApiController.php
@@ -10,7 +10,6 @@ use Illuminate\Http\JsonResponse;
 abstract class ApiController extends Controller
 {
     protected $rules = [];
 abstract class ApiController extends Controller
 {
     protected $rules = [];

-    protected $fieldsToExpose = [];

 
     /**
      * Provide a paginated listing JSON response in a standard format
 
     /**
      * Provide a paginated listing JSON response in a standard format


@@ -31,7 +30,7 @@ abstract class ApiController extends Controller
      * Get the validation rules for this controller.
      * Defaults to a $rules property but can be a rules() method.
      */
      * Get the validation rules for this controller.
      * Defaults to a $rules property but can be a rules() method.
      */

-    public function getValdationRules(): array
+    public function getValidationRules(): array

     {
         if (method_exists($this, 'rules')) {
             return $this->rules();
     {
         if (method_exists($this, 'rules')) {
             return $this->rules();

diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php
index 6ca31f0fda440e1b67633a3a9167e1ed5f89c1ae..88350e0ea1078ac6f6659d471e4e811653af93d4 100644 (file)
--- a/app/Http/Controllers/Api/UserApiController.php
+++ b/app/Http/Controllers/Api/UserApiController.php
@@ -6,6 +6,8 @@ use BookStack\Auth\User;
 use BookStack\Auth\UserRepo;
 use Closure;
 use Illuminate\Http\Request;
 use BookStack\Auth\UserRepo;
 use Closure;
 use Illuminate\Http\Request;

+use Illuminate\Validation\Rules\Password;
+use Illuminate\Validation\Rules\Unique;

 
 class UserApiController extends ApiController
 {
 
 class UserApiController extends ApiController
 {


@@ -15,21 +17,35 @@ class UserApiController extends ApiController
         'email', 'created_at', 'updated_at', 'last_activity_at', 'external_auth_id'
     ];
 
         'email', 'created_at', 'updated_at', 'last_activity_at', 'external_auth_id'
     ];
 

-    protected $rules = [
-        'create' => [
-        ],
-        'update' => [
-        ],
-        'delete' => [
-            'migrate_ownership_id' => ['integer', 'exists:users,id'],
-        ],
-    ];
-

     public function __construct(UserRepo $userRepo)
     {
         $this->userRepo = $userRepo;
     }
 
     public function __construct(UserRepo $userRepo)
     {
         $this->userRepo = $userRepo;
     }
 

+    protected function rules(int $userId = null): array
+    {
+        return [
+            'create' => [
+            ],
+            'update' => [
+                'name' => ['min:2'],
+                'email' => [
+                    'min:2',
+                    'email',
+                    (new Unique('users', 'email'))->ignore($userId ?? null)
+                ],
+                'external_auth_id' => ['string'],
+                'language' => ['string'],
+                'password' => [Password::default()],
+                'roles' => ['array'],
+                'roles.*' => ['integer'],
+            ],
+            'delete' => [
+                'migrate_ownership_id' => ['integer', 'exists:users,id'],
+            ],
+        ];
+    }
+

     /**
      * Get a listing of users in the system.
      * Requires permission to manage users.
     /**
      * Get a listing of users in the system.
      * Requires permission to manage users.


@@ -54,10 +70,26 @@ class UserApiController extends ApiController
     {
         $this->checkPermission('users-manage');
 
     {
         $this->checkPermission('users-manage');
 

-        $singleUser = $this->userRepo->getById($id);
-        $this->singleFormatter($singleUser);
+        $user = $this->userRepo->getById($id);
+        $this->singleFormatter($user);
+
+        return response()->json($user);
+    }
+
+    /**
+     * Update an existing user in the system.
+     * @throws \BookStack\Exceptions\UserUpdateException
+     */
+    public function update(Request $request, string $id)
+    {
+        $this->checkPermission('users-manage');
+
+        $data = $this->validate($request, $this->rules($id)['update']);
+        $user = $this->userRepo->getById($id);
+        $this->userRepo->update($user, $data, userCan('users-manage'));
+        $this->singleFormatter($user);

 
 

-        return response()->json($singleUser);
+        return response()->json($user);

     }
 
     /**
     }
 
     /**

diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php
index 511b4d33ce3fba1b256fdc67d3e79d332b4c97ff..9e702a1d74bf5f14fc860a0d895c8d830ec83dc7 100644 (file)
--- a/app/Http/Controllers/UserController.php
+++ b/app/Http/Controllers/UserController.php
@@ -168,51 +168,19 @@ class UserController extends Controller
         $this->preventAccessInDemoMode();
         $this->checkPermissionOrCurrentUser('users-manage', $id);
 
         $this->preventAccessInDemoMode();
         $this->checkPermissionOrCurrentUser('users-manage', $id);
 

-        $this->validate($request, [
+        $validated = $this->validate($request, [

             'name'             => ['min:2'],
             'email'            => ['min:2', 'email', 'unique:users,email,' . $id],
             'password'         => ['required_with:password_confirm', Password::default()],
             'password-confirm' => ['same:password', 'required_with:password'],
             'name'             => ['min:2'],
             'email'            => ['min:2', 'email', 'unique:users,email,' . $id],
             'password'         => ['required_with:password_confirm', Password::default()],
             'password-confirm' => ['same:password', 'required_with:password'],

-            'setting'          => ['array'],
+            'language'         => ['string'],
+            'roles'            => ['array'],
+            'roles.*'          => ['integer'],

             'profile_image'    => array_merge(['nullable'], $this->getImageValidationRules()),
         ]);
 
         $user = $this->userRepo->getById($id);
             'profile_image'    => array_merge(['nullable'], $this->getImageValidationRules()),
         ]);
 
         $user = $this->userRepo->getById($id);

-        $user->fill($request->except(['email']));
-
-        // Email updates
-        if (userCan('users-manage') && $request->filled('email')) {
-            $user->email = $request->get('email');
-        }
-
-        // Refresh the slug if the user's name has changed
-        if ($user->isDirty('name')) {
-            $user->refreshSlug();
-        }
-
-        // Role updates
-        if (userCan('users-manage') && $request->filled('roles')) {
-            $roles = $request->get('roles');
-            $this->userRepo->setUserRoles($user, $roles);
-        }
-
-        // Password updates
-        if ($request->filled('password')) {
-            $password = $request->get('password');
-            $user->password = bcrypt($password);
-        }
-
-        // External auth id updates
-        if (user()->can('users-manage') && $request->filled('external_auth_id')) {
-            $user->external_auth_id = $request->get('external_auth_id');
-        }
-
-        // Save user-specific settings
-        if ($request->filled('setting')) {
-            foreach ($request->get('setting') as $key => $value) {
-                setting()->putUser($user, $key, $value);
-            }
-        }
+        $this->userRepo->update($user, $validated, userCan('users-manage'));

 
         // Save profile image if in request
         if ($request->hasFile('profile_image')) {
 
         // Save profile image if in request
         if ($request->hasFile('profile_image')) {


@@ -220,6 +188,7 @@ class UserController extends Controller
             $this->imageRepo->destroyImage($user->avatar);
             $image = $this->imageRepo->saveNew($imageUpload, 'user', $user->id);
             $user->image_id = $image->id;
             $this->imageRepo->destroyImage($user->avatar);
             $image = $this->imageRepo->saveNew($imageUpload, 'user', $user->id);
             $user->image_id = $image->id;

+            $user->save();

         }
 
         // Delete the profile image if reset option is in request
         }
 
         // Delete the profile image if reset option is in request


@@ -227,11 +196,7 @@ class UserController extends Controller
             $this->imageRepo->destroyImage($user->avatar);
         }
 
             $this->imageRepo->destroyImage($user->avatar);
         }
 

-        $user->save();
-        $this->showSuccessNotification(trans('settings.users_edit_success'));
-        $this->logActivity(ActivityType::USER_UPDATE, $user);
-
-        $redirectUrl = userCan('users-manage') ? '/settings/users' : ('/settings/users/' . $user->id);
+        $redirectUrl = userCan('users-manage') ? '/settings/users' : "/settings/users/{$user->id}";

 
         return redirect($redirectUrl);
     }
 
         return redirect($redirectUrl);
     }

diff --git a/app/Providers/AuthServiceProvider.php b/app/Providers/AuthServiceProvider.php
index b301604a519e9b95e5bf5881f7a93c0f78cfb88e..a4022cc500968da5fcc4aa5973517a9375749082 100644 (file)
--- a/app/Providers/AuthServiceProvider.php
+++ b/app/Providers/AuthServiceProvider.php
@@ -23,6 +23,7 @@ class AuthServiceProvider extends ServiceProvider
     public function boot()
     {
         // Password Configuration
     public function boot()
     {
         // Password Configuration

+        // Changes here must be reflected in ApiDocsGenerate@getValidationAsString.

         Password::defaults(function () {
             return Password::min(8);
         });
         Password::defaults(function () {
             return Password::min(8);
         });

diff --git a/resources/lang/en/activities.php b/resources/lang/en/activities.php
index b0d180298886ae0fef2f66f16851019f45d9e29e..77c39b50c8c3be5cb6110c074b016ef9aca122e8 100644 (file)
--- a/resources/lang/en/activities.php
+++ b/resources/lang/en/activities.php
@@ -60,6 +60,7 @@ return [
     'webhook_delete_notification' => 'Webhook successfully deleted',
 
     // Users
     'webhook_delete_notification' => 'Webhook successfully deleted',
 
     // Users

+    'user_update_notification' => 'User successfully updated',

     'user_delete_notification' => 'User successfully removed',
 
     // Other
     'user_delete_notification' => 'User successfully removed',
 
     // Other

diff --git a/resources/lang/en/settings.php b/resources/lang/en/settings.php
index d6a35650803304a468c72ac2b72c1a7b746dc399..bfe99c98f82cb9abb4523d6cebe1ede5082fa003 100755 (executable)
--- a/resources/lang/en/settings.php
+++ b/resources/lang/en/settings.php
@@ -190,7 +190,6 @@ return [
     'users_none_selected' => 'No user selected',
     'users_edit' => 'Edit User',
     'users_edit_profile' => 'Edit Profile',
     'users_none_selected' => 'No user selected',
     'users_edit' => 'Edit User',
     'users_edit_profile' => 'Edit Profile',

-    'users_edit_success' => 'User successfully updated',

     'users_avatar' => 'User Avatar',
     'users_avatar_desc' => 'Select an image to represent this user. This should be approx 256px square.',
     'users_preferred_language' => 'Preferred Language',
     'users_avatar' => 'User Avatar',
     'users_avatar_desc' => 'Select an image to represent this user. This should be approx 256px square.',
     'users_preferred_language' => 'Preferred Language',

diff --git a/resources/views/users/parts/language-option-row.blade.php b/resources/views/users/parts/language-option-row.blade.php
index 82907b53dc617f49c9abd6978e38d7cfb5507d48..cbb0b05264474d4a440b179edab2fbed32cfa3ef 100644 (file)
--- a/resources/views/users/parts/language-option-row.blade.php
+++ b/resources/views/users/parts/language-option-row.blade.php
@@ -9,7 +9,7 @@ $value - Currently selected lanuage value
         </p>
     </div>
     <div>
         </p>
     </div>
     <div>

-        <select name="setting[language]" id="user-language">
+        <select name="language" id="user-language">

             @foreach(trans('settings.language_select') as $lang => $label)
                 <option @if($value === $lang) selected @endif value="{{ $lang }}">{{ $label }}</option>
             @endforeach
             @foreach(trans('settings.language_select') as $lang => $label)
                 <option @if($value === $lang) selected @endif value="{{ $lang }}">{{ $label }}</option>
             @endforeach

diff --git a/routes/api.php b/routes/api.php
index 01564c7d3f7c9c3cc937150b7cd7794f46b02e89..0325d7c2af568515d0942ff9b85b01a83a92dfc7 100644 (file)
--- a/routes/api.php
+++ b/routes/api.php
@@ -69,4 +69,5 @@ Route::delete('shelves/{id}', [BookshelfApiController::class, 'delete']);
 
 Route::get('users', [UserApiController::class, 'list']);
 Route::get('users/{id}', [UserApiController::class, 'read']);
 
 Route::get('users', [UserApiController::class, 'list']);
 Route::get('users/{id}', [UserApiController::class, 'read']);

+Route::put('users/{id}', [UserApiController::class, 'update']);

 Route::delete('users/{id}', [UserApiController::class, 'delete']);
\ No newline at end of file
 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 4a3c4724af731a9505f683218199cd0f5686e2eb..19b7b0adcad20356d0e43edd397784dc4de932fa 100644 (file)
--- a/tests/Api/UsersApiTest.php
+++ b/tests/Api/UsersApiTest.php
@@ -4,6 +4,8 @@ namespace Tests\Api;
 
 use BookStack\Auth\Role;
 use BookStack\Auth\User;
 
 use BookStack\Auth\Role;
 use BookStack\Auth\User;

+use Illuminate\Support\Facades\Auth;
+use Illuminate\Support\Facades\Hash;

 use Tests\TestCase;
 
 class UsersApiTest extends TestCase
 use Tests\TestCase;
 
 class UsersApiTest extends TestCase


@@ -70,6 +72,53 @@ class UsersApiTest extends TestCase
         ]);
     }
 
         ]);
     }
 

+    public function test_update_endpoint()
+    {
+        $this->actingAsApiAdmin();
+        /** @var User $user */
+        $user = $this->getAdmin();
+        $roles = Role::query()->pluck('id');
+        $resp = $this->putJson($this->baseEndpoint . "/{$user->id}", [
+            'name' => 'My updated user',
+            'email' => '[email protected]',
+            'roles' => $roles,
+            'external_auth_id' => 'btest',
+            'password' => 'barrytester',
+            'language' => 'fr',
+        ]);
+
+        $resp->assertStatus(200);
+        $resp->assertJson([
+            'id' => $user->id,
+            'name' => 'My updated user',
+            'email' => '[email protected]',
+            'external_auth_id' => 'btest',
+        ]);
+        $user->refresh();
+        $this->assertEquals('fr', setting()->getUser($user, 'language'));
+        $this->assertEquals(count($roles), $user->roles()->count());
+        $this->assertNotEquals('barrytester', $user->password);
+        $this->assertTrue(Hash::check('barrytester', $user->password));
+    }
+
+    public function test_update_endpoint_does_not_remove_info_if_not_provided()
+    {
+        $this->actingAsApiAdmin();
+        /** @var User $user */
+        $user = $this->getAdmin();
+        $roleCount = $user->roles()->count();
+        $resp = $this->putJson($this->baseEndpoint . "/{$user->id}", []);
+
+        $resp->assertStatus(200);
+        $this->assertDatabaseHas('users', [
+            'id' => $user->id,
+            'name' => $user->name,
+            'email' => $user->email,
+            'password' => $user->password,
+        ]);
+        $this->assertEquals($roleCount, $user->roles()->count());
+    }
+

     public function test_delete_endpoint()
     {
         $this->actingAsApiAdmin();
     public function test_delete_endpoint()
     {
         $this->actingAsApiAdmin();