Merge pull request #9493 from nextcloud/fix/horde-no-password-provided

fix(jobs): Skip background jobs if no authentication is possible
nextcloud · Mar 25, 2024 · 133da74 · 133da74
2 parents 538fd37 + f1d3fda
commit 133da74
Show file tree

Hide file tree

Showing 8 changed files with 151 additions and 43 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -95,54 +95,47 @@ 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');
 
 		$this->job->run(self::$argument);
 	}
 
 	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');
 
 		$this->job->run(self::$argument);
 	}
 
 	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();