From 4129089f87cf0716ea149498d145cbd48bd3efc7 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Tue, 14 Jun 2022 11:01:41 +0200 Subject: Avoid calendar & addressbook activities being created on user being deleted Addressbooks and Calendar data are destroyed through hook OC_User::pre_deleteUser, which when it reaches the backends sends AddressBookDeletedEvent/CalendarDeletedEvent typed events, which in turns generates activities that aren't deleted until they expire. This can probably lead to old activities being visible for a new user created with the same uid. Signed-off-by: Thomas Citharel --- apps/dav/lib/CalDAV/Activity/Backend.php | 12 +++++++- apps/dav/lib/CardDAV/Activity/Backend.php | 13 ++++++++- .../dav/tests/unit/CalDAV/Activity/BackendTest.php | 33 ++++++++++++++++++++-- .../tests/unit/CardDAV/Activity/BackendTest.php | 28 +++++++++++++++++- 4 files changed, 81 insertions(+), 5 deletions(-) diff --git a/apps/dav/lib/CalDAV/Activity/Backend.php b/apps/dav/lib/CalDAV/Activity/Backend.php index af2d790e10d..d66ab50907f 100644 --- a/apps/dav/lib/CalDAV/Activity/Backend.php +++ b/apps/dav/lib/CalDAV/Activity/Backend.php @@ -34,6 +34,7 @@ use OCP\App\IAppManager; use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; +use OCP\IUserManager; use OCP\IUserSession; use Sabre\VObject\Reader; @@ -56,11 +57,15 @@ class Backend { /** @var IAppManager */ protected $appManager; - public function __construct(IActivityManager $activityManager, IGroupManager $groupManager, IUserSession $userSession, IAppManager $appManager) { + /** @var IUserManager */ + protected $userManager; + + public function __construct(IActivityManager $activityManager, IGroupManager $groupManager, IUserSession $userSession, IAppManager $appManager, IUserManager $userManager) { $this->activityManager = $activityManager; $this->groupManager = $groupManager; $this->userSession = $userSession; $this->appManager = $appManager; + $this->userManager = $userManager; } /** @@ -165,6 +170,11 @@ class Backend { } foreach ($users as $user) { + if ($action === Calendar::SUBJECT_DELETE && !$this->userManager->userExists($user)) { + // Avoid creating calendar_delete activities for deleted users + continue; + } + $event->setAffectedUser($user) ->setSubject( $user === $currentUser ? $action . '_self' : $action, diff --git a/apps/dav/lib/CardDAV/Activity/Backend.php b/apps/dav/lib/CardDAV/Activity/Backend.php index 184b3f0cb10..8c8c643e287 100644 --- a/apps/dav/lib/CardDAV/Activity/Backend.php +++ b/apps/dav/lib/CardDAV/Activity/Backend.php @@ -32,6 +32,7 @@ use OCP\App\IAppManager; use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; +use OCP\IUserManager; use OCP\IUserSession; use Sabre\CardDAV\Plugin; use Sabre\VObject\Reader; @@ -50,14 +51,19 @@ class Backend { /** @var IAppManager */ protected $appManager; + /** @var IUserManager */ + protected $userManager; + public function __construct(IActivityManager $activityManager, IGroupManager $groupManager, IUserSession $userSession, - IAppManager $appManager) { + IAppManager $appManager, + IUserManager $userManager) { $this->activityManager = $activityManager; $this->groupManager = $groupManager; $this->userSession = $userSession; $this->appManager = $appManager; + $this->userManager = $userManager; } /** @@ -139,6 +145,11 @@ class Backend { } foreach ($users as $user) { + if ($action === Addressbook::SUBJECT_DELETE && !$this->userManager->userExists($user)) { + // Avoid creating addressbook_delete activities for deleted users + continue; + } + $event->setAffectedUser($user) ->setSubject( $user === $currentUser ? $action . '_self' : $action, diff --git a/apps/dav/tests/unit/CalDAV/Activity/BackendTest.php b/apps/dav/tests/unit/CalDAV/Activity/BackendTest.php index 21cbbd169ff..1ad6da177ca 100644 --- a/apps/dav/tests/unit/CalDAV/Activity/BackendTest.php +++ b/apps/dav/tests/unit/CalDAV/Activity/BackendTest.php @@ -33,6 +33,7 @@ use OCP\App\IAppManager; use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; +use OCP\IUserManager; use OCP\IUserSession; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -51,12 +52,16 @@ class BackendTest extends TestCase { /** @var IAppManager|MockObject */ protected $appManager; + /** @var IUserManager|MockObject */ + protected $userManager; + protected function setUp(): void { parent::setUp(); $this->activityManager = $this->createMock(IManager::class); $this->groupManager = $this->createMock(IGroupManager::class); $this->userSession = $this->createMock(IUserSession::class); $this->appManager = $this->createMock(IAppManager::class); + $this->userManager = $this->createMock(IUserManager::class); } /** @@ -69,7 +74,8 @@ class BackendTest extends TestCase { $this->activityManager, $this->groupManager, $this->userSession, - $this->appManager + $this->appManager, + $this->userManager ); } else { return $this->getMockBuilder(Backend::class) @@ -78,8 +84,9 @@ class BackendTest extends TestCase { $this->groupManager, $this->userSession, $this->appManager, + $this->userManager ]) - ->setMethods($methods) + ->onlyMethods($methods) ->getMock(); } } @@ -252,6 +259,10 @@ class BackendTest extends TestCase { ->with($author) ->willReturnSelf(); + $this->userManager->expects($action === Calendar::SUBJECT_DELETE ? $this->exactly(sizeof($users)) : $this->never()) + ->method('userExists') + ->willReturn(true); + $event->expects($this->exactly(sizeof($users))) ->method('setAffectedUser') ->willReturnSelf(); @@ -269,6 +280,24 @@ class BackendTest extends TestCase { $this->invokePrivate($backend, 'triggerCalendarActivity', [$action, $data, $shares, $changedProperties]); } + public function testUserDeletionDoesNotCreateActivity() { + $backend = $this->getBackend(); + + $this->userManager->expects($this->once()) + ->method('userExists') + ->willReturn(false); + + $this->activityManager->expects($this->never()) + ->method('publish'); + + $this->invokePrivate($backend, 'triggerCalendarActivity', [Calendar::SUBJECT_DELETE, [ + 'principaluri' => 'principal/user/admin', + 'id' => 42, + 'uri' => 'this-uri', + '{DAV:}displayname' => 'Name of calendar', + ], [], []]); + } + public function dataGetUsersForShares() { return [ [ diff --git a/apps/dav/tests/unit/CardDAV/Activity/BackendTest.php b/apps/dav/tests/unit/CardDAV/Activity/BackendTest.php index bd88294ce81..dbc2c3550e7 100644 --- a/apps/dav/tests/unit/CardDAV/Activity/BackendTest.php +++ b/apps/dav/tests/unit/CardDAV/Activity/BackendTest.php @@ -34,6 +34,7 @@ use OCP\App\IAppManager; use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; +use OCP\IUserManager; use OCP\IUserSession; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -58,6 +59,7 @@ class BackendTest extends TestCase { $this->groupManager = $this->createMock(IGroupManager::class); $this->userSession = $this->createMock(IUserSession::class); $this->appManager = $this->createMock(IAppManager::class); + $this->userManager = $this->createMock(IUserManager::class); } /** @@ -70,7 +72,8 @@ class BackendTest extends TestCase { $this->activityManager, $this->groupManager, $this->userSession, - $this->appManager + $this->appManager, + $this->userManager ); } else { return $this->getMockBuilder(Backend::class) @@ -79,6 +82,7 @@ class BackendTest extends TestCase { $this->groupManager, $this->userSession, $this->appManager, + $this->userManager ]) ->onlyMethods($methods) ->getMock(); @@ -229,6 +233,10 @@ class BackendTest extends TestCase { ->with($author) ->willReturnSelf(); + $this->userManager->expects($action === Addressbook::SUBJECT_DELETE ? $this->exactly(sizeof($users)) : $this->never()) + ->method('userExists') + ->willReturn(true); + $event->expects($this->exactly(sizeof($users))) ->method('setAffectedUser') ->willReturnSelf(); @@ -253,6 +261,24 @@ class BackendTest extends TestCase { $this->assertEmpty($this->invokePrivate($backend, 'triggerAddressbookActivity', [Addressbook::SUBJECT_ADD, ['principaluri' => 'principals/system/system'], [], [], '', '', null, []])); } + public function testUserDeletionDoesNotCreateActivity() { + $backend = $this->getBackend(); + + $this->userManager->expects($this->once()) + ->method('userExists') + ->willReturn(false); + + $this->activityManager->expects($this->never()) + ->method('publish'); + + $this->invokePrivate($backend, 'triggerAddressbookActivity', [Addressbook::SUBJECT_DELETE, [ + 'principaluri' => 'principal/user/admin', + 'id' => 42, + 'uri' => 'this-uri', + '{DAV:}displayname' => 'Name of addressbook', + ], [], []]); + } + public function dataTriggerCardActivity(): array { $cardData = "BEGIN:VCARD\r\nVERSION:3.0\r\nPRODID:-//Sabre//Sabre VObject 3.4.8//EN\r\nUID:test-user\r\nFN:test-user\r\nN:test-user;;;;\r\nEND:VCARD\r\n\r\n"; -- cgit v1.2.3