Refactored permission system components

Split joint permission creation into chunks

Fixes #374

diff --git a/app/Entity.php b/app/Entity.php
index 6aeb66481dc436ca4654d8acd5c1ed3603d6665e..e5dd04bf28bdb01d30690c18e5b9a2b6986a7929 100644 (file)
--- a/app/Entity.php
+++ b/app/Entity.php
@@ -94,17 +94,6 @@ class Entity extends Ownable
             ->where('action', '=', $action)->count() > 0;
     }
 
-    /**
-     * Check if this entity has live (active) restrictions in place.
-     * @param $role_id
-     * @param $action
-     * @return bool
-     */
-    public function hasActiveRestriction($role_id, $action)
-    {
-        return $this->getRawAttribute('restricted') && $this->hasRestriction($role_id, $action);
-    }
-
     /**
      * Get the entity jointPermissions this is connected to.
      * @return \Illuminate\Database\Eloquent\Relations\MorphMany
@@ -176,5 +165,11 @@ class Entity extends Ownable
      */
     public function entityRawQuery(){return '';}
 
+    /**
+     * Get the url of this entity
+     * @param $path
+     * @return string
+     */
+    public function getUrl($path){return '/';}
 
 }

diff --git a/app/Http/Controllers/BookController.php b/app/Http/Controllers/BookController.php
index fe9ece5b252e53b30addf58a4d08a1689c873b65..8996ae64aec248d3dc61a2d0307a982d4b496e10 100644 (file)
--- a/app/Http/Controllers/BookController.php
+++ b/app/Http/Controllers/BookController.php
@@ -1,6 +1,7 @@
 <?php namespace BookStack\Http\Controllers;
 
 use Activity;
+use BookStack\Book;
 use BookStack\Repos\EntityRepo;
 use BookStack\Repos\UserRepo;
 use BookStack\Services\ExportService;
@@ -207,13 +208,12 @@ class BookController extends Controller
 
         // Add activity for books
         foreach ($sortedBooks as $bookId) {
+            /** @var Book $updatedBook */
             $updatedBook = $this->entityRepo->getById('book', $bookId);
+            $this->entityRepo->buildJointPermissionsForBook($updatedBook);
             Activity::add($updatedBook, 'book_sort', $updatedBook->id);
         }
 
-        // Update permissions on changed models
-        if (count($updatedModels) === 0) $this->entityRepo->buildJointPermissions($updatedModels);
-
         return redirect($book->getUrl());
     }
 

diff --git a/app/Repos/EntityRepo.php b/app/Repos/EntityRepo.php
index 9a572be547f1ea87eb626ef0d29c98269598f39d..f86c7d0a56b4f2e7faa4cfd41cc3ef53ba0e911b 100644 (file)
--- a/app/Repos/EntityRepo.php
+++ b/app/Repos/EntityRepo.php
@@ -533,11 +533,11 @@ class EntityRepo
 
     /**
      * Alias method to update the book jointPermissions in the PermissionService.
-     * @param Collection $collection collection on entities
+     * @param Book $book
      */
-    public function buildJointPermissions(Collection $collection)
+    public function buildJointPermissionsForBook(Book $book)
     {
-        $this->permissionService->buildJointPermissionsForEntities($collection);
+        $this->permissionService->buildJointPermissionsForEntity($book);
     }
 
     /**

diff --git a/app/Services/PermissionService.php b/app/Services/PermissionService.php
index 1e75308a07e3734ac6771cf2b78a5c5babe8f3d7..35e5640d63ccffce6c51aac1ac8a0b9392abed22 100644 (file)
--- a/app/Services/PermissionService.php
+++ b/app/Services/PermissionService.php
@@ -3,6 +3,7 @@
 use BookStack\Book;
 use BookStack\Chapter;
 use BookStack\Entity;
+use BookStack\EntityPermission;
 use BookStack\JointPermission;
 use BookStack\Ownable;
 use BookStack\Page;
@@ -11,6 +12,7 @@ use BookStack\User;
 use Illuminate\Database\Connection;
 use Illuminate\Database\Eloquent\Builder;
 use Illuminate\Support\Collection;
+use Illuminate\Support\Facades\Log;
 
 class PermissionService
 {
@@ -28,22 +30,25 @@ class PermissionService
 
     protected $jointPermission;
     protected $role;
+    protected $entityPermission;
 
     protected $entityCache;
 
     /**
      * PermissionService constructor.
      * @param JointPermission $jointPermission
+     * @param EntityPermission $entityPermission
      * @param Connection $db
      * @param Book $book
      * @param Chapter $chapter
      * @param Page $page
      * @param Role $role
      */
