From b80dd1f6dbc92fe6ef17e5c7ce65b8a1c382e2f1 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Nov 2023 09:54:04 +0100 Subject: [PATCH] fix(2fa-backupcodes): Don't remember disabled and deleted users over and over again Signed-off-by: Joas Schilling --- .../lib/BackgroundJob/CheckBackupCodes.php | 4 ++ .../BackgroundJob/RememberBackupCodesJob.php | 6 ++ .../BackgroundJob/CheckBackupCodeTest.php | 24 +++++++ .../RememberBackupCodesJobTest.php | 70 +++++++++++++++++-- 4 files changed, 98 insertions(+), 6 deletions(-) diff --git a/apps/twofactor_backupcodes/lib/BackgroundJob/CheckBackupCodes.php b/apps/twofactor_backupcodes/lib/BackgroundJob/CheckBackupCodes.php index b1be49270838e..3e256408415a2 100644 --- 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 index 592d8dab00cbc..a33e38a184758 100644 --- 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 index a14712e26849d..57dbb5674cbcc 100644 --- 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 index d8f9e2836a121..3640cb29187f1 100644 --- 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']]); } }