Notifications: Fixed send content permission checking

Added test and changed logic to properly check the view permissions for
the notification receiver before sending.
Required change to permissions applicator to allow the user to be
manually determined, and a service provider update to provide the class
as a singleton without a specific user, so it checks the current logged
in user on demand.

diff --git a/app/Activity/Notifications/Handlers/BaseNotificationHandler.php b/app/Activity/Notifications/Handlers/BaseNotificationHandler.php
index f1742592e792d0808f1bc1f51c5c38a7d56fa703..b5f339b2ce0ba3f98f680df4339fc56910476d79 100644 (file)
--- a/app/Activity/Notifications/Handlers/BaseNotificationHandler.php
+++ b/app/Activity/Notifications/Handlers/BaseNotificationHandler.php
@@ -10,11 +10,6 @@ use BookStack\Users\Models\User;
 
 abstract class BaseNotificationHandler implements NotificationHandler
 {
-    public function __construct(
-        protected PermissionApplicator $permissionApplicator
-    ) {
-    }
-
     /**
      * @param class-string<BaseActivityNotification> $notification
      * @param int[] $userIds
@@ -35,7 +30,8 @@ abstract class BaseNotificationHandler implements NotificationHandler
             }
 
             // Prevent sending if the user does not have access to the related content
-            if (!$this->permissionApplicator->checkOwnableUserAccess($relatedModel, 'view')) {
+            $permissions = new PermissionApplicator($user);
+            if (!$permissions->checkOwnableUserAccess($relatedModel, 'view')) {
                 continue;
             }
 

diff --git a/app/Activity/Notifications/NotificationManager.php b/app/Activity/Notifications/NotificationManager.php
index fc6a5f57c60c75e661d3bf79641ded997da55849..294f56ebbcf4ab86772e33866df46c4b22d1430d 100644 (file)
--- a/app/Activity/Notifications/NotificationManager.php
+++ b/app/Activity/Notifications/NotificationManager.php
@@ -24,7 +24,7 @@ class NotificationManager
         $handlersToRun = $this->handlers[$activityType] ?? [];
         foreach ($handlersToRun as $handlerClass) {
             /** @var NotificationHandler $handler */
-            $handler = app()->make($handlerClass);
+            $handler = new $handlerClass();
             $handler->handle($activity, $detail, $user);
         }
     }

diff --git a/app/App/Providers/AppServiceProvider.php b/app/App/Providers/AppServiceProvider.php
index 0c0895bf45309624c9aedd6d439be1f8eb860aa9..deb664ba697d2ff639b42b00c812f4503d1286dc 100644 (file)
--- a/app/App/Providers/AppServiceProvider.php
+++ b/app/App/Providers/AppServiceProvider.php
@@ -9,6 +9,7 @@ use BookStack\Entities\Models\Bookshelf;
 use BookStack\Entities\Models\Chapter;
 use BookStack\Entities\Models\Page;
 use BookStack\Exceptions\BookStackExceptionHandlerPage;
+use BookStack\Permissions\PermissionApplicator;
 use BookStack\Settings\SettingService;
 use BookStack\Util\CspService;
 use GuzzleHttp\Client;
@@ -79,5 +80,9 @@ class AppServiceProvider extends ServiceProvider
                 'timeout' => 3,
             ]);
         });
+
+        $this->app->singleton(PermissionApplicator::class, function ($app) {
+            return new PermissionApplicator(null);
+        });
     }
 }

diff --git a/app/Permissions/PermissionApplicator.php b/app/Permissions/PermissionApplicator.php
index b4fafaa9ee1756f6604d5cc20f46e2091ed5f787..a796bdaeee4e70e42776bcada7e726d8344b405e 100644 (file)
--- a/app/Permissions/PermissionApplicator.php
+++ b/app/Permissions/PermissionApplicator.php
@@ -8,7 +8,6 @@ use BookStack\Entities\Models\Page;
 use BookStack\Permissions\Models\EntityPermission;
 use BookStack\Users\Models\HasCreatorAndUpdater;
 use BookStack\Users\Models\HasOwner;
-use BookStack\Users\Models\Role;
 use BookStack\Users\Models\User;
 use Illuminate\Database\Eloquent\Builder;
 use Illuminate\Database\Query\Builder as QueryBuilder;
@@ -16,6 +15,11 @@ use InvalidArgumentException;
 
 class PermissionApplicator
 {
+    public function __construct(
+        protected ?User $user = null
+    ) {
+    }
+
     /**
      * Checks if an entity has a restriction set upon it.
      *
@@ -173,7 +177,7 @@ class PermissionApplicator
      */
     protected function currentUser(): User
     {
-        return user();
+        return $this->user ?? user();
     }
 
     /**

diff --git a/tests/Activity/WatchTest.php b/tests/Activity/WatchTest.php
index e8cb3cbd8b104567eb66354776173412c557eda2..00f3f5e4db51b9fa90b598fd2d6df3d730edb43f 100644 (file)
--- a/tests/Activity/WatchTest.php
+++ b/tests/Activity/WatchTest.php
@@ -312,4 +312,21 @@ class WatchTest extends TestCase
                 && str_contains($mailContent, 'Created By: ' . $admin->name);
         });
     }
+
+    public function test_notifications_not_sent_if_lacking_view_permission_for_related_item()
+    {
+        $notifications = Notification::fake();
+        $editor = $this->users->editor();
+        $page = $this->entities->page();
+
+        $watches = new UserEntityWatchOptions($editor, $page);
+        $watches->updateWatchLevel('comments');
+        $this->permissions->disableEntityInheritedPermissions($page);
+
+        $this->asAdmin()->post("/comment/{$page->id}", [
+            'text' => 'My new comment response',
+        ])->assertOk();
+
+        $notifications->assertNothingSentTo($editor);
+    }
 }