Perms: Added transactions around permission effecting actions

app/Entities/Controllers/BookController.php

@@ -18,6 +18,7 @@ use BookStack\Exceptions\NotFoundException;
 use BookStack\Facades\Activity;
 use BookStack\Http\Controller;
 use BookStack\References\ReferenceFetcher;
+use BookStack\Util\DatabaseTransaction;
 use BookStack\Util\SimpleListOptions;
 use Illuminate\Http\Request;
 use Illuminate\Validation\ValidationException;
@@ -263,7 +264,9 @@ class BookController extends Controller
         $this->checkPermission('bookshelf-create-all');
         $this->checkPermission('book-create-all');
 
-        $shelf = $transformer->transformBookToShelf($book);
+        $shelf = (new DatabaseTransaction(function () use ($book, $transformer) {
+            return $transformer->transformBookToShelf($book);
+        }))->run();
 
         return redirect($shelf->getUrl());
     }

app/Entities/Controllers/ChapterController.php

@@ -18,6 +18,7 @@ use BookStack\Exceptions\NotifyException;
 use BookStack\Exceptions\PermissionsException;
 use BookStack\Http\Controller;
 use BookStack\References\ReferenceFetcher;
+use BookStack\Util\DatabaseTransaction;
 use Illuminate\Http\Request;
 use Illuminate\Validation\ValidationException;
 use Throwable;
@@ -269,7 +270,9 @@ class ChapterController extends Controller
         $this->checkOwnablePermission('chapter-delete', $chapter);
         $this->checkPermission('book-create-all');
 
-        $book = $transformer->transformChapterToBook($chapter);
+        $book = (new DatabaseTransaction(function () use ($chapter, $transformer) {
+            return $transformer->transformChapterToBook($chapter);
+        }))->run();
 
         return redirect($book->getUrl());
     }

app/Entities/Repos/BookRepo.php

@@ -10,6 +10,7 @@ use BookStack\Exceptions\ImageUploadException;
 use BookStack\Facades\Activity;
 use BookStack\Sorting\SortRule;
 use BookStack\Uploads\ImageRepo;
+use BookStack\Util\DatabaseTransaction;
 use Exception;
 use Illuminate\Http\UploadedFile;
 
