Skip to content

Commit 817581a

Browse files
committed
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
1 parent 1cd19c7 commit 817581a

File tree

10 files changed

+123
-13
lines changed

10 files changed

+123
-13
lines changed

app/Entities/EntityProvider.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ public function __construct()
3737
* Fetch all core entity types as an associated array
3838
* with their basic names as the keys.
3939
*
40-
* @return array<Entity>
40+
* @return array<string, Entity>
4141
*/
4242
public function all(): array
4343
{

app/Entities/Models/Entity.php

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
use BookStack\Activity\Models\Tag;
1111
use BookStack\Activity\Models\View;
1212
use BookStack\Activity\Models\Viewable;
13+
use BookStack\Activity\Models\Watch;
1314
use BookStack\App\Model;
1415
use BookStack\App\Sluggable;
1516
use BookStack\Entities\Tools\SlugGenerator;
@@ -330,6 +331,14 @@ public function isFavourite(): bool
330331
->exists();
331332
}
332333

334+
/**
335+
* Get the related watches for this entity.
336+
*/
337+
public function watches(): MorphMany
338+
{
339+
return $this->morphMany(Watch::class, 'watchable');
340+
}
341+
333342
/**
334343
* {@inheritdoc}
335344
*/

app/Entities/Tools/TrashCan.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -376,6 +376,7 @@ protected function destroyCommonRelations(Entity $entity)
376376
$entity->searchTerms()->delete();
377377
$entity->deletions()->delete();
378378
$entity->favourites()->delete();
379+
$entity->watches()->delete();
379380
$entity->referencesTo()->delete();
380381
$entity->referencesFrom()->delete();
381382

app/Permissions/PermissionApplicator.php

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
namespace BookStack\Permissions;
44

55
use BookStack\App\Model;
6+
use BookStack\Entities\EntityProvider;
67
use BookStack\Entities\Models\Entity;
78
use BookStack\Entities\Models\Page;
89
use BookStack\Permissions\Models\EntityPermission;
@@ -11,6 +12,7 @@
1112
use BookStack\Users\Models\User;
1213
use Illuminate\Database\Eloquent\Builder;
1314
use Illuminate\Database\Query\Builder as QueryBuilder;
15+
use Illuminate\Database\Query\JoinClause;
1416
use InvalidArgumentException;
1517

1618
class PermissionApplicator
@@ -147,6 +149,42 @@ public function restrictEntityRelationQuery(Builder $query, string $tableName, s
147149
});
148150
}
149151

152+
/**
153+
* Filter out items that have related entity relations where
154+
* the entity is marked as deleted.
155+
*/
156+
public function filterDeletedFromEntityRelationQuery(Builder $query, string $tableName, string $entityIdColumn, string $entityTypeColumn): Builder
157+
{
158+
$tableDetails = ['tableName' => $tableName, 'entityIdColumn' => $entityIdColumn, 'entityTypeColumn' => $entityTypeColumn];
159+
$entityProvider = new EntityProvider();
160+
161+
$joinQuery = function ($query) use ($entityProvider) {
162+
$first = true;
163+
/** @var Builder $query */
164+
foreach ($entityProvider->all() as $entity) {
165+
$entityQuery = function ($query) use ($entity) {
166+
/** @var Builder $query */
167+
$query->select(['id', 'deleted_at'])
168+
->selectRaw("'{$entity->getMorphClass()}' as type")
169+
->from($entity->getTable())
170+
->whereNotNull('deleted_at');
171+
};
172+
173+
if ($first) {
174+
$entityQuery($query);
175+
$first = false;
176+
} else {
177+
$query->union($entityQuery);
178+
}
179+
}
180+
};
181+
182+
return $query->leftJoinSub($joinQuery, 'deletions', function (JoinClause $join) use ($tableDetails) {
183+
$join->on($tableDetails['tableName'] . '.' . $tableDetails['entityIdColumn'], '=', 'deletions.id')
184+
->on($tableDetails['tableName'] . '.' . $tableDetails['entityTypeColumn'], '=', 'deletions.type');
185+
})->whereNull('deletions.deleted_at');
186+
}
187+
150188
/**
151189
* Add conditions to a query for a model that's a relation of a page, so only the model results
152190
* on visible pages are returned by the query.

app/Users/Controllers/UserPreferencesController.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
namespace BookStack\Users\Controllers;
44

5-
use BookStack\Activity\Models\Watch;
65
use BookStack\Http\Controller;
76
use BookStack\Permissions\PermissionApplicator;
87
use BookStack\Settings\UserNotificationPreferences;
@@ -68,8 +67,9 @@ public function showNotifications(PermissionApplicator $permissions)
6867

6968
$preferences = (new UserNotificationPreferences(user()));
7069

71-
$query = Watch::query()->where('user_id', '=', user()->id);
70+
$query = user()->watches()->getQuery();
7271
$query = $permissions->restrictEntityRelationQuery($query, 'watches', 'watchable_id', 'watchable_type');
72+
$query = $permissions->filterDeletedFromEntityRelationQuery($query, 'watches', 'watchable_id', 'watchable_type');
7373
$watches = $query->with('watchable')->paginate(20);
7474

7575
$this->setPageTitle(trans('preferences.notifications'));

app/Users/Models/User.php

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
use BookStack\Access\SocialAccount;
77
use BookStack\Activity\Models\Favourite;
88
use BookStack\Activity\Models\Loggable;
9+
use BookStack\Activity\Models\Watch;
910
use BookStack\Api\ApiToken;
1011
use BookStack\App\Model;
1112
use BookStack\App\Sluggable;
@@ -291,6 +292,14 @@ public function mfaValues(): HasMany
291292
return $this->hasMany(MfaValue::class);
292293
}
293294

295+
/**
296+
* Get the tracked entity watches for this user.
297+
*/
298+
public function watches(): HasMany
299+
{
300+
return $this->hasMany(Watch::class);
301+
}
302+
294303
/**
295304
* Get the last activity time for this user.
296305
*/

