From 8e274a5a84cc11c20c0d62f9ecc0b08cd9410305 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Wed, 2 Mar 2016 22:35:01 +0000 Subject: [PATCH] Refactored some permission controls and increased testing for roles system --- app/Exceptions/PermissionsException.php | 6 + app/Http/Controllers/PermissionController.php | 89 +++-------- app/Repos/PermissionsRepo.php | 141 ++++++++++++++++++ app/Role.php | 14 -- app/User.php | 2 +- database/factories/ModelFactory.php | 8 + tests/RolesTest.php | 45 +++++- tests/TestCase.php | 4 +- 8 files changed, 216 insertions(+), 93 deletions(-) create mode 100644 app/Exceptions/PermissionsException.php create mode 100644 app/Repos/PermissionsRepo.php diff --git a/app/Exceptions/PermissionsException.php b/app/Exceptions/PermissionsException.php new file mode 100644 index 000000000..ae4c676d6 --- /dev/null +++ b/app/Exceptions/PermissionsException.php @@ -0,0 +1,6 @@ +role = $role; - $this->permission = $permission; + $this->permissionsRepo = $permissionsRepo; parent::__construct(); } @@ -32,7 +26,7 @@ class PermissionController extends Controller public function listRoles() { $this->checkPermission('user-roles-manage'); - $roles = $this->role->all(); + $roles = $this->permissionsRepo->getAllRoles(); return view('settings/roles/index', ['roles' => $roles]); } @@ -59,22 +53,7 @@ class PermissionController extends Controller 'description' => 'max:250' ]); - $role = $this->role->newInstance($request->all()); - $role->name = str_replace(' ', '-', strtolower($request->get('display_name'))); - // Prevent duplicate names - while ($this->role->where('name', '=', $role->name)->count() > 0) { - $role->name .= strtolower(str_random(2)); - } - $role->save(); - - if ($request->has('permissions')) { - $permissionsNames = array_keys($request->get('permissions')); - $permissions = $this->permission->whereIn('name', $permissionsNames)->pluck('id')->toArray(); - $role->permissions()->sync($permissions); - } else { - $role->permissions()->sync([]); - } - + $this->permissionsRepo->saveNewRole($request->all()); session()->flash('success', 'Role successfully created'); return redirect('/settings/roles'); } @@ -87,7 +66,7 @@ class PermissionController extends Controller public function editRole($id) { $this->checkPermission('user-roles-manage'); - $role = $this->role->findOrFail($id); + $role = $this->permissionsRepo->getRoleById($id); return view('settings/roles/edit', ['role' => $role]); } @@ -105,24 +84,7 @@ class PermissionController extends Controller 'description' => 'max:250' ]); - $role = $this->role->findOrFail($id); - if ($request->has('permissions')) { - $permissionsNames = array_keys($request->get('permissions')); - $permissions = $this->permission->whereIn('name', $permissionsNames)->pluck('id')->toArray(); - $role->permissions()->sync($permissions); - } else { - $role->permissions()->sync([]); - } - - // Ensure admin account always has all permissions - if ($role->name === 'admin') { - $permissions = $this->permission->all()->pluck('id')->toArray(); - $role->permissions()->sync($permissions); - } - - $role->fill($request->all()); - $role->save(); - + $this->permissionsRepo->updateRole($id, $request->all()); session()->flash('success', 'Role successfully updated'); return redirect('/settings/roles'); } @@ -136,9 +98,9 @@ class PermissionController extends Controller public function showDeleteRole($id) { $this->checkPermission('user-roles-manage'); - $role = $this->role->findOrFail($id); - $roles = $this->role->where('id', '!=', $id)->get(); - $blankRole = $this->role->newInstance(['display_name' => 'Don\'t migrate users']); + $role = $this->permissionsRepo->getRoleById($id); + $roles = $this->permissionsRepo->getAllRolesExcept($role); + $blankRole = $role->newInstance(['display_name' => 'Don\'t migrate users']); $roles->prepend($blankRole); return view('settings/roles/delete', ['role' => $role, 'roles' => $roles]); } @@ -153,29 +115,14 @@ class PermissionController extends Controller public function deleteRole($id, Request $request) { $this->checkPermission('user-roles-manage'); - $role = $this->role->findOrFail($id); - // Prevent deleting admin role - if ($role->name === 'admin') { - session()->flash('error', 'The admin role cannot be deleted'); + try { + $this->permissionsRepo->deleteRole($id, $request->get('migrate_role_id')); + } catch (PermissionsException $e) { + session()->flash('error', $e->getMessage()); return redirect()->back(); } - if ($role->id == \Setting::get('registration-role')) { - session()->flash('error', 'This role cannot be deleted while set as the default registration role.'); - return redirect()->back(); - } - - if ($request->has('migration_role_id')) { - $newRole = $this->role->find($request->get('migration_role_id')); - if ($newRole) { - $users = $role->users->pluck('id')->toArray(); - $newRole->users()->sync($users); - } - } - - $role->delete(); - session()->flash('success', 'Role successfully deleted'); return redirect('/settings/roles'); } diff --git a/app/Repos/PermissionsRepo.php b/app/Repos/PermissionsRepo.php new file mode 100644 index 000000000..c35f29d10 --- /dev/null +++ b/app/Repos/PermissionsRepo.php @@ -0,0 +1,141 @@ +permission = $permission; + $this->role = $role; + } + + /** + * Get all the user roles from the system. + * @return \Illuminate\Database\Eloquent\Collection|static[] + */ + public function getAllRoles() + { + return $this->role->all(); + } + + /** + * Get all the roles except for the provided one. + * @param Role $role + * @return mixed + */ + public function getAllRolesExcept(Role $role) + { + return $this->role->where('id', '!=', $role->id)->get(); + } + + /** + * Get a role via its ID. + * @param $id + * @return mixed + */ + public function getRoleById($id) + { + return $this->role->findOrFail($id); + } + + /** + * Save a new role into the system. + * @param array $roleData + * @return Role + */ + public function saveNewRole($roleData) + { + $role = $this->role->newInstance($roleData); + $role->name = str_replace(' ', '-', strtolower($roleData['display_name'])); + // Prevent duplicate names + while ($this->role->where('name', '=', $role->name)->count() > 0) { + $role->name .= strtolower(str_random(2)); + } + $role->save(); + + $permissions = isset($roleData['permissions']) ? array_keys($roleData['permissions']) : []; + $this->assignRolePermissions($role, $permissions); + return $role; + } + + /** + * Updates an existing role. + * Ensure Admin role always has all permissions. + * @param $roleId + * @param $roleData + */ + public function updateRole($roleId, $roleData) + { + $role = $this->role->findOrFail($roleId); + $permissions = isset($roleData['permissions']) ? array_keys($roleData['permissions']) : []; + $this->assignRolePermissions($role, $permissions); + + if ($role->name === 'admin') { + $permissions = $this->permission->all()->pluck('id')->toArray(); + $role->permissions()->sync($permissions); + } + + $role->fill($roleData); + $role->save(); + } + + /** + * Assign an list of permission names to an role. + * @param Role $role + * @param array $permissionNameArray + */ + public function assignRolePermissions(Role $role, $permissionNameArray = []) + { + $permissions = []; + if ($permissionNameArray && count($permissionNameArray) > 0) { + $permissions = $this->permission->whereIn('name', $permissionNameArray)->pluck('id')->toArray(); + } + $role->permissions()->sync($permissions); + } + + /** + * 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 + * will be added to the role of the specified id. + * @param $roleId + * @param $migrateRoleId + * @throws PermissionsException + */ + public function deleteRole($roleId, $migrateRoleId) + { + $role = $this->role->findOrFail($roleId); + + // Prevent deleting admin role or default registration role. + if ($role->name === 'admin') { + throw new PermissionsException('The admin role cannot be deleted'); + } else if ($role->id == Setting::get('registration-role')) { + throw new PermissionsException('This role cannot be deleted while set as the default registration role.'); + } + + if ($migrateRoleId) { + $newRole = $this->role->find($migrateRoleId); + if ($newRole) { + $users = $role->users->pluck('id')->toArray(); + $newRole->users()->sync($users); + } + } + + $role->delete(); + } + +} \ No newline at end of file diff --git a/app/Role.php b/app/Role.php index 8d5ed7d66..270e4e0b8 100644 --- a/app/Role.php +++ b/app/Role.php @@ -8,11 +8,6 @@ class Role extends Model { protected $fillable = ['display_name', 'description']; - /** - * Sets the default role name for newly registered users. - * @var string - */ - protected static $default = 'viewer'; /** * The roles that belong to the role. @@ -48,15 +43,6 @@ class Role extends Model $this->permissions()->attach($permission->id); } - /** - * Get an instance of the default role. - * @return Role - */ - public static function getDefault() - { - return static::getRole(static::$default); - } - /** * Get the role object for the specified role. * @param $roleName diff --git a/app/User.php b/app/User.php index b062aa78f..2d14c6e6e 100644 --- a/app/User.php +++ b/app/User.php @@ -106,7 +106,7 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon */ public function attachRoleId($id) { - $this->roles()->sync([$id]); + $this->roles()->attach([$id]); } /** diff --git a/database/factories/ModelFactory.php b/database/factories/ModelFactory.php index e0f155087..2840356e8 100644 --- a/database/factories/ModelFactory.php +++ b/database/factories/ModelFactory.php @@ -17,6 +17,7 @@ $factory->define(BookStack\User::class, function ($faker) { 'email' => $faker->email, 'password' => str_random(10), 'remember_token' => str_random(10), + 'email_confirmed' => 1 ]; }); @@ -45,3 +46,10 @@ $factory->define(BookStack\Page::class, function ($faker) { 'text' => strip_tags($html) ]; }); + +$factory->define(BookStack\Role::class, function ($faker) { + return [ + 'display_name' => $faker->sentence(3), + 'description' => $faker->sentence(10) + ]; +}); \ No newline at end of file diff --git a/tests/RolesTest.php b/tests/RolesTest.php index 140290bea..7349c2968 100644 --- a/tests/RolesTest.php +++ b/tests/RolesTest.php @@ -9,13 +9,14 @@ class RolesTest extends TestCase parent::setUp(); } + /** + * Create a new basic role for testing purposes. + * @return static + */ protected function createNewRole() { - return \BookStack\Role::forceCreate([ - 'name' => 'test-role', - 'display_name' => 'Test Role', - 'description' => 'This is a role for testing' - ]); + $permissionRepo = app('BookStack\Repos\PermissionsRepo'); + return $permissionRepo->saveNewRole(factory(\BookStack\Role::class)->make()->toArray()); } public function test_admin_can_see_settings() @@ -45,4 +46,38 @@ class RolesTest extends TestCase ->see('cannot be deleted'); } + public function test_role_create_update_delete_flow() + { + $testRoleName = 'Test Role'; + $testRoleDesc = 'a little test description'; + $testRoleUpdateName = 'An Super Updated role'; + + // Creation + $this->asAdmin()->visit('/settings') + ->click('Roles') + ->seePageIs('/settings/roles') + ->click('Add new role') + ->type('Test Role', 'display_name') + ->type('A little test description', 'description') + ->press('Save Role') + ->seeInDatabase('roles', ['display_name' => $testRoleName, 'name' => 'test-role', 'description' => $testRoleDesc]) + ->seePageIs('/settings/roles'); + // Updating + $this->asAdmin()->visit('/settings/roles') + ->see($testRoleDesc) + ->click($testRoleName) + ->type($testRoleUpdateName, '#display_name') + ->press('Save Role') + ->seeInDatabase('roles', ['display_name' => $testRoleUpdateName, 'name' => 'test-role', 'description' => $testRoleDesc]) + ->seePageIs('/settings/roles'); + // Deleting + $this->asAdmin()->visit('/settings/roles') + ->click($testRoleUpdateName) + ->click('Delete Role') + ->see($testRoleUpdateName) + ->press('Confirm') + ->seePageIs('/settings/roles') + ->dontSee($testRoleUpdateName); + } + } diff --git a/tests/TestCase.php b/tests/TestCase.php index ce4e93b12..a521fd076 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -79,8 +79,8 @@ class TestCase extends Illuminate\Foundation\Testing\TestCase protected function getNewUser($attributes = []) { $user = factory(\BookStack\User::class)->create($attributes); - $userRepo = app('BookStack\Repos\UserRepo'); - $userRepo->attachDefaultRole($user); + $role = \BookStack\Role::getRole('editor'); + $user->attachRole($role);; return $user; }