diff --git a/app/Auth/Access/ExternalAuthService.php b/app/Auth/Access/ExternalAuthService.php index db8bd2dfb..4c71db21a 100644 --- a/app/Auth/Access/ExternalAuthService.php +++ b/app/Auth/Access/ExternalAuthService.php @@ -3,6 +3,8 @@ use BookStack\Auth\Role; use BookStack\Auth\User; use Illuminate\Database\Eloquent\Builder; +use Illuminate\Support\Collection; +use Illuminate\Support\Facades\DB; class ExternalAuthService { @@ -39,22 +41,14 @@ class ExternalAuthService /** * Match an array of group names to BookStack system roles. * Formats group names to be lower-case and hyphenated. - * @param array $groupNames - * @return \Illuminate\Support\Collection */ - protected function matchGroupsToSystemsRoles(array $groupNames) + protected function matchGroupsToSystemsRoles(array $groupNames): Collection { foreach ($groupNames as $i => $groupName) { $groupNames[$i] = str_replace(' ', '-', trim(strtolower($groupName))); } - $roles = Role::query()->where(function (Builder $query) use ($groupNames) { - $query->whereIn('name', $groupNames); - foreach ($groupNames as $groupName) { - $query->orWhere('external_auth_id', 'LIKE', '%' . $groupName . '%'); - } - })->get(); - + $roles = Role::query()->get(['id', 'external_auth_id', 'display_name']); $matchedRoles = $roles->filter(function (Role $role) use ($groupNames) { return $this->roleMatchesGroupNames($role, $groupNames); }); diff --git a/app/Auth/Permissions/PermissionsRepo.php b/app/Auth/Permissions/PermissionsRepo.php index 56ef19301..ce61093cc 100644 --- a/app/Auth/Permissions/PermissionsRepo.php +++ b/app/Auth/Permissions/PermissionsRepo.php @@ -1,8 +1,9 @@ permission = $permission; $this->role = $role; @@ -29,46 +27,34 @@ class PermissionsRepo /** * Get all the user roles from the system. - * @return \Illuminate\Database\Eloquent\Collection|static[] */ - public function getAllRoles() + public function getAllRoles(): Collection { return $this->role->all(); } /** * Get all the roles except for the provided one. - * @param Role $role - * @return mixed */ - public function getAllRolesExcept(Role $role) + public function getAllRolesExcept(Role $role): Collection { return $this->role->where('id', '!=', $role->id)->get(); } /** * Get a role via its ID. - * @param $id - * @return mixed */ - public function getRoleById($id) + public function getRoleById($id): Role { - return $this->role->findOrFail($id); + return $this->role->newQuery()->findOrFail($id); } /** * Save a new role into the system. - * @param array $roleData - * @return Role */ - public function saveNewRole($roleData) + public function saveNewRole(array $roleData): Role { $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']) : []; @@ -80,13 +66,11 @@ class PermissionsRepo /** * Updates an existing role. * Ensure Admin role always have core permissions. - * @param $roleId - * @param $roleData - * @throws PermissionsException */ - public function updateRole($roleId, $roleData) + public function updateRole($roleId, array $roleData) { - $role = $this->role->findOrFail($roleId); + /** @var Role $role */ + $role = $this->role->newQuery()->findOrFail($roleId); $permissions = isset($roleData['permissions']) ? array_keys($roleData['permissions']) : []; if ($role->system_name === 'admin') { @@ -108,16 +92,19 @@ class PermissionsRepo /** * Assign an list of permission names to an role. - * @param Role $role - * @param array $permissionNameArray */ - public function assignRolePermissions(Role $role, $permissionNameArray = []) + public function assignRolePermissions(Role $role, array $permissionNameArray = []) { $permissions = []; $permissionNameArray = array_values($permissionNameArray); - if ($permissionNameArray && count($permissionNameArray) > 0) { - $permissions = $this->permission->whereIn('name', $permissionNameArray)->pluck('id')->toArray(); + + if ($permissionNameArray) { + $permissions = $this->permission->newQuery() + ->whereIn('name', $permissionNameArray) + ->pluck('id') + ->toArray(); } + $role->permissions()->sync($permissions); } @@ -126,13 +113,13 @@ class PermissionsRepo * 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 + * @throws Exception */ public function deleteRole($roleId, $migrateRoleId) { - $role = $this->role->findOrFail($roleId); + /** @var Role $role */ + $role = $this->role->newQuery()->findOrFail($roleId); // Prevent deleting admin role or default registration role. if ($role->system_name && in_array($role->system_name, $this->systemRoles)) { @@ -142,9 +129,9 @@ class PermissionsRepo } if ($migrateRoleId) { - $newRole = $this->role->find($migrateRoleId); + $newRole = $this->role->newQuery()->find($migrateRoleId); if ($newRole) { - $users = $role->users->pluck('id')->toArray(); + $users = $role->users()->pluck('id')->toArray(); $newRole->users()->sync($users); } } diff --git a/app/Auth/Permissions/RolePermission.php b/app/Auth/Permissions/RolePermission.php index 8b07b3073..7f44ff815 100644 --- a/app/Auth/Permissions/RolePermission.php +++ b/app/Auth/Permissions/RolePermission.php @@ -3,6 +3,9 @@ use BookStack\Auth\Role; use BookStack\Model; +/** + * @property int $id + */ class RolePermission extends Model { /** diff --git a/app/Auth/Role.php b/app/Auth/Role.php index df9b1cea9..13ec6df16 100644 --- a/app/Auth/Role.php +++ b/app/Auth/Role.php @@ -3,13 +3,16 @@ use BookStack\Auth\Permissions\JointPermission; use BookStack\Auth\Permissions\RolePermission; use BookStack\Model; +use Illuminate\Database\Eloquent\Collection; +use Illuminate\Database\Eloquent\Relations\HasMany; /** * Class Role + * @property int $id * @property string $display_name * @property string $description * @property string $external_auth_id - * @package BookStack\Auth + * @property string $system_name */ class Role extends Model { @@ -26,9 +29,8 @@ class Role extends Model /** * Get all related JointPermissions. - * @return \Illuminate\Database\Eloquent\Relations\HasMany */ - public function jointPermissions() + public function jointPermissions(): HasMany { return $this->hasMany(JointPermission::class); } @@ -43,10 +45,8 @@ class Role extends Model /** * Check if this role has a permission. - * @param $permissionName - * @return bool */ - public function hasPermission($permissionName) + public function hasPermission(string $permissionName): bool { $permissions = $this->getRelationValue('permissions'); foreach ($permissions as $permission) { @@ -59,7 +59,6 @@ class Role extends Model /** * Add a permission to this role. - * @param RolePermission $permission */ public function attachPermission(RolePermission $permission) { @@ -68,7 +67,6 @@ class Role extends Model /** * Detach a single permission from this role. - * @param RolePermission $permission */ public function detachPermission(RolePermission $permission) { @@ -76,39 +74,33 @@ class Role extends Model } /** - * Get the role object for the specified role. - * @param $roleName - * @return Role + * Get the role of the specified display name. */ - public static function getRole($roleName) + public static function getRole(string $displayName): ?Role { - return static::query()->where('name', '=', $roleName)->first(); + return static::query()->where('display_name', '=', $displayName)->first(); } /** * Get the role object for the specified system role. - * @param $roleName - * @return Role */ - public static function getSystemRole($roleName) + public static function getSystemRole(string $systemName): ?Role { - return static::query()->where('system_name', '=', $roleName)->first(); + return static::query()->where('system_name', '=', $systemName)->first(); } /** * Get all visible roles - * @return mixed */ - public static function visible() + public static function visible(): Collection { return static::query()->where('hidden', '=', false)->orderBy('name')->get(); } /** * Get the roles that can be restricted. - * @return \Illuminate\Database\Eloquent\Builder[]|\Illuminate\Database\Eloquent\Collection */ - public static function restrictable() + public static function restrictable(): Collection { return static::query()->where('system_name', '!=', 'admin')->get(); } diff --git a/app/Auth/User.php b/app/Auth/User.php index 40718beb6..f65ef5316 100644 --- a/app/Auth/User.php +++ b/app/Auth/User.php @@ -101,12 +101,10 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon /** * Check if the user has a role. - * @param $role - * @return mixed */ - public function hasRole($role) + public function hasRole($roleId): bool { - return $this->roles->pluck('name')->contains($role); + return $this->roles->pluck('id')->contains($roleId); } /** @@ -163,7 +161,6 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon /** * Attach a role to this user. - * @param Role $role */ public function attachRole(Role $role) { diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php index cfa7bfce1..fdb8c0923 100644 --- a/app/Auth/UserRepo.php +++ b/app/Auth/UserRepo.php @@ -238,7 +238,7 @@ class UserRepo */ public function getAllRoles() { - return $this->role->newQuery()->orderBy('name', 'asc')->get(); + return $this->role->newQuery()->orderBy('display_name', 'asc')->get(); } /** diff --git a/app/Http/Controllers/PermissionController.php b/app/Http/Controllers/PermissionController.php index 148ae5cd6..1200d44ab 100644 --- a/app/Http/Controllers/PermissionController.php +++ b/app/Http/Controllers/PermissionController.php @@ -2,7 +2,9 @@ use BookStack\Auth\Permissions\PermissionsRepo; use BookStack\Exceptions\PermissionsException; +use Exception; use Illuminate\Http\Request; +use Illuminate\Validation\ValidationException; class PermissionController extends Controller { @@ -11,7 +13,6 @@ class PermissionController extends Controller /** * PermissionController constructor. - * @param \BookStack\Auth\Permissions\PermissionsRepo $permissionsRepo */ public function __construct(PermissionsRepo $permissionsRepo) { @@ -31,7 +32,6 @@ class PermissionController extends Controller /** * Show the form to create a new role - * @return \Illuminate\Contracts\View\Factory|\Illuminate\View\View */ public function createRole() { @@ -41,15 +41,13 @@ class PermissionController extends Controller /** * Store a new role in the system. - * @param Request $request - * @return \Illuminate\Http\RedirectResponse|\Illuminate\Routing\Redirector */ public function storeRole(Request $request) { $this->checkPermission('user-roles-manage'); $this->validate($request, [ - 'display_name' => 'required|min:3|max:200', - 'description' => 'max:250' + 'display_name' => 'required|min:3|max:180', + 'description' => 'max:180' ]); $this->permissionsRepo->saveNewRole($request->all()); @@ -59,11 +57,9 @@ class PermissionController extends Controller /** * Show the form for editing a user role. - * @param $id - * @return \Illuminate\Contracts\View\Factory|\Illuminate\View\View * @throws PermissionsException */ - public function editRole($id) + public function editRole(string $id) { $this->checkPermission('user-roles-manage'); $role = $this->permissionsRepo->getRoleById($id); @@ -75,18 +71,14 @@ class PermissionController extends Controller /** * Updates a user role. - * @param Request $request - * @param $id - * @return \Illuminate\Http\RedirectResponse|\Illuminate\Routing\Redirector - * @throws PermissionsException - * @throws \Illuminate\Validation\ValidationException + * @throws ValidationException */ - public function updateRole(Request $request, $id) + public function updateRole(Request $request, string $id) { $this->checkPermission('user-roles-manage'); $this->validate($request, [ - 'display_name' => 'required|min:3|max:200', - 'description' => 'max:250' + 'display_name' => 'required|min:3|max:180', + 'description' => 'max:180' ]); $this->permissionsRepo->updateRole($id, $request->all()); @@ -97,10 +89,8 @@ class PermissionController extends Controller /** * Show the view to delete a role. * Offers the chance to migrate users. - * @param $id - * @return \Illuminate\Contracts\View\Factory|\Illuminate\View\View */ - public function showDeleteRole($id) + public function showDeleteRole(string $id) { $this->checkPermission('user-roles-manage'); $role = $this->permissionsRepo->getRoleById($id); @@ -113,11 +103,9 @@ class PermissionController extends Controller /** * Delete a role from the system, * Migrate from a previous role if set. - * @param Request $request - * @param $id - * @return \Illuminate\Http\RedirectResponse|\Illuminate\Routing\Redirector + * @throws Exception */ - public function deleteRole(Request $request, $id) + public function deleteRole(Request $request, string $id) { $this->checkPermission('user-roles-manage'); diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 97ec31dcc..651dedc0d 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -66,8 +66,8 @@ class UserController extends Controller { $this->checkPermission('users-manage'); $validationRules = [ - 'name' => 'required', - 'email' => 'required|email|unique:users,email' + 'name' => 'required', + 'email' => 'required|email|unique:users,email' ]; $authMethod = config('auth.method'); diff --git a/database/migrations/2020_08_04_131052_remove_role_name_field.php b/database/migrations/2020_08_04_131052_remove_role_name_field.php new file mode 100644 index 000000000..f3cafb732 --- /dev/null +++ b/database/migrations/2020_08_04_131052_remove_role_name_field.php @@ -0,0 +1,37 @@ +dropColumn('name'); + }); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + Schema::table('roles', function (Blueprint $table) { + $table->string('name')->index(); + }); + + DB::table('roles')->update([ + "name" => DB::raw("lower(replace(`display_name`, ' ', '-'))"), + ]); + } +} diff --git a/resources/views/form/role-checkboxes.blade.php b/resources/views/form/role-checkboxes.blade.php index 580b332f3..fc6ad93a8 100644 --- a/resources/views/form/role-checkboxes.blade.php +++ b/resources/views/form/role-checkboxes.blade.php @@ -3,10 +3,10 @@ @foreach($roles as $role)
@include('components.custom-checkbox', [ - 'name' => $name . '[' . str_replace('.', 'DOT', $role->name) . ']', + 'name' => $name . '[' . strval($role->id) . ']', 'label' => $role->display_name, 'value' => $role->id, - 'checked' => old($name . '.' . str_replace('.', 'DOT', $role->name)) || (!old('name') && isset($model) && $model->hasRole($role->name)) + 'checked' => old($name . '.' . strval($role->id)) || (!old('name') && isset($model) && $model->hasRole($role->id)) ])
@endforeach diff --git a/resources/views/settings/index.blade.php b/resources/views/settings/index.blade.php index 0c8ff843a..b688d9646 100644 --- a/resources/views/settings/index.blade.php +++ b/resources/views/settings/index.blade.php @@ -231,7 +231,8 @@