@@ -28,7 +29,9 @@ class BookRepo
      */
     public function create(array $input): Book
     {
+        return (new DatabaseTransaction(function () use ($input) {
             $book = new Book();
+
             $this->baseRepo->create($book, $input);
             $this->baseRepo->updateCoverImage($book, $input['image'] ?? null);
             $this->baseRepo->updateDefaultTemplate($book, intval($input['default_template_id'] ?? null));
@@ -41,6 +44,7 @@ class BookRepo
             }
 
             return $book;
+        }))->run();
     }
 
     /**

app/Entities/Repos/BookshelfRepo.php

@@ -7,6 +7,7 @@ use BookStack\Entities\Models\Bookshelf;
 use BookStack\Entities\Queries\BookQueries;
 use BookStack\Entities\Tools\TrashCan;
 use BookStack\Facades\Activity;
+use BookStack\Util\DatabaseTransaction;
 use Exception;
 
 class BookshelfRepo
@@ -23,13 +24,13 @@ class BookshelfRepo
      */
     public function create(array $input, array $bookIds): Bookshelf
     {
+        return (new DatabaseTransaction(function () use ($input, $bookIds) {
             $shelf = new Bookshelf();
             $this->baseRepo->create($shelf, $input);
             $this->baseRepo->updateCoverImage($shelf, $input['image'] ?? null);
             $this->updateBooks($shelf, $bookIds);
             Activity::add(ActivityType::BOOKSHELF_CREATE, $shelf);
-
+        }))->run();
-        return $shelf;
     }
 
     /**

app/Entities/Repos/ChapterRepo.php

@@ -11,6 +11,7 @@ use BookStack\Entities\Tools\TrashCan;
 use BookStack\Exceptions\MoveOperationException;
 use BookStack\Exceptions\PermissionsException;
 use BookStack\Facades\Activity;
+use BookStack\Util\DatabaseTransaction;
 use Exception;
 
 class ChapterRepo
@@ -27,6 +28,7 @@ class ChapterRepo
      */
     public function create(array $input, Book $parentBook): Chapter
     {
+        return (new DatabaseTransaction(function () use ($input, $parentBook) {
             $chapter = new Chapter();
             $chapter->book_id = $parentBook->id;
             $chapter->priority = (new BookContents($parentBook))->getLastPriority() + 1;
@@ -35,8 +37,7 @@ class ChapterRepo
             Activity::add(ActivityType::CHAPTER_CREATE, $chapter);
 
             $this->baseRepo->sortParent($chapter);
-
+        }))->run();
-        return $chapter;
     }
 
     /**
@@ -88,6 +89,7 @@ class ChapterRepo
             throw new PermissionsException('User does not have permission to create a chapter within the chosen book');
         }
 
+        return (new DatabaseTransaction(function () use ($chapter, $parent) {
             $chapter->changeBook($parent->id);
             $chapter->rebuildPermissions();
             Activity::add(ActivityType::CHAPTER_MOVE, $chapter);
@@ -95,5 +97,6 @@ class ChapterRepo
             $this->baseRepo->sortParent($chapter);
 
             return $parent;
+        }))->run();
     }
 }

app/Entities/Repos/PageRepo.php

@@ -18,6 +18,7 @@ use BookStack\Exceptions\PermissionsException;
 use BookStack\Facades\Activity;
 use BookStack\References\ReferenceStore;
 use BookStack\References\ReferenceUpdater;
+use BookStack\Util\DatabaseTransaction;
 use Exception;
 
 class PageRepo
@@ -61,8 +62,10 @@ class PageRepo
             ]);
         }
 
+        (new DatabaseTransaction(function () use ($page) {
             $page->save();
             $page->refresh()->rebuildPermissions();
+        }))->run();
 
         return $page;
     }
@@ -72,6 +75,7 @@ class PageRepo
      */
     public function publishDraft(Page $draft, array $input): Page
     {
+        return (new DatabaseTransaction(function () use ($draft, $input) {
             $draft->draft = false;
             $draft->revision_count = 1;
             $draft->priority = $this->getNewPriority($draft);
@@ -87,6 +91,7 @@ class PageRepo
             $this->baseRepo->sortParent($draft);
 
             return $draft;
+        }))->run();
     }
 
     /**
@@ -117,7 +122,7 @@ class PageRepo
         $page->revision_count++;
         $page->save();
 
-        // Remove all update drafts for this user & page.
+        // Remove all update drafts for this user and page.
         $this->revisionRepo->deleteDraftsForCurrentUser($page);
 
         // Save a revision after updating
@@ -270,6 +275,7 @@ class PageRepo
             throw new PermissionsException('User does not have permission to create a page within the new parent');
         }
 
+        return (new DatabaseTransaction(function () use ($page, $parent) {
             $page->chapter_id = ($parent instanceof Chapter) ? $parent->id : null;
             $newBookId = ($parent instanceof Chapter) ? $parent->book->id : $parent->id;
             $page->changeBook($newBookId);
@@ -280,6 +286,7 @@ class PageRepo
             $this->baseRepo->sortParent($page);
 
             return $parent;
+        }))->run();
     }
 
     /**

app/Entities/Tools/HierarchyTransformer.php

@@ -13,17 +13,12 @@ use BookStack\Facades\Activity;
 
 class HierarchyTransformer
 {
-    protected BookRepo $bookRepo;
+    public function __construct(
-    protected BookshelfRepo $shelfRepo;
+        protected BookRepo $bookRepo,
-    protected Cloner $cloner;
+        protected BookshelfRepo $shelfRepo,
-    protected TrashCan $trashCan;
+        protected Cloner $cloner,
-
+        protected TrashCan $trashCan
-    public function __construct(BookRepo $bookRepo, BookshelfRepo $shelfRepo, Cloner $cloner, TrashCan $trashCan)
+    ) {
-    {
-        $this->bookRepo = $bookRepo;
-        $this->shelfRepo = $shelfRepo;
-        $this->cloner = $cloner;
-        $this->trashCan = $trashCan;
     }
 
     /**

app/Entities/Tools/TrashCan.php

@@ -15,6 +15,7 @@ use BookStack\Exceptions\NotifyException;
 use BookStack\Facades\Activity;
 use BookStack\Uploads\AttachmentService;
 use BookStack\Uploads\ImageService;
+use BookStack\Util\DatabaseTransaction;
 use Exception;
 use Illuminate\Database\Eloquent\Builder;
 use Illuminate\Support\Carbon;
@@ -357,25 +358,26 @@ class TrashCan
 
     /**
      * Destroy the given entity.
+     * Returns the number of total entities destroyed in the operation.
      *
      * @throws Exception
      */
     public function destroyEntity(Entity $entity): int
     {
+        $result = (new DatabaseTransaction(function () use ($entity) {
             if ($entity instanceof Page) {
                 return $this->destroyPage($entity);
-        }
+            } else if ($entity instanceof Chapter) {
-        if ($entity instanceof Chapter) {
                 return $this->destroyChapter($entity);
-        }
+            } else if ($entity instanceof Book) {
-        if ($entity instanceof Book) {
                 return $this->destroyBook($entity);
-        }
+            } else if ($entity instanceof Bookshelf) {
-        if ($entity instanceof Bookshelf) {
                 return $this->destroyShelf($entity);
             }
+            return null;
+        }))->run();
 
-        return 0;
+        return $result ?? 0;
     }
 
     /**

app/Permissions/JointPermissionBuilder.php

@@ -29,7 +29,7 @@ class JointPermissionBuilder
     /**
      * Re-generate all entity permission from scratch.
      */