-    public function __construct(JointPermission $jointPermission, Connection $db, Book $book, Chapter $chapter, Page $page, Role $role)
+    public function __construct(JointPermission $jointPermission, EntityPermission $entityPermission, Connection $db, Book $book, Chapter $chapter, Page $page, Role $role)
     {
         $this->db = $db;
         $this->jointPermission = $jointPermission;
+        $this->entityPermission = $entityPermission;
         $this->role = $role;
         $this->book = $book;
         $this->chapter = $chapter;
@@ -136,19 +141,31 @@ class PermissionService
         $roles = $this->role->with('permissions')->get();
 
         // Chunk through all books
-        $this->book->with('permissions')->chunk(500, function ($books) use ($roles) {
-            $this->createManyJointPermissions($books, $roles);
+        $this->book->newQuery()->with('chapters', 'pages')->chunk(5, function ($books) use ($roles) {
+            $this->buildJointPermissionsForBooks($books, $roles);
         });
+    }
 
-        // Chunk through all chapters
-        $this->chapter->with('book', 'permissions')->chunk(500, function ($chapters) use ($roles) {
-            $this->createManyJointPermissions($chapters, $roles);
-        });
+    /**
+     * Build joint permissions for an array of books
+     * @param Collection $books
+     * @param Collection $roles
+     * @param bool $deleteOld
+     */
+    protected function buildJointPermissionsForBooks($books, $roles, $deleteOld = false) {
+        $entities = clone $books;
 
-        // Chunk through all pages
-        $this->page->with('book', 'chapter', 'permissions')->chunk(500, function ($pages) use ($roles) {
-            $this->createManyJointPermissions($pages, $roles);
-        });
+        foreach ($books as $book) {
+            foreach ($book->chapters as $chapter) {
+                $entities->push($chapter);
+            }
+            foreach ($book->pages as $page) {
+                $entities->push($page);
+            }
+        }
+
+        if ($deleteOld) $this->deleteManyJointPermissionsForEntities($entities->all());
+        $this->createManyJointPermissions($entities, $roles);
     }
 
     /**
@@ -157,18 +174,9 @@ class PermissionService
      */
     public function buildJointPermissionsForEntity(Entity $entity)
     {
-        $roles = $this->role->get();
-        $entities = collect([$entity]);
-
-        if ($entity->isA('book')) {
-            $entities = $entities->merge($entity->chapters);
-            $entities = $entities->merge($entity->pages);
-        } elseif ($entity->isA('chapter')) {
-            $entities = $entities->merge($entity->pages);
-        }
-
-        $this->deleteManyJointPermissionsForEntities($entities);
-        $this->createManyJointPermissions($entities, $roles);
+        $roles = $this->role->newQuery()->get();
+        $book = ($entity->isA('book')) ? $entity : $entity->book;
+        $this->buildJointPermissionsForBooks(collect([$book]), $roles, true);
     }
 
     /**
@@ -178,7 +186,7 @@ class PermissionService
     public function buildJointPermissionsForEntities(Collection $entities)
     {
         $roles = $this->role->get();
-        $this->deleteManyJointPermissionsForEntities($entities);
+        $this->deleteManyJointPermissionsForEntities($entities->all());
         $this->createManyJointPermissions($entities, $roles);
     }
 
@@ -193,18 +201,8 @@ class PermissionService
         $this->deleteManyJointPermissionsForRoles($roles);
 
         // Chunk through all books
-        $this->book->with('permissions')->chunk(500, function ($books) use ($roles) {
-            $this->createManyJointPermissions($books, $roles);
-        });
-
-        // Chunk through all chapters
-        $this->chapter->with('book', 'permissions')->chunk(500, function ($books) use ($roles) {
-            $this->createManyJointPermissions($books, $roles);
-        });
-
-        // Chunk through all pages
-        $this->page->with('book', 'chapter', 'permissions')->chunk(500, function ($books) use ($roles) {
-            $this->createManyJointPermissions($books, $roles);
+        $this->book->with('chapters', 'pages')->chunk(5, function ($books) use ($roles) {
+            $this->buildJointPermissionsForBooks($books, $roles);
         });
     }
 
@@ -245,12 +243,14 @@ class PermissionService
     {
         if (count($entities) === 0) return;
         $query = $this->jointPermission->newQuery();
-            foreach ($entities as $entity) {
-                $query->orWhere(function($query) use ($entity) {
-                    $query->where('entity_id', '=', $entity->id)
-                        ->where('entity_type', '=', $entity->getMorphClass());
-                });
-            }
+
+        foreach ($entities as $entity) {
+            $query->orWhere(function($query) use ($entity) {
+                $query->where('entity_id', '=', $entity->id)
+                    ->where('entity_type', '=', $entity->getMorphClass());
+            });
+        }
+
         $query->delete();
     }
 
@@ -263,34 +263,59 @@ class PermissionService
     {
         $this->readyEntityCache();
         $jointPermissions = [];
+
+        // Fetch Entity Permissions and create a mapping of entity restricted statuses
+        $entityRestrictedMap = [];
+        $permissionFetch = $this->entityPermission->newQuery();
+        foreach ($entities as $entity) {
+            $entityRestrictedMap[$entity->getMorphClass() . ':' . $entity->id] = boolval($entity->getRawAttribute('restricted'));
+            $permissionFetch->orWhere(function($query) use ($entity) {
+                $query->where('restrictable_id', '=', $entity->id)->where('restrictable_type', '=', $entity->getMorphClass());
+            });
+        }
+        $permissions = $permissionFetch->get();
+
+        // Create a mapping of explicit entity permissions
+        $permissionMap = [];
+        foreach ($permissions as $permission) {
+            $key = $permission->restrictable_type . ':' . $permission->restrictable_id . ':' . $permission->role_id . ':' . $permission->action;
+            $isRestricted = $entityRestrictedMap[$permission->restrictable_type . ':' . $permission->restrictable_id];
+            $permissionMap[$key] = $isRestricted;
+        }
+
+        // Create a mapping of role permissions
+        $rolePermissionMap = [];
+        foreach ($roles as $role) {
+            foreach ($role->getRelationValue('permissions') as $permission) {
+                $rolePermissionMap[$role->getRawAttribute('id') . ':' . $permission->getRawAttribute('name')] = true;
+            }
+        }
+
+        // Create Joint Permission Data
         foreach ($entities as $entity) {
             foreach ($roles as $role) {
                 foreach ($this->getActions($entity) as $action) {
-                    $jointPermissions[] = $this->createJointPermissionData($entity, $role, $action);
+                    $jointPermissions[] = $this->createJointPermissionData($entity, $role, $action, $permissionMap, $rolePermissionMap);
                 }
             }
         }
-        $this->jointPermission->insert($jointPermissions);
+        foreach (array_chunk($jointPermissions, 5000) as $jointPermissionChunk) {
+            $this->jointPermission->insert($jointPermissionChunk);
+        }
     }
 
 
     /**
      * Get the actions related to an entity.
-     * @param $entity
+     * @param Entity $entity
      * @return array
      */
-    protected function getActions($entity)
+    protected function getActions(Entity $entity)
     {
         $baseActions = ['view', 'update', 'delete'];
-
-        if ($entity->isA('chapter')) {
-            $baseActions[] = 'page-create';
-        } else if ($entity->isA('book')) {
-            $baseActions[] = 'page-create';
-            $baseActions[] = 'chapter-create';
-        }
-
-         return $baseActions;
+        if ($entity->isA('chapter') || $entity->isA('book')) $baseActions[] = 'page-create';
+        if ($entity->isA('book')) $baseActions[] = 'chapter-create';
+        return $baseActions;
     }
 
     /**
@@ -298,14 +323,16 @@ class PermissionService
      * for a particular action.
      * @param Entity $entity
      * @param Role $role
-     * @param $action
+     * @param string $action
+     * @param array $permissionMap
+     * @param array $rolePermissionMap
      * @return array
      */
-    protected function createJointPermissionData(Entity $entity, Role $role, $action)
+    protected function createJointPermissionData(Entity $entity, Role $role, $action, $permissionMap, $rolePermissionMap)
     {
         $permissionPrefix = (strpos($action, '-') === false ? ($entity->getType() . '-') : '') . $action;
-        $roleHasPermission = $role->hasPermission($permissionPrefix . '-all');
-        $roleHasPermissionOwn = $role->hasPermission($permissionPrefix . '-own');
+        $roleHasPermission = isset($rolePermissionMap[$role->getRawAttribute('id') . ':' . $permissionPrefix . '-all']);
+        $roleHasPermissionOwn = isset($rolePermissionMap[$role->getRawAttribute('id') . ':' . $permissionPrefix . '-own']);
         $explodedAction = explode('-', $action);
         $restrictionAction = end($explodedAction);
 
@@ -313,54 +340,46 @@ class PermissionService
             return $this->createJointPermissionDataArray($entity, $role, $action, true, true);
         }
 
-        if ($entity->isA('book')) {
-
-            if (!$entity->restricted) {
-                return $this->createJointPermissionDataArray($entity, $role, $action, $roleHasPermission, $roleHasPermissionOwn);
-            } else {
-                $hasAccess = $entity->hasActiveRestriction($role->id, $restrictionAction);
-                return $this->createJointPermissionDataArray($entity, $role, $action, $hasAccess, $hasAccess);
-            }
+        if ($entity->restricted) {
+            $hasAccess = $this->mapHasActiveRestriction($permissionMap, $entity, $role, $restrictionAction);
+            return $this->createJointPermissionDataArray($entity, $role, $action, $hasAccess, $hasAccess);
+        }
 
-        } elseif ($entity->isA('chapter')) {
+        if ($entity->isA('book')) {
+            return $this->createJointPermissionDataArray($entity, $role, $action, $roleHasPermission, $roleHasPermissionOwn);
+        }
 
-            if (!$entity->restricted) {
-                $book = $this->getBook($entity->book_id);
-                $hasExplicitAccessToBook = $book->hasActiveRestriction($role->id, $restrictionAction);
-                $hasPermissiveAccessToBook = !$book->restricted;
-                return $this->createJointPermissionDataArray($entity, $role, $action,
-                    ($hasExplicitAccessToBook || ($roleHasPermission && $hasPermissiveAccessToBook)),
-                    ($hasExplicitAccessToBook || ($roleHasPermissionOwn && $hasPermissiveAccessToBook)));
-            } else {
-                $hasAccess = $entity->hasActiveRestriction($role->id, $restrictionAction);
-                return $this->createJointPermissionDataArray($entity, $role, $action, $hasAccess, $hasAccess);
+        // For chapters and pages, Check if explicit permissions are set on the Book.
+        $book = $this->getBook($entity->book_id);
+        $hasExplicitAccessToParents = $this->mapHasActiveRestriction($permissionMap, $book, $role, $restrictionAction);
+        $hasPermissiveAccessToParents = !$book->restricted;
+
+        // For pages with a chapter, Check if explicit permissions are set on the Chapter
+        if ($entity->isA('page') && $entity->chapter_id !== 0) {
+            $chapter = $this->getChapter($entity->chapter_id);
+            $hasPermissiveAccessToParents = $hasPermissiveAccessToParents && !$chapter->restricted;
+            if ($chapter->restricted) {
+                $hasExplicitAccessToParents = $this->mapHasActiveRestriction($permissionMap, $chapter, $role, $restrictionAction);
             }
+        }
 
-        } elseif ($entity->isA('page')) {
-
-            if (!$entity->restricted) {
-                $book = $this->getBook($entity->book_id);
-                $hasExplicitAccessToBook = $book->hasActiveRestriction($role->id, $restrictionAction);
-                $hasPermissiveAccessToBook = !$book->restricted;
-
-                $chapter = $this->getChapter($entity->chapter_id);
-                $hasExplicitAccessToChapter = $chapter && $chapter->hasActiveRestriction($role->id, $restrictionAction);
-                $hasPermissiveAccessToChapter = $chapter && !$chapter->restricted;
-                $acknowledgeChapter = ($chapter && $chapter->restricted);
-
-                $hasExplicitAccessToParents = $acknowledgeChapter ? $hasExplicitAccessToChapter : $hasExplicitAccessToBook;
-                $hasPermissiveAccessToParents = $acknowledgeChapter ? $hasPermissiveAccessToChapter : $hasPermissiveAccessToBook;
-
-                return $this->createJointPermissionDataArray($entity, $role, $action,
-                    ($hasExplicitAccessToParents || ($roleHasPermission && $hasPermissiveAccessToParents)),
-                    ($hasExplicitAccessToParents || ($roleHasPermissionOwn && $hasPermissiveAccessToParents))
-                );
-            } else {
-                $hasAccess = $entity->hasRestriction($role->id, $action);
-                return $this->createJointPermissionDataArray($entity, $role, $action, $hasAccess, $hasAccess);
-            }
+        return $this->createJointPermissionDataArray($entity, $role, $action,
+            ($hasExplicitAccessToParents || ($roleHasPermission && $hasPermissiveAccessToParents)),
+            ($hasExplicitAccessToParents || ($roleHasPermissionOwn && $hasPermissiveAccessToParents))
+        );
+    }
 
-        }
+    /**
+     * Check for an active restriction in an entity map.
+     * @param $entityMap
+     * @param Entity $entity
+     * @param Role $role
+     * @param $action
+     * @return bool
+     */
+    protected function mapHasActiveRestriction($entityMap, Entity $entity, Role $role, $action) {
+        $key = $entity->getMorphClass() . ':' . $entity->getRawAttribute('id') . ':' . $role->getRawAttribute('id') . ':' . $action;
+        return isset($entityMap[$key]) ? $entityMap[$key] : false;
     }
 
     /**
@@ -375,11 +394,10 @@ class PermissionService
      */
     protected function createJointPermissionDataArray(Entity $entity, Role $role, $action, $permissionAll, $permissionOwn)
     {
-        $entityClass = get_class($entity);
         return [
             'role_id'            => $role->getRawAttribute('id'),
             'entity_id'          => $entity->getRawAttribute('id'),
-            'entity_type'        => $entityClass,
+            'entity_type'        => $entity->getMorphClass(),
             'action'             => $action,
             'has_permission'     => $permissionAll,
             'has_permission_own' => $permissionOwn,

diff --git a/database/seeds/DummyContentSeeder.php b/database/seeds/DummyContentSeeder.php
index 6f6b3ddc57e7b826a1417af290c84274df895ec5..3d92efab1674626df417be98d90de67c379e86d9 100644 (file)
--- a/database/seeds/DummyContentSeeder.php
+++ b/database/seeds/DummyContentSeeder.php
@@ -28,6 +28,12 @@ class DummyContentSeeder extends Seeder
                 $book->pages()->saveMany($pages);
             });
 
+        $largeBook = factory(\BookStack\Book::class)->create(['name' => 'Large book' . str_random(10), 'created_by' => $user->id, 'updated_by' => $user->id]);
+        $pages = factory(\BookStack\Page::class, 200)->make(['created_by' => $user->id, 'updated_by' => $user->id]);
+        $chapters = factory(\BookStack\Chapter::class, 50)->make(['created_by' => $user->id, 'updated_by' => $user->id]);
+        $largeBook->pages()->saveMany($pages);
+        $largeBook->chapters()->saveMany($chapters);
+
         app(\BookStack\Services\PermissionService::class)->buildJointPermissions();
         app(\BookStack\Services\SearchService::class)->indexAllEntities();
     }

diff --git a/tests/Entity/EntitySearchTest.php b/tests/Entity/EntitySearchTest.php
index 9f77972c4cd1cc8ec6796e6d817b73ac5864d483..94e28e94450db50de762fdde2e62383150eb5767 100644 (file)
--- a/tests/Entity/EntitySearchTest.php
+++ b/tests/Entity/EntitySearchTest.php
@@ -42,7 +42,7 @@ class EntitySearchTest extends TestCase
 
     public function test_book_search()
     {
-        $book = \BookStack\Book::all()->first();
+        $book = \BookStack\Book::first();
         $page = $book->pages->last();
         $chapter = $book->chapters->last();
 

diff --git a/tests/Entity/TagTest.php b/tests/Entity/TagTest.php
index 257c207891e0befcc92cedd00db213faecc47d58..1ef7b7bde0fcfcf1556b8d82bd60b647efde5772 100644 (file)
--- a/tests/Entity/TagTest.php
+++ b/tests/Entity/TagTest.php
@@ -1,5 +1,6 @@
 <?php namespace Tests;
 
+use BookStack\Role;
 use BookStack\Tag;
 use BookStack\Page;
 use BookStack\Services\PermissionService;

diff --git a/tests/Permissions/RestrictionsTest.php b/tests/Permissions/RestrictionsTest.php
index 58be1ea73e0488411b12471febc9dfd6f56da9c6..faceab92c7322dd7c00f662e70673a2ceb8eae9a 100644 (file)
--- a/tests/Permissions/RestrictionsTest.php
+++ b/tests/Permissions/RestrictionsTest.php
@@ -226,6 +226,7 @@ class RestrictionsTest extends BrowserKitTest
             ->type('test content', 'html')
             ->press('Save Page')
             ->seePageIs($chapter->book->getUrl() . '/page/test-page');
+
         $this->visit($chapterUrl)->seeInElement('.action-buttons', 'New Page');
     }