app/Users/UserRepo.php

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,18 +18,13 @@
1818

1919
class UserRepo
2020
{
21-
protected UserAvatars $userAvatar;
22-
protected UserInviteService $inviteService;
23-
24-
/**
25-
* UserRepo constructor.
26-
*/
27-
public function __construct(UserAvatars $userAvatar, UserInviteService $inviteService)
28-
{
29-
$this->userAvatar = $userAvatar;
30-
$this->inviteService = $inviteService;
21+
public function __construct(
22+
protected UserAvatars $userAvatar,
23+
protected UserInviteService $inviteService
24+
) {
3125
}
3226

27+
3328
/**
3429
* Get a user by their email address.
3530
*/
@@ -155,6 +150,7 @@ public function destroy(User $user, ?int $newOwnerId = null)
155150
$user->apiTokens()->delete();
156151
$user->favourites()->delete();
157152
$user->mfaValues()->delete();
153+
$user->watches()->delete();
158154
$user->delete();
159155

160156
// Delete user profile images

tests/Activity/WatchTest.php

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use BookStack\Activity\Tools\UserEntityWatchOptions;
1313
use BookStack\Activity\WatchLevels;
1414
use BookStack\Entities\Models\Entity;
15+
use BookStack\Entities\Tools\TrashCan;
1516
use BookStack\Settings\UserNotificationPreferences;
1617
use Illuminate\Support\Facades\Notification;
1718
use Tests\TestCase;
@@ -370,4 +371,32 @@ public function test_notifications_not_sent_if_lacking_view_permission_for_relat
370371

371372
$notifications->assertNothingSentTo($editor);
372373
}
374+
375+
public function test_watches_deleted_on_user_delete()
376+
{
377+
$editor = $this->users->editor();
378+
$page = $this->entities->page();
379+
380+
$watches = new UserEntityWatchOptions($editor, $page);
381+
$watches->updateLevelByValue(WatchLevels::COMMENTS);
382+
$this->assertDatabaseHas('watches', ['user_id' => $editor->id]);
383+
384+
$this->asAdmin()->delete($editor->getEditUrl());
385+
386+
$this->assertDatabaseMissing('watches', ['user_id' => $editor->id]);
387+
}
388+
389+
public function test_watches_deleted_on_item_delete()
390+
{
391+
$editor = $this->users->editor();
392+
$page = $this->entities->page();
393+
394+
$watches = new UserEntityWatchOptions($editor, $page);
395+
$watches->updateLevelByValue(WatchLevels::COMMENTS);
396+
$this->assertDatabaseHas('watches', ['watchable_type' => 'page', 'watchable_id' => $page->id]);
397+
398+
$this->entities->destroy($page);
399+
400+
$this->assertDatabaseMissing('watches', ['watchable_type' => 'page', 'watchable_id' => $page->id]);
401+
}
373402
}

tests/Helpers/EntityProvider.php

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use BookStack\Entities\Repos\BookshelfRepo;
1212
use BookStack\Entities\Repos\ChapterRepo;
1313
use BookStack\Entities\Repos\PageRepo;
14+
use BookStack\Entities\Tools\TrashCan;
1415
use BookStack\Users\Models\User;
1516
use Illuminate\Database\Eloquent\Builder;
1617

@@ -197,6 +198,16 @@ public function newDraftPage(array $input = ['name' => 'test page', 'html' => 'M
197198
return $draftPage;
198199
}
199200

201+
/**
202+
* Fully destroy the given entity from the system, bypassing the recycle bin
203+
* stage. Still runs through main app deletion logic.
204+
*/
205+
public function destroy(Entity $entity)
206+
{
207+
$trash = app()->make(TrashCan::class);
208+
$trash->destroyEntity($entity);
209+
}
210+
200211
/**
201212
* @param Entity|Entity[] $entities
202213
*/

tests/User/UserPreferencesTest.php

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,23 @@ public function test_notification_preferences_show_watches()
124124
$resp->assertDontSee('All Page Updates & Comments');
125125
}
126126

127+
public function test_notification_preferences_dont_error_on_deleted_items()
128+
{
129+
$editor = $this->users->editor();
130+
$book = $this->entities->book();
131+
132+
$options = new UserEntityWatchOptions($editor, $book);
133+
$options->updateLevelByValue(WatchLevels::COMMENTS);
134+
135+
$this->actingAs($editor)->delete($book->getUrl());
136+
$book->refresh();
137+
$this->assertNotNull($book->deleted_at);
138+
139+
$resp = $this->actingAs($editor)->get('/preferences/notifications');
140+
$resp->assertOk();
141+
$resp->assertDontSee($book->name);
142+
}
143+
127144
public function test_notification_preferences_not_accessible_to_guest()
128145
{
129146
$this->setSettings(['app-public' => 'true']);

0 commit comments

Comments
 (0)