-    public function rebuildForAll()
+    public function rebuildForAll(): void
     {
         JointPermission::query()->truncate();
 
@@ -51,7 +51,7 @@ class JointPermissionBuilder
     /**
      * Rebuild the entity jointPermissions for a particular entity.
      */
-    public function rebuildForEntity(Entity $entity)
+    public function rebuildForEntity(Entity $entity): void
     {
         $entities = [$entity];
         if ($entity instanceof Book) {
@@ -119,7 +119,7 @@ class JointPermissionBuilder
     /**
      * Build joint permissions for the given book and role combinations.
      */
-    protected function buildJointPermissionsForBooks(EloquentCollection $books, array $roles, bool $deleteOld = false)
+    protected function buildJointPermissionsForBooks(EloquentCollection $books, array $roles, bool $deleteOld = false): void
     {
         $entities = clone $books;
 
@@ -143,7 +143,7 @@ class JointPermissionBuilder
     /**
      * Rebuild the entity jointPermissions for a collection of entities.
      */
-    protected function buildJointPermissionsForEntities(array $entities)
+    protected function buildJointPermissionsForEntities(array $entities): void
     {
         $roles = Role::query()->get()->values()->all();
         $this->deleteManyJointPermissionsForEntities($entities);
@@ -155,12 +155,11 @@ class JointPermissionBuilder
      *
      * @param Entity[] $entities
      */
-    protected function deleteManyJointPermissionsForEntities(array $entities)
+    protected function deleteManyJointPermissionsForEntities(array $entities): void
     {
         $simpleEntities = $this->entitiesToSimpleEntities($entities);
         $idsByType = $this->entitiesToTypeIdMap($simpleEntities);
 
-        DB::transaction(function () use ($idsByType) {
         foreach ($idsByType as $type => $ids) {
             foreach (array_chunk($ids, 1000) as $idChunk) {
                 DB::table('joint_permissions')
@@ -169,7 +168,6 @@ class JointPermissionBuilder
                     ->delete();
             }
         }
-        });
     }
 
     /**
@@ -195,7 +193,7 @@ class JointPermissionBuilder
      * @param Entity[] $originalEntities
      * @param Role[]   $roles
      */
-    protected function createManyJointPermissions(array $originalEntities, array $roles)
+    protected function createManyJointPermissions(array $originalEntities, array $roles): void
     {
         $entities = $this->entitiesToSimpleEntities($originalEntities);
         $jointPermissions = [];
@@ -225,11 +223,9 @@ class JointPermissionBuilder
             }
         }
 
-        DB::transaction(function () use ($jointPermissions) {
         foreach (array_chunk($jointPermissions, 1000) as $jointPermissionChunk) {
             DB::table('joint_permissions')->insert($jointPermissionChunk);
         }
-        });
     }
 
     /**

app/Permissions/PermissionsController.php

@@ -7,6 +7,7 @@ use BookStack\Entities\Tools\PermissionsUpdater;
 use BookStack\Http\Controller;
 use BookStack\Permissions\Models\EntityPermission;
 use BookStack\Users\Models\Role;
+use BookStack\Util\DatabaseTransaction;
 use Illuminate\Http\Request;
 
 class PermissionsController extends Controller
@@ -40,7 +41,9 @@ class PermissionsController extends Controller
         $page = $this->queries->pages->findVisibleBySlugsOrFail($bookSlug, $pageSlug);
         $this->checkOwnablePermission('restrictions-manage', $page);
 
+        (new DatabaseTransaction(function () use ($page, $request) {
             $this->permissionsUpdater->updateFromPermissionsForm($page, $request);
+        }))->run();
 
         $this->showSuccessNotification(trans('entities.pages_permissions_success'));
 
@@ -70,7 +73,9 @@ class PermissionsController extends Controller
         $chapter = $this->queries->chapters->findVisibleBySlugsOrFail($bookSlug, $chapterSlug);
         $this->checkOwnablePermission('restrictions-manage', $chapter);
 
+        (new DatabaseTransaction(function () use ($chapter, $request) {
             $this->permissionsUpdater->updateFromPermissionsForm($chapter, $request);
+        }))->run();
 
         $this->showSuccessNotification(trans('entities.chapters_permissions_success'));
 
@@ -100,7 +105,9 @@ class PermissionsController extends Controller
         $book = $this->queries->books->findVisibleBySlugOrFail($slug);
         $this->checkOwnablePermission('restrictions-manage', $book);
 
+        (new DatabaseTransaction(function () use ($book, $request) {
             $this->permissionsUpdater->updateFromPermissionsForm($book, $request);
+        }))->run();
 
         $this->showSuccessNotification(trans('entities.books_permissions_updated'));
 
@@ -130,7 +137,9 @@ class PermissionsController extends Controller
         $shelf = $this->queries->shelves->findVisibleBySlugOrFail($slug);
         $this->checkOwnablePermission('restrictions-manage', $shelf);
 
+        (new DatabaseTransaction(function () use ($shelf, $request) {
             $this->permissionsUpdater->updateFromPermissionsForm($shelf, $request);
+        }))->run();
 
         $this->showSuccessNotification(trans('entities.shelves_permissions_updated'));
 
@@ -145,7 +154,10 @@ class PermissionsController extends Controller
         $shelf = $this->queries->shelves->findVisibleBySlugOrFail($slug);
         $this->checkOwnablePermission('restrictions-manage', $shelf);
 
-        $updateCount = $this->permissionsUpdater->updateBookPermissionsFromShelf($shelf);
+        $updateCount = (new DatabaseTransaction(function () use ($shelf) {
+            return $this->permissionsUpdater->updateBookPermissionsFromShelf($shelf);
+        }))->run();
+
         $this->showSuccessNotification(trans('entities.shelves_copy_permission_success', ['count' => $updateCount]));
 
         return redirect($shelf->getUrl());

app/Permissions/PermissionsRepo.php

@@ -7,6 +7,7 @@ use BookStack\Exceptions\PermissionsException;
 use BookStack\Facades\Activity;
 use BookStack\Permissions\Models\RolePermission;
 use BookStack\Users\Models\Role;
+use BookStack\Util\DatabaseTransaction;
 use Exception;
 use Illuminate\Database\Eloquent\Collection;
 
@@ -48,6 +49,7 @@ class PermissionsRepo
      */
     public function saveNewRole(array $roleData): Role
     {
+        return (new DatabaseTransaction(function () use ($roleData) {
             $role = new Role($roleData);
             $role->mfa_enforced = boolval($roleData['mfa_enforced'] ?? false);
             $role->save();
@@ -59,16 +61,18 @@ class PermissionsRepo
             Activity::add(ActivityType::ROLE_CREATE, $role);
 
             return $role;
+        }))->run();
     }
 
     /**
      * Updates an existing role.
-     * Ensures Admin system role always have core permissions.
+     * Ensures the Admin system role always has core permissions.
      */
     public function updateRole($roleId, array $roleData): Role
     {
         $role = $this->getRoleById($roleId);
 
+        return (new DatabaseTransaction(function () use ($role, $roleData) {
             if (isset($roleData['permissions'])) {
                 $this->assignRolePermissions($role, $roleData['permissions']);
             }
@@ -80,6 +84,7 @@ class PermissionsRepo
             Activity::add(ActivityType::ROLE_UPDATE, $role);
 
             return $role;
+        }))->run();
     }
 
     /**
@@ -114,7 +119,7 @@ class PermissionsRepo
     /**
      * Delete a role from the system.
      * Check it's not an admin role or set as default before deleting.
-     * If a migration Role ID is specified the users assign to the current role
+     * If a migration Role ID is specified, the users assigned to the current role
      * will be added to the role of the specified id.
      *
      * @throws PermissionsException
@@ -131,6 +136,7 @@ class PermissionsRepo
             throw new PermissionsException(trans('errors.role_registration_default_cannot_delete'));
         }
 
+        (new DatabaseTransaction(function () use ($migrateRoleId, $role) {
             if ($migrateRoleId !== 0) {
                 $newRole = Role::query()->find($migrateRoleId);
                 if ($newRole) {
@@ -143,5 +149,6 @@ class PermissionsRepo
             $role->jointPermissions()->delete();
             Activity::add(ActivityType::ROLE_DELETE, $role);
             $role->delete();
+        }))->run();
     }
 }

app/Sorting/BookSortController.php

@@ -7,6 +7,7 @@ use BookStack\Entities\Queries\BookQueries;
 use BookStack\Entities\Tools\BookContents;
 use BookStack\Facades\Activity;
 use BookStack\Http\Controller;
+use BookStack\Util\DatabaseTransaction;
 use Illuminate\Http\Request;
 
 class BookSortController extends Controller
@@ -55,16 +56,18 @@ class BookSortController extends Controller
 
         // Sort via map
         if ($request->filled('sort-tree')) {
+            (new DatabaseTransaction(function () use ($book, $request, $sorter, &$loggedActivityForBook) {
                 $sortMap = BookSortMap::fromJson($request->get('sort-tree'));
                 $booksInvolved = $sorter->sortUsingMap($sortMap);
 
-            // Rebuild permissions and add activity for involved books.
+                // Add activity for involved books.
                 foreach ($booksInvolved as $bookInvolved) {
                     Activity::add(ActivityType::BOOK_SORT, $bookInvolved);
                     if ($bookInvolved->id === $book->id) {
                         $loggedActivityForBook = true;
                     }
                 }
+            }))->run();
         }
 
         if ($request->filled('auto-sort')) {

app/Util/DatabaseTransaction.php

@@ -0,0 +1,42 @@
+<?php
+
+namespace BookStack\Util;
+
+use Closure;
+use Illuminate\Support\Facades\DB;
+use Throwable;
+
+/**
+ * Run the given code within a database transactions.
+ * Wraps Laravel's own transaction method, but sets a specific runtime isolation method.
+ * This sets a session level since this won't cause issues if already within a transaction,
+ * and this should apply to the next transactions anyway.
+ *
+ * "READ COMMITTED" ensures that changes from other transactions can be read within
+ * a transaction, even if started afterward (and for example, it was blocked by the initial
+ * transaction). This is quite important for things like permission generation, where we would
+ * want to consider the changes made by other committed transactions by the time we come to
+ * regenerate permission access.
+ *
+ * @throws Throwable
+ * @template TReturn of mixed
+ */
+class DatabaseTransaction
+{
+    /**
+     * @param  (Closure(static): TReturn)  $callback
+     */
+    public function __construct(
+        protected Closure $callback
+    ) {
+    }
+
+    /**
+     * @return TReturn
+     */
+    public function run(): mixed
+    {
+        DB::statement('SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED');
+        return DB::transaction($this->callback);
+    }
+}