From 1d875ccfb7c62854c9c3253a0d83b39310fefbf8 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 16 Jul 2022 13:17:08 +0100 Subject: [PATCH] Continued removal of joint permission non-view queries Cleaned up PermissionApplicator to remove old cache system which was hardly ever actuall caching anything since it was reset after each public method run. Changed the scope of 'userCanOnAny' to just check entity permissions, and added protections of action scope creep, in case a role permission action was passed by mistake. --- app/Auth/Permissions/PermissionApplicator.php | 109 ++++++------------ app/Entities/Models/Entity.php | 11 +- app/Entities/Queries/Popular.php | 2 +- app/Entities/Queries/RecentlyViewed.php | 3 +- app/Entities/Queries/TopFavourites.php | 2 +- app/Http/Controllers/BookshelfController.php | 4 +- app/helpers.php | 8 +- resources/views/chapters/show.blade.php | 2 +- resources/views/pages/show.blade.php | 2 +- 9 files changed, 49 insertions(+), 94 deletions(-) diff --git a/app/Auth/Permissions/PermissionApplicator.php b/app/Auth/Permissions/PermissionApplicator.php index 40a7f6116..cf95f2854 100644 --- a/app/Auth/Permissions/PermissionApplicator.php +++ b/app/Auth/Permissions/PermissionApplicator.php @@ -11,37 +11,10 @@ use BookStack\Traits\HasCreatorAndUpdater; use BookStack\Traits\HasOwner; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Query\Builder as QueryBuilder; +use InvalidArgumentException; class PermissionApplicator { - /** - * @var ?array - */ - protected $userRoles = null; - - /** - * @var ?User - */ - protected $currentUserModel = null; - - /** - * Get the roles for the current logged in user. - */ - protected function getCurrentUserRoles(): array - { - if (!is_null($this->userRoles)) { - return $this->userRoles; - } - - if (auth()->guest()) { - $this->userRoles = [Role::getSystemRole('public')->id]; - } else { - $this->userRoles = $this->currentUser()->roles->pluck('id')->values()->all(); - } - - return $this->userRoles; - } - /** * Checks if an entity has a restriction set upon it. * @@ -74,7 +47,6 @@ class PermissionApplicator // TODO - Use a non-query based check $hasAccess = $this->entityRestrictionQuery($baseQuery, $action)->count() > 0; - $this->clean(); return $hasAccess; } @@ -83,25 +55,23 @@ class PermissionApplicator * Checks if a user has the given permission for any items in the system. * Can be passed an entity instance to filter on a specific type. */ - public function checkUserHasPermissionOnAnything(string $permission, ?string $entityClass = null): bool + public function checkUserHasEntityPermissionOnAny(string $action, string $entityClass = ''): bool { - $userRoleIds = $this->currentUser()->roles()->select('id')->pluck('id')->toArray(); - $userId = $this->currentUser()->id; + if (strpos($action, '-') !== false) { + throw new InvalidArgumentException("Action should be a simple entity permission action, not a role permission"); + } - $permissionQuery = JointPermission::query() - ->where('action', '=', $permission) - ->whereIn('role_id', $userRoleIds) - ->where(function (Builder $query) use ($userId) { - $this->addJointHasPermissionCheck($query, $userId); - }); + $permissionQuery = EntityPermission::query() + ->where('action', '=', $action) + ->whereIn('role_id', $this->getCurrentUserRoleIds()); - if (!is_null($entityClass)) { - $entityInstance = app($entityClass); - $permissionQuery = $permissionQuery->where('entity_type', '=', $entityInstance->getMorphClass()); + if (!empty($entityClass)) { + /** @var Entity $entityInstance */ + $entityInstance = app()->make($entityClass); + $permissionQuery = $permissionQuery->where('restrictable_type', '=', $entityInstance->getMorphClass()); } $hasPermission = $permissionQuery->count() > 0; - $this->clean(); return $hasPermission; } @@ -114,7 +84,8 @@ class PermissionApplicator { $q = $query->where(function ($parentQuery) use ($action) { $parentQuery->whereHas('jointPermissions', function ($permissionQuery) use ($action) { - $permissionQuery->whereIn('role_id', $this->getCurrentUserRoles()) + $permissionQuery->whereIn('role_id', $this->getCurrentUserRoleIds()) + // TODO - Delete line once only views ->where('action', '=', $action) ->where(function (Builder $query) { $this->addJointHasPermissionCheck($query, $this->currentUser()->id); @@ -122,23 +93,20 @@ class PermissionApplicator }); }); - $this->clean(); - return $q; } /** * Limited the given entity query so that the query will only - * return items that the user has permission for the given ability. + * return items that the user has view permission for. */ - public function restrictEntityQuery(Builder $query, string $ability = 'view'): Builder + public function restrictEntityQuery(Builder $query): Builder { - $this->clean(); - - return $query->where(function (Builder $parentQuery) use ($ability) { - $parentQuery->whereHas('jointPermissions', function (Builder $permissionQuery) use ($ability) { - $permissionQuery->whereIn('role_id', $this->getCurrentUserRoles()) - ->where('action', '=', $ability) + return $query->where(function (Builder $parentQuery) { + $parentQuery->whereHas('jointPermissions', function (Builder $permissionQuery) { + $permissionQuery->whereIn('role_id', $this->getCurrentUserRoleIds()) + // TODO - Delete line once only views + ->where('action', '=', 'view') ->where(function (Builder $query) { $this->addJointHasPermissionCheck($query, $this->currentUser()->id); }); @@ -181,18 +149,18 @@ class PermissionApplicator * * @param Builder|QueryBuilder $query */ - public function filterRestrictedEntityRelations($query, string $tableName, string $entityIdColumn, string $entityTypeColumn, string $action = 'view') + public function filterRestrictedEntityRelations($query, string $tableName, string $entityIdColumn, string $entityTypeColumn) { $tableDetails = ['tableName' => $tableName, 'entityIdColumn' => $entityIdColumn, 'entityTypeColumn' => $entityTypeColumn]; $pageMorphClass = (new Page())->getMorphClass(); - $q = $query->whereExists(function ($permissionQuery) use (&$tableDetails, $action) { + $q = $query->whereExists(function ($permissionQuery) use (&$tableDetails) { /** @var Builder $permissionQuery */ $permissionQuery->select(['role_id'])->from('joint_permissions') ->whereColumn('joint_permissions.entity_id', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityIdColumn']) ->whereColumn('joint_permissions.entity_type', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityTypeColumn']) - ->where('joint_permissions.action', '=', $action) - ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoles()) + ->where('joint_permissions.action', '=', 'view') + ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoleIds()) ->where(function (QueryBuilder $query) { $this->addJointHasPermissionCheck($query, $this->currentUser()->id); }); @@ -207,8 +175,6 @@ class PermissionApplicator }); }); - $this->clean(); - return $q; } @@ -228,7 +194,7 @@ class PermissionApplicator ->whereColumn('joint_permissions.entity_id', '=', $fullEntityIdColumn) ->where('joint_permissions.entity_type', '=', $morphClass) ->where('joint_permissions.action', '=', 'view') - ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoles()) + ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoleIds()) ->where(function (QueryBuilder $query) { $this->addJointHasPermissionCheck($query, $this->currentUser()->id); }); @@ -251,8 +217,6 @@ class PermissionApplicator }); } - $this->clean(); - return $q; } @@ -273,21 +237,22 @@ class PermissionApplicator /** * Get the current user. */ - private function currentUser(): User + protected function currentUser(): User { - if (is_null($this->currentUserModel)) { - $this->currentUserModel = user(); - } - - return $this->currentUserModel; + return user(); } /** - * Clean the cached user elements. + * Get the roles for the current logged-in user. + * + * @return int[] */ - private function clean(): void + protected function getCurrentUserRoleIds(): array { - $this->currentUserModel = null; - $this->userRoles = null; + if (auth()->guest()) { + return [Role::getSystemRole('public')->id]; + } + + return $this->currentUser()->roles->pluck('id')->values()->all(); } } diff --git a/app/Entities/Models/Entity.php b/app/Entities/Models/Entity.php index 84d31d82d..17f018a56 100644 --- a/app/Entities/Models/Entity.php +++ b/app/Entities/Models/Entity.php @@ -44,7 +44,6 @@ use Illuminate\Database\Eloquent\SoftDeletes; * @property Collection $tags * * @method static Entity|Builder visible() - * @method static Entity|Builder hasPermission(string $permission) * @method static Builder withLastView() * @method static Builder withViewCount() */ @@ -69,15 +68,7 @@ abstract class Entity extends Model implements Sluggable, Favouritable, Viewable */ public function scopeVisible(Builder $query): Builder { - return $this->scopeHasPermission($query, 'view'); - } - - /** - * Scope the query to those entities that the current user has the given permission for. - */ - public function scopeHasPermission(Builder $query, string $permission) - { - return app()->make(PermissionApplicator::class)->restrictEntityQuery($query, $permission); + return app()->make(PermissionApplicator::class)->restrictEntityQuery($query); } /** diff --git a/app/Entities/Queries/Popular.php b/app/Entities/Queries/Popular.php index 66006df1b..82786e3c6 100644 --- a/app/Entities/Queries/Popular.php +++ b/app/Entities/Queries/Popular.php @@ -10,7 +10,7 @@ class Popular extends EntityQuery public function run(int $count, int $page, array $filterModels = null) { $query = $this->permissionService() - ->filterRestrictedEntityRelations(View::query(), 'views', 'viewable_id', 'viewable_type', 'view') + ->filterRestrictedEntityRelations(View::query(), 'views', 'viewable_id', 'viewable_type') ->select('*', 'viewable_id', 'viewable_type', DB::raw('SUM(views) as view_count')) ->groupBy('viewable_id', 'viewable_type') ->orderBy('view_count', 'desc'); diff --git a/app/Entities/Queries/RecentlyViewed.php b/app/Entities/Queries/RecentlyViewed.php index 5a29ecd72..38d1f1be4 100644 --- a/app/Entities/Queries/RecentlyViewed.php +++ b/app/Entities/Queries/RecentlyViewed.php @@ -18,8 +18,7 @@ class RecentlyViewed extends EntityQuery View::query(), 'views', 'viewable_id', - 'viewable_type', - 'view' + 'viewable_type' ) ->orderBy('views.updated_at', 'desc') ->where('user_id', '=', user()->id); diff --git a/app/Entities/Queries/TopFavourites.php b/app/Entities/Queries/TopFavourites.php index 7522a894d..5d138679a 100644 --- a/app/Entities/Queries/TopFavourites.php +++ b/app/Entities/Queries/TopFavourites.php @@ -15,7 +15,7 @@ class TopFavourites extends EntityQuery } $query = $this->permissionService() - ->filterRestrictedEntityRelations(Favourite::query(), 'favourites', 'favouritable_id', 'favouritable_type', 'view') + ->filterRestrictedEntityRelations(Favourite::query(), 'favourites', 'favouritable_id', 'favouritable_type') ->select('favourites.*') ->leftJoin('views', function (JoinClause $join) { $join->on('favourites.favouritable_id', '=', 'views.viewable_id'); diff --git a/app/Http/Controllers/BookshelfController.php b/app/Http/Controllers/BookshelfController.php index a294bf731..18adaa627 100644 --- a/app/Http/Controllers/BookshelfController.php +++ b/app/Http/Controllers/BookshelfController.php @@ -68,7 +68,7 @@ class BookshelfController extends Controller public function create() { $this->checkPermission('bookshelf-create-all'); - $books = Book::hasPermission('update')->get(); + $books = Book::visible()->get(); $this->setPageTitle(trans('entities.shelves_create')); return view('shelves.create', ['books' => $books]); @@ -139,7 +139,7 @@ class BookshelfController extends Controller $this->checkOwnablePermission('bookshelf-update', $shelf); $shelfBookIds = $shelf->books()->get(['id'])->pluck('id'); - $books = Book::hasPermission('update')->whereNotIn('id', $shelfBookIds)->get(); + $books = Book::visible()->whereNotIn('id', $shelfBookIds)->get(); $this->setPageTitle(trans('entities.shelves_edit_named', ['name' => $shelf->getShortName()])); diff --git a/app/helpers.php b/app/helpers.php index cfdf55445..191eddf4d 100644 --- a/app/helpers.php +++ b/app/helpers.php @@ -71,14 +71,14 @@ function userCan(string $permission, Model $ownable = null): bool } /** - * Check if the current user has the given permission - * on any item in the system. + * Check if the current user can perform the given action on any items in the system. + * Can be provided the class name of an entity to filter ability to that specific entity type. */ -function userCanOnAny(string $permission, string $entityClass = null): bool +function userCanOnAny(string $action, string $entityClass = ''): bool { $permissions = app(PermissionApplicator::class); - return $permissions->checkUserHasPermissionOnAnything($permission, $entityClass); + return $permissions->checkUserHasEntityPermissionOnAny($action, $entityClass); } /** diff --git a/resources/views/chapters/show.blade.php b/resources/views/chapters/show.blade.php index 3e015616a..8a86900fb 100644 --- a/resources/views/chapters/show.blade.php +++ b/resources/views/chapters/show.blade.php @@ -120,7 +120,7 @@ {{ trans('common.edit') }} @endif - @if(userCanOnAny('chapter-create')) + @if(userCanOnAny('create', \BookStack\Entities\Models\Book::class) || userCan('chapter-create-all') || userCan('chapter-create-own')) @icon('copy') {{ trans('common.copy') }} diff --git a/resources/views/pages/show.blade.php b/resources/views/pages/show.blade.php index 2a71c6021..f1aed730b 100644 --- a/resources/views/pages/show.blade.php +++ b/resources/views/pages/show.blade.php @@ -148,7 +148,7 @@ {{ trans('common.edit') }} @endif - @if(userCanOnAny('page-create')) + @if(userCanOnAny('create', \BookStack\Entities\Models\Book::class) || userCanOnAny('create', \BookStack\Entities\Models\Chapter::class) || userCan('page-create-all') || userCan('page-create-own')) @icon('copy') {{ trans('common.copy') }}