Skip to content

Avoid calendar & addressbook activities being created on user being deleted #32869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
blizzz merged 1 commit into from
Jul 22, 2022
Merged

Avoid calendar & addressbook activities being created on user being deleted #32869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Avoid calendar & addressbook activities being created on user being d… 
…eleted

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 <[email protected]>
  • Loading branch information
@tcitworld
tcitworld committed Jun 14, 2022
commit 4129089f87cf0716ea149498d145cbd48bd3efc7
12 changes: 11 additions & 1 deletion apps/dav/lib/CalDAV/Activity/Backend.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
use OCP\IGroup;
use OCP\IGroupManager;
use OCP\IUser;
use OCP\IUserManager;
use OCP\IUserSession;
use Sabre\VObject\Reader;

Expand All @@ -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;
}

/**
Expand Down Expand Up @@ -165,6 +170,11 @@ protected function triggerCalendarActivity($action, array $calendarData, array $
}

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,
Expand Down
13 changes: 12 additions & 1 deletion apps/dav/lib/CardDAV/Activity/Backend.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
use OCP\IGroup;
use OCP\IGroupManager;
use OCP\IUser;
use OCP\IUserManager;
use OCP\IUserSession;
use Sabre\CardDAV\Plugin;
use Sabre\VObject\Reader;
Expand All @@ -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;
}

/**
Expand Down Expand Up @@ -139,6 +145,11 @@ protected function triggerAddressbookActivity(string $action, array $addressbook
}

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,
Expand Down
33 changes: 31 additions & 2 deletions apps/dav/tests/unit/CalDAV/Activity/BackendTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
use OCP\IGroup;
use OCP\IGroupManager;
use OCP\IUser;
use OCP\IUserManager;
use OCP\IUserSession;
use PHPUnit\Framework\MockObject\MockObject;
use Test\TestCase;
Expand All @@ -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);
}

/**
Expand All @@ -69,7 +74,8 @@ protected function getBackend(array $methods = []) {
$this->activityManager,
$this->groupManager,
$this->userSession,
$this->appManager
$this->appManager,
$this->userManager
);
} else {
return $this->getMockBuilder(Backend::class)
Expand All @@ -78,8 +84,9 @@ protected function getBackend(array $methods = []) {
$this->groupManager,
$this->userSession,
$this->appManager,
$this->userManager
])
->setMethods($methods)
->onlyMethods($methods)
->getMock();
}
}
Expand Down Expand Up @@ -252,6 +259,10 @@ public function testTriggerCalendarActivity($action, array $data, array $shares,
->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();
Expand All @@ -269,6 +280,24 @@ public function testTriggerCalendarActivity($action, array $data, array $shares,
$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 [
[
Expand Down
28 changes: 27 additions & 1 deletion apps/dav/tests/unit/CardDAV/Activity/BackendTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
use OCP\IGroup;
use OCP\IGroupManager;
use OCP\IUser;
use OCP\IUserManager;
use OCP\IUserSession;
use PHPUnit\Framework\MockObject\MockObject;
use Test\TestCase;
Expand All @@ -58,6 +59,7 @@ protected function setUp(): void {
$this->groupManager = $this->createMock(IGroupManager::class);
$this->userSession = $this->createMock(IUserSession::class);
$this->appManager = $this->createMock(IAppManager::class);
$this->userManager = $this->createMock(IUserManager::class);
}

/**
Expand All @@ -70,7 +72,8 @@ protected function getBackend(array $methods = []) {
$this->activityManager,
$this->groupManager,
$this->userSession,
$this->appManager
$this->appManager,
$this->userManager
);
} else {
return $this->getMockBuilder(Backend::class)
Expand All @@ -79,6 +82,7 @@ protected function getBackend(array $methods = []) {
$this->groupManager,
$this->userSession,
$this->appManager,
$this->userManager
])
->onlyMethods($methods)
->getMock();
Expand Down Expand Up @@ -229,6 +233,10 @@ public function testTriggerAddressBookActivity(string $action, array $data, arra
->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();
Expand All @@ -253,6 +261,24 @@ public function testNoAddressbookActivityCreatedForSystemAddressbook(): void {
$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";

Expand Down