Watching: Prevent issues when watchable or user is deleted

- Adds filtering to the watched items list in notification preferences
  so that deleted (recycle bin) items are removed via query.
- Adds relations and logic to properly remove watches upon user and
  entity delete events, to old watches in database do not linger.
- Adds testing to cover the above.

Did not add migration for existing data, since patch will be close to
introduction, and lingering DB entries don't open a security concern,
just some potential confusion in specific potential scenarios.
Probably not work extra migration risk, although could add in future if
concerns/issues are found.

Related to #4499

diff --git a/app/Entities/EntityProvider.php b/app/Entities/EntityProvider.php
index 365daf7ebe66c9ec70e42159c0e3b8600e3e69b1..3276a6c7a91d103b3b36b2098339d5ae437a308c 100644 (file)
--- a/app/Entities/EntityProvider.php
+++ b/app/Entities/EntityProvider.php
@@ -37,7 +37,7 @@ class EntityProvider
      * Fetch all core entity types as an associated array
      * with their basic names as the keys.
      *
-     * @return array<Entity>
+     * @return array<string, Entity>
      */
     public function all(): array
     {

diff --git a/app/Entities/Models/Entity.php b/app/Entities/Models/Entity.php
index 496ea46b67834ff8cad3d46049b22b6573ef81d9..33251067297de3ace16de5939382a7dbf3ba30fd 100644 (file)
--- a/app/Entities/Models/Entity.php
+++ b/app/Entities/Models/Entity.php
@@ -10,6 +10,7 @@ use BookStack\Activity\Models\Loggable;
 use BookStack\Activity\Models\Tag;
 use BookStack\Activity\Models\View;
 use BookStack\Activity\Models\Viewable;
+use BookStack\Activity\Models\Watch;
 use BookStack\App\Model;
 use BookStack\App\Sluggable;
 use BookStack\Entities\Tools\SlugGenerator;
@@ -330,6 +331,14 @@ abstract class Entity extends Model implements Sluggable, Favouritable, Viewable
             ->exists();
     }
 
+    /**
+     * Get the related watches for this entity.
+     */
+    public function watches(): MorphMany
+    {
+        return $this->morphMany(Watch::class, 'watchable');
+    }
+
     /**
      * {@inheritdoc}
      */

diff --git a/app/Entities/Tools/TrashCan.php b/app/Entities/Tools/TrashCan.php
index 7341a032816ac5a10b08b39277d1cf6cc3b2c0e7..3c497024fb2d55a7ceaaa5f596f025e40c111d49 100644 (file)
--- a/app/Entities/Tools/TrashCan.php
+++ b/app/Entities/Tools/TrashCan.php
@@ -376,6 +376,7 @@ class TrashCan
         $entity->searchTerms()->delete();
         $entity->deletions()->delete();
         $entity->favourites()->delete();
+        $entity->watches()->delete();
         $entity->referencesTo()->delete();
         $entity->referencesFrom()->delete();
 

diff --git a/app/Permissions/PermissionApplicator.php b/app/Permissions/PermissionApplicator.php
index a796bdaeee4e70e42776bcada7e726d8344b405e..7b62ac0a7eb2b250735a2a29aac799f3db83ceac 100644 (file)
--- a/app/Permissions/PermissionApplicator.php
+++ b/app/Permissions/PermissionApplicator.php
@@ -3,6 +3,7 @@
 namespace BookStack\Permissions;
 
 use BookStack\App\Model;
+use BookStack\Entities\EntityProvider;
 use BookStack\Entities\Models\Entity;
 use BookStack\Entities\Models\Page;
 use BookStack\Permissions\Models\EntityPermission;
@@ -11,6 +12,7 @@ use BookStack\Users\Models\HasOwner;
 use BookStack\Users\Models\User;
 use Illuminate\Database\Eloquent\Builder;
 use Illuminate\Database\Query\Builder as QueryBuilder;
+use Illuminate\Database\Query\JoinClause;
 use InvalidArgumentException;
 
 class PermissionApplicator
@@ -147,6 +149,42 @@ class PermissionApplicator
             });
     }
 
