From d089623aac6b39641a0ff610c124cb3a01609efd Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 3 Feb 2022 12:33:26 +0000 Subject: [PATCH] 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, + ] + ], + ]); + } +}