Merge pull request #32869 from nextcloud/dav-no-deleted-user-activities

Avoid calendar & addressbook activities being created on user being deleted

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,

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,

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 [
 			[

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";