Merge pull request #6174 from nextcloud/refactor/imap-client-clean-lo…

…gout-resource Handle IMAP disconnects more explicitly
nextcloud · Apr 4, 2022 · b391116 · b391116
2 parents 9640827 + a23e823
commit b391116
Show file tree

Hide file tree

Showing 18 changed files with 411 additions and 320 deletions.
diff --git a/lib/Command/DiagnoseAccount.php b/lib/Command/DiagnoseAccount.php
@@ -84,7 +84,6 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 			$output->writeln('<error>No IMAP passwort set. The user might have to log into their account to set it.</error>');
 		}
 		$imapClient = $this->clientFactory->getClient($account);
-
 		try {
 			$this->printCapabilitiesStats($output, $imapClient);
 			$this->printMailboxesMessagesStats($output, $imapClient);
@@ -94,6 +93,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 			]);
 			$output->writeln("<error>Horde error occurred: " . $e->getMessage() . ". See nextcloud.log for more details.</error>");
 			return 2;
+		} finally {
+			$imapClient->logout();
 		}
 
 		return 0;

diff --git a/lib/IMAP/IMAPClientFactory.php b/lib/IMAP/IMAPClientFactory.php
@@ -41,8 +41,6 @@ class IMAPClientFactory {
 	/** @var ICacheFactory */
 	private $cacheFactory;
 
-	private $cache = [];
-
 	/**
 	 * @param ICrypto $crypto
 	 * @param IConfig $config
@@ -55,47 +53,49 @@ public function __construct(ICrypto $crypto, IConfig $config, ICacheFactory $cac
 	}
 
 	/**
+	 * Get the connection object for the given account
+	 *
+	 * Connections are not closed until destruction, so the caller site is
+	 * responsible to log out as soon as possible to keep the number of open
+	 * (and stale) connections at a minimum.
+	 *
 	 * @param Account $account
 	 * @return Horde_Imap_Client_Socket
 	 */
 	public function getClient(Account $account): Horde_Imap_Client_Socket {
-		if (!isset($this->cache[$account->getId()])) {
-			$host = $account->getMailAccount()->getInboundHost();
-			$user = $account->getMailAccount()->getInboundUser();
-			$password = $account->getMailAccount()->getInboundPassword();
-			$password = $this->crypto->decrypt($password);
-			$port = $account->getMailAccount()->getInboundPort();
-			$sslMode = $account->getMailAccount()->getInboundSslMode();
-			if ($sslMode === 'none') {
-				$sslMode = false;
-			}
+		$host = $account->getMailAccount()->getInboundHost();
+		$user = $account->getMailAccount()->getInboundUser();
+		$password = $account->getMailAccount()->getInboundPassword();
+		$password = $this->crypto->decrypt($password);
+		$port = $account->getMailAccount()->getInboundPort();
+		$sslMode = $account->getMailAccount()->getInboundSslMode();
+		if ($sslMode === 'none') {
+			$sslMode = false;
+		}
 
-			$params = [
-				'username' => $user,
-				'password' => $password,
-				'hostspec' => $host,
-				'port' => $port,
-				'secure' => $sslMode,
-				'timeout' => (int)$this->config->getSystemValue('app.mail.imap.timeout', 5),
-				'context' => [
-					'ssl' => [
-						'verify_peer' => $this->config->getSystemValueBool('app.mail.verify-tls-peer', true),
-						'verify_peer_name' => $this->config->getSystemValueBool('app.mail.verify-tls-peer', true),
-					],
+		$params = [
+			'username' => $user,
+			'password' => $password,
+			'hostspec' => $host,
+			'port' => $port,
+			'secure' => $sslMode,
+			'timeout' => (int)$this->config->getSystemValue('app.mail.imap.timeout', 5),
+			'context' => [
+				'ssl' => [
+					'verify_peer' => $this->config->getSystemValueBool('app.mail.verify-tls-peer', true),
+					'verify_peer_name' => $this->config->getSystemValueBool('app.mail.verify-tls-peer', true),
 				],
-			];
-			if ($this->cacheFactory->isAvailable()) {
-				$params['cache'] = [
-					'backend' => new Cache([
-						'cacheob' => $this->cacheFactory->createDistributed(md5((string)$account->getId())),
-					])];
-			}
-			if ($this->config->getSystemValue('debug', false)) {
-				$params['debug'] = $this->config->getSystemValue('datadirectory') . '/horde_imap.log';
-			}
-			$this->cache[$account->getId()] = new Horde_Imap_Client_Socket($params);
+			],
+		];
+		if ($this->cacheFactory->isAvailable()) {
+			$params['cache'] = [
+				'backend' => new Cache([
+					'cacheob' => $this->cacheFactory->createDistributed(md5((string)$account->getId())),
+				])];
 		}
-
-		return $this->cache[$account->getId()];
+		if ($this->config->getSystemValue('debug', false)) {
+			$params['debug'] = $this->config->getSystemValue('datadirectory') . '/horde_imap.log';
+		}
+		return new Horde_Imap_Client_Socket($params);
 	}
 }
diff --git a/lib/IMAP/MailboxSync.php b/lib/IMAP/MailboxSync.php
@@ -98,6 +98,8 @@ public function sync(Account $account,
 			);
 		} catch (Horde_Imap_Client_Exception $e) {
 			$logger->debug('Getting namespaces for account ' . $account->getId() . ' failed: ' . $e->getMessage());
+		} finally {
+			$client->logout();
 		}
 
 		try {
@@ -137,7 +139,6 @@ public function sync(Account $account,
 	 */
 	public function syncStats(Account $account, Mailbox $mailbox): void {
 		$client = $this->imapClientFactory->getClient($account);
-
 		try {
 			$stats = $this->folderMapper->getFoldersStatusAsObject($client, $mailbox->getName());
 		} catch (Horde_Imap_Client_Exception $e) {
@@ -147,6 +148,8 @@ public function syncStats(Account $account, Mailbox $mailbox): void {
 				(int)$e->getCode(),
 				$e
 			);
+		} finally {
+			$client->logout();
 		}
 
 		$mailbox->setMessages($stats->getTotal());

diff --git a/lib/IMAP/PreviewEnhancer.php b/lib/IMAP/PreviewEnhancer.php
@@ -81,9 +81,10 @@ public function process(Account $account, Mailbox $mailbox, array $messages): ar
 			return $messages;
 		}
 
+		$client = $this->clientFactory->getClient($account);
 		try {
 			$data = $this->imapMapper->getBodyStructureData(
-				$this->clientFactory->getClient($account),
+				$client,
 				$mailbox->getName(),
 				$needAnalyze
 			);
@@ -94,6 +95,8 @@ public function process(Account $account, Mailbox $mailbox, array $messages): ar
 			]);
 
 			return $messages;
+		} finally {
+			$client->logout();
 		}
 
 		return $this->mapper->updatePreviewDataBulk(...array_map(function (Message $message) use ($data) {

diff --git a/lib/IMAP/Search/Provider.php b/lib/IMAP/Search/Provider.php
@@ -51,14 +51,15 @@ public function findMatches(Account $account,
 								Mailbox $mailbox,
 								SearchQuery $searchQuery): array {
 		$client = $this->clientFactory->getClient($account);
-
 		try {
 			$fetchResult = $client->search(
 				$mailbox->getName(),
 				$this->convertMailQueryToHordeQuery($searchQuery)
 			);
 		} catch (Horde_Imap_Client_Exception $e) {
 			throw new ServiceException('Could not get message IDs: ' . $e->getMessage(), 0, $e);
+		} finally {
+			$client->logout();
 		}
 
 		return $fetchResult['match']->ids;

diff --git a/lib/Listener/DeleteDraftListener.php b/lib/Listener/DeleteDraftListener.php
@@ -90,6 +90,8 @@ private function deleteDraft(Account $account, Message $draft): void {
 		} catch (DoesNotExistException $e) {
 			$this->logger->warning("Account has no draft mailbox set, can't delete the draft");
 			return;
+		} finally {
+			$client->logout();
 		}
 
 		try {

diff --git a/lib/Listener/FlagRepliedMessageListener.php b/lib/Listener/FlagRepliedMessageListener.php
@@ -78,8 +78,8 @@ public function handle(Event $event): void {
 			return;
 		}
 
+		$client = $this->imapClientFactory->getClient($event->getAccount());
 		try {
-			$client = $this->imapClientFactory->getClient($event->getAccount());
 			$this->messageMapper->addFlag(
 				$client,
 				$mailbox,
@@ -90,6 +90,8 @@ public function handle(Event $event): void {
 			$this->logger->warning('Could not flag replied message: ' . $e, [
 				'exception' => $e,
 			]);
+		} finally {
+			$client->logout();
 		}
 	}
 }
diff --git a/lib/Listener/SaveSentMessageListener.php b/lib/Listener/SaveSentMessageListener.php
@@ -83,14 +83,17 @@ public function handle(Event $event): void {
 			return;
 		}
 
+		$client = $this->imapClientFactory->getClient($event->getAccount());
 		try {
 			$this->messageMapper->save(
-				$this->imapClientFactory->getClient($event->getAccount()),
+				$client,
 				$sentMailbox,
 				$event->getMail()
 			);
 		} catch (Horde_Imap_Client_Exception $e) {
 			throw new ServiceException('Could not save sent message on IMAP', 0, $e);
+		} finally {
+			$client->logout();
 		}
 	}
 }
diff --git a/lib/Migration/MigrateImportantFromImapAndDb.php b/lib/Migration/MigrateImportantFromImapAndDb.php
@@ -65,34 +65,42 @@ public function __construct(IMAPClientFactory $clientFactory,
 
 	public function migrateImportantOnImap(Account $account, Mailbox $mailbox): void {
 		$client = $this->clientFactory->getClient($account);
-		//get all messages that have an $important label from IMAP
 		try {
-			$uids = $this->messageMapper->getFlagged($client, $mailbox, '$important');
-		} catch (Horde_Imap_Client_Exception $e) {
-			throw new ServiceException("Could not fetch UIDs of important messages: " . $e->getMessage(), 0, $e);
-		}
-		// add $label1 for all that are tagged on IMAP
-		if (!empty($uids)) {
+			//get all messages that have an $important label from IMAP
 			try {
-				$this->messageMapper->addFlag($client, $mailbox, $uids, Tag::LABEL_IMPORTANT);
+				$uids = $this->messageMapper->getFlagged($client, $mailbox, '$important');
 			} catch (Horde_Imap_Client_Exception $e) {
-				$this->logger->debug('Could not flag messages in mailbox <' . $mailbox->getId() . '>');
-				throw new ServiceException($e->getMessage(), 0, $e);
+				throw new ServiceException("Could not fetch UIDs of important messages: " . $e->getMessage(), 0, $e);
+			}
+			// add $label1 for all that are tagged on IMAP
+			if (!empty($uids)) {
+				try {
+					$this->messageMapper->addFlag($client, $mailbox, $uids, Tag::LABEL_IMPORTANT);
+				} catch (Horde_Imap_Client_Exception $e) {
+					$this->logger->debug('Could not flag messages in mailbox <' . $mailbox->getId() . '>');
+					throw new ServiceException($e->getMessage(), 0, $e);
+				}
 			}
+		} finally {
+			$client->logout();
 		}
 	}
 
 	public function migrateImportantFromDb(Account $account, Mailbox $mailbox): void {
 		$client = $this->clientFactory->getClient($account);
-		$uids = $this->mailboxMapper->findFlaggedImportantUids($mailbox->getId());
-		// store our data on imap
-		if (!empty($uids)) {
-			try {
-				$this->messageMapper->addFlag($client, $mailbox, $uids, Tag::LABEL_IMPORTANT);
-			} catch (Horde_Imap_Client_Exception $e) {
-				$this->logger->debug('Could not flag messages in mailbox <' . $mailbox->getId() . '>');
-				throw new ServiceException($e->getMessage(), 0, $e);
+		try {
+			$uids = $this->mailboxMapper->findFlaggedImportantUids($mailbox->getId());
+			// store our data on imap
+			if (!empty($uids)) {
+				try {
+					$this->messageMapper->addFlag($client, $mailbox, $uids, Tag::LABEL_IMPORTANT);
+				} catch (Horde_Imap_Client_Exception $e) {
+					$this->logger->debug('Could not flag messages in mailbox <' . $mailbox->getId() . '>');
+					throw new ServiceException($e->getMessage(), 0, $e);
+				}
 			}
+		} finally {
+			$client->logout();
 		}
 	}
 }
diff --git a/lib/Service/AutoConfig/ImapConnector.php b/lib/Service/AutoConfig/ImapConnector.php
@@ -85,8 +85,12 @@ public function connect(string $email,
 
 		$a = new Account($account);
 		$client = $this->clientFactory->getClient($a);
-		$client->login();
-		$this->logger->info("Test-Account-Successful: $this->userId, $host, $port, $user, $encryptionProtocol");
-		return $account;
+		try {
+			$client->login();
+			$this->logger->info("Test-Account-Successful: $this->userId, $host, $port, $user, $encryptionProtocol");
+			return $account;
+		} finally {
+			$client->logout();
+		}
 	}
 }
diff --git a/lib/Service/ItineraryService.php b/lib/Service/ItineraryService.php
@@ -81,18 +81,21 @@ public function extract(Account $account, string $mailbox, int $id): Itinerary {
 		}
 
 		$client = $this->clientFactory->getClient($account);
-
-		$itinerary = new Itinerary();
-		$htmlBody = $this->messageMapper->getHtmlBody($client, $mailbox->getName(), $id);
-		if ($htmlBody !== null) {
-			$itinerary = $itinerary->merge(
-				$this->extractor->extract($htmlBody)
-			);
-			$this->logger->debug('Extracted ' . count($itinerary) . ' itinerary entries from the message HTML body');
-		} else {
-			$this->logger->debug('Message does not have an HTML body, can\'t extract itinerary info');
+		try {
+			$itinerary = new Itinerary();
+			$htmlBody = $this->messageMapper->getHtmlBody($client, $mailbox->getName(), $id);
+			if ($htmlBody !== null) {
+				$itinerary = $itinerary->merge(
+					$this->extractor->extract($htmlBody)
+				);
+				$this->logger->debug('Extracted ' . count($itinerary) . ' itinerary entries from the message HTML body');
+			} else {
+				$this->logger->debug('Message does not have an HTML body, can\'t extract itinerary info');
+			}
+			$attachments = $this->messageMapper->getRawAttachments($client, $mailbox->getName(), $id);
+		} finally {
+			$client->logout();
 		}
-		$attachments = $this->messageMapper->getRawAttachments($client, $mailbox->getName(), $id);
 		$itinerary = array_reduce($attachments, function (Itinerary $combined, string $attachment) {
 			$extracted = $this->extractor->extract($attachment);
 			$this->logger->debug('Extracted ' . count($extracted) . ' itinerary entries from an attachment');