From 5570e858e580a66d798ff7aba18b4cc58e8ca13e Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 30 Apr 2017 11:38:58 +0100 Subject: [PATCH] Made more efficiency improvements to permission system --- .../Commands/RegeneratePermissions.php | 1 + app/Console/Commands/RegenerateSearch.php | 1 + app/Services/PermissionService.php | 85 +++++++++++++------ app/Services/SearchService.php | 9 ++ tests/BrowserKitTest.php | 9 +- tests/Permissions/RolesTest.php | 38 +++++---- 6 files changed, 93 insertions(+), 50 deletions(-) diff --git a/app/Console/Commands/RegeneratePermissions.php b/app/Console/Commands/RegeneratePermissions.php index 1dc25f9aa..9cd577a17 100644 --- a/app/Console/Commands/RegeneratePermissions.php +++ b/app/Console/Commands/RegeneratePermissions.php @@ -49,6 +49,7 @@ class RegeneratePermissions extends Command $connection = \DB::getDefaultConnection(); if ($this->option('database') !== null) { \DB::setDefaultConnection($this->option('database')); + $this->permissionService->setConnection(\DB::connection($this->option('database'))); } $this->permissionService->buildJointPermissions(); diff --git a/app/Console/Commands/RegenerateSearch.php b/app/Console/Commands/RegenerateSearch.php index 35ecd46c0..1757911a7 100644 --- a/app/Console/Commands/RegenerateSearch.php +++ b/app/Console/Commands/RegenerateSearch.php @@ -44,6 +44,7 @@ class RegenerateSearch extends Command $connection = \DB::getDefaultConnection(); if ($this->option('database') !== null) { \DB::setDefaultConnection($this->option('database')); + $this->searchService->setConnection(\DB::connection($this->option('database'))); } $this->searchService->indexAllEntities(); diff --git a/app/Services/PermissionService.php b/app/Services/PermissionService.php index 35e5640d6..83905ec44 100644 --- a/app/Services/PermissionService.php +++ b/app/Services/PermissionService.php @@ -11,8 +11,8 @@ use BookStack\Role; use BookStack\User; use Illuminate\Database\Connection; use Illuminate\Database\Eloquent\Builder; +use Illuminate\Database\Query\Builder as QueryBuilder; use Illuminate\Support\Collection; -use Illuminate\Support\Facades\Log; class PermissionService { @@ -56,6 +56,15 @@ class PermissionService // TODO - Update so admin still goes through filters } + /** + * Set the database connection + * @param Connection $connection + */ + public function setConnection(Connection $connection) + { + $this->db = $connection; + } + /** * Prepare the local entity cache and ensure it's empty */ @@ -138,10 +147,14 @@ class PermissionService $this->readyEntityCache(); // Get all roles (Should be the most limited dimension) - $roles = $this->role->with('permissions')->get(); + $roles = $this->role->with('permissions')->get()->all(); // Chunk through all books - $this->book->newQuery()->with('chapters', 'pages')->chunk(5, function ($books) use ($roles) { + $this->book->newQuery()->select(['id', 'restricted', 'created_by'])->with(['chapters' => function($query) { + $query->select(['id', 'restricted', 'created_by', 'book_id']); + }, 'pages' => function($query) { + $query->select(['id', 'restricted', 'created_by', 'book_id', 'chapter_id']); + }])->chunk(5, function ($books) use ($roles) { $this->buildJointPermissionsForBooks($books, $roles); }); } @@ -149,17 +162,18 @@ class PermissionService /** * Build joint permissions for an array of books * @param Collection $books - * @param Collection $roles + * @param array $roles * @param bool $deleteOld */ protected function buildJointPermissionsForBooks($books, $roles, $deleteOld = false) { $entities = clone $books; - foreach ($books as $book) { - foreach ($book->chapters as $chapter) { + /** @var Book $book */ + foreach ($books->all() as $book) { + foreach ($book->getRelation('chapters') as $chapter) { $entities->push($chapter); } - foreach ($book->pages as $page) { + foreach ($book->getRelation('pages') as $page) { $entities->push($page); } } @@ -176,7 +190,12 @@ class PermissionService { $roles = $this->role->newQuery()->get(); $book = ($entity->isA('book')) ? $entity : $entity->book; - $this->buildJointPermissionsForBooks(collect([$book]), $roles, true); + $book = $this->book->newQuery()->select(['id', 'restricted', 'created_by'])->with(['chapters' => function($query) { + $query->select(['id', 'restricted', 'created_by', 'book_id']); + }, 'pages' => function($query) { + $query->select(['id', 'restricted', 'created_by', 'book_id', 'chapter_id']); + }])->where('id', '=', $book->id)->get(); + $this->buildJointPermissionsForBooks($book, $roles, true); } /** @@ -196,12 +215,15 @@ class PermissionService */ public function buildJointPermissionForRole(Role $role) { - $roles = collect([$role]); - + $roles = [$role]; $this->deleteManyJointPermissionsForRoles($roles); // Chunk through all books - $this->book->with('chapters', 'pages')->chunk(5, function ($books) use ($roles) { + $this->book->newQuery()->select(['id', 'restricted', 'created_by'])->with(['chapters' => function($query) { + $query->select(['id', 'restricted', 'created_by', 'book_id']); + }, 'pages' => function($query) { + $query->select(['id', 'restricted', 'created_by', 'book_id', 'chapter_id']); + }])->chunk(5, function ($books) use ($roles) { $this->buildJointPermissionsForBooks($books, $roles); }); } @@ -221,9 +243,10 @@ class PermissionService */ protected function deleteManyJointPermissionsForRoles($roles) { - foreach ($roles as $role) { - $role->jointPermissions()->delete(); - } + $roleIds = array_map(function($role) { + return $role->id; + }, $roles); + $this->jointPermission->newQuery()->whereIn('id', $roleIds)->delete(); } /** @@ -242,22 +265,27 @@ class PermissionService protected function deleteManyJointPermissionsForEntities($entities) { 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()); - }); - } + $this->db->transaction(function() use ($entities) { - $query->delete(); + foreach (array_chunk($entities, 1000) as $entityChunk) { + $query = $this->db->table('joint_permissions'); + foreach ($entityChunk as $entity) { + $query->orWhere(function(QueryBuilder $query) use ($entity) { + $query->where('entity_id', '=', $entity->id) + ->where('entity_type', '=', $entity->getMorphClass()); + }); + } + $query->delete(); + } + + }); } /** * Create & Save entity jointPermissions for many entities and jointPermissions. * @param Collection $entities - * @param Collection $roles + * @param array $roles */ protected function createManyJointPermissions($entities, $roles) { @@ -299,9 +327,12 @@ class PermissionService } } } - foreach (array_chunk($jointPermissions, 5000) as $jointPermissionChunk) { - $this->jointPermission->insert($jointPermissionChunk); - } + + $this->db->transaction(function() use ($jointPermissions) { + foreach (array_chunk($jointPermissions, 1000) as $jointPermissionChunk) { + $this->db->table('joint_permissions')->insert($jointPermissionChunk); + } + }); } @@ -494,7 +525,7 @@ class PermissionService * @param integer $book_id * @param bool $filterDrafts * @param bool $fetchPageContent - * @return \Illuminate\Database\Query\Builder + * @return QueryBuilder */ public function bookChildrenQuery($book_id, $filterDrafts = false, $fetchPageContent = false) { $pageSelect = $this->db->table('pages')->selectRaw($this->page->entityRawQuery($fetchPageContent))->where('book_id', '=', $book_id)->where(function($query) use ($filterDrafts) { diff --git a/app/Services/SearchService.php b/app/Services/SearchService.php index 670c1545d..3d1d45c3b 100644 --- a/app/Services/SearchService.php +++ b/app/Services/SearchService.php @@ -50,6 +50,15 @@ class SearchService $this->permissionService = $permissionService; } + /** + * Set the database connection + * @param Connection $connection + */ + public function setConnection(Connection $connection) + { + $this->db = $connection; + } + /** * Search all entities in the system. * @param string $searchString diff --git a/tests/BrowserKitTest.php b/tests/BrowserKitTest.php index f8d60239d..c665bfc23 100644 --- a/tests/BrowserKitTest.php +++ b/tests/BrowserKitTest.php @@ -1,6 +1,7 @@ create(['created_by' => $creatorUser->id, 'updated_by' => $updaterUser->id]); - $chapter = factory(\BookStack\Chapter::class)->create(['created_by' => $creatorUser->id, 'updated_by' => $updaterUser->id]); - $page = factory(\BookStack\Page::class)->create(['created_by' => $creatorUser->id, 'updated_by' => $updaterUser->id, 'book_id' => $book->id]); - $book->chapters()->saveMany([$chapter]); - $chapter->pages()->saveMany([$page]); - $restrictionService = $this->app[\BookStack\Services\PermissionService::class]; + $chapter = factory(\BookStack\Chapter::class)->create(['created_by' => $creatorUser->id, 'updated_by' => $updaterUser->id, 'book_id' => $book->id]); + $page = factory(\BookStack\Page::class)->create(['created_by' => $creatorUser->id, 'updated_by' => $updaterUser->id, 'book_id' => $book->id, 'chapter_id' => $chapter->id]); + $restrictionService = $this->app[PermissionService::class]; $restrictionService->buildJointPermissionsForEntity($book); return [ 'book' => $book, diff --git a/tests/Permissions/RolesTest.php b/tests/Permissions/RolesTest.php index 24b8ae0f5..83d1b98a8 100644 --- a/tests/Permissions/RolesTest.php +++ b/tests/Permissions/RolesTest.php @@ -1,5 +1,8 @@ make()->toArray(); $roleData['permissions'] = array_flip($permissions); return $permissionRepo->saveNewRole($roleData); @@ -107,16 +110,16 @@ class RolesTest extends BrowserKitTest public function test_manage_user_permission() { - $this->actingAs($this->user)->visit('/')->visit('/settings/users') + $this->actingAs($this->user)->visit('/settings/users') ->seePageIs('/'); $this->giveUserPermissions($this->user, ['users-manage']); - $this->actingAs($this->user)->visit('/')->visit('/settings/users') + $this->actingAs($this->user)->visit('/settings/users') ->seePageIs('/settings/users'); } public function test_user_roles_manage_permission() { - $this->actingAs($this->user)->visit('/')->visit('/settings/roles') + $this->actingAs($this->user)->visit('/settings/roles') ->seePageIs('/')->visit('/settings/roles/1')->seePageIs('/'); $this->giveUserPermissions($this->user, ['user-roles-manage']); $this->actingAs($this->user)->visit('/settings/roles') @@ -126,10 +129,10 @@ class RolesTest extends BrowserKitTest public function test_settings_manage_permission() { - $this->actingAs($this->user)->visit('/')->visit('/settings') + $this->actingAs($this->user)->visit('/settings') ->seePageIs('/'); $this->giveUserPermissions($this->user, ['settings-manage']); - $this->actingAs($this->user)->visit('/')->visit('/settings') + $this->actingAs($this->user)->visit('/settings') ->seePageIs('/settings')->press('Save Settings')->see('Settings Saved'); } @@ -181,27 +184,26 @@ class RolesTest extends BrowserKitTest * @param string $permission * @param array $accessUrls Urls that are only accessible after having the permission * @param array $visibles Check this text, In the buttons toolbar, is only visible with the permission - * @param null $callback */ private function checkAccessPermission($permission, $accessUrls = [], $visibles = []) { foreach ($accessUrls as $url) { - $this->actingAs($this->user)->visit('/')->visit($url) + $this->actingAs($this->user)->visit($url) ->seePageIs('/'); } foreach ($visibles as $url => $text) { - $this->actingAs($this->user)->visit('/')->visit($url) + $this->actingAs($this->user)->visit($url) ->dontSeeInElement('.action-buttons',$text); } $this->giveUserPermissions($this->user, [$permission]); foreach ($accessUrls as $url) { - $this->actingAs($this->user)->visit('/')->visit($url) + $this->actingAs($this->user)->visit($url) ->seePageIs($url); } foreach ($visibles as $url => $text) { - $this->actingAs($this->user)->visit('/')->visit($url) + $this->actingAs($this->user)->visit($url) ->see($text); } } @@ -391,8 +393,8 @@ class RolesTest extends BrowserKitTest public function test_page_create_own_permissions() { - $book = \BookStack\Book::take(1)->get()->first(); - $chapter = \BookStack\Chapter::take(1)->get()->first(); + $book = \BookStack\Book::first(); + $chapter = \BookStack\Chapter::first(); $entities = $this->createEntityChainBelongingToUser($this->user); $ownBook = $entities['book']; @@ -405,7 +407,7 @@ class RolesTest extends BrowserKitTest $accessUrls = [$createUrl, $createUrlChapter]; foreach ($accessUrls as $url) { - $this->actingAs($this->user)->visit('/')->visit($url) + $this->actingAs($this->user)->visit($url) ->seePageIs('/'); } @@ -417,7 +419,7 @@ class RolesTest extends BrowserKitTest $this->giveUserPermissions($this->user, ['page-create-own']); foreach ($accessUrls as $index => $url) { - $this->actingAs($this->user)->visit('/')->visit($url); + $this->actingAs($this->user)->visit($url); $expectedUrl = \BookStack\Page::where('draft', '=', true)->orderBy('id', 'desc')->first()->getUrl(); $this->seePageIs($expectedUrl); } @@ -449,7 +451,7 @@ class RolesTest extends BrowserKitTest $accessUrls = [$createUrl, $createUrlChapter]; foreach ($accessUrls as $url) { - $this->actingAs($this->user)->visit('/')->visit($url) + $this->actingAs($this->user)->visit($url) ->seePageIs('/'); } @@ -461,7 +463,7 @@ class RolesTest extends BrowserKitTest $this->giveUserPermissions($this->user, ['page-create-all']); foreach ($accessUrls as $index => $url) { - $this->actingAs($this->user)->visit('/')->visit($url); + $this->actingAs($this->user)->visit($url); $expectedUrl = \BookStack\Page::where('draft', '=', true)->orderBy('id', 'desc')->first()->getUrl(); $this->seePageIs($expectedUrl); }