From f1d3fda3015c1ace393124a25cf588e37c879981 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Fri, 22 Mar 2024 15:20:06 +0100 Subject: [PATCH] fix(jobs): Skip background jobs if no authentication is possible Signed-off-by: Anna Larch --- .../PreviewEnhancementProcessingJob.php | 11 ++- lib/BackgroundJob/QuotaJob.php | 5 ++ lib/BackgroundJob/SyncJob.php | 11 ++- .../TrainImportanceClassifierJob.php | 6 +- lib/Db/MailAccount.php | 4 + tests/Unit/BackgroundJob/QuotaJobTest.php | 78 +++++++++++++++++-- tests/Unit/BackgroundJob/SyncJobTest.php | 42 ++++++++++ .../PreviewEnhancementProcessingJobTest.php | 37 ++++----- 8 files changed, 151 insertions(+), 43 deletions(-) diff --git a/lib/BackgroundJob/PreviewEnhancementProcessingJob.php b/lib/BackgroundJob/PreviewEnhancementProcessingJob.php index 26968a25da..7d506196dd 100644 --- a/lib/BackgroundJob/PreviewEnhancementProcessingJob.php +++ b/lib/BackgroundJob/PreviewEnhancementProcessingJob.php @@ -74,6 +74,11 @@ public function run($argument) { return; } + if (!$account->getMailAccount()->canAuthenticateImap()) { + $this->logger->info('Ignoring preprocessing job for provisioned account as athentication on IMAP not possible'); + return; + } + $user = $this->userManager->get($account->getUserId()); if ($user === null || !$user->isEnabled()) { $this->logger->debug(sprintf( @@ -84,12 +89,6 @@ public function run($argument) { return; } - $dbAccount = $account->getMailAccount(); - if (!is_null($dbAccount->getProvisioningId()) && $dbAccount->getInboundPassword() === null) { - $this->logger->info("Ignoring preprocessing job for provisioned account that has no password set yet"); - return; - } - $limitTimestamp = $this->time->getTime() - (60 * 60 * 24 * 14); // Two weeks into the past $this->preprocessingService->process($limitTimestamp, $account); } diff --git a/lib/BackgroundJob/QuotaJob.php b/lib/BackgroundJob/QuotaJob.php index 3fdd2c0914..05f394ea99 100644 --- a/lib/BackgroundJob/QuotaJob.php +++ b/lib/BackgroundJob/QuotaJob.php @@ -79,6 +79,11 @@ protected function run($argument): void { return; } + if(!$account->getMailAccount()->canAuthenticateImap()) { + $this->logger->debug('No authentication on IMAP possible, skipping quota job'); + return; + } + $user = $this->userManager->get($account->getUserId()); if ($user === null || !$user->isEnabled()) { $this->logger->debug(sprintf( diff --git a/lib/BackgroundJob/SyncJob.php b/lib/BackgroundJob/SyncJob.php index f3925bf6c2..d182d3d891 100644 --- a/lib/BackgroundJob/SyncJob.php +++ b/lib/BackgroundJob/SyncJob.php @@ -90,6 +90,11 @@ protected function run($argument) { return; } + if(!$account->getMailAccount()->canAuthenticateImap()) { + $this->logger->debug('No authentication on IMAP possible, skipping background sync job'); + return; + } + $user = $this->userManager->get($account->getUserId()); if ($user === null || !$user->isEnabled()) { $this->logger->debug(sprintf( @@ -100,12 +105,6 @@ protected function run($argument) { return; } - $dbAccount = $account->getMailAccount(); - if (!is_null($dbAccount->getProvisioningId()) && $dbAccount->getInboundPassword() === null) { - $this->logger->info("Ignoring cron sync for provisioned account that has no password set yet"); - return; - } - try { $this->mailboxSync->sync($account, $this->logger, true); $this->syncService->syncAccount($account, $this->logger); diff --git a/lib/BackgroundJob/TrainImportanceClassifierJob.php b/lib/BackgroundJob/TrainImportanceClassifierJob.php index a8023755eb..99396f9019 100644 --- a/lib/BackgroundJob/TrainImportanceClassifierJob.php +++ b/lib/BackgroundJob/TrainImportanceClassifierJob.php @@ -74,11 +74,11 @@ protected function run($argument) { return; } - $dbAccount = $account->getMailAccount(); - if (!is_null($dbAccount->getProvisioningId()) && $dbAccount->getInboundPassword() === null) { - $this->logger->info("Ignoring cron training for provisioned account that has no password set yet"); + if(!$account->getMailAccount()->canAuthenticateImap()) { + $this->logger->debug('Cron importance classifier training not possible: no authentication on IMAP possible'); return; } + if ($this->preferences->getPreference($account->getUserId(), 'tag-classified-messages') === 'false') { $this->logger->debug("classification is turned off for account $accountId"); return; diff --git a/lib/Db/MailAccount.php b/lib/Db/MailAccount.php index be011943a5..fba82946d0 100644 --- a/lib/Db/MailAccount.php +++ b/lib/Db/MailAccount.php @@ -290,6 +290,10 @@ public function setOutOfOfficeFollowsSystem(bool $outOfOfficeFollowsSystem): voi $this->setOooFollowsSystem($outOfOfficeFollowsSystem); } + public function canAuthenticateImap(): bool { + return isset($this->inboundPassword) || isset($this->oauthAccessToken); + } + /** * @return array */ diff --git a/tests/Unit/BackgroundJob/QuotaJobTest.php b/tests/Unit/BackgroundJob/QuotaJobTest.php index f2d954b694..e1bd112d99 100644 --- a/tests/Unit/BackgroundJob/QuotaJobTest.php +++ b/tests/Unit/BackgroundJob/QuotaJobTest.php @@ -91,9 +91,13 @@ public function testAccountDoesntExist(): void { } public function testUserDoesntExist(): void { + $mailAccount = $this->createConfiguredMock(MailAccount::class, [ + 'canAuthenticateImap' => true, + ]); $account = $this->createMock(Account::class); $account->method('getId')->willReturn(123); $account->method('getUserId')->willReturn('user123'); + $account->method('getMailAccount')->willReturn($mailAccount); $this->serviceMock->getParameter('accountService') ->expects(self::once()) ->method('findById') @@ -121,11 +125,67 @@ public function testUserDoesntExist(): void { ); } + public function testQuotaNoAuthentication(): void { + $oldQuota = 10; + $newQuota = 20; + $quotaDTO = new Quota(20, 100); + $mailAccount = $this->createConfiguredMock(MailAccount::class, [ + 'canAuthenticateImap' => false, + ]); + $account = $this->createConfiguredMock(Account::class, [ + 'getId' => 123, + 'getUserId' => 'user123', + 'getMailAccount' => $mailAccount, + ]); + $user = $this->createConfiguredMock(IUser::class, [ + 'isEnabled' => true, + ]); + + $this->serviceMock->getParameter('accountService') + ->expects(self::once()) + ->method('findById') + ->with(123) + ->willReturn($account); + $account->expects(self::once()) + ->method('getMailAccount') + ->willReturn($mailAccount); + $mailAccount->expects(self::once()) + ->method('canAuthenticateImap'); + $this->serviceMock->getParameter('userManager') + ->expects(self::never()) + ->method('get'); + $this->serviceMock->getParameter('logger') + ->expects(self::once()) + ->method('debug'); + $this->serviceMock->getParameter('mailManager') + ->expects(self::never()) + ->method('getQuota'); + $account->expects(self::never()) + ->method('calculateAndSetQuotaPercentage') + ->with($quotaDTO); + $account->expects(self::never()) + ->method('getQuotaPercentage') + ->willReturn($newQuota); + $this->serviceMock->getParameter('accountService') + ->expects(self::never()) + ->method('update') + ->with($mailAccount); + + $this->job->setArgument([ + 'accountId' => 123, + ]); + $this->job->start( + $this->createMock(JobList::class), + ); + } + public function testQuotaTooLow(): void { $oldQuota = 10; $newQuota = 20; $quotaDTO = new Quota(20, 100); - $mailAccount = $this->createMock(MailAccount::class); + $mailAccount = $this->createConfiguredMock(MailAccount::class, [ + 'canAuthenticateImap' => true, + ]); $account = $this->createConfiguredMock(Account::class, [ 'getId' => 123, 'getUserId' => 'user123', @@ -140,6 +200,8 @@ public function testQuotaTooLow(): void { ->method('findById') ->with(123) ->willReturn($account); + $mailAccount->expects(self::once()) + ->method('canAuthenticateImap'); $this->serviceMock->getParameter('userManager') ->expects(self::once()) ->method('get') @@ -155,7 +217,7 @@ public function testQuotaTooLow(): void { $account->expects(self::once()) ->method('calculateAndSetQuotaPercentage') ->with($quotaDTO); - $account->expects(self::exactly(2)) + $account->expects(self::exactly(3)) ->method('getMailAccount') ->willReturn($mailAccount); $account->expects(self::once()) @@ -178,7 +240,9 @@ public function testQuotaWithNotification(): void { $oldQuota = 85; $newQuota = 95; $quotaDTO = new Quota(95, 100); - $mailAccount = $this->createMock(MailAccount::class); + $mailAccount = $this->createConfiguredMock(MailAccount::class, [ + 'canAuthenticateImap' => true, + ]); $account = $this->createConfiguredMock(Account::class, [ 'getId' => 123, 'getUserId' => 'user123', @@ -207,7 +271,7 @@ public function testQuotaWithNotification(): void { $account->expects(self::once()) ->method('calculateAndSetQuotaPercentage') ->with($quotaDTO); - $account->expects(self::exactly(2)) + $account->expects(self::exactly(3)) ->method('getMailAccount') ->willReturn($mailAccount); $account->expects(self::once()) @@ -282,7 +346,9 @@ public function testQuotaZero(): void { $oldQuota = 0; $newQuota = 0; $quotaDTO = new Quota(0, 0); - $mailAccount = $this->createMock(MailAccount::class); + $mailAccount = $this->createConfiguredMock(MailAccount::class, [ + 'canAuthenticateImap' => true, + ]); $account = $this->createConfiguredMock(Account::class, [ 'getId' => 123, 'getUserId' => 'user123', @@ -312,7 +378,7 @@ public function testQuotaZero(): void { $account->expects(self::once()) ->method('calculateAndSetQuotaPercentage') ->with($quotaDTO); - $account->expects(self::exactly(2)) + $account->expects(self::exactly(3)) ->method('getMailAccount') ->willReturn($mailAccount); $account->expects(self::once()) diff --git a/tests/Unit/BackgroundJob/SyncJobTest.php b/tests/Unit/BackgroundJob/SyncJobTest.php index 6acf525b7e..958e74f54c 100644 --- a/tests/Unit/BackgroundJob/SyncJobTest.php +++ b/tests/Unit/BackgroundJob/SyncJobTest.php @@ -30,6 +30,7 @@ use OC\BackgroundJob\JobList; use OCA\Mail\Account; use OCA\Mail\BackgroundJob\SyncJob; +use OCA\Mail\Db\MailAccount; use OCP\AppFramework\Db\DoesNotExistException; use OCP\ILogger; use OCP\IUser; @@ -90,10 +91,51 @@ public function testAccountDoesntExist(): void { ); } + public function testNoAuthentication(): void { + $mailAccount = $this->createConfiguredMock(MailAccount::class, [ + 'canAuthenticateImap' => false, + ]); + $account = $this->createMock(Account::class); + $account->method('getId')->willReturn(123); + $account->method('getUserId')->willReturn('user123'); + $account->method('getMailAccount')->willReturn($mailAccount); + + $this->serviceMock->getParameter('accountService') + ->expects(self::once()) + ->method('findById') + ->with(123) + ->willReturn($account); + $this->serviceMock->getParameter('logger') + ->expects(self::once()) + ->method('debug') + ->with('No authentication on IMAP possible, skipping background sync job'); + $this->serviceMock->getParameter('userManager') + ->expects(self::never()) + ->method('get'); + $this->serviceMock->getParameter('mailboxSync') + ->expects(self::never()) + ->method('sync'); + $this->serviceMock->getParameter('syncService') + ->expects(self::never()) + ->method('syncAccount'); + + $this->job->setArgument([ + 'accountId' => 123, + ]); + $this->job->execute( + $this->createMock(JobList::class), + $this->createMock(ILogger::class) + ); + } + public function testUserDoesntExist(): void { + $mailAccount = $this->createConfiguredMock(MailAccount::class, [ + 'canAuthenticateImap' => true, + ]); $account = $this->createMock(Account::class); $account->method('getId')->willReturn(123); $account->method('getUserId')->willReturn('user123'); + $account->method('getMailAccount')->willReturn($mailAccount); $this->serviceMock->getParameter('accountService') ->expects(self::once()) ->method('findById') diff --git a/tests/Unit/Job/PreviewEnhancementProcessingJobTest.php b/tests/Unit/Job/PreviewEnhancementProcessingJobTest.php index 62768c3104..2d32759af9 100644 --- a/tests/Unit/Job/PreviewEnhancementProcessingJobTest.php +++ b/tests/Unit/Job/PreviewEnhancementProcessingJobTest.php @@ -95,18 +95,18 @@ public function testNoAccount(): void { } public function testNoUser(): void { - $mailAccount = new MailAccount(); - $mailAccount->setUserId(1); - $account = new Account($mailAccount); + $mailAccount = $this->createMock(MailAccount::class); + $mailAccount->method('canAuthenticateImap')->willReturn(true); + $account = $this->createMock(Account::class); + $account->method('getUserId')->willReturn('user123'); + $account->method('getMailAccount')->willReturn($mailAccount); $this->accountService->expects(self::once()) ->method('findById') ->with(self::$argument['accountId']) ->willReturn($account); $this->manager->expects(self::once()) - ->method('get') - ->with($account->getUserId()) - ->willReturn(null); + ->method('get'); $this->logger->expects(self::once()) ->method('debug'); @@ -114,25 +114,16 @@ public function testNoUser(): void { } public function testProvisionedNoPassword(): void { - $mailAccount = new MailAccount(); - $mailAccount->setUserId(1); - $mailAccount->setProvisioningId(1); - $mailAccount->setInboundPassword(null); + $mailAccount = $this->createMock(MailAccount::class); + $mailAccount->method('canAuthenticateImap')->willReturn(false); $account = new Account($mailAccount); - $user = $this->createMock(IUser::class); - $user->setEnabled(); $this->accountService->expects(self::once()) ->method('findById') ->with(self::$argument['accountId']) ->willReturn($account); - $this->manager->expects(self::once()) - ->method('get') - ->with($account->getUserId()) - ->willReturn($user); - $user->expects(self::once()) - ->method('isEnabled') - ->willReturn(true); + $this->manager->expects(self::never()) + ->method('get'); $this->logger->expects(self::once()) ->method('info'); @@ -140,9 +131,11 @@ public function testProvisionedNoPassword(): void { } public function testProcessing(): void { - $mailAccount = new MailAccount(); - $mailAccount->setUserId(1); - $account = new Account($mailAccount); + $mailAccount = $this->createMock(MailAccount::class); + $mailAccount->method('canAuthenticateImap')->willReturn(true); + $account = $this->createMock(Account::class); + $account->method('getUserId')->willReturn('user123'); + $account->method('getMailAccount')->willReturn($mailAccount); $time = time(); $user = $this->createMock(IUser::class); $user->setEnabled();