Fixed failed permission checks due to non-loaded fields

Added additional exceptions to prevent such cases in the future, so
that they are caught in dev ideally.
Added test case specifically for reported favourite scenario.

diff --git a/app/Auth/Permissions/PermissionApplicator.php b/app/Auth/Permissions/PermissionApplicator.php
index d855a6170616f0fbf4974d240b04e88866f4046a..9a39f3e901a4520a9bf9b3dd6d03f0b0497c589d 100644 (file)
--- a/app/Auth/Permissions/PermissionApplicator.php
+++ b/app/Auth/Permissions/PermissionApplicator.php
@@ -34,7 +34,13 @@ class PermissionApplicator
         $ownRolePermission = $user->can($fullPermission . '-own');
         $nonJointPermissions = ['restrictions', 'image', 'attachment', 'comment'];
         $ownerField = ($ownable instanceof Entity) ? 'owned_by' : 'created_by';
-        $isOwner = $user->id === $ownable->getAttribute($ownerField);
+        $ownableFieldVal = $ownable->getAttribute($ownerField);
+
+        if (is_null($ownableFieldVal)) {
+            throw new InvalidArgumentException("{$ownerField} field used but has not been loaded");
+        }
+
+        $isOwner = $user->id === $ownableFieldVal;
         $hasRolePermission = $allRolePermission || ($isOwner && $ownRolePermission);
 
         // Handle non entity specific jointPermissions
@@ -68,6 +74,11 @@ class PermissionApplicator
         }
 
         foreach ($chain as $currentEntity) {
+
+            if (is_null($currentEntity->restricted)) {
+                throw new InvalidArgumentException("Entity restricted field used but has not been loaded");
+            }
+
             if ($currentEntity->restricted) {
                 return $currentEntity->permissions()
                     ->whereIn('role_id', $userRoleIds)

diff --git a/app/Entities/Repos/BaseRepo.php b/app/Entities/Repos/BaseRepo.php
index 9e1b41672128b92054b5fda8efaa93df8ed9d7cb..39b901383529effbb3b453d24a75336b24f9a666 100644 (file)
--- a/app/Entities/Repos/BaseRepo.php
+++ b/app/Entities/Repos/BaseRepo.php
@@ -38,6 +38,7 @@ class BaseRepo
             $this->tagRepo->saveTagsToEntity($entity, $input['tags']);
         }
 
+        $entity->refresh();
         $entity->rebuildPermissions();
         $entity->indexForSearch();
     }

diff --git a/app/Entities/Repos/BookshelfRepo.php b/app/Entities/Repos/BookshelfRepo.php
index b85289b97d24d73ef7c3c17a5c9ab179c2114c62..1f144b1a8d919683ad9c15790f49ac61968152bc 100644 (file)
--- a/app/Entities/Repos/BookshelfRepo.php
+++ b/app/Entities/Repos/BookshelfRepo.php
@@ -140,7 +140,7 @@ class BookshelfRepo
     public function copyDownPermissions(Bookshelf $shelf, $checkUserPermissions = true): int
     {
         $shelfPermissions = $shelf->permissions()->get(['role_id', 'action'])->toArray();
-        $shelfBooks = $shelf->books()->get(['id', 'restricted']);
+        $shelfBooks = $shelf->books()->get(['id', 'restricted', 'owned_by']);
         $updatedBookCount = 0;
 
         /** @var Book $book */

diff --git a/app/Entities/Tools/SearchRunner.php b/app/Entities/Tools/SearchRunner.php
index 78659b7864565d6d1f6edfeb0e843aecb0ba0678..22f0f66c7d43217a2c7f2811cb9ba3b210264e01 100644 (file)
--- a/app/Entities/Tools/SearchRunner.php
+++ b/app/Entities/Tools/SearchRunner.php
@@ -163,7 +163,7 @@ class SearchRunner
         $entityQuery = $entityModelInstance->newQuery()->scopes('visible');
 
         if ($entityModelInstance instanceof Page) {
-            $entityQuery->select($entityModelInstance::$listAttributes);
+            $entityQuery->select(array_merge($entityModelInstance::$listAttributes, ['restricted', 'owned_by']));
         } else {
             $entityQuery->select(['*']);
         }

diff --git a/app/Http/Controllers/FavouriteController.php b/app/Http/Controllers/FavouriteController.php
index b4cbdf5c2aa35f1eea2507a3e14fd8aedca9563e..f77b04843ec049f1a0cdf0ccd32413755cb08c08 100644 (file)
--- a/app/Http/Controllers/FavouriteController.php
+++ b/app/Http/Controllers/FavouriteController.php
@@ -87,7 +87,7 @@ class FavouriteController extends Controller
 
         $modelInstance = $model->newQuery()
             ->where('id', '=', $modelInfo['id'])
-            ->first(['id', 'name']);
+            ->first(['id', 'name', 'restricted', 'owned_by']);
 
         $inaccessibleEntity = ($modelInstance instanceof Entity && !userCan('view', $modelInstance));
         if (is_null($modelInstance) || $inaccessibleEntity) {

diff --git a/tests/FavouriteTest.php b/tests/FavouriteTest.php
index 017dd889fe202b7609253ee85188d5f4ac040e0e..032e46d0c043580838e4fb822fe230c5cd4bf75a 100644 (file)
--- a/tests/FavouriteTest.php
+++ b/tests/FavouriteTest.php
@@ -1,11 +1,11 @@
-<?php
+<?php namespace Tests;
 
 use BookStack\Actions\Favourite;
+use BookStack\Auth\User;
 use BookStack\Entities\Models\Book;
 use BookStack\Entities\Models\Bookshelf;
 use BookStack\Entities\Models\Chapter;
 use BookStack\Entities\Models\Page;
-use Tests\TestCase;
 
 class FavouriteTest extends TestCase
 {
@@ -58,6 +58,30 @@ class FavouriteTest extends TestCase
         ]);
     }
 
+    public function test_favourite_flow_with_own_permissions()
+    {
+        /** @var Book $book */
+        $book = Book::query()->first();
+        $user = User::factory()->create();
+        $book->owned_by = $user->id;
+        $book->save();
+
+        $this->giveUserPermissions($user, ['book-view-own']);
+
+        $this->actingAs($user)->get($book->getUrl());
+        $resp = $this->post('/favourites/add', [
+            'type' => get_class($book),
+            'id'   => $book->id,
+        ]);
+        $resp->assertRedirect($book->getUrl());
+
+        $this->assertDatabaseHas('favourites', [
+            'user_id'           => $user->id,
+            'favouritable_type' => $book->getMorphClass(),
+            'favouritable_id'   => $book->id,
+        ]);
+    }
+
     public function test_book_chapter_shelf_pages_contain_favourite_button()
     {
         $entities = [