]> BookStack Code Mirror - bookstack/commitdiff
Continued removal of joint permission non-view queries
authorDan Brown <redacted>
Sat, 16 Jul 2022 12:17:08 +0000 (13:17 +0100)
committerDan Brown <redacted>
Sat, 16 Jul 2022 12:17:08 +0000 (13:17 +0100)
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
app/Entities/Models/Entity.php
app/Entities/Queries/Popular.php
app/Entities/Queries/RecentlyViewed.php
app/Entities/Queries/TopFavourites.php
app/Http/Controllers/BookshelfController.php
app/helpers.php
resources/views/chapters/show.blade.php
resources/views/pages/show.blade.php

index 40a7f61162336937192871ae23bb336cfbaca9e1..cf95f28544967ac3841abebd053c951c9cbf6e6d 100644 (file)
@@ -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<int>
-     */
-    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;
-
-        $permissionQuery = JointPermission::query()
-            ->where('action', '=', $permission)
-            ->whereIn('role_id', $userRoleIds)
-            ->where(function (Builder $query) use ($userId) {
-                $this->addJointHasPermissionCheck($query, $userId);
-            });
+        if (strpos($action, '-') !== false) {
+            throw new InvalidArgumentException("Action should be a simple entity permission action, not a role permission");
+        }
+
+        $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();
     }
 }
index 84d31d82dcde318326d71dd1b920b1edfc41964c..17f018a566a01f76db1e39d64557b71cf2671c1e 100644 (file)
@@ -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);
     }
 
     /**
index 66006df1be2fbda84fb556eb3c62c4a647888dfd..82786e3c6c7611526d69d19fdc543fd28875305f 100644 (file)
@@ -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');
index 5a29ecd7240c865c66738334e9c8f9dbff2fea7e..38d1f1be46fe2b18dbeee1ac6b2c4b69a67de52b 100644 (file)
@@ -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);
index 7522a894daa71b95bf6a85f806cbf52be6913a92..5d138679a66e5f90fb7a89adba0e4e3bff480631 100644 (file)
@@ -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');
index a294bf7318c2a35a44d42ff3772b15f900a5123d..18adaa62756d55d9ec904078d9680fd098267913 100644 (file)
@@ -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()]));
 
index cfdf554457b73923330043061f1ff307f17c0c06..191eddf4d0596e33c5f5ba7af0a783620a2bda5d 100644 (file)
@@ -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);
 }
 
 /**
index 3e015616ad34384e1cf0f633543f4785e4d86844..8a86900fb1b744e81f1ed0b87a4aabf1e24f7a68 100644 (file)
                     <span>{{ trans('common.edit') }}</span>
                 </a>
             @endif
-            @if(userCanOnAny('chapter-create'))
+            @if(userCanOnAny('create', \BookStack\Entities\Models\Book::class) || userCan('chapter-create-all') || userCan('chapter-create-own'))
                 <a href="{{ $chapter->getUrl('/copy') }}" class="icon-list-item">
                     <span>@icon('copy')</span>
                     <span>{{ trans('common.copy') }}</span>
index 2a71c60214183698826027c2ecc0797cc1444e34..f1aed730b24b6bc9cb063fd272022db77a94108b 100644 (file)
                     <span>{{ trans('common.edit') }}</span>
                 </a>
             @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'))
                 <a href="{{ $page->getUrl('/copy') }}" class="icon-list-item">
                     <span>@icon('copy')</span>
                     <span>{{ trans('common.copy') }}</span>