Merge pull request #41569 from nextcloud/bugfix/noid/dont-notify-disa…

…bled-users-again fix(2fa-backupcodes): Don't remember disabled and deleted users over …
nextcloud · Nov 18, 2023 · be69ea9 · be69ea9
2 parents e38ebeb + b80dd1f
commit be69ea9
Show file tree

Hide file tree

Showing 4 changed files with 98 additions and 6 deletions.
diff --git a/apps/twofactor_backupcodes/lib/BackgroundJob/CheckBackupCodes.php b/apps/twofactor_backupcodes/lib/BackgroundJob/CheckBackupCodes.php
@@ -58,6 +58,10 @@ public function __construct(ITimeFactory $timeFactory, IUserManager $userManager
 
 	protected function run($argument) {
 		$this->userManager->callForSeenUsers(function (IUser $user) {
+			if (!$user->isEnabled()) {
+				return;
+			}
+
 			$providers = $this->registry->getProviderStates($user);
 			$isTwoFactorAuthenticated = $this->twofactorManager->isTwoFactorAuthenticated($user);
 

diff --git a/apps/twofactor_backupcodes/lib/BackgroundJob/RememberBackupCodesJob.php b/apps/twofactor_backupcodes/lib/BackgroundJob/RememberBackupCodesJob.php
@@ -70,6 +70,7 @@ protected function run($argument) {
 
 		if ($user === null) {
 			// We can't run with an invalid user
+			$this->jobList->remove(self::class, $argument);
 			return;
 		}
 
@@ -98,6 +99,11 @@ protected function run($argument) {
 			->setSubject('create_backupcodes');
 		$this->notificationManager->markProcessed($notification);
 
+		if (!$user->isEnabled()) {
+			// Don't recreate a notification for a user that can not read it
+			$this->jobList->remove(self::class, $argument);
+			return;
+		}
 		$notification->setDateTime($date);
 		$this->notificationManager->notify($notification);
 	}

diff --git a/apps/twofactor_backupcodes/tests/Unit/BackgroundJob/CheckBackupCodeTest.php b/apps/twofactor_backupcodes/tests/Unit/BackgroundJob/CheckBackupCodeTest.php
@@ -82,6 +82,9 @@ protected function setUp(): void {
 	}
 
 	public function testRunAlreadyGenerated() {
+		$this->user->method('isEnabled')
+			->willReturn(true);
+
 		$this->registry->method('getProviderStates')
 			->with($this->user)
 			->willReturn(['backup_codes' => true]);
@@ -97,6 +100,8 @@ public function testRunAlreadyGenerated() {
 	public function testRun() {
 		$this->user->method('getUID')
 			->willReturn('myUID');
+		$this->user->method('isEnabled')
+			->willReturn(true);
 
 		$this->registry->expects($this->once())
 			->method('getProviderStates')
@@ -117,7 +122,26 @@ public function testRun() {
 		$this->invokePrivate($this->checkBackupCodes, 'run', [[]]);
 	}
 
+	public function testRunDisabledUser() {
+		$this->user->method('getUID')
+			->willReturn('myUID');
+		$this->user->method('isEnabled')
+			->willReturn(false);
+
+		$this->registry->expects($this->never())
+			->method('getProviderStates')
+			->with($this->user);
+
+		$this->jobList->expects($this->never())
+			->method('add');
+
+		$this->invokePrivate($this->checkBackupCodes, 'run', [[]]);
+	}
+
 	public function testRunNoProviders() {
+		$this->user->method('isEnabled')
+			->willReturn(true);
+
 		$this->registry->expects($this->once())
 			->method('getProviderStates')
 			->with($this->user)

diff --git a/apps/twofactor_backupcodes/tests/Unit/BackgroundJob/RememberBackupCodesJobTest.php b/apps/twofactor_backupcodes/tests/Unit/BackgroundJob/RememberBackupCodesJobTest.php
@@ -33,6 +33,7 @@
 use OCP\IUserManager;
 use OCP\Notification\IManager;
 use OCP\Notification\INotification;
+use OCP\Server;
 use Test\TestCase;
 
 class RememberBackupCodesJobTest extends TestCase {
@@ -82,16 +83,25 @@ public function testInvalidUID() {
 
 		$this->notificationManager->expects($this->never())
 			->method($this->anything());
+		$this->jobList->expects($this->once())
+			->method('remove')
+			->with(
+				RememberBackupCodesJob::class,
+				['uid' => 'invalidUID']
+			);
 		$this->jobList->expects($this->never())
-			->method($this->anything());
+			->method('add');
 
-		$this->invokePrivate($this->job, 'run', [['uid' => 'invalidUID']]);
+		self::invokePrivate($this->job, 'run', [['uid' => 'invalidUID']]);
 	}
 
 	public function testBackupCodesGenerated() {
 		$user = $this->createMock(IUser::class);
 		$user->method('getUID')
 			->willReturn('validUID');
+		$user->method('isEnabled')
+			->willReturn(true);
+
 		$this->userManager->method('get')
 			->with('validUID')
 			->willReturn($user);
@@ -112,7 +122,7 @@ public function testBackupCodesGenerated() {
 		$this->notificationManager->expects($this->never())
 			->method($this->anything());
 
-		$this->invokePrivate($this->job, 'run', [['uid' => 'validUID']]);
+		self::invokePrivate($this->job, 'run', [['uid' => 'validUID']]);
 	}
 
 	public function testNoActiveProvider() {
@@ -140,13 +150,15 @@ public function testNoActiveProvider() {
 		$this->notificationManager->expects($this->never())
 			->method($this->anything());
 
-		$this->invokePrivate($this->job, 'run', [['uid' => 'validUID']]);
+		self::invokePrivate($this->job, 'run', [['uid' => 'validUID']]);
 	}
 
 	public function testNotificationSend() {
 		$user = $this->createMock(IUser::class);
 		$user->method('getUID')
 			->willReturn('validUID');
+		$user->method('isEnabled')
+			->willReturn(true);
 		$this->userManager->method('get')
 			->with('validUID')
 			->willReturn($user);
@@ -165,7 +177,7 @@ public function testNotificationSend() {
 		$date->setTimestamp($this->time->getTime());
 
 		$this->notificationManager->method('createNotification')
-			->willReturn(\OC::$server->query(IManager::class)->createNotification());
+			->willReturn(Server::get(IManager::class)->createNotification());
 
 		$this->notificationManager->expects($this->once())
 			->method('notify')
@@ -178,6 +190,52 @@ public function testNotificationSend() {
 					$n->getSubject() === 'create_backupcodes';
 			}));
 
-		$this->invokePrivate($this->job, 'run', [['uid' => 'validUID']]);
+		self::invokePrivate($this->job, 'run', [['uid' => 'validUID']]);
+	}
+
+	public function testNotificationNotSendForDisabledUser() {
+		$user = $this->createMock(IUser::class);
+		$user->method('getUID')
+			->willReturn('validUID');
+		$user->method('isEnabled')
+			->willReturn(false);
+		$this->userManager->method('get')
+			->with('validUID')
+			->willReturn($user);
+
+		$this->registry->method('getProviderStates')
+			->with($user)
+			->willReturn([
+				'backup_codes' => false,
+				'foo' => true,
+			]);
+
+		$this->jobList->expects($this->once())
+			->method('remove')
+			->with(
+				RememberBackupCodesJob::class,
+				['uid' => 'validUID']
+			);
+
+		$date = new \DateTime();
+		$date->setTimestamp($this->time->getTime());
+
+		$this->notificationManager->method('createNotification')
+			->willReturn(Server::get(IManager::class)->createNotification());
+
+		$this->notificationManager->expects($this->once())
+			->method('markProcessed')
+			->with($this->callback(function (INotification $n) {
+				return $n->getApp() === 'twofactor_backupcodes' &&
+					$n->getUser() === 'validUID' &&
+					$n->getObjectType() === 'create' &&
+					$n->getObjectId() === 'codes' &&
+					$n->getSubject() === 'create_backupcodes';
+			}));
+
+		$this->notificationManager->expects($this->never())
+			->method('notify');
+
+		self::invokePrivate($this->job, 'run', [['uid' => 'validUID']]);
 	}
 }