From 723f108bd9b7f53ab90ff113d1a3ecb6958db801 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 18 Feb 2023 18:36:34 +0000 Subject: [PATCH] Aded roles API controller methods Altered & updated permissions repo, and existing connected RoleController to suit. Also extracts in-app success notifications to auto activity system. Tweaked tests where required. --- app/Auth/Permissions/PermissionsRepo.php | 68 ++++----- app/Auth/Role.php | 2 +- app/Auth/User.php | 2 +- app/Http/Controllers/Api/ApiController.php | 11 +- .../Controllers/Api/RoleApiController.php | 133 ++++++++++++++++++ app/Http/Controllers/RoleController.php | 32 +++-- lang/en/activities.php | 5 + lang/en/settings.php | 3 - routes/api.php | 7 + tests/Api/RolesApiTest.php | 43 +++--- tests/Helpers/UserRoleProvider.php | 2 +- tests/Permissions/RolesTest.php | 2 +- 12 files changed, 238 insertions(+), 72 deletions(-) create mode 100644 app/Http/Controllers/Api/RoleApiController.php diff --git a/app/Auth/Permissions/PermissionsRepo.php b/app/Auth/Permissions/PermissionsRepo.php index 6dcef7256..75354e79b 100644 --- a/app/Auth/Permissions/PermissionsRepo.php +++ b/app/Auth/Permissions/PermissionsRepo.php @@ -12,11 +12,8 @@ use Illuminate\Database\Eloquent\Collection; class PermissionsRepo { protected JointPermissionBuilder $permissionBuilder; - protected $systemRoles = ['admin', 'public']; + protected array $systemRoles = ['admin', 'public']; - /** - * PermissionsRepo constructor. - */ public function __construct(JointPermissionBuilder $permissionBuilder) { $this->permissionBuilder = $permissionBuilder; @@ -41,7 +38,7 @@ class PermissionsRepo /** * Get a role via its ID. */ - public function getRoleById($id): Role + public function getRoleById(int $id): Role { return Role::query()->findOrFail($id); } @@ -52,10 +49,10 @@ class PermissionsRepo public function saveNewRole(array $roleData): Role { $role = new Role($roleData); - $role->mfa_enforced = ($roleData['mfa_enforced'] ?? 'false') === 'true'; + $role->mfa_enforced = boolval($roleData['mfa_enforced'] ?? false); $role->save(); - $permissions = isset($roleData['permissions']) ? array_keys($roleData['permissions']) : []; + $permissions = $roleData['permissions'] ?? []; $this->assignRolePermissions($role, $permissions); $this->permissionBuilder->rebuildForRole($role); @@ -66,42 +63,45 @@ class PermissionsRepo /** * Updates an existing role. - * Ensure Admin role always have core permissions. + * Ensures Admin system role always have core permissions. */ - public function updateRole($roleId, array $roleData) + public function updateRole($roleId, array $roleData): Role { $role = $this->getRoleById($roleId); - $permissions = isset($roleData['permissions']) ? array_keys($roleData['permissions']) : []; + if (isset($roleData['permissions'])) { + $this->assignRolePermissions($role, $roleData['permissions']); + } + + $role->fill($roleData); + $role->save(); + $this->permissionBuilder->rebuildForRole($role); + + Activity::add(ActivityType::ROLE_UPDATE, $role); + + return $role; + } + + /** + * Assign a list of permission names to the given role. + */ + protected function assignRolePermissions(Role $role, array $permissionNameArray = []): void + { + $permissions = []; + $permissionNameArray = array_values($permissionNameArray); + + // Ensure the admin system role retains vital system permissions if ($role->system_name === 'admin') { - $permissions = array_merge($permissions, [ + $permissionNameArray = array_unique(array_merge($permissionNameArray, [ 'users-manage', 'user-roles-manage', 'restrictions-manage-all', 'restrictions-manage-own', 'settings-manage', - ]); + ])); } - $this->assignRolePermissions($role, $permissions); - - $role->fill($roleData); - $role->mfa_enforced = ($roleData['mfa_enforced'] ?? 'false') === 'true'; - $role->save(); - $this->permissionBuilder->rebuildForRole($role); - - Activity::add(ActivityType::ROLE_UPDATE, $role); - } - - /** - * Assign a list of permission names to a role. - */ - protected function assignRolePermissions(Role $role, array $permissionNameArray = []) - { - $permissions = []; - $permissionNameArray = array_values($permissionNameArray); - - if ($permissionNameArray) { + if (!empty($permissionNameArray)) { $permissions = RolePermission::query() ->whereIn('name', $permissionNameArray) ->pluck('id') @@ -114,13 +114,13 @@ class PermissionsRepo /** * Delete a role from the system. * Check it's not an admin role or set as default before deleting. - * If an migration Role ID is specified the users assign to the current role + * If a migration Role ID is specified the users assign to the current role * will be added to the role of the specified id. * * @throws PermissionsException * @throws Exception */ - public function deleteRole($roleId, $migrateRoleId) + public function deleteRole(int $roleId, int $migrateRoleId = 0): void { $role = $this->getRoleById($roleId); @@ -131,7 +131,7 @@ class PermissionsRepo throw new PermissionsException(trans('errors.role_registration_default_cannot_delete')); } - if ($migrateRoleId) { + if ($migrateRoleId !== 0) { $newRole = Role::query()->find($migrateRoleId); if ($newRole) { $users = $role->users()->pluck('id')->toArray(); diff --git a/app/Auth/Role.php b/app/Auth/Role.php index b293d1af2..0f43dd18c 100644 --- a/app/Auth/Role.php +++ b/app/Auth/Role.php @@ -27,7 +27,7 @@ class Role extends Model implements Loggable { use HasFactory; - protected $fillable = ['display_name', 'description', 'external_auth_id']; + protected $fillable = ['display_name', 'description', 'external_auth_id', 'mfa_enforced']; protected $hidden = ['pivot']; diff --git a/app/Auth/User.php b/app/Auth/User.php index cf9f20e52..90bb3d68e 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', 'user_id', + 'created_at', 'updated_at', 'image_id', 'roles', 'avatar', 'user_id', 'pivot', ]; /** diff --git a/app/Http/Controllers/Api/ApiController.php b/app/Http/Controllers/Api/ApiController.php index 9652654be..5c448e49f 100644 --- a/app/Http/Controllers/Api/ApiController.php +++ b/app/Http/Controllers/Api/ApiController.php @@ -32,10 +32,15 @@ abstract class ApiController extends Controller */ public function getValidationRules(): array { - if (method_exists($this, 'rules')) { - return $this->rules(); - } + return $this->rules(); + } + /** + * Get the validation rules for the actions in this controller. + * Defaults to a $rules property but can be a rules() method. + */ + protected function rules(): array + { return $this->rules; } } diff --git a/app/Http/Controllers/Api/RoleApiController.php b/app/Http/Controllers/Api/RoleApiController.php new file mode 100644 index 000000000..119279822 --- /dev/null +++ b/app/Http/Controllers/Api/RoleApiController.php @@ -0,0 +1,133 @@ + [ + 'display_name' => ['required', 'min:3', 'max:180'], + 'description' => ['max:180'], + 'mfa_enforced' => ['boolean'], + 'external_auth_id' => ['string'], + 'permissions' => ['array'], + 'permissions.*' => ['string'], + ], + 'update' => [ + 'display_name' => ['min:3', 'max:180'], + 'description' => ['max:180'], + 'mfa_enforced' => ['boolean'], + 'external_auth_id' => ['string'], + 'permissions' => ['array'], + 'permissions.*' => ['string'], + ] + ]; + + public function __construct(PermissionsRepo $permissionsRepo) + { + $this->permissionsRepo = $permissionsRepo; + + // Checks for all endpoints in this controller + $this->middleware(function ($request, $next) { + $this->checkPermission('user-roles-manage'); + + return $next($request); + }); + } + + /** + * Get a listing of roles in the system. + * Requires permission to manage roles. + */ + public function list() + { + $roles = Role::query()->select(['*']) + ->withCount(['users', 'permissions']); + + return $this->apiListingResponse($roles, [ + ...$this->fieldsToExpose, + 'permissions_count', + 'users_count', + ]); + } + + /** + * Create a new role in the system. + * Requires permission to manage roles. + */ + public function create(Request $request) + { + $data = $this->validate($request, $this->rules()['create']); + + $role = null; + DB::transaction(function () use ($data, &$role) { + $role = $this->permissionsRepo->saveNewRole($data); + }); + + $this->singleFormatter($role); + + return response()->json($role); + } + + /** + * View the details of a single user. + * Requires permission to manage roles. + */ + public function read(string $id) + { + $user = $this->permissionsRepo->getRoleById($id); + $this->singleFormatter($user); + + return response()->json($user); + } + + /** + * Update an existing role in the system. + * Requires permission to manage roles. + */ + public function update(Request $request, string $id) + { + $data = $this->validate($request, $this->rules()['update']); + $role = $this->permissionsRepo->updateRole($id, $data); + + $this->singleFormatter($role); + + return response()->json($role); + } + + /** + * 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 roles. + */ + public function delete(string $id) + { + $this->permissionsRepo->deleteRole(intval($id)); + + return response('', 204); + } + + /** + * Format the given role model for single-result display. + */ + protected function singleFormatter(Role $role) + { + $role->load('users:id,name,slug'); + $role->unsetRelation('permissions'); + $role->setAttribute('permissions', $role->permissions()->pluck('name')); + $role->makeVisible(['users', 'permissions']); + } +} diff --git a/app/Http/Controllers/RoleController.php b/app/Http/Controllers/RoleController.php index a9be19e0c..2bf0a7b19 100644 --- a/app/Http/Controllers/RoleController.php +++ b/app/Http/Controllers/RoleController.php @@ -74,13 +74,17 @@ class RoleController extends Controller public function store(Request $request) { $this->checkPermission('user-roles-manage'); - $this->validate($request, [ + $data = $this->validate($request, [ 'display_name' => ['required', 'min:3', 'max:180'], 'description' => ['max:180'], + 'external_auth_id' => ['string'], + 'permissions' => ['array'], + 'mfa_enforced' => ['string'], ]); - $this->permissionsRepo->saveNewRole($request->all()); - $this->showSuccessNotification(trans('settings.role_create_success')); + $data['permissions'] = array_keys($data['permissions'] ?? []); + $data['mfa_enforced'] = ($data['mfa_enforced'] ?? 'false') === 'true'; + $this->permissionsRepo->saveNewRole($data); return redirect('/settings/roles'); } @@ -100,19 +104,27 @@ class RoleController extends Controller /** * Updates a user role. - * - * @throws ValidationException */ public function update(Request $request, string $id) { $this->checkPermission('user-roles-manage'); - $this->validate($request, [ + $data = $this->validate($request, [ 'display_name' => ['required', 'min:3', 'max:180'], 'description' => ['max:180'], + 'external_auth_id' => ['string'], + 'permissions' => ['array'], + 'mfa_enforced' => ['string'], ]); - $this->permissionsRepo->updateRole($id, $request->all()); - $this->showSuccessNotification(trans('settings.role_update_success')); + if (isset($data['permissions'])) { + $data['permissions'] = array_keys($data['permissions']); + } + + if (isset($data['mfa_enforced'])) { + $data['mfa_enforced'] = $data['mfa_enforced'] === 'true'; + } + + $this->permissionsRepo->updateRole($id, $data); return redirect('/settings/roles'); } @@ -145,15 +157,13 @@ class RoleController extends Controller $this->checkPermission('user-roles-manage'); try { - $this->permissionsRepo->deleteRole($id, $request->get('migrate_role_id')); + $this->permissionsRepo->deleteRole($id, $request->get('migrate_role_id', 0)); } catch (PermissionsException $e) { $this->showErrorNotification($e->getMessage()); return redirect()->back(); } - $this->showSuccessNotification(trans('settings.role_delete_success')); - return redirect('/settings/roles'); } } diff --git a/lang/en/activities.php b/lang/en/activities.php index f348bff1f..e89b8eab2 100644 --- a/lang/en/activities.php +++ b/lang/en/activities.php @@ -67,6 +67,11 @@ return [ 'user_update_notification' => 'User successfully updated', 'user_delete_notification' => 'User successfully removed', + // Roles + 'role_create_notification' => 'Role successfully created', + 'role_update_notification' => 'Role successfully updated', + 'role_delete_notification' => 'Role successfully deleted', + // Other 'commented_on' => 'commented on', 'permissions_update' => 'updated permissions', diff --git a/lang/en/settings.php b/lang/en/settings.php index 6f4376d42..5a94a824a 100644 --- a/lang/en/settings.php +++ b/lang/en/settings.php @@ -143,13 +143,11 @@ return [ 'roles_assigned_users' => 'Assigned Users', 'roles_permissions_provided' => 'Provided Permissions', 'role_create' => 'Create New Role', - 'role_create_success' => 'Role successfully created', 'role_delete' => 'Delete Role', 'role_delete_confirm' => 'This will delete the role with the name \':roleName\'.', 'role_delete_users_assigned' => 'This role has :userCount users assigned to it. If you would like to migrate the users from this role select a new role below.', 'role_delete_no_migration' => "Don't migrate users", 'role_delete_sure' => 'Are you sure you want to delete this role?', - 'role_delete_success' => 'Role successfully deleted', 'role_edit' => 'Edit Role', 'role_details' => 'Role Details', 'role_name' => 'Role Name', @@ -175,7 +173,6 @@ return [ 'role_own' => 'Own', 'role_controlled_by_asset' => 'Controlled by the asset they are uploaded to', 'role_save' => 'Save Role', - 'role_update_success' => 'Role successfully updated', 'role_users' => 'Users in this role', 'role_users_none' => 'No users are currently assigned to this role', diff --git a/routes/api.php b/routes/api.php index d350fd86b..aa3f66b60 100644 --- a/routes/api.php +++ b/routes/api.php @@ -16,6 +16,7 @@ use BookStack\Http\Controllers\Api\ChapterExportApiController; use BookStack\Http\Controllers\Api\PageApiController; use BookStack\Http\Controllers\Api\PageExportApiController; use BookStack\Http\Controllers\Api\RecycleBinApiController; +use BookStack\Http\Controllers\Api\RoleApiController; use BookStack\Http\Controllers\Api\SearchApiController; use BookStack\Http\Controllers\Api\UserApiController; use Illuminate\Support\Facades\Route; @@ -75,6 +76,12 @@ Route::get('users/{id}', [UserApiController::class, 'read']); Route::put('users/{id}', [UserApiController::class, 'update']); Route::delete('users/{id}', [UserApiController::class, 'delete']); +Route::get('roles', [RoleApiController::class, 'list']); +Route::post('roles', [RoleApiController::class, 'create']); +Route::get('roles/{id}', [RoleApiController::class, 'read']); +Route::put('roles/{id}', [RoleApiController::class, 'update']); +Route::delete('roles/{id}', [RoleApiController::class, 'delete']); + Route::get('recycle-bin', [RecycleBinApiController::class, 'list']); Route::put('recycle-bin/{deletionId}', [RecycleBinApiController::class, 'restore']); Route::delete('recycle-bin/{deletionId}', [RecycleBinApiController::class, 'destroy']); diff --git a/tests/Api/RolesApiTest.php b/tests/Api/RolesApiTest.php index 38026a40a..e231c167c 100644 --- a/tests/Api/RolesApiTest.php +++ b/tests/Api/RolesApiTest.php @@ -40,14 +40,15 @@ class RolesApiTest extends TestCase $resp = $this->getJson($this->baseEndpoint . '?count=1&sort=+id'); $resp->assertJson(['data' => [ [ - 'id' => $firstRole->id, - 'display_name' => $firstRole->display_name, - 'description' => $firstRole->description, - 'mfa_enforced' => $firstRole->mfa_enforced, + 'id' => $firstRole->id, + 'display_name' => $firstRole->display_name, + 'description' => $firstRole->description, + 'mfa_enforced' => $firstRole->mfa_enforced, + 'external_auth_id' => $firstRole->external_auth_id, 'permissions_count' => $firstRole->permissions()->count(), - 'users_count' => $firstRole->users()->count(), - 'created_at' => $firstRole->created_at->toJSON(), - 'updated_at' => $firstRole->updated_at->toJSON(), + 'users_count' => $firstRole->users()->count(), + 'created_at' => $firstRole->created_at->toJSON(), + 'updated_at' => $firstRole->updated_at->toJSON(), ], ]]); @@ -64,11 +65,12 @@ class RolesApiTest extends TestCase 'display_name' => 'My awesome role', 'description' => 'My great role description', 'mfa_enforced' => true, + 'external_auth_id' => 'auth_id', 'permissions' => [ 'content-export', - 'users-manage', - 'page-view-own', 'page-view-all', + 'page-view-own', + 'users-manage', ] ]); @@ -77,11 +79,12 @@ class RolesApiTest extends TestCase 'display_name' => 'My awesome role', 'description' => 'My great role description', 'mfa_enforced' => true, + 'external_auth_id' => 'auth_id', 'permissions' => [ 'content-export', - 'users-manage', - 'page-view-own', 'page-view-all', + 'page-view-own', + 'users-manage', ] ]); @@ -89,6 +92,7 @@ class RolesApiTest extends TestCase 'display_name' => 'My awesome role', 'description' => 'My great role description', 'mfa_enforced' => true, + 'external_auth_id' => 'auth_id', ]); /** @var Role $role */ @@ -107,7 +111,7 @@ class RolesApiTest extends TestCase 'description' => 'My new role', ]); $resp->assertStatus(422); - $resp->assertJson($this->validationResponse(['display_name' => ['The display_name field is required.']])); + $resp->assertJson($this->validationResponse(['display_name' => ['The display name field is required.']])); $resp = $this->postJson($this->baseEndpoint, [ 'name' => 'My great role with a too long desc', @@ -128,6 +132,7 @@ class RolesApiTest extends TestCase 'display_name' => $role->display_name, 'description' => $role->description, 'mfa_enforced' => $role->mfa_enforced, + 'external_auth_id' => $role->external_auth_id, 'permissions' => $role->permissions()->pluck('name')->toArray(), 'users' => $role->users()->get()->map(function (User $user) { return [ @@ -147,11 +152,12 @@ class RolesApiTest extends TestCase 'display_name' => 'My updated role', 'description' => 'My great role description', 'mfa_enforced' => true, + 'external_auth_id' => 'updated_auth_id', 'permissions' => [ 'content-export', - 'users-manage', - 'page-view-own', 'page-view-all', + 'page-view-own', + 'users-manage', ] ]); @@ -161,16 +167,18 @@ class RolesApiTest extends TestCase 'display_name' => 'My updated role', 'description' => 'My great role description', 'mfa_enforced' => true, + 'external_auth_id' => 'updated_auth_id', 'permissions' => [ 'content-export', - 'users-manage', - 'page-view-own', 'page-view-all', + 'page-view-own', + 'users-manage', ] ]); $role->refresh(); $this->assertEquals(4, $role->permissions()->count()); + $this->assertActivityExists(ActivityType::ROLE_UPDATE); } public function test_update_endpoint_does_not_remove_info_if_not_provided() @@ -181,10 +189,11 @@ class RolesApiTest extends TestCase $permissionCount = $role->permissions()->count(); $resp->assertStatus(200); - $this->assertDatabaseHas('users', [ + $this->assertDatabaseHas('roles', [ 'id' => $role->id, 'display_name' => $role->display_name, 'description' => $role->description, + 'external_auth_id' => $role->external_auth_id, ]); $role->refresh(); diff --git a/tests/Helpers/UserRoleProvider.php b/tests/Helpers/UserRoleProvider.php index 355c1687c..a06112189 100644 --- a/tests/Helpers/UserRoleProvider.php +++ b/tests/Helpers/UserRoleProvider.php @@ -90,7 +90,7 @@ class UserRoleProvider { $permissionRepo = app(PermissionsRepo::class); $roleData = Role::factory()->make()->toArray(); - $roleData['permissions'] = array_flip($rolePermissions); + $roleData['permissions'] = $rolePermissions; return $permissionRepo->saveNewRole($roleData); } diff --git a/tests/Permissions/RolesTest.php b/tests/Permissions/RolesTest.php index 8bf700c07..d4d975dbd 100644 --- a/tests/Permissions/RolesTest.php +++ b/tests/Permissions/RolesTest.php @@ -869,7 +869,7 @@ class RolesTest extends TestCase $this->asAdmin()->put('/settings/roles/' . $viewerRole->id, [ 'display_name' => $viewerRole->display_name, 'description' => $viewerRole->description, - 'permission' => [], + 'permissions' => [], ])->assertStatus(302); $this->actingAs($viewer)->get($page->getUrl())->assertStatus(404);