From 38829f8a38a5bd90f9cce2535c79131c04ae01d6 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Thu, 17 Aug 2023 17:57:31 +0100
Subject: [PATCH] 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.
---
 .../Handlers/BaseNotificationHandler.php        |  8 ++------
 .../Notifications/NotificationManager.php       |  2 +-
 app/App/Providers/AppServiceProvider.php        |  5 +++++
 app/Permissions/PermissionApplicator.php        |  8 ++++++--
 tests/Activity/WatchTest.php                    | 17 +++++++++++++++++
 5 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/app/Activity/Notifications/Handlers/BaseNotificationHandler.php b/app/Activity/Notifications/Handlers/BaseNotificationHandler.php
index f1742592e..b5f339b2c 100644
--- 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 fc6a5f57c..294f56ebb 100644
--- 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 0c0895bf4..deb664ba6 100644
--- 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 b4fafaa9e..a796bdaee 100644
--- 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 e8cb3cbd8..00f3f5e4d 100644
--- 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);
+    }
 }