From 07626669dad962856e52dddeacb1a9f000f93150 Mon Sep 17 00:00:00 2001 From: Jascha Sticher Date: Wed, 5 May 2021 13:46:14 +0200 Subject: [PATCH 1/8] Test API Endpoint for users --- app/Auth/UserRepo.php | 8 ++++ .../Controllers/Api/UserApiController.php | 42 +++++++++++++++++++ routes/api.php | 2 + 3 files changed, 52 insertions(+) create mode 100644 app/Http/Controllers/Api/UserApiController.php diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php index e437ff1e3..89d5ba4b7 100644 --- a/app/Auth/UserRepo.php +++ b/app/Auth/UserRepo.php @@ -61,6 +61,14 @@ class UserRepo return User::query()->with('roles', 'avatar')->orderBy('name', 'asc')->get(); } + /** + * Get all users as Builder for API + */ + public function getUsersBuilder(): Builder + { + $query = User::query()->select(['*']); + return $query; + } /** * Get all the users with their permissions in a paginated format. */ diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php new file mode 100644 index 000000000..e8b98525d --- /dev/null +++ b/app/Http/Controllers/Api/UserApiController.php @@ -0,0 +1,42 @@ + [ +# ], +# 'update' => [ +# ], +# ]; + + public function __construct(User $user, UserRepo $userRepo) + { + $this->user = $user; + $this->userRepo = $userRepo; + } + + /** + * Get a listing of pages visible to the user. + */ + public function list() + { + $users = $this->userRepo->getUsersBuilder(); + + return $this->apiListingResponse($users, [ + 'id', 'name', 'slug', + 'email', 'created_at', 'updated_at', + ]); + } +} diff --git a/routes/api.php b/routes/api.php index 44643d6d4..0a9f99f50 100644 --- a/routes/api.php +++ b/routes/api.php @@ -44,3 +44,5 @@ Route::post('shelves', 'BookshelfApiController@create'); Route::get('shelves/{id}', 'BookshelfApiController@read'); Route::put('shelves/{id}', 'BookshelfApiController@update'); Route::delete('shelves/{id}', 'BookshelfApiController@delete'); + +Route::get('users', 'UserApiController@list'); From 4cbd1a9eb526bcd5fe5d9446dbf27c5813042678 Mon Sep 17 00:00:00 2001 From: Jascha Sticher Date: Thu, 6 May 2021 11:10:49 +0200 Subject: [PATCH 2/8] Extend /users API endpoint * add /users/{id} to get a single user * add variable to print fields that are otherwise hidden (e.g. email) --- app/Api/ListingResponseBuilder.php | 5 +++- app/Auth/UserRepo.php | 6 +++-- app/Http/Controllers/Api/ApiController.php | 5 ++-- .../Controllers/Api/UserApiController.php | 27 ++++++++++++++++--- routes/api.php | 1 + 5 files changed, 35 insertions(+), 9 deletions(-) diff --git a/app/Api/ListingResponseBuilder.php b/app/Api/ListingResponseBuilder.php index df4cb8bf1..06802808e 100644 --- a/app/Api/ListingResponseBuilder.php +++ b/app/Api/ListingResponseBuilder.php @@ -10,6 +10,7 @@ class ListingResponseBuilder protected $query; protected $request; protected $fields; + protected $hiddenFields; protected $filterOperators = [ 'eq' => '=', @@ -24,11 +25,12 @@ class ListingResponseBuilder /** * ListingResponseBuilder constructor. */ - public function __construct(Builder $query, Request $request, array $fields) + public function __construct(Builder $query, Request $request, array $fields, array $hiddenFields ) { $this->query = $query; $this->request = $request; $this->fields = $fields; + $this->hiddenFields = $hiddenFields; } /** @@ -40,6 +42,7 @@ class ListingResponseBuilder $total = $filteredQuery->count(); $data = $this->fetchData($filteredQuery); + $data = $data->makeVisible($this->hiddenFields); return response()->json([ 'data' => $data, diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php index 89d5ba4b7..4444c734c 100644 --- a/app/Auth/UserRepo.php +++ b/app/Auth/UserRepo.php @@ -64,9 +64,11 @@ class UserRepo /** * Get all users as Builder for API */ - public function getUsersBuilder(): Builder + public function getUsersBuilder(int $id = null ) : Builder { - $query = User::query()->select(['*']); + $query = User::query()->select(['*']) + ->withLastActivityAt() + ->with(['roles', 'avatar']); return $query; } /** diff --git a/app/Http/Controllers/Api/ApiController.php b/app/Http/Controllers/Api/ApiController.php index f143ea5cd..5eb8b1e3d 100644 --- a/app/Http/Controllers/Api/ApiController.php +++ b/app/Http/Controllers/Api/ApiController.php @@ -9,14 +9,15 @@ abstract class ApiController extends Controller { protected $rules = []; + protected $printHidden = []; /** * Provide a paginated listing JSON response in a standard format * taking into account any pagination parameters passed by the user. */ - protected function apiListingResponse(Builder $query, array $fields): JsonResponse + protected function apiListingResponse(Builder $query, array $fields, array $protectedFieldsToPrint = []): JsonResponse { - $listing = new ListingResponseBuilder($query, request(), $fields); + $listing = new ListingResponseBuilder($query, request(), $fields, $protectedFieldsToPrint); return $listing->toResponse(); } diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php index e8b98525d..328241a83 100644 --- a/app/Http/Controllers/Api/UserApiController.php +++ b/app/Http/Controllers/Api/UserApiController.php @@ -13,6 +13,10 @@ class UserApiController extends ApiController protected $user; protected $userRepo; + protected $printHidden = [ + 'email', 'created_at', 'updated_at', 'last_activity_at' + ]; + # TBD: Endpoints to create / update users # protected $rules = [ # 'create' => [ @@ -28,15 +32,30 @@ class UserApiController extends ApiController } /** - * Get a listing of pages visible to the user. + * Get a listing of users */ public function list() { + $this->checkPermission('users-manage'); + $users = $this->userRepo->getUsersBuilder(); return $this->apiListingResponse($users, [ - 'id', 'name', 'slug', - 'email', 'created_at', 'updated_at', - ]); + 'id', 'name', 'slug', 'email', + 'created_at', 'updated_at', 'last_activity_at', + ], $this->printHidden); + } + + /** + * View the details of a single user + */ + public function read(string $id) + { + $this->checkPermission('users-manage'); + + $singleUser = $this->userRepo->getById($id); + $singleUser = $singleUser->makeVisible($this->printHidden); + + return response()->json($singleUser); } } diff --git a/routes/api.php b/routes/api.php index 0a9f99f50..063fbd72a 100644 --- a/routes/api.php +++ b/routes/api.php @@ -46,3 +46,4 @@ Route::put('shelves/{id}', 'BookshelfApiController@update'); Route::delete('shelves/{id}', 'BookshelfApiController@delete'); Route::get('users', 'UserApiController@list'); +Route::get('users/{id}', 'UserApiController@read'); From d089623aac6b39641a0ff610c124cb3a01609efd Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 3 Feb 2022 12:33:26 +0000 Subject: [PATCH 3/8] Refactored existing user API work - Updated routes to use new format. - Changed how hidden fields are exposed to be more flexible to different use-cases. - Updated properties available on read/list results. - Started adding testing coverage. - Removed old unused UserRepo 'getAllUsers' function. Related to #2701, Progression of #2734 --- app/Api/ListingResponseBuilder.php | 32 ++++++++-- app/Auth/Role.php | 2 + app/Auth/User.php | 2 +- app/Auth/UserRepo.php | 17 ++--- app/Http/Controllers/Api/ApiController.php | 10 ++- .../Controllers/Api/UserApiController.php | 60 +++++++++++------ routes/api.php | 5 +- tests/Api/UsersApiTest.php | 64 +++++++++++++++++++ 8 files changed, 145 insertions(+), 47 deletions(-) create mode 100644 tests/Api/UsersApiTest.php diff --git a/app/Api/ListingResponseBuilder.php b/app/Api/ListingResponseBuilder.php index 3dbe954b8..6da92040b 100644 --- a/app/Api/ListingResponseBuilder.php +++ b/app/Api/ListingResponseBuilder.php @@ -2,8 +2,10 @@ namespace BookStack\Api; +use BookStack\Model; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Collection; +use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; class ListingResponseBuilder @@ -11,7 +13,11 @@ class ListingResponseBuilder protected $query; protected $request; protected $fields; - protected $hiddenFields; + + /** + * @var array + */ + protected $resultModifiers = []; protected $filterOperators = [ 'eq' => '=', @@ -25,25 +31,28 @@ class ListingResponseBuilder /** * ListingResponseBuilder constructor. + * The given fields will be forced visible within the model results. */ - public function __construct(Builder $query, Request $request, array $fields, array $hiddenFields ) + public function __construct(Builder $query, Request $request, array $fields) { $this->query = $query; $this->request = $request; $this->fields = $fields; - $this->hiddenFields = $hiddenFields; } /** * Get the response from this builder. */ - public function toResponse() + public function toResponse(): JsonResponse { $filteredQuery = $this->filterQuery($this->query); $total = $filteredQuery->count(); - $data = $this->fetchData($filteredQuery); - $data = $data->makeVisible($this->hiddenFields); + $data = $this->fetchData($filteredQuery)->each(function($model) { + foreach ($this->resultModifiers as $modifier) { + $modifier($model); + } + }); return response()->json([ 'data' => $data, @@ -52,7 +61,16 @@ class ListingResponseBuilder } /** - * Fetch the data to return in the response. + * Add a callback to modify each element of the results + * @param (callable(Model)) $modifier + */ + public function modifyResults($modifier): void + { + $this->resultModifiers[] = $modifier; + } + + /** + * Fetch the data to return within the response. */ protected function fetchData(Builder $query): Collection { diff --git a/app/Auth/Role.php b/app/Auth/Role.php index 71da88e19..51b2ce301 100644 --- a/app/Auth/Role.php +++ b/app/Auth/Role.php @@ -28,6 +28,8 @@ class Role extends Model implements Loggable protected $fillable = ['display_name', 'description', 'external_auth_id']; + protected $hidden = ['pivot']; + /** * The roles that belong to the role. */ diff --git a/app/Auth/User.php b/app/Auth/User.php index f969b351f..c2b241381 100644 --- a/app/Auth/User.php +++ b/app/Auth/User.php @@ -72,7 +72,7 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon */ protected $hidden = [ 'password', 'remember_token', 'system_name', 'email_confirmed', 'external_auth_id', 'email', - 'created_at', 'updated_at', 'image_id', + 'created_at', 'updated_at', 'image_id', 'roles', 'avatar', ]; /** diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php index 0dea41725..1341e70bc 100644 --- a/app/Auth/UserRepo.php +++ b/app/Auth/UserRepo.php @@ -52,23 +52,14 @@ class UserRepo return User::query()->where('slug', '=', $slug)->firstOrFail(); } - /** - * Get all the users with their permissions. - */ - public function getAllUsers(): Collection - { - return User::query()->with('roles', 'avatar')->orderBy('name', 'asc')->get(); - } - /** * Get all users as Builder for API */ - public function getUsersBuilder(int $id = null ) : Builder + public function getApiUsersBuilder() : Builder { - $query = User::query()->select(['*']) - ->withLastActivityAt() - ->with(['roles', 'avatar']); - return $query; + return User::query()->select(['*']) + ->scopes('withLastActivityAt') + ->with(['avatar']); } /** * Get all the users with their permissions in a paginated format. diff --git a/app/Http/Controllers/Api/ApiController.php b/app/Http/Controllers/Api/ApiController.php index 5d6f4a926..63f942412 100644 --- a/app/Http/Controllers/Api/ApiController.php +++ b/app/Http/Controllers/Api/ApiController.php @@ -10,15 +10,19 @@ use Illuminate\Http\JsonResponse; abstract class ApiController extends Controller { protected $rules = []; - protected $printHidden = []; + protected $fieldsToExpose = []; /** * Provide a paginated listing JSON response in a standard format * taking into account any pagination parameters passed by the user. */ - protected function apiListingResponse(Builder $query, array $fields, array $protectedFieldsToPrint = []): JsonResponse + protected function apiListingResponse(Builder $query, array $fields, array $modifiers = []): JsonResponse { - $listing = new ListingResponseBuilder($query, request(), $fields, $protectedFieldsToPrint); + $listing = new ListingResponseBuilder($query, request(), $fields); + + foreach ($modifiers as $modifier) { + $listing->modifyResults($modifier); + } return $listing->toResponse(); } diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php index 328241a83..ed1a4b13d 100644 --- a/app/Http/Controllers/Api/UserApiController.php +++ b/app/Http/Controllers/Api/UserApiController.php @@ -2,60 +2,78 @@ namespace BookStack\Http\Controllers\Api; -use BookStack\Exceptions\PermissionsException; use BookStack\Auth\User; use BookStack\Auth\UserRepo; -use Exception; -use Illuminate\Http\Request; +use Closure; class UserApiController extends ApiController { - protected $user; protected $userRepo; - protected $printHidden = [ - 'email', 'created_at', 'updated_at', 'last_activity_at' + protected $fieldsToExpose = [ + 'email', 'created_at', 'updated_at', 'last_activity_at', 'external_auth_id' ]; -# TBD: Endpoints to create / update users -# protected $rules = [ -# 'create' => [ -# ], -# 'update' => [ -# ], -# ]; + protected $rules = [ + 'create' => [ + ], + 'update' => [ + ], + ]; - public function __construct(User $user, UserRepo $userRepo) + public function __construct(UserRepo $userRepo) { - $this->user = $user; $this->userRepo = $userRepo; } /** - * Get a listing of users + * Get a listing of users in the system. + * Requires permission to manage users. */ public function list() { $this->checkPermission('users-manage'); - $users = $this->userRepo->getUsersBuilder(); + $users = $this->userRepo->getApiUsersBuilder(); return $this->apiListingResponse($users, [ - 'id', 'name', 'slug', 'email', + 'id', 'name', 'slug', 'email', 'external_auth_id', 'created_at', 'updated_at', 'last_activity_at', - ], $this->printHidden); + ], [Closure::fromCallable([$this, 'listFormatter'])]); } /** - * View the details of a single user + * View the details of a single user. + * Requires permission to manage users. */ public function read(string $id) { $this->checkPermission('users-manage'); $singleUser = $this->userRepo->getById($id); - $singleUser = $singleUser->makeVisible($this->printHidden); + $this->singleFormatter($singleUser); return response()->json($singleUser); } + + /** + * Format the given user model for single-result display. + */ + protected function singleFormatter(User $user) + { + $this->listFormatter($user); + $user->load('roles:id,display_name'); + $user->makeVisible(['roles']); + } + + /** + * Format the given user model for a listing multi-result display. + */ + protected function listFormatter(User $user) + { + $user->makeVisible($this->fieldsToExpose); + $user->setAttribute('profile_url', $user->getProfileUrl()); + $user->setAttribute('edit_url', $user->getEditUrl()); + $user->setAttribute('avatar_url', $user->getAvatar()); + } } diff --git a/routes/api.php b/routes/api.php index cd8dd355a..2adc3f775 100644 --- a/routes/api.php +++ b/routes/api.php @@ -10,6 +10,7 @@ use BookStack\Http\Controllers\Api\ChapterExportApiController; use BookStack\Http\Controllers\Api\PageApiController; use BookStack\Http\Controllers\Api\PageExportApiController; use BookStack\Http\Controllers\Api\SearchApiController; +use BookStack\Http\Controllers\Api\UserApiController; use Illuminate\Support\Facades\Route; /** @@ -66,5 +67,5 @@ Route::get('shelves/{id}', [BookshelfApiController::class, 'read']); Route::put('shelves/{id}', [BookshelfApiController::class, 'update']); Route::delete('shelves/{id}', [BookshelfApiController::class, 'delete']); -Route::get('users', 'UserApiController@list'); -Route::get('users/{id}', 'UserApiController@read'); \ No newline at end of file +Route::get('users', [UserApiController::class, 'list']); +Route::get('users/{id}', [UserApiController::class, 'read']); \ No newline at end of file diff --git a/tests/Api/UsersApiTest.php b/tests/Api/UsersApiTest.php new file mode 100644 index 000000000..24c825f8f --- /dev/null +++ b/tests/Api/UsersApiTest.php @@ -0,0 +1,64 @@ +actingAsApiAdmin(); + /** @var User $firstUser */ + $firstUser = User::query()->orderBy('id', 'asc')->first(); + + $resp = $this->getJson($this->baseEndpoint . '?count=1&sort=+id'); + $resp->assertJson(['data' => [ + [ + 'id' => $firstUser->id, + 'name' => $firstUser->name, + 'slug' => $firstUser->slug, + 'email' => $firstUser->email, + 'profile_url' => $firstUser->getProfileUrl(), + 'edit_url' => $firstUser->getEditUrl(), + 'avatar_url' => $firstUser->getAvatar(), + ], + ]]); + } + + public function test_read_endpoint() + { + $this->actingAsApiAdmin(); + /** @var User $user */ + $user = User::query()->first(); + /** @var Role $userRole */ + $userRole = $user->roles()->first(); + + $resp = $this->getJson($this->baseEndpoint . "/{$user->id}"); + + $resp->assertStatus(200); + $resp->assertJson([ + 'id' => $user->id, + 'slug' => $user->slug, + 'email' => $user->email, + 'external_auth_id' => $user->external_auth_id, + 'roles' => [ + [ + 'id' => $userRole->id, + 'display_name' => $userRole->display_name, + ] + ], + ]); + } +} From 2cd7a48044ede60c487bbe575fc20ed99c702571 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 3 Feb 2022 15:12:50 +0000 Subject: [PATCH 4/8] 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)); + } } From 9e1c8ec82aa856d2f17a07b9f16e9e4cafe1e073 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 3 Feb 2022 16:52:28 +0000 Subject: [PATCH 5/8] 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(); From eb653bda16f2f1e82e5e0f0a047d63871f0b9389 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Fri, 4 Feb 2022 00:26:19 +0000 Subject: [PATCH 6/8] Added user-create API endpoint - Required extracting logic into repo. - Changed some existing creation paths to standardise behaviour. - Added test to cover new endpoint. - Added extra test for user delete to test migration. - Changed how permission errors are thrown to ensure the right status code can be reported when handled in API. --- app/Auth/Access/Guards/LdapSessionGuard.php | 2 +- app/Auth/Access/RegistrationService.php | 3 +- app/Auth/UserRepo.php | 64 +++++--- app/Console/Commands/CreateAdmin.php | 3 +- app/Exceptions/Handler.php | 4 + app/Exceptions/NotifyException.php | 12 +- .../Controllers/Api/UserApiController.php | 47 ++++-- app/Http/Controllers/Controller.php | 12 +- app/Http/Controllers/UserController.php | 76 +++------- routes/api.php | 1 + tests/Api/TestsApi.php | 8 + tests/Api/UsersApiTest.php | 138 +++++++++++++++++- 12 files changed, 261 insertions(+), 109 deletions(-) diff --git a/app/Auth/Access/Guards/LdapSessionGuard.php b/app/Auth/Access/Guards/LdapSessionGuard.php index 078487224..5a902af76 100644 --- a/app/Auth/Access/Guards/LdapSessionGuard.php +++ b/app/Auth/Access/Guards/LdapSessionGuard.php @@ -84,7 +84,7 @@ class LdapSessionGuard extends ExternalBaseSessionGuard try { $user = $this->createNewFromLdapAndCreds($userDetails, $credentials); } catch (UserRegistrationException $exception) { - throw new LoginAttemptException($exception->message); + throw new LoginAttemptException($exception->getMessage()); } } diff --git a/app/Auth/Access/RegistrationService.php b/app/Auth/Access/RegistrationService.php index dcdb68bd5..6fcb404ee 100644 --- a/app/Auth/Access/RegistrationService.php +++ b/app/Auth/Access/RegistrationService.php @@ -96,7 +96,8 @@ class RegistrationService } // Create the user - $newUser = $this->userRepo->registerNew($userData, $emailConfirmed); + $newUser = $this->userRepo->createWithoutActivity($userData, $emailConfirmed); + $newUser->attachDefaultRole(); // Assign social account if given if ($socialAccount) { diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php index cb0c0d2fa..c87fda4c8 100644 --- a/app/Auth/UserRepo.php +++ b/app/Auth/UserRepo.php @@ -3,6 +3,7 @@ namespace BookStack\Auth; use BookStack\Actions\ActivityType; +use BookStack\Auth\Access\UserInviteService; use BookStack\Entities\EntityProvider; use BookStack\Entities\Models\Book; use BookStack\Entities\Models\Bookshelf; @@ -18,17 +19,20 @@ use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Collection; use Illuminate\Pagination\LengthAwarePaginator; use Illuminate\Support\Facades\Log; +use Illuminate\Support\Str; class UserRepo { protected $userAvatar; + protected $inviteService; /** * UserRepo constructor. */ - public function __construct(UserAvatars $userAvatar) + public function __construct(UserAvatars $userAvatar, UserInviteService $inviteService) { $this->userAvatar = $userAvatar; + $this->inviteService = $inviteService; } /** @@ -92,18 +96,6 @@ class UserRepo return $query->paginate($count); } - /** - * Creates a new user and attaches a role to them. - */ - public function registerNew(array $data, bool $emailConfirmed = false): User - { - $user = $this->create($data, $emailConfirmed); - $user->attachDefaultRole(); - $this->downloadAndAssignUserAvatar($user); - - return $user; - } - /** * Assign a user to a system-level role. * @@ -166,23 +158,47 @@ class UserRepo } /** - * Create a new basic instance of user. + * Create a new basic instance of user with the given pre-validated data. + * @param array{name: string, email: string, password: ?string, external_auth_id: ?string, language: ?string, roles: ?array} $data */ - public function create(array $data, bool $emailConfirmed = false): User + public function createWithoutActivity(array $data, bool $emailConfirmed = false): User { - $details = [ - 'name' => $data['name'], - 'email' => $data['email'], - 'password' => bcrypt($data['password']), - 'email_confirmed' => $emailConfirmed, - 'external_auth_id' => $data['external_auth_id'] ?? '', - ]; - $user = new User(); - $user->forceFill($details); + $user->name = $data['name']; + $user->email = $data['email']; + $user->password = bcrypt(empty($data['password']) ? Str::random(32) : $data['password']); + $user->email_confirmed = $emailConfirmed; + $user->external_auth_id = $data['external_auth_id'] ?? ''; + $user->refreshSlug(); $user->save(); + if (!empty($data['language'])) { + setting()->putUser($user, 'language', $data['language']); + } + + if (isset($data['roles'])) { + $this->setUserRoles($user, $data['roles']); + } + + $this->downloadAndAssignUserAvatar($user); + + return $user; + } + + /** + * As per "createWithoutActivity" but records a "create" activity. + * @param array{name: string, email: string, password: ?string, external_auth_id: ?string, language: ?string, roles: ?array} $data + */ + public function create(array $data, bool $sendInvite = false): User + { + $user = $this->createWithoutActivity($data, false); + + if ($sendInvite) { + $this->inviteService->sendInvitation($user); + } + + Activity::add(ActivityType::USER_CREATE, $user); return $user; } diff --git a/app/Console/Commands/CreateAdmin.php b/app/Console/Commands/CreateAdmin.php index c3faef79c..c571d383e 100644 --- a/app/Console/Commands/CreateAdmin.php +++ b/app/Console/Commands/CreateAdmin.php @@ -84,9 +84,8 @@ class CreateAdmin extends Command return SymfonyCommand::FAILURE; } - $user = $this->userRepo->create($validator->validated()); + $user = $this->userRepo->createWithoutActivity($validator->validated()); $this->userRepo->attachSystemRole($user, 'admin'); - $this->userRepo->downloadAndAssignUserAvatar($user); $user->email_confirmed = true; $user->save(); diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php index 7ec502525..317b011d8 100644 --- a/app/Exceptions/Handler.php +++ b/app/Exceptions/Handler.php @@ -101,6 +101,10 @@ class Handler extends ExceptionHandler $code = $e->status; } + if (method_exists($e, 'getStatus')) { + $code = $e->getStatus(); + } + $responseData['error']['code'] = $code; return new JsonResponse($responseData, $code, $headers); diff --git a/app/Exceptions/NotifyException.php b/app/Exceptions/NotifyException.php index 8e748a21d..e09247208 100644 --- a/app/Exceptions/NotifyException.php +++ b/app/Exceptions/NotifyException.php @@ -9,17 +9,27 @@ class NotifyException extends Exception implements Responsable { public $message; public $redirectLocation; + protected $status; /** * NotifyException constructor. */ - public function __construct(string $message, string $redirectLocation = '/') + public function __construct(string $message, string $redirectLocation = '/', int $status = 500) { $this->message = $message; $this->redirectLocation = $redirectLocation; + $this->status = $status; parent::__construct(); } + /** + * Get the desired status code for this exception. + */ + public function getStatus(): int + { + return $this->status; + } + /** * Send the response for this type of exception. * diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php index 88350e0ea..cd97dead1 100644 --- a/app/Http/Controllers/Api/UserApiController.php +++ b/app/Http/Controllers/Api/UserApiController.php @@ -4,8 +4,10 @@ namespace BookStack\Http\Controllers\Api; use BookStack\Auth\User; use BookStack\Auth\UserRepo; +use BookStack\Exceptions\UserUpdateException; use Closure; use Illuminate\Http\Request; +use Illuminate\Support\Facades\DB; use Illuminate\Validation\Rules\Password; use Illuminate\Validation\Rules\Unique; @@ -20,12 +22,29 @@ class UserApiController extends ApiController public function __construct(UserRepo $userRepo) { $this->userRepo = $userRepo; + + // Checks for all endpoints in this controller + $this->middleware(function ($request, $next) { + $this->checkPermission('users-manage'); + $this->preventAccessInDemoMode(); + return $next($request); + }); } protected function rules(int $userId = null): array { return [ 'create' => [ + 'name' => ['required', 'min:2'], + 'email' => [ + 'required', 'min:2', 'email', new Unique('users', 'email') + ], + 'external_auth_id' => ['string'], + 'language' => ['string'], + 'password' => [Password::default()], + 'roles' => ['array'], + 'roles.*' => ['integer'], + 'send_invite' => ['boolean'], ], 'update' => [ 'name' => ['min:2'], @@ -52,8 +71,6 @@ class UserApiController extends ApiController */ public function list() { - $this->checkPermission('users-manage'); - $users = $this->userRepo->getApiUsersBuilder(); return $this->apiListingResponse($users, [ @@ -62,14 +79,30 @@ class UserApiController extends ApiController ], [Closure::fromCallable([$this, 'listFormatter'])]); } + /** + * Create a new user in the system. + */ + public function create(Request $request) + { + $data = $this->validate($request, $this->rules()['create']); + $sendInvite = ($data['send_invite'] ?? false) === true; + + $user = null; + DB::transaction(function () use ($data, $sendInvite, &$user) { + $user = $this->userRepo->create($data, $sendInvite); + }); + + $this->singleFormatter($user); + + return response()->json($user); + } + /** * View the details of a single user. * Requires permission to manage users. */ public function read(string $id) { - $this->checkPermission('users-manage'); - $user = $this->userRepo->getById($id); $this->singleFormatter($user); @@ -78,12 +111,10 @@ class UserApiController extends ApiController /** * Update an existing user in the system. - * @throws \BookStack\Exceptions\UserUpdateException + * @throws 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')); @@ -100,8 +131,6 @@ class UserApiController extends ApiController */ public function delete(Request $request, string $id) { - $this->checkPermission('users-manage'); - $user = $this->userRepo->getById($id); $newOwnerId = $request->get('migrate_ownership_id', null); diff --git a/app/Http/Controllers/Controller.php b/app/Http/Controllers/Controller.php index 2c4c2df1e..13a86f6f7 100644 --- a/app/Http/Controllers/Controller.php +++ b/app/Http/Controllers/Controller.php @@ -2,13 +2,13 @@ namespace BookStack\Http\Controllers; +use BookStack\Exceptions\NotifyException; use BookStack\Facades\Activity; use BookStack\Interfaces\Loggable; use BookStack\Model; use BookStack\Util\WebSafeMimeSniffer; use Illuminate\Foundation\Bus\DispatchesJobs; use Illuminate\Foundation\Validation\ValidatesRequests; -use Illuminate\Http\Exceptions\HttpResponseException; use Illuminate\Http\JsonResponse; use Illuminate\Http\Response; use Illuminate\Routing\Controller as BaseController; @@ -53,14 +53,8 @@ abstract class Controller extends BaseController */ protected function showPermissionError() { - if (request()->wantsJson()) { - $response = response()->json(['error' => trans('errors.permissionJson')], 403); - } else { - $response = redirect('/'); - $this->showErrorNotification(trans('errors.permission')); - } - - throw new HttpResponseException($response); + $message = request()->wantsJson() ? trans('errors.permissionJson') : trans('errors.permission'); + throw new NotifyException($message, '/', 403); } /** diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 9e702a1d7..46e858d9b 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -2,9 +2,7 @@ namespace BookStack\Http\Controllers; -use BookStack\Actions\ActivityType; use BookStack\Auth\Access\SocialAuthService; -use BookStack\Auth\Access\UserInviteService; use BookStack\Auth\User; use BookStack\Auth\UserRepo; use BookStack\Exceptions\ImageUploadException; @@ -13,25 +11,20 @@ use BookStack\Uploads\ImageRepo; use Exception; use Illuminate\Http\Request; use Illuminate\Support\Facades\DB; -use Illuminate\Support\Str; use Illuminate\Validation\Rules\Password; use Illuminate\Validation\ValidationException; class UserController extends Controller { - protected $user; protected $userRepo; - protected $inviteService; protected $imageRepo; /** * UserController constructor. */ - public function __construct(User $user, UserRepo $userRepo, UserInviteService $inviteService, ImageRepo $imageRepo) + public function __construct(UserRepo $userRepo, ImageRepo $imageRepo) { - $this->user = $user; $this->userRepo = $userRepo; - $this->inviteService = $inviteService; $this->imageRepo = $imageRepo; } @@ -68,63 +61,34 @@ class UserController extends Controller } /** - * Store a newly created user in storage. + * Store a new user in storage. * - * @throws UserUpdateException * @throws ValidationException */ public function store(Request $request) { $this->checkPermission('users-manage'); - $validationRules = [ - 'name' => ['required'], - 'email' => ['required', 'email', 'unique:users,email'], - 'setting' => ['array'], - ]; $authMethod = config('auth.method'); $sendInvite = ($request->get('send_invite', 'false') === 'true'); + $externalAuth = $authMethod === 'ldap' || $authMethod === 'saml2' || $authMethod === 'oidc'; + $passwordRequired = ($authMethod === 'standard' && !$sendInvite); - if ($authMethod === 'standard' && !$sendInvite) { - $validationRules['password'] = ['required', Password::default()]; - $validationRules['password-confirm'] = ['required', 'same:password']; - } elseif ($authMethod === 'ldap' || $authMethod === 'saml2' || $authMethod === 'openid') { - $validationRules['external_auth_id'] = ['required']; - } - $this->validate($request, $validationRules); + $validationRules = [ + 'name' => ['required'], + 'email' => ['required', 'email', 'unique:users,email'], + 'language' => ['string'], + 'roles' => ['array'], + 'roles.*' => ['integer'], + 'password' => $passwordRequired ? ['required', Password::default()] : null, + 'password-confirm' => $passwordRequired ? ['required', 'same:password'] : null, + 'external_auth_id' => $externalAuth ? ['required'] : null, + ]; - $user = $this->user->fill($request->all()); + $validated = $this->validate($request, array_filter($validationRules)); - if ($authMethod === 'standard') { - $user->password = bcrypt($request->get('password', Str::random(32))); - } elseif ($authMethod === 'ldap' || $authMethod === 'saml2' || $authMethod === 'openid') { - $user->external_auth_id = $request->get('external_auth_id'); - } - - $user->refreshSlug(); - - DB::transaction(function () use ($user, $sendInvite, $request) { - $user->save(); - - // Save user-specific settings - if ($request->filled('setting')) { - foreach ($request->get('setting') as $key => $value) { - setting()->putUser($user, $key, $value); - } - } - - if ($sendInvite) { - $this->inviteService->sendInvitation($user); - } - - if ($request->filled('roles')) { - $roles = $request->get('roles'); - $this->userRepo->setUserRoles($user, $roles); - } - - $this->userRepo->downloadAndAssignUserAvatar($user); - - $this->logActivity(ActivityType::USER_CREATE, $user); + DB::transaction(function () use ($validated, $sendInvite) { + $this->userRepo->create($validated, $sendInvite); }); return redirect('/settings/users'); @@ -138,7 +102,7 @@ class UserController extends Controller $this->checkPermissionOrCurrentUser('users-manage', $id); /** @var User $user */ - $user = $this->user->newQuery()->with(['apiTokens', 'mfaValues'])->findOrFail($id); + $user = User::query()->with(['apiTokens', 'mfaValues'])->findOrFail($id); $authMethod = ($user->system_name) ? 'system' : config('auth.method'); @@ -312,7 +276,7 @@ class UserController extends Controller $newState = $request->get('expand', 'false'); - $user = $this->user->findOrFail($id); + $user = $this->userRepo->getById($id); setting()->putUser($user, 'section_expansion#' . $key, $newState); return response('', 204); @@ -335,7 +299,7 @@ class UserController extends Controller $order = 'asc'; } - $user = $this->user->findOrFail($userId); + $user = $this->userRepo->getById($userId); $sortKey = $listName . '_sort'; $orderKey = $listName . '_sort_order'; setting()->putUser($user, $sortKey, $sort); diff --git a/routes/api.php b/routes/api.php index 0325d7c2a..c7b8887b6 100644 --- a/routes/api.php +++ b/routes/api.php @@ -68,6 +68,7 @@ Route::put('shelves/{id}', [BookshelfApiController::class, 'update']); Route::delete('shelves/{id}', [BookshelfApiController::class, 'delete']); Route::get('users', [UserApiController::class, 'list']); +Route::post('users', [UserApiController::class, 'create']); 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/TestsApi.php b/tests/Api/TestsApi.php index 97ca82ea7..0cdd93741 100644 --- a/tests/Api/TestsApi.php +++ b/tests/Api/TestsApi.php @@ -35,6 +35,14 @@ trait TestsApi return ['error' => ['code' => $code, 'message' => $message]]; } + /** + * Get the structure that matches a permission error response. + */ + protected function permissionErrorResponse(): array + { + return $this->errorResponse('You do not have permission to perform the requested action.', 403); + } + /** * Format the given (field_name => ["messages"]) array * into a standard validation response format. diff --git a/tests/Api/UsersApiTest.php b/tests/Api/UsersApiTest.php index 19b7b0adc..e1bcb02d5 100644 --- a/tests/Api/UsersApiTest.php +++ b/tests/Api/UsersApiTest.php @@ -2,10 +2,13 @@ namespace Tests\Api; +use BookStack\Actions\ActivityType; use BookStack\Auth\Role; use BookStack\Auth\User; -use Illuminate\Support\Facades\Auth; +use BookStack\Entities\Models\Entity; +use BookStack\Notifications\UserInvite; use Illuminate\Support\Facades\Hash; +use Illuminate\Support\Facades\Notification; use Tests\TestCase; class UsersApiTest extends TestCase @@ -14,17 +17,34 @@ class UsersApiTest extends TestCase protected $baseEndpoint = '/api/users'; + protected $endpointMap = [ + ['get', '/api/users'], + ['post', '/api/users'], + ['get', '/api/users/1'], + ['put', '/api/users/1'], + ['delete', '/api/users/1'], + ]; + public function test_users_manage_permission_needed_for_all_endpoints() { - // TODO + $this->actingAsApiEditor(); + foreach ($this->endpointMap as [$method, $uri]) { + $resp = $this->json($method, $uri); + $resp->assertStatus(403); + $resp->assertJson($this->permissionErrorResponse()); + } } 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 + config()->set('app.env', 'demo'); + $this->actingAsApiAdmin(); + + foreach ($this->endpointMap as [$method, $uri]) { + $resp = $this->json($method, $uri); + $resp->assertStatus(403); + $resp->assertJson($this->permissionErrorResponse()); + } } public function test_index_endpoint_returns_expected_shelf() @@ -47,6 +67,85 @@ class UsersApiTest extends TestCase ]]); } + public function test_create_endpoint() + { + $this->actingAsApiAdmin(); + /** @var Role $role */ + $role = Role::query()->first(); + + $resp = $this->postJson($this->baseEndpoint, [ + 'name' => 'Benny Boris', + 'email' => 'bboris@example.com', + 'password' => 'mysuperpass', + 'language' => 'it', + 'roles' => [$role->id], + 'send_invite' => false, + ]); + + $resp->assertStatus(200); + $resp->assertJson([ + 'name' => 'Benny Boris', + 'email' => 'bboris@example.com', + 'external_auth_id' => '', + 'roles' => [ + [ + 'id' => $role->id, + 'display_name' => $role->display_name, + ] + ], + ]); + $this->assertDatabaseHas('users', ['email' => 'bboris@example.com']); + + /** @var User $user */ + $user = User::query()->where('email', '=', 'bboris@example.com')->first(); + $this->assertActivityExists(ActivityType::USER_CREATE, null, $user->logDescriptor()); + $this->assertEquals(1, $user->roles()->count()); + $this->assertEquals('it', setting()->getUser($user, 'language')); + } + + public function test_create_with_send_invite() + { + $this->actingAsApiAdmin(); + Notification::fake(); + + $resp = $this->postJson($this->baseEndpoint, [ + 'name' => 'Benny Boris', + 'email' => 'bboris@example.com', + 'send_invite' => true, + ]); + + $resp->assertStatus(200); + /** @var User $user */ + $user = User::query()->where('email', '=', 'bboris@example.com')->first(); + Notification::assertSentTo($user, UserInvite::class); + } + + public function test_create_name_and_email_validation() + { + $this->actingAsApiAdmin(); + /** @var User $existingUser */ + $existingUser = User::query()->first(); + + $resp = $this->postJson($this->baseEndpoint, [ + 'email' => 'bboris@example.com', + ]); + $resp->assertStatus(422); + $resp->assertJson($this->validationResponse(['name' => ['The name field is required.']])); + + $resp = $this->postJson($this->baseEndpoint, [ + 'name' => 'Benny Boris', + ]); + $resp->assertStatus(422); + $resp->assertJson($this->validationResponse(['email' => ['The email field is required.']])); + + $resp = $this->postJson($this->baseEndpoint, [ + 'email' => $existingUser->email, + 'name' => 'Benny Boris', + ]); + $resp->assertStatus(422); + $resp->assertJson($this->validationResponse(['email' => ['The email has already been taken.']])); + } + public function test_read_endpoint() { $this->actingAsApiAdmin(); @@ -133,6 +232,33 @@ class UsersApiTest extends TestCase $this->assertActivityExists('user_delete', null, $user->logDescriptor()); } + public function test_delete_endpoint_with_ownership_migration_user() + { + $this->actingAsApiAdmin(); + /** @var User $user */ + $user = User::query()->where('id', '!=', $this->getAdmin()->id) + ->whereNull('system_name') + ->first(); + $entityChain = $this->createEntityChainBelongingToUser($user); + /** @var User $newOwner */ + $newOwner = User::query()->where('id', '!=', $user->id)->first(); + + /** @var Entity $entity */ + foreach ($entityChain as $entity) { + $this->assertEquals($user->id, $entity->owned_by); + } + + $resp = $this->deleteJson($this->baseEndpoint . "/{$user->id}", [ + 'migrate_ownership_id' => $newOwner->id, + ]); + + $resp->assertStatus(204); + /** @var Entity $entity */ + foreach ($entityChain as $entity) { + $this->assertEquals($newOwner->id, $entity->refresh()->owned_by); + } + } + public function test_delete_endpoint_fails_deleting_only_admin() { $this->actingAsApiAdmin(); From 46e6e239dc644b6949e6609c27339c62a84bc711 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Fri, 4 Feb 2022 00:44:56 +0000 Subject: [PATCH 7/8] Added user API examples --- app/Auth/User.php | 2 +- .../Controllers/Api/UserApiController.php | 2 ++ dev/api/requests/users-create.json | 7 ++++ dev/api/requests/users-update.json | 7 ++++ dev/api/responses/users-create.json | 19 +++++++++++ dev/api/responses/users-list.json | 33 +++++++++++++++++++ dev/api/responses/users-read.json | 19 +++++++++++ dev/api/responses/users-update.json | 19 +++++++++++ 8 files changed, 107 insertions(+), 1 deletion(-) create mode 100644 dev/api/requests/users-create.json create mode 100644 dev/api/requests/users-update.json create mode 100644 dev/api/responses/users-create.json create mode 100644 dev/api/responses/users-list.json create mode 100644 dev/api/responses/users-read.json create mode 100644 dev/api/responses/users-update.json diff --git a/app/Auth/User.php b/app/Auth/User.php index c2b241381..b7f88b590 100644 --- a/app/Auth/User.php +++ b/app/Auth/User.php @@ -72,7 +72,7 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon */ protected $hidden = [ 'password', 'remember_token', 'system_name', 'email_confirmed', 'external_auth_id', 'email', - 'created_at', 'updated_at', 'image_id', 'roles', 'avatar', + 'created_at', 'updated_at', 'image_id', 'roles', 'avatar', 'user_id', ]; /** diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php index cd97dead1..aa2a2481c 100644 --- a/app/Http/Controllers/Api/UserApiController.php +++ b/app/Http/Controllers/Api/UserApiController.php @@ -81,6 +81,7 @@ class UserApiController extends ApiController /** * Create a new user in the system. + * Requires permission to manage users. */ public function create(Request $request) { @@ -111,6 +112,7 @@ class UserApiController extends ApiController /** * Update an existing user in the system. + * Requires permission to manage users. * @throws UserUpdateException */ public function update(Request $request, string $id) diff --git a/dev/api/requests/users-create.json b/dev/api/requests/users-create.json new file mode 100644 index 000000000..ccc43d9e4 --- /dev/null +++ b/dev/api/requests/users-create.json @@ -0,0 +1,7 @@ +{ + "name": "Dan Brown", + "email": "dannyb@example.com", + "roles": [1], + "language": "fr", + "send_invite": true +} \ No newline at end of file diff --git a/dev/api/requests/users-update.json b/dev/api/requests/users-update.json new file mode 100644 index 000000000..d7787ed1a --- /dev/null +++ b/dev/api/requests/users-update.json @@ -0,0 +1,7 @@ +{ + "name": "Dan Spaggleforth", + "email": "dspaggles@example.com", + "roles": [2], + "language": "de", + "password": "hunter2000" +} \ No newline at end of file diff --git a/dev/api/responses/users-create.json b/dev/api/responses/users-create.json new file mode 100644 index 000000000..509d88941 --- /dev/null +++ b/dev/api/responses/users-create.json @@ -0,0 +1,19 @@ +{ + "id": 1, + "name": "Dan Brown", + "email": "dannyb@example.com", + "created_at": "2022-02-03T16:27:55.000000Z", + "updated_at": "2022-02-03T16:27:55.000000Z", + "external_auth_id": "abc123456", + "slug": "dan-brown", + "last_activity_at": "2022-02-03T16:27:55.000000Z", + "profile_url": "https://docs.example.com/user/dan-brown", + "edit_url": "https://docs.example.com/settings/users/1", + "avatar_url": "https://docs.example.com/uploads/images/user/2021-10/thumbs-50-50/profile-2021.jpg", + "roles": [ + { + "id": 1, + "display_name": "Admin" + } + ] +} \ No newline at end of file diff --git a/dev/api/responses/users-list.json b/dev/api/responses/users-list.json new file mode 100644 index 000000000..e070ee6a6 --- /dev/null +++ b/dev/api/responses/users-list.json @@ -0,0 +1,33 @@ +{ + "data": [ + { + "id": 1, + "name": "Dan Brown", + "email": "dannyb@example.com", + "created_at": "2022-02-03T16:27:55.000000Z", + "updated_at": "2022-02-03T16:27:55.000000Z", + "external_auth_id": "abc123456", + "slug": "dan-brown", + "user_id": 1, + "last_activity_at": "2022-02-03T16:27:55.000000Z", + "profile_url": "https://docs.example.com/user/dan-brown", + "edit_url": "https://docs.example.com/settings/users/1", + "avatar_url": "https://docs.example.com/uploads/images/user/2021-10/thumbs-50-50/profile-2021.jpg" + }, + { + "id": 2, + "name": "Benny", + "email": "benny@example.com", + "created_at": "2022-01-31T20:39:24.000000Z", + "updated_at": "2021-11-18T17:10:58.000000Z", + "external_auth_id": "", + "slug": "benny", + "user_id": 2, + "last_activity_at": "2022-01-31T20:39:24.000000Z", + "profile_url": "https://docs.example.com/user/benny", + "edit_url": "https://docs.example.com/settings/users/2", + "avatar_url": "https://docs.example.com/uploads/images/user/2021-11/thumbs-50-50/guest.jpg" + } + ], + "total": 28 +} \ No newline at end of file diff --git a/dev/api/responses/users-read.json b/dev/api/responses/users-read.json new file mode 100644 index 000000000..509d88941 --- /dev/null +++ b/dev/api/responses/users-read.json @@ -0,0 +1,19 @@ +{ + "id": 1, + "name": "Dan Brown", + "email": "dannyb@example.com", + "created_at": "2022-02-03T16:27:55.000000Z", + "updated_at": "2022-02-03T16:27:55.000000Z", + "external_auth_id": "abc123456", + "slug": "dan-brown", + "last_activity_at": "2022-02-03T16:27:55.000000Z", + "profile_url": "https://docs.example.com/user/dan-brown", + "edit_url": "https://docs.example.com/settings/users/1", + "avatar_url": "https://docs.example.com/uploads/images/user/2021-10/thumbs-50-50/profile-2021.jpg", + "roles": [ + { + "id": 1, + "display_name": "Admin" + } + ] +} \ No newline at end of file diff --git a/dev/api/responses/users-update.json b/dev/api/responses/users-update.json new file mode 100644 index 000000000..611e4e14a --- /dev/null +++ b/dev/api/responses/users-update.json @@ -0,0 +1,19 @@ +{ + "id": 1, + "name": "Dan Spaggleforth", + "email": "dspaggles@example.com", + "created_at": "2022-02-03T16:27:55.000000Z", + "updated_at": "2022-02-03T16:27:55.000000Z", + "external_auth_id": "abc123456", + "slug": "dan-spaggleforth", + "last_activity_at": "2022-02-03T16:27:55.000000Z", + "profile_url": "https://docs.example.com/user/dan-spaggleforth", + "edit_url": "https://docs.example.com/settings/users/1", + "avatar_url": "https://docs.example.com/uploads/images/user/2021-10/thumbs-50-50/profile-2021.jpg", + "roles": [ + { + "id": 2, + "display_name": "Editors" + } + ] +} \ No newline at end of file From c8be6ee8a6be840e8573309b8505fb53e9a9982c Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Fri, 4 Feb 2022 01:02:13 +0000 Subject: [PATCH 8/8] Addressed test failures from users API changes --- app/Auth/UserRepo.php | 2 +- app/Exceptions/NotifyException.php | 5 +++++ app/Http/Controllers/UserController.php | 1 + tests/Auth/UserInviteTest.php | 4 +--- tests/User/UserManagementTest.php | 2 +- 5 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php index c87fda4c8..f9cfc078e 100644 --- a/app/Auth/UserRepo.php +++ b/app/Auth/UserRepo.php @@ -192,7 +192,7 @@ class UserRepo */ public function create(array $data, bool $sendInvite = false): User { - $user = $this->createWithoutActivity($data, false); + $user = $this->createWithoutActivity($data, true); if ($sendInvite) { $this->inviteService->sendInvitation($user); diff --git a/app/Exceptions/NotifyException.php b/app/Exceptions/NotifyException.php index e09247208..ced478090 100644 --- a/app/Exceptions/NotifyException.php +++ b/app/Exceptions/NotifyException.php @@ -39,6 +39,11 @@ class NotifyException extends Exception implements Responsable { $message = $this->getMessage(); + // Front-end JSON handling. API-side handling managed via handler. + if ($request->wantsJson()) { + return response()->json(['error' => $message], 403); + } + if (!empty($message)) { session()->flash('error', $message); } diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 46e858d9b..3b443aa81 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -140,6 +140,7 @@ class UserController extends Controller 'language' => ['string'], 'roles' => ['array'], 'roles.*' => ['integer'], + 'external_auth_id' => ['string'], 'profile_image' => array_merge(['nullable'], $this->getImageValidationRules()), ]); diff --git a/tests/Auth/UserInviteTest.php b/tests/Auth/UserInviteTest.php index 922a98ef3..38124cc1a 100644 --- a/tests/Auth/UserInviteTest.php +++ b/tests/Auth/UserInviteTest.php @@ -45,9 +45,7 @@ class UserInviteTest extends TestCase 'name' => 'Barry', 'email' => $email, 'send_invite' => 'true', - 'setting' => [ - 'language' => 'de', - ], + 'language' => 'de', ]); $resp->assertRedirect('/settings/users'); diff --git a/tests/User/UserManagementTest.php b/tests/User/UserManagementTest.php index 2fbbee7e2..5870b6827 100644 --- a/tests/User/UserManagementTest.php +++ b/tests/User/UserManagementTest.php @@ -189,7 +189,7 @@ class UserManagementTest extends TestCase foreach ($langs as $lang) { config()->set('app.locale', $lang); $resp = $this->asAdmin()->get('/settings/users/create'); - $resp->assertElementExists('select[name="setting[language]"] option[value="' . $lang . '"][selected]'); + $resp->assertElementExists('select[name="language"] option[value="' . $lang . '"][selected]'); } }