Refactored some permission controls and increased testing for roles system

app/Exceptions/PermissionsException.php

@@ -0,0 +1,6 @@
+<?php namespace BookStack\Exceptions;
+
+
+use Exception;
+
+class PermissionsException extends Exception {}

app/Http/Controllers/PermissionController.php

@@ -1,28 +1,22 @@
-<?php
+<?php namespace BookStack\Http\Controllers;
 
-namespace BookStack\Http\Controllers;
+use BookStack\Exceptions\PermissionsException;
-
+use BookStack\Repos\PermissionsRepo;
-use BookStack\Permission;
-use BookStack\Role;
 use Illuminate\Http\Request;
 use BookStack\Http\Requests;
 
 class PermissionController extends Controller
 {
 
-    protected $role;
+    protected $permissionsRepo;
-    protected $permission;
 
     /**
      * PermissionController constructor.
-     * @param Role $role
+     * @param PermissionsRepo $permissionsRepo
-     * @param Permission $permission
-     * @internal param $user
      */
-    public function __construct(Role $role, Permission $permission)
+    public function __construct(PermissionsRepo $permissionsRepo)
     {
-        $this->role = $role;
+        $this->permissionsRepo = $permissionsRepo;
-        $this->permission = $permission;
         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());
+        $this->permissionsRepo->saveNewRole($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([]);
-        }
-
         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);
+        $this->permissionsRepo->updateRole($id, $request->all());
-        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();
-
         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);
+        $role = $this->permissionsRepo->getRoleById($id);
-        $roles = $this->role->where('id', '!=', $id)->get();
+        $roles = $this->permissionsRepo->getAllRolesExcept($role);
-        $blankRole = $this->role->newInstance(['display_name' => 'Don\'t migrate users']);
+        $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
+        try {
-        if ($role->name === 'admin') {
+            $this->permissionsRepo->deleteRole($id, $request->get('migrate_role_id'));
-            session()->flash('error', 'The admin role cannot be deleted');
+        } 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');
     }

app/Repos/PermissionsRepo.php

@@ -0,0 +1,141 @@
+<?php namespace BookStack\Repos;
+
+
+use BookStack\Exceptions\PermissionsException;
+use BookStack\Permission;
+use BookStack\Role;
+use Setting;
+
+class PermissionsRepo
+{
+
+    protected $permission;
+    protected $role;
+
+    /**
+     * PermissionsRepo constructor.
+     * @param $permission
+     * @param $role
+     */
+    public function __construct(Permission $permission, Role $role)
+    {
+        $this->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();
+    }
+
+}

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

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]);
     }
 
     /**

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)
+    ];
+});

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([
+        $permissionRepo = app('BookStack\Repos\PermissionsRepo');
-            'name' => 'test-role',
+        return $permissionRepo->saveNewRole(factory(\BookStack\Role::class)->make()->toArray());
-            'display_name' => 'Test Role',
-            'description' => 'This is a role for testing'
-        ]);
     }
 
     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);
+    }
+
 }

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');
+        $role = \BookStack\Role::getRole('editor');
-        $userRepo->attachDefaultRole($user);
+        $user->attachRole($role);;
         return $user;
     }