From 23324018540624d7a6beafd0514f4b7dbe327431 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sat, 16 Jul 2022 20:55:32 +0100
Subject: [PATCH] Fixed a couple of non-intended logical permission issues

Both caught in tests:
Fixed loss of permissions for admin users when entity restrictions were
active, since there are no entity-restrictions for the admin role but
we'd force generate them in joint permissions, which would be queried.
Fixed new role permission checks when permissions given with only the
action (eg. 'view'), since the type prefix would be required for role
permission checks. Was previously not needed as only the simpler form
was used in the jointpermissions after merge & calculation.
---
 app/Auth/Permissions/PermissionApplicator.php | 30 +++++++++----------
 app/Entities/Models/Bookshelf.php             |  6 ----
 app/Entities/Tools/ShelfContext.php           |  1 +
 app/Http/Controllers/BookshelfController.php  |  2 +-
 4 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/app/Auth/Permissions/PermissionApplicator.php b/app/Auth/Permissions/PermissionApplicator.php
index 91a7c72ae..4b648532a 100644
--- a/app/Auth/Permissions/PermissionApplicator.php
+++ b/app/Auth/Permissions/PermissionApplicator.php
@@ -25,11 +25,13 @@ class PermissionApplicator
     {
         $explodedPermission = explode('-', $permission);
         $action = $explodedPermission[1] ?? $explodedPermission[0];
+        $fullPermission = count($explodedPermission) > 1 ? $permission : $ownable->getMorphClass() . '-' . $permission;
+
         $user = $this->currentUser();
         $userRoleIds = $this->getCurrentUserRoleIds();
 
-        $allRolePermission = $user->can($permission . '-all');
-        $ownRolePermission = $user->can($permission . '-own');
+        $allRolePermission = $user->can($fullPermission . '-all');
+        $ownRolePermission = $user->can($fullPermission . '-own');
         $nonJointPermissions = ['restrictions', 'image', 'attachment', 'comment'];
         $ownerField = ($ownable instanceof Entity) ? 'owned_by' : 'created_by';
         $isOwner = $user->id === $ownable->getAttribute($ownerField);
@@ -40,23 +42,22 @@ class PermissionApplicator
             return $hasRolePermission;
         }
 
-        $entityPermissions = $this->getApplicableEntityPermissions($ownable, $userRoleIds, $action);
-        if (is_null($entityPermissions)) {
-            return $hasRolePermission;
-        }
+        $hasApplicableEntityPermissions = $this->hasEntityPermission($ownable, $userRoleIds, $action);
 
-        return count($entityPermissions) > 0;
+        return is_null($hasApplicableEntityPermissions) ? $hasRolePermission : $hasApplicableEntityPermissions;
     }
 
     /**
-     * Get the permissions that are applicable for the given entity item.
-     * Returns null when no entity permissions apply otherwise entity permissions
-     * are active, even if the returned array is empty.
-     *
-     * @returns EntityPermission[]
+     * Check if there are permissions that are applicable for the given entity item, action and roles.
+     * Returns null when no entity permissions are in force.
      */
-    protected function getApplicableEntityPermissions(Entity $entity, array $userRoleIds, string $action): ?array
+    protected function hasEntityPermission(Entity $entity, array $userRoleIds, string $action): ?bool
     {
+        $adminRoleId = Role::getSystemRole('admin')->id;
+        if (in_array($adminRoleId, $userRoleIds)) {
+            return true;
+        }
+
         $chain = [$entity];
         if ($entity instanceof Page && $entity->chapter_id) {
             $chain[] = $entity->chapter;
@@ -71,8 +72,7 @@ class PermissionApplicator
                 return $currentEntity->permissions()
                     ->whereIn('role_id', $userRoleIds)
                     ->where('action', '=', $action)
-                    ->get()
-                    ->all();
+                    ->count() > 0;
             }
         }
 
diff --git a/app/Entities/Models/Bookshelf.php b/app/Entities/Models/Bookshelf.php
index b9ebab92e..b2dab252a 100644
--- a/app/Entities/Models/Bookshelf.php
+++ b/app/Entities/Models/Bookshelf.php
@@ -91,10 +91,6 @@ class Bookshelf extends Entity implements HasCoverImage
 
     /**
      * Check if this shelf contains the given book.
-     *
-     * @param Book $book
-     *
-     * @return bool
      */
     public function contains(Book $book): bool
     {
@@ -103,8 +99,6 @@ class Bookshelf extends Entity implements HasCoverImage
 
     /**
      * Add a book to the end of this shelf.
-     *
-     * @param Book $book
      */
     public function appendBook(Book $book)
     {
diff --git a/app/Entities/Tools/ShelfContext.php b/app/Entities/Tools/ShelfContext.php
index 50d798171..50c7047d9 100644
--- a/app/Entities/Tools/ShelfContext.php
+++ b/app/Entities/Tools/ShelfContext.php
@@ -20,6 +20,7 @@ class ShelfContext
             return null;
         }
 
+        /** @var Bookshelf $shelf */
         $shelf = Bookshelf::visible()->find($contextBookshelfId);
         $shelfContainsBook = $shelf && $shelf->contains($book);
 
diff --git a/app/Http/Controllers/BookshelfController.php b/app/Http/Controllers/BookshelfController.php
index 18adaa627..feb581c78 100644
--- a/app/Http/Controllers/BookshelfController.php
+++ b/app/Http/Controllers/BookshelfController.php
@@ -104,7 +104,7 @@ class BookshelfController extends Controller
     public function show(ActivityQueries $activities, string $slug)
     {
         $shelf = $this->bookshelfRepo->getBySlug($slug);
-        $this->checkOwnablePermission('book-view', $shelf);
+        $this->checkOwnablePermission('bookshelf-view', $shelf);
 
         $sort = setting()->getForCurrentUser('shelf_books_sort', 'default');
         $order = setting()->getForCurrentUser('shelf_books_sort_order', 'asc');