webapp/bookstack
1
0
Fork 0
mirror of https://github.com/BookStackApp/BookStack.git synced 2025-07-30 04:23:11 +03:00
Code Activity

Removed role 'name' field from database

Browse Source 
The 'name' field was really redundant and caused confusion in the
codebase, since the 'Display' name is often used and we have a
'system_name' for the admin and public role.

This fixes #2032, Where external auth group matching has confusing
behaviour as matching was done against the display_name, if no
external_auth field is set, but only roles with a match 'name' field
would be considered.

This also fixes and error where the role users migration, on role
delete, would not actually fire due to mis-matching http body keys.
Looks like this has been an issue from the start. Added some testing to
cover. Fixes #2211.

Also converted phpdoc to typehints in many areas of the reviewed code
during the above.
This commit is contained in:
Dan Brown
2020-08-04 14:55:01 +01:00
parent a9f02550f0
commit 5f1ee5fb0e
15 changed files with 142 additions and 122 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
app/Auth/Access/ExternalAuthService.php
View File

@ -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);
});

61
app/Auth/Permissions/PermissionsRepo.php
View File

@ -1,8 +1,9 @@
<?php namespace BookStack\Auth\Permissions;
use BookStack\Auth\Permissions;
use BookStack\Auth\Role;
use BookStack\Exceptions\PermissionsException;
use Exception;
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Support\Str;
class PermissionsRepo
@ -16,11 +17,8 @@ class PermissionsRepo
/**
* PermissionsRepo constructor.
* @param RolePermission $permission
* @param Role $role
* @param \BookStack\Auth\Permissions\PermissionService $permissionService
*/
public function __construct(RolePermission $permission, Role $role, Permissions\PermissionService $permissionService)
public function __construct(RolePermission $permission, Role $role, PermissionService $permissionService)
{
$this->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);
}
}

3
app/Auth/Permissions/RolePermission.php
View File

@ -3,6 +3,9 @@
use BookStack\Auth\Role;
use BookStack\Model;
/**
* @property int $id
*/
class RolePermission extends Model
{
/**

34
app/Auth/Role.php
View File

@ -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();
}

7
app/Auth/User.php
View File

@ -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)
{

2
app/Auth/UserRepo.php
View File

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

36
app/Http/Controllers/PermissionController.php
View File

@ -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');

4
app/Http/Controllers/UserController.php
View File

@ -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');