From 9e1c8ec82aa856d2f17a07b9f16e9e4cafe1e073 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 3 Feb 2022 16:52:28 +0000 Subject: [PATCH] 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. --- app/Api/ApiDocsGenerator.php | 29 ++++++++- app/Auth/UserRepo.php | 63 +++++++++++++++---- app/Http/Controllers/Api/ApiController.php | 3 +- .../Controllers/Api/UserApiController.php | 58 +++++++++++++---- app/Http/Controllers/UserController.php | 49 +++------------ app/Providers/AuthServiceProvider.php | 1 + resources/lang/en/activities.php | 1 + resources/lang/en/settings.php | 1 - .../users/parts/language-option-row.blade.php | 2 +- routes/api.php | 1 + tests/Api/UsersApiTest.php | 49 +++++++++++++++ 11 files changed, 185 insertions(+), 72 deletions(-) diff --git a/app/Api/ApiDocsGenerator.php b/app/Api/ApiDocsGenerator.php index 4cba7900b..76157c9a5 100644 --- a/app/Api/ApiDocsGenerator.php +++ b/app/Api/ApiDocsGenerator.php @@ -3,11 +3,13 @@ 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\Validation\Rules\Password; use ReflectionClass; use ReflectionException; use ReflectionMethod; @@ -100,11 +102,36 @@ class ApiDocsGenerator $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; } + /** + * 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. */ diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php index 41cdc1c70..cb0c0d2fa 100644 --- a/app/Auth/UserRepo.php +++ b/app/Auth/UserRepo.php @@ -58,12 +58,13 @@ class UserRepo /** * Get all users as Builder for API */ - public function getApiUsersBuilder() : Builder + public function getApiUsersBuilder(): Builder { 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 @@ -170,10 +171,10 @@ class UserRepo 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'] ?? '', ]; @@ -185,6 +186,44 @@ class UserRepo return $user; } + /** + * Update the given user with the given data. + * @param array{name: ?string, email: ?string, external_auth_id: ?string, password: ?string, roles: ?array, 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. * @@ -252,10 +291,10 @@ class UserRepo }; return [ - 'pages' => $query(Page::visible()->where('draft', '=', false)), + 'pages' => $query(Page::visible()->where('draft', '=', false)), '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 [ - '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 63f942412..9652654be 100644 --- 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 = []; - protected $fieldsToExpose = []; /** * 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. */ - public function getValdationRules(): array + public function getValidationRules(): array { 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 6ca31f0fd..88350e0ea 100644 --- 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 Illuminate\Validation\Rules\Password; +use Illuminate\Validation\Rules\Unique; class UserApiController extends ApiController { @@ -15,21 +17,35 @@ class UserApiController extends ApiController '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; } + 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. @@ -54,10 +70,26 @@ class UserApiController extends ApiController { $this->checkPermission('users-manage'); - $singleUser = $this->userRepo->getById($id); - $this->singleFormatter($singleUser); + $user = $this->userRepo->getById($id); + $this->singleFormatter($user); - return response()->json($singleUser); + 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($user); } /** diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 511b4d33c..9e702a1d7 100644 --- 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->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'], - 'setting' => ['array'], + 'language' => ['string'], + 'roles' => ['array'], + 'roles.*' => ['integer'], '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')) { @@ -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; + $user->save(); } // Delete the profile image if reset option is in request @@ -227,11 +196,7 @@ class UserController extends Controller $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); } diff --git a/app/Providers/AuthServiceProvider.php b/app/Providers/AuthServiceProvider.php index b301604a5..a4022cc50 100644 --- a/app/Providers/AuthServiceProvider.php +++ b/app/Providers/AuthServiceProvider.php @@ -23,6 +23,7 @@ class AuthServiceProvider extends ServiceProvider public function boot() { // Password Configuration + // Changes here must be reflected in ApiDocsGenerate@getValidationAsString. Password::defaults(function () { return Password::min(8); }); diff --git a/resources/lang/en/activities.php b/resources/lang/en/activities.php index b0d180298..77c39b50c 100644 --- a/resources/lang/en/activities.php +++ b/resources/lang/en/activities.php @@ -60,6 +60,7 @@ return [ 'webhook_delete_notification' => 'Webhook successfully deleted', // Users + 'user_update_notification' => 'User successfully updated', 'user_delete_notification' => 'User successfully removed', // Other diff --git a/resources/lang/en/settings.php b/resources/lang/en/settings.php index d6a356508..bfe99c98f 100755 --- 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_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', diff --git a/resources/views/users/parts/language-option-row.blade.php b/resources/views/users/parts/language-option-row.blade.php index 82907b53d..cbb0b0526 100644 --- 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

- @foreach(trans('settings.language_select') as $lang => $label) @endforeach diff --git a/routes/api.php b/routes/api.php index 01564c7d3..0325d7c2a 100644 --- 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::put('users/{id}', [UserApiController::class, 'update']); 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 4a3c4724a..19b7b0adc 100644 --- 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 Illuminate\Support\Facades\Auth; +use Illuminate\Support\Facades\Hash; 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' => 'barrytest@example.com', + 'roles' => $roles, + 'external_auth_id' => 'btest', + 'password' => 'barrytester', + 'language' => 'fr', + ]); + + $resp->assertStatus(200); + $resp->assertJson([ + 'id' => $user->id, + 'name' => 'My updated user', + 'email' => 'barrytest@example.com', + '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();