+    /**
+     * Filter out items that have related entity relations where
+     * the entity is marked as deleted.
+     */
+    public function filterDeletedFromEntityRelationQuery(Builder $query, string $tableName, string $entityIdColumn, string $entityTypeColumn): Builder
+    {
+        $tableDetails = ['tableName' => $tableName, 'entityIdColumn' => $entityIdColumn, 'entityTypeColumn' => $entityTypeColumn];
+        $entityProvider = new EntityProvider();
+
+        $joinQuery = function ($query) use ($entityProvider) {
+            $first = true;
+            /** @var Builder $query */
+            foreach ($entityProvider->all() as $entity) {
+                $entityQuery = function ($query) use ($entity) {
+                    /** @var Builder $query */
+                    $query->select(['id', 'deleted_at'])
+                        ->selectRaw("'{$entity->getMorphClass()}' as type")
+                        ->from($entity->getTable())
+                        ->whereNotNull('deleted_at');
+                };
+
+                if ($first) {
+                    $entityQuery($query);
+                    $first = false;
+                } else {
+                    $query->union($entityQuery);
+                }
+            }
+        };
+
+        return $query->leftJoinSub($joinQuery, 'deletions', function (JoinClause $join) use ($tableDetails) {
+            $join->on($tableDetails['tableName'] . '.' . $tableDetails['entityIdColumn'], '=', 'deletions.id')
+                ->on($tableDetails['tableName'] . '.' . $tableDetails['entityTypeColumn'], '=', 'deletions.type');
+        })->whereNull('deletions.deleted_at');
+    }
+
     /**
      * Add conditions to a query for a model that's a relation of a page, so only the model results
      * on visible pages are returned by the query.

diff --git a/app/Users/Controllers/UserPreferencesController.php b/app/Users/Controllers/UserPreferencesController.php
index 503aeaeb0c9d23032746cf72fd67634678f2aa43..9c38ff2af750fd7542102d7ae744b8aecf2649ec 100644 (file)
--- a/app/Users/Controllers/UserPreferencesController.php
+++ b/app/Users/Controllers/UserPreferencesController.php
@@ -2,7 +2,6 @@
 
 namespace BookStack\Users\Controllers;
 
-use BookStack\Activity\Models\Watch;
 use BookStack\Http\Controller;
 use BookStack\Permissions\PermissionApplicator;
 use BookStack\Settings\UserNotificationPreferences;
@@ -68,8 +67,9 @@ class UserPreferencesController extends Controller
 
         $preferences = (new UserNotificationPreferences(user()));
 
-        $query = Watch::query()->where('user_id', '=', user()->id);
+        $query = user()->watches()->getQuery();
         $query = $permissions->restrictEntityRelationQuery($query, 'watches', 'watchable_id', 'watchable_type');
+        $query = $permissions->filterDeletedFromEntityRelationQuery($query, 'watches', 'watchable_id', 'watchable_type');
         $watches = $query->with('watchable')->paginate(20);
 
         $this->setPageTitle(trans('preferences.notifications'));

diff --git a/app/Users/Models/User.php b/app/Users/Models/User.php
index a2b54f708f4865b9752a9fab1348d8c2542a38e6..e3d856a8d9964aa1c3ac3db5dbb0e385c1bed700 100644 (file)
--- a/app/Users/Models/User.php
+++ b/app/Users/Models/User.php
@@ -6,6 +6,7 @@ use BookStack\Access\Mfa\MfaValue;
 use BookStack\Access\SocialAccount;
 use BookStack\Activity\Models\Favourite;
 use BookStack\Activity\Models\Loggable;
+use BookStack\Activity\Models\Watch;
 use BookStack\Api\ApiToken;
 use BookStack\App\Model;
 use BookStack\App\Sluggable;
@@ -291,6 +292,14 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon
         return $this->hasMany(MfaValue::class);
     }
 
+    /**
+     * Get the tracked entity watches for this user.
+     */
+    public function watches(): HasMany
+    {
+        return $this->hasMany(Watch::class);
+    }
+
     /**
      * Get the last activity time for this user.
      */

diff --git a/app/Users/UserRepo.php b/app/Users/UserRepo.php
index 408ee6a8e5d21514dc0441f58aaaf49d3d1eb450..32e23ecdec132aced9684ab4413d86b2d50641a3 100644 (file)
--- a/app/Users/UserRepo.php
+++ b/app/Users/UserRepo.php
@@ -18,18 +18,13 @@ use Illuminate\Support\Str;
 
 class UserRepo
 {
-    protected UserAvatars $userAvatar;
-    protected UserInviteService $inviteService;
-
-    /**
-     * UserRepo constructor.
-     */
-    public function __construct(UserAvatars $userAvatar, UserInviteService $inviteService)
-    {
-        $this->userAvatar = $userAvatar;
-        $this->inviteService = $inviteService;
+    public function __construct(
+        protected UserAvatars $userAvatar,
+        protected UserInviteService $inviteService
+    ) {
     }
 
+
     /**
      * Get a user by their email address.
      */
@@ -155,6 +150,7 @@ class UserRepo
         $user->apiTokens()->delete();
         $user->favourites()->delete();
         $user->mfaValues()->delete();
+        $user->watches()->delete();
         $user->delete();
 
         // Delete user profile images

diff --git a/tests/Activity/WatchTest.php b/tests/Activity/WatchTest.php
index 72e6b37a58931ee35e75c7340fa272d8d21c6465..fa50d8c79f1d5e0d584784210460571fcaf28348 100644 (file)
--- a/tests/Activity/WatchTest.php
+++ b/tests/Activity/WatchTest.php
@@ -12,6 +12,7 @@ use BookStack\Activity\Tools\ActivityLogger;
 use BookStack\Activity\Tools\UserEntityWatchOptions;
 use BookStack\Activity\WatchLevels;
 use BookStack\Entities\Models\Entity;
+use BookStack\Entities\Tools\TrashCan;
 use BookStack\Settings\UserNotificationPreferences;
 use Illuminate\Support\Facades\Notification;
 use Tests\TestCase;
@@ -370,4 +371,32 @@ class WatchTest extends TestCase
 
         $notifications->assertNothingSentTo($editor);
     }
+
+    public function test_watches_deleted_on_user_delete()
+    {
+        $editor = $this->users->editor();
+        $page = $this->entities->page();
+
+        $watches = new UserEntityWatchOptions($editor, $page);
+        $watches->updateLevelByValue(WatchLevels::COMMENTS);
+        $this->assertDatabaseHas('watches', ['user_id' => $editor->id]);
+
+        $this->asAdmin()->delete($editor->getEditUrl());
+
+        $this->assertDatabaseMissing('watches', ['user_id' => $editor->id]);
+    }
+
+    public function test_watches_deleted_on_item_delete()
+    {
+        $editor = $this->users->editor();
+        $page = $this->entities->page();
+
+        $watches = new UserEntityWatchOptions($editor, $page);
+        $watches->updateLevelByValue(WatchLevels::COMMENTS);
+        $this->assertDatabaseHas('watches', ['watchable_type' => 'page', 'watchable_id' => $page->id]);
+
+        $this->entities->destroy($page);
+
+        $this->assertDatabaseMissing('watches', ['watchable_type' => 'page', 'watchable_id' => $page->id]);
+    }
 }

diff --git a/tests/Helpers/EntityProvider.php b/tests/Helpers/EntityProvider.php
index ddc854290b17c5a1384561aaf96ed0ccddefe347..3cb8c44d32ca75c095cae6b73d49b6d7bf6bde20 100644 (file)
--- a/tests/Helpers/EntityProvider.php
+++ b/tests/Helpers/EntityProvider.php
@@ -11,6 +11,7 @@ use BookStack\Entities\Repos\BookRepo;
 use BookStack\Entities\Repos\BookshelfRepo;
 use BookStack\Entities\Repos\ChapterRepo;
 use BookStack\Entities\Repos\PageRepo;
+use BookStack\Entities\Tools\TrashCan;
 use BookStack\Users\Models\User;
 use Illuminate\Database\Eloquent\Builder;
 
@@ -197,6 +198,16 @@ class EntityProvider
         return $draftPage;
     }
 
+    /**
+     * Fully destroy the given entity from the system, bypassing the recycle bin
+     * stage. Still runs through main app deletion logic.
+     */
+    public function destroy(Entity $entity)
+    {
+        $trash = app()->make(TrashCan::class);
+        $trash->destroyEntity($entity);
+    }
+
     /**
      * @param Entity|Entity[] $entities
      */

diff --git a/tests/User/UserPreferencesTest.php b/tests/User/UserPreferencesTest.php
index 9d72f4e1496e0e9c7ca5a60c073859f2f6781b4a..1b16b0b4544994d5e34f0bad54b190a92fbd0a85 100644 (file)
--- a/tests/User/UserPreferencesTest.php
+++ b/tests/User/UserPreferencesTest.php
@@ -124,6 +124,23 @@ class UserPreferencesTest extends TestCase
         $resp->assertDontSee('All Page Updates & Comments');
     }
 
+    public function test_notification_preferences_dont_error_on_deleted_items()
+    {
+        $editor = $this->users->editor();
+        $book = $this->entities->book();
+
+        $options = new UserEntityWatchOptions($editor, $book);
+        $options->updateLevelByValue(WatchLevels::COMMENTS);
+
+        $this->actingAs($editor)->delete($book->getUrl());
+        $book->refresh();
+        $this->assertNotNull($book->deleted_at);
+
+        $resp = $this->actingAs($editor)->get('/preferences/notifications');
+        $resp->assertOk();
+        $resp->assertDontSee($book->name);
+    }
+
     public function test_notification_preferences_not_accessible_to_guest()
     {
         $this->setSettings(['app-public' => 'true']);