diff --git a/lib/Command/DiagnoseAccount.php b/lib/Command/DiagnoseAccount.php index 94e156b619..e661df9768 100644 --- a/lib/Command/DiagnoseAccount.php +++ b/lib/Command/DiagnoseAccount.php @@ -84,7 +84,6 @@ protected function execute(InputInterface $input, OutputInterface $output): int $output->writeln('No IMAP passwort set. The user might have to log into their account to set it.'); } $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("Horde error occurred: " . $e->getMessage() . ". See nextcloud.log for more details."); return 2; + } finally { + $imapClient->logout(); } return 0; diff --git a/lib/IMAP/IMAPClientFactory.php b/lib/IMAP/IMAPClientFactory.php index 6ef1c7b335..7d53fe13aa 100644 --- 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 index dc61307be8..7960c7a632 100644 --- 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 index 1f20bdf32e..c07d65d76c 100644 --- 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 index 8f168f2115..ab403698a7 100644 --- a/lib/IMAP/Search/Provider.php +++ b/lib/IMAP/Search/Provider.php @@ -51,7 +51,6 @@ public function findMatches(Account $account, Mailbox $mailbox, SearchQuery $searchQuery): array { $client = $this->clientFactory->getClient($account); - try { $fetchResult = $client->search( $mailbox->getName(), @@ -59,6 +58,8 @@ public function findMatches(Account $account, ); } 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 index 8d89d229b4..61ffe386bf 100644 --- 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 index 977b95fafa..367ec1f7ea 100644 --- 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 index 37eedd93c9..307cc939a2 100644 --- 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 index 17f13e912c..f6e693d5fc 100644 --- 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 index 81478427f1..6407f338f0 100644 --- 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 index a4c0ee7bd9..3f28f106ea 100644 --- 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'); diff --git a/lib/Service/MailManager.php b/lib/Service/MailManager.php index 6ad1244875..1776cf6701 100644 --- a/lib/Service/MailManager.php +++ b/lib/Service/MailManager.php @@ -150,9 +150,8 @@ public function getMailboxes(Account $account): array { */ public function createMailbox(Account $account, string $name): Mailbox { $client = $this->imapClientFactory->getClient($account); - - $folder = $this->folderMapper->createFolder($client, $account, $name); try { + $folder = $this->folderMapper->createFolder($client, $account, $name); $this->folderMapper->getFoldersStatus([$folder], $client); } catch (Horde_Imap_Client_Exception $e) { throw new ServiceException( @@ -161,6 +160,8 @@ public function createMailbox(Account $account, string $name): Mailbox { (int)$e->getCode(), $e ); + } finally { + $client->logout(); } $this->folderMapper->detectFolderSpecialUse([$folder]); @@ -174,7 +175,6 @@ public function getImapMessage(Account $account, int $uid, bool $loadBody = false): IMAPMessage { $client = $this->imapClientFactory->getClient($account); - try { return $this->imapMessageMapper->find( $client, @@ -188,6 +188,8 @@ public function getImapMessage(Account $account, (int)$e->getCode(), $e ); + } finally { + $client->logout(); } } @@ -215,7 +217,6 @@ public function getMessage(string $uid, int $id): Message { */ public function getSource(Account $account, string $mailbox, int $uid): ?string { $client = $this->imapClientFactory->getClient($account); - try { return $this->imapMessageMapper->getFullText( $client, @@ -224,6 +225,8 @@ public function getSource(Account $account, string $mailbox, int $uid): ?string ); } catch (Horde_Imap_Client_Exception | DoesNotExistException $e) { throw new ServiceException("Could not load message", 0, $e); + } finally { + $client->logout(); } } @@ -295,20 +298,25 @@ public function deleteMessage(Account $account, throw new ServiceException("No trash folder", 0, $e); } - if ($mailboxId === $trashMailbox->getName()) { - // Delete inside trash -> expunge - $this->imapMessageMapper->expunge( - $this->imapClientFactory->getClient($account), - $sourceMailbox->getName(), - $messageId - ); - } else { - $this->imapMessageMapper->move( - $this->imapClientFactory->getClient($account), - $sourceMailbox->getName(), - $messageId, - $trashMailbox->getName() - ); + $client = $this->imapClientFactory->getClient($account); + try { + if ($mailboxId === $trashMailbox->getName()) { + // Delete inside trash -> expunge + $this->imapMessageMapper->expunge( + $client, + $sourceMailbox->getName(), + $messageId + ); + } else { + $this->imapMessageMapper->move( + $client, + $sourceMailbox->getName(), + $messageId, + $trashMailbox->getName() + ); + } + } finally { + $client->logout(); } $this->eventDispatcher->dispatch( @@ -332,14 +340,20 @@ private function moveMessageOnSameAccount(Account $account, string $destFolderId, int $messageId): void { $client = $this->imapClientFactory->getClient($account); - - $this->imapMessageMapper->move($client, $sourceFolderId, $messageId, $destFolderId); + try { + $this->imapMessageMapper->move($client, $sourceFolderId, $messageId, $destFolderId); + } finally { + $client->logout(); + } } public function markFolderAsRead(Account $account, Mailbox $mailbox): void { $client = $this->imapClientFactory->getClient($account); - - $this->imapMessageMapper->markAllRead($client, $mailbox->getName()); + try { + $this->imapMessageMapper->markAllRead($client, $mailbox->getName()); + } finally { + $client->logout(); + } } public function updateSubscription(Account $account, Mailbox $mailbox, bool $subscribed): Mailbox { @@ -355,6 +369,8 @@ public function updateSubscription(Account $account, Mailbox $mailbox, bool $sub (int)$e->getCode(), $e ); + } finally { + $client->logout(); } /** @@ -376,16 +392,16 @@ public function enableMailboxBackgroundSync(Mailbox $mailbox, } public function flagMessage(Account $account, string $mailbox, int $uid, string $flag, bool $value): void { - $client = $this->imapClientFactory->getClient($account); try { $mb = $this->mailboxMapper->find($account, $mailbox); } catch (DoesNotExistException $e) { throw new ClientException("Mailbox $mailbox does not exist", 0, $e); } - // Only send system flags to the IMAP server as other flags might not be supported - $imapFlags = $this->filterFlags($account, $flag, $mailbox); + $client = $this->imapClientFactory->getClient($account); try { + // Only send system flags to the IMAP server as other flags might not be supported + $imapFlags = $this->filterFlags($account, $flag, $mailbox); foreach ($imapFlags as $imapFlag) { if (empty($imapFlag) === true) { continue; @@ -402,6 +418,8 @@ public function flagMessage(Account $account, string $mailbox, int $uid, string (int)$e->getCode(), $e ); + } finally { + $client->logout(); } $this->eventDispatcher->dispatch( @@ -433,13 +451,13 @@ public function flagMessage(Account $account, string $mailbox, int $uid, string * @link https://github.com/nextcloud/mail/issues/25 */ public function tagMessage(Account $account, string $mailbox, Message $message, Tag $tag, bool $value): void { - $client = $this->imapClientFactory->getClient($account); try { $mb = $this->mailboxMapper->find($account, $mailbox); } catch (DoesNotExistException $e) { throw new ClientException("Mailbox $mailbox does not exist", 0, $e); } if ($this->isPermflagsEnabled($account, $mailbox) === true) { + $client = $this->imapClientFactory->getClient($account); try { if ($value) { // imap keywords and flags work the same way @@ -453,6 +471,8 @@ public function tagMessage(Account $account, string $mailbox, Message $message, (int)$e->getCode(), $e ); + } finally { + $client->logout(); } } if ($value) { @@ -469,17 +489,19 @@ public function tagMessage(Account $account, string $mailbox, Message $message, * @see https://tools.ietf.org/html/rfc2087 */ public function getQuota(Account $account): ?Quota { - $client = $this->imapClientFactory->getClient($account); /** * Get all the quotas roots of the user's mailboxes */ + $client = $this->imapClientFactory->getClient($account); try { $quotas = array_map(static function (Folder $mb) use ($client) { return $client->getQuotaRoot($mb->getMailbox()); }, $this->folderMapper->getFolders($account, $client)); } catch (Horde_Imap_Client_Exception_NoSupportExtension $ex) { return null; + } finally { + $client->logout(); } /** @@ -516,11 +538,16 @@ public function renameMailbox(Account $account, Mailbox $mailbox, string $name): /* * 1. Rename on IMAP */ - $this->folderMapper->renameFolder( - $this->imapClientFactory->getClient($account), - $mailbox->getName(), - $name - ); + $client = $this->imapClientFactory->getClient($account); + try { + $this->folderMapper->renameFolder( + $client, + $mailbox->getName(), + $name + ); + } finally { + $client->logout(); + } /** * 2. Get the IMAP changes into our database cache @@ -546,7 +573,11 @@ public function renameMailbox(Account $account, Mailbox $mailbox, string $name): public function deleteMailbox(Account $account, Mailbox $mailbox): void { $client = $this->imapClientFactory->getClient($account); - $this->folderMapper->delete($client, $mailbox->getName()); + try { + $this->folderMapper->delete($client, $mailbox->getName()); + } finally { + $client->logout(); + } $this->mailboxMapper->delete($mailbox); } @@ -557,7 +588,12 @@ public function deleteMailbox(Account $account, * @return array[] */ public function getMailAttachments(Account $account, Mailbox $mailbox, Message $message): array { - return $this->imapMessageMapper->getAttachments($this->imapClientFactory->getClient($account), $mailbox->getName(), $message->getUid()); + $client = $this->imapClientFactory->getClient($account); + try { + return $this->imapMessageMapper->getAttachments($client, $mailbox->getName(), $message->getUid()); + } finally { + $client->logout(); + } } /** @@ -612,6 +648,8 @@ public function isPermflagsEnabled(Account $account, string $mailbox): bool { (int)$e->getCode(), $e ); + } finally { + $client->logout(); } return (is_array($capabilities) === true && array_key_exists('permflags', $capabilities) === true && in_array("\*", $capabilities['permflags'], true) === true); } diff --git a/lib/Service/MailTransmission.php b/lib/Service/MailTransmission.php index 23fe8bdda4..33aefce7b0 100644 --- a/lib/Service/MailTransmission.php +++ b/lib/Service/MailTransmission.php @@ -281,12 +281,12 @@ public function saveDraft(NewMessageData $message, Message $previousDraft = null $perfLogger->step('build draft message'); // 'Send' the message + $client = $this->imapClientFactory->getClient($account); try { $transport = new Horde_Mail_Transport_Null(); $mail->send($transport, false, false); $perfLogger->step('create IMAP message'); // save the message in the drafts folder - $client = $this->imapClientFactory->getClient($account); $draftsMailboxId = $account->getMailAccount()->getDraftsMailboxId(); if ($draftsMailboxId === null) { throw new ClientException("No drafts mailbox configured"); @@ -303,6 +303,8 @@ public function saveDraft(NewMessageData $message, Message $previousDraft = null throw new ServiceException('Drafts mailbox does not exist', 0, $e); } catch (Horde_Exception $e) { throw new ServiceException('Could not save draft message', 0, $e); + } finally { + $client->logout(); } $this->eventDispatcher->dispatch( @@ -404,11 +406,16 @@ private function handleForwardedMessageAttachment(Account $account, array $attac $attachmentMessage = $this->mailManager->getMessage($userId, (int)$attachment['id']); $mailbox = $this->mailManager->getMailbox($userId, $attachmentMessage->getMailboxId()); - $fullText = $this->messageMapper->getFullText( - $this->imapClientFactory->getClient($account), - $mailbox->getName(), - $attachmentMessage->getUid() - ); + $client = $this->imapClientFactory->getClient($account); + try { + $fullText = $this->messageMapper->getFullText( + $client, + $mailbox->getName(), + $attachmentMessage->getUid() + ); + } finally { + $client->logout(); + } $message->addRawAttachment( $attachment['displayName'] ?? $attachmentMessage->getSubject() . '.eml', @@ -429,11 +436,16 @@ private function handleEmbeddedMessageAttachments(Account $account, array $attac $attachmentMessage = $this->mailManager->getMessage($userId, (int)$attachment['id']); $mailbox = $this->mailManager->getMailbox($userId, $attachmentMessage->getMailboxId()); - $fullText = $this->messageMapper->getFullText( - $this->imapClientFactory->getClient($account), - $mailbox->getName(), - $attachmentMessage->getUid() - ); + $client = $this->imapClientFactory->getClient($account); + try { + $fullText = $this->messageMapper->getFullText( + $client, + $mailbox->getName(), + $attachmentMessage->getUid() + ); + } finally { + $client->logout(); + } $message->addEmbeddedMessageAttachment( $attachment['displayName'] ?? $attachmentMessage->getSubject() . '.eml', @@ -454,14 +466,19 @@ private function handleForwardedAttachment(Account $account, array $attachment, $userId = $account->getMailAccount()->getUserId(); $attachmentMessage = $this->mailManager->getMessage($userId, (int)$attachment['messageId']); $mailbox = $this->mailManager->getMailbox($userId, $attachmentMessage->getMailboxId()); - $attachments = $this->messageMapper->getRawAttachments( - $this->imapClientFactory->getClient($account), - $mailbox->getName(), - $attachmentMessage->getUid(), - [ - $attachment['id'] - ] - ); + $client = $this->imapClientFactory->getClient($account); + try { + $attachments = $this->messageMapper->getRawAttachments( + $client, + $mailbox->getName(), + $attachmentMessage->getUid(), + [ + $attachment['id'] + ] + ); + } finally { + $client->logout(); + } // Attaches attachment to new message $message->addRawAttachment($attachment['fileName'], $attachments[0]); @@ -495,8 +512,6 @@ private function handleCloudAttachment(array $attachment, IMessage $message) { } public function sendMdn(Account $account, Mailbox $mailbox, Message $message): void { - $imapClient = $this->imapClientFactory->getClient($account); - $query = new Horde_Imap_Client_Fetch_Query(); $query->flags(); $query->uid(); @@ -506,10 +521,15 @@ public function sendMdn(Account $account, Mailbox $mailbox, Message $message): v 'peek' => true, ]); - /** @var Horde_Imap_Client_Data_Fetch[] $fetchResults */ - $fetchResults = iterator_to_array($imapClient->fetch($mailbox->getName(), $query, [ - 'ids' => new Horde_Imap_Client_Ids([$message->getUid()]), - ]), false); + $imapClient = $this->imapClientFactory->getClient($account); + try { + /** @var Horde_Imap_Client_Data_Fetch[] $fetchResults */ + $fetchResults = iterator_to_array($imapClient->fetch($mailbox->getName(), $query, [ + 'ids' => new Horde_Imap_Client_Ids([$message->getUid()]), + ]), false); + } finally { + $imapClient->logout(); + } if (count($fetchResults) < 1) { throw new ServiceException('Message "' .$message->getId() . '" not found.'); diff --git a/lib/Service/SetupService.php b/lib/Service/SetupService.php index 89152eb96f..9ab0b1374d 100644 --- a/lib/Service/SetupService.php +++ b/lib/Service/SetupService.php @@ -167,6 +167,8 @@ protected function testConnectivity(Account $account): void { $imapClient->login(); } catch (Horde_Imap_Client_Exception $e) { throw new CouldNotConnectException($e, 'IMAP', $mailAccount->getInboundHost(), $mailAccount->getInboundPort()); + } finally { + $imapClient->logout(); } $transport = $this->smtpClientFactory->create($account); diff --git a/lib/Service/Sync/ImapToDbSynchronizer.php b/lib/Service/Sync/ImapToDbSynchronizer.php index ea000c4067..ef24441e4a 100644 --- a/lib/Service/Sync/ImapToDbSynchronizer.php +++ b/lib/Service/Sync/ImapToDbSynchronizer.php @@ -287,37 +287,41 @@ private function runInitialSync(Account $account, $highestKnownUid = $this->dbMapper->findHighestUid($mailbox); $client = $this->clientFactory->getClient($account); try { - $imapMessages = $this->imapMapper->findAll( - $client, - $mailbox->getName(), - self::MAX_NEW_MESSAGES, - $highestKnownUid ?? 0 - ); - $perf->step('fetch all messages from IMAP'); - } catch (Horde_Imap_Client_Exception $e) { - throw new ServiceException('Can not get messages from mailbox ' . $mailbox->getName() . ': ' . $e->getMessage(), 0, $e); - } + try { + $imapMessages = $this->imapMapper->findAll( + $client, + $mailbox->getName(), + self::MAX_NEW_MESSAGES, + $highestKnownUid ?? 0 + ); + $perf->step('fetch all messages from IMAP'); + } catch (Horde_Imap_Client_Exception $e) { + throw new ServiceException('Can not get messages from mailbox ' . $mailbox->getName() . ': ' . $e->getMessage(), 0, $e); + } - foreach (array_chunk($imapMessages['messages'], 500) as $chunk) { - $this->dbMapper->insertBulk($account, ...array_map(function (IMAPMessage $imapMessage) use ($mailbox, $account) { - return $imapMessage->toDbMessage($mailbox->getId(), $account->getMailAccount()); - }, $chunk)); - } - $perf->step('persist messages in database'); + foreach (array_chunk($imapMessages['messages'], 500) as $chunk) { + $this->dbMapper->insertBulk($account, ...array_map(function (IMAPMessage $imapMessage) use ($mailbox, $account) { + return $imapMessage->toDbMessage($mailbox->getId(), $account->getMailAccount()); + }, $chunk)); + } + $perf->step('persist messages in database'); - if (!$imapMessages['all']) { - // We might need more attempts to fill the cache - $perf->end(); + if (!$imapMessages['all']) { + // We might need more attempts to fill the cache + $perf->end(); - $loggingMailboxId = $account->getId() . ':' . $mailbox->getName(); - $total = $imapMessages['total']; - $cached = count($this->messageMapper->findAllUids($mailbox)); - throw new IncompleteSyncException("Initial sync is not complete for $loggingMailboxId ($cached of $total messages cached)."); - } + $loggingMailboxId = $account->getId() . ':' . $mailbox->getName(); + $total = $imapMessages['total']; + $cached = count($this->messageMapper->findAllUids($mailbox)); + throw new IncompleteSyncException("Initial sync is not complete for $loggingMailboxId ($cached of $total messages cached)."); + } - $mailbox->setSyncNewToken($client->getSyncToken($mailbox->getName())); - $mailbox->setSyncChangedToken($client->getSyncToken($mailbox->getName())); - $mailbox->setSyncVanishedToken($client->getSyncToken($mailbox->getName())); + $mailbox->setSyncNewToken($client->getSyncToken($mailbox->getName())); + $mailbox->setSyncChangedToken($client->getSyncToken($mailbox->getName())); + $mailbox->setSyncVanishedToken($client->getSyncToken($mailbox->getName())); + } finally { + $client->logout(); + } $this->mailboxMapper->update($mailbox); $perf->end(); @@ -340,104 +344,108 @@ private function runPartialSync(Account $account, ); $client = $this->clientFactory->getClient($account); - $uids = $knownUids ?? $this->dbMapper->findAllUids($mailbox); - $perf->step('get all known UIDs'); - - if ($criteria & Horde_Imap_Client::SYNC_NEWMSGSUIDS) { - $response = $this->synchronizer->sync( - $client, - new Request( - $mailbox->getName(), - $mailbox->getSyncNewToken(), - $uids - ), - Horde_Imap_Client::SYNC_NEWMSGSUIDS - ); - $perf->step('get new messages via Horde'); + try { + $uids = $knownUids ?? $this->dbMapper->findAllUids($mailbox); + $perf->step('get all known UIDs'); + + if ($criteria & Horde_Imap_Client::SYNC_NEWMSGSUIDS) { + $response = $this->synchronizer->sync( + $client, + new Request( + $mailbox->getName(), + $mailbox->getSyncNewToken(), + $uids + ), + Horde_Imap_Client::SYNC_NEWMSGSUIDS + ); + $perf->step('get new messages via Horde'); + + $highestKnownUid = $this->dbMapper->findHighestUid($mailbox); + if ($highestKnownUid === null) { + // Everything is relevant + $newMessages = $response->getNewMessages(); + } else { + // Filter out anything that is already in the DB. Ideally this never happens, but if there is an error + // during a consecutive chunk INSERT, the sync token won't be updated. In that case the same message(s) + // will be seen as *new* and therefore cause conflicts. + $newMessages = array_filter($response->getNewMessages(), function (IMAPMessage $imapMessage) use ($highestKnownUid) { + return $imapMessage->getUid() > $highestKnownUid; + }); + } - $highestKnownUid = $this->dbMapper->findHighestUid($mailbox); - if ($highestKnownUid === null) { - // Everything is relevant - $newMessages = $response->getNewMessages(); - } else { - // Filter out anything that is already in the DB. Ideally this never happens, but if there is an error - // during a consecutive chunk INSERT, the sync token won't be updated. In that case the same message(s) - // will be seen as *new* and therefore cause conflicts. - $newMessages = array_filter($response->getNewMessages(), function (IMAPMessage $imapMessage) use ($highestKnownUid) { - return $imapMessage->getUid() > $highestKnownUid; - }); - } + foreach (array_chunk($newMessages, 500) as $chunk) { + $dbMessages = array_map(function (IMAPMessage $imapMessage) use ($mailbox, $account) { + return $imapMessage->toDbMessage($mailbox->getId(), $account->getMailAccount()); + }, $chunk); - foreach (array_chunk($newMessages, 500) as $chunk) { - $dbMessages = array_map(function (IMAPMessage $imapMessage) use ($mailbox, $account) { - return $imapMessage->toDbMessage($mailbox->getId(), $account->getMailAccount()); - }, $chunk); + $this->dbMapper->insertBulk($account, ...$dbMessages); - $this->dbMapper->insertBulk($account, ...$dbMessages); + $this->dispatcher->dispatch( + NewMessagesSynchronized::class, + new NewMessagesSynchronized($account, $mailbox, $dbMessages) + ); + $perf->step('classified a chunk of new messages'); + } + $perf->step('persist new messages'); - $this->dispatcher->dispatch( - NewMessagesSynchronized::class, - new NewMessagesSynchronized($account, $mailbox, $dbMessages) - ); - $perf->step('classified a chunk of new messages'); + $mailbox->setSyncNewToken($client->getSyncToken($mailbox->getName())); } - $perf->step('persist new messages'); - - $mailbox->setSyncNewToken($client->getSyncToken($mailbox->getName())); - } - if ($criteria & Horde_Imap_Client::SYNC_FLAGSUIDS) { - $response = $this->synchronizer->sync( - $client, - new Request( - $mailbox->getName(), - $mailbox->getSyncChangedToken(), - $uids - ), - Horde_Imap_Client::SYNC_FLAGSUIDS - ); - $perf->step('get changed messages via Horde'); + if ($criteria & Horde_Imap_Client::SYNC_FLAGSUIDS) { + $response = $this->synchronizer->sync( + $client, + new Request( + $mailbox->getName(), + $mailbox->getSyncChangedToken(), + $uids + ), + Horde_Imap_Client::SYNC_FLAGSUIDS + ); + $perf->step('get changed messages via Horde'); - $permflagsEnabled = $this->mailManager->isPermflagsEnabled($account, $mailbox->getName()); + $permflagsEnabled = $this->mailManager->isPermflagsEnabled($account, $mailbox->getName()); - foreach (array_chunk($response->getChangedMessages(), 500) as $chunk) { - $this->dbMapper->updateBulk($account, $permflagsEnabled, ...array_map(static function (IMAPMessage $imapMessage) use ($mailbox, $account) { - return $imapMessage->toDbMessage($mailbox->getId(), $account->getMailAccount()); - }, $chunk)); - } - $perf->step('persist changed messages'); - - // If a list of UIDs was *provided* (as opposed to loaded from the DB, - // we can not assume that all changes were detected, hence this is kinda - // a silent sync and we don't update the change token until the next full - // mailbox sync - if ($knownUids === null) { - $mailbox->setSyncChangedToken($client->getSyncToken($mailbox->getName())); + foreach (array_chunk($response->getChangedMessages(), 500) as $chunk) { + $this->dbMapper->updateBulk($account, $permflagsEnabled, ...array_map(static function (IMAPMessage $imapMessage) use ($mailbox, $account) { + return $imapMessage->toDbMessage($mailbox->getId(), $account->getMailAccount()); + }, $chunk)); + } + $perf->step('persist changed messages'); + + // If a list of UIDs was *provided* (as opposed to loaded from the DB, + // we can not assume that all changes were detected, hence this is kinda + // a silent sync and we don't update the change token until the next full + // mailbox sync + if ($knownUids === null) { + $mailbox->setSyncChangedToken($client->getSyncToken($mailbox->getName())); + } } - } - if ($criteria & Horde_Imap_Client::SYNC_VANISHEDUIDS) { - $response = $this->synchronizer->sync( - $client, - new Request( - $mailbox->getName(), - $mailbox->getSyncVanishedToken(), - $uids - ), - Horde_Imap_Client::SYNC_VANISHEDUIDS - ); - $perf->step('get vanished messages via Horde'); + if ($criteria & Horde_Imap_Client::SYNC_VANISHEDUIDS) { + $response = $this->synchronizer->sync( + $client, + new Request( + $mailbox->getName(), + $mailbox->getSyncVanishedToken(), + $uids + ), + Horde_Imap_Client::SYNC_VANISHEDUIDS + ); + $perf->step('get vanished messages via Horde'); - foreach (array_chunk($response->getVanishedMessageUids(), 500) as $chunk) { - $this->dbMapper->deleteByUid($mailbox, ...$chunk); - } - $perf->step('persist new messages'); - - // If a list of UIDs was *provided* (as opposed to loaded from the DB, - // we can not assume that all changes were detected, hence this is kinda - // a silent sync and we don't update the vanish token until the next full - // mailbox sync - if ($knownUids === null) { - $mailbox->setSyncVanishedToken($client->getSyncToken($mailbox->getName())); + foreach (array_chunk($response->getVanishedMessageUids(), 500) as $chunk) { + $this->dbMapper->deleteByUid($mailbox, ...$chunk); + } + $perf->step('persist new messages'); + + // If a list of UIDs was *provided* (as opposed to loaded from the DB, + // we can not assume that all changes were detected, hence this is kinda + // a silent sync and we don't update the vanish token until the next full + // mailbox sync + if ($knownUids === null) { + $mailbox->setSyncVanishedToken($client->getSyncToken($mailbox->getName())); + } } + } finally { + $client->logout(); } $this->mailboxMapper->update($mailbox); $perf->end(); diff --git a/tests/Integration/Framework/ImapTest.php b/tests/Integration/Framework/ImapTest.php index 1a68ad4157..da18d3d1d0 100644 --- a/tests/Integration/Framework/ImapTest.php +++ b/tests/Integration/Framework/ImapTest.php @@ -52,7 +52,7 @@ trait ImapTest { /** * @return Horde_Imap_Client_Socket */ - private function getTestClient() { + private function getTestClient(): Horde_Imap_Client_Socket { if ($this->client === null) { $this->client = new Horde_Imap_Client_Socket([ 'username' => 'user@domain.tld', @@ -134,8 +134,6 @@ public function getMessageBuilder() { * @return int id of the new message */ public function saveMessage(string $mailbox, SimpleMessage $message, MailAccount $account = null) { - $client = $this->getClient($account); - $headers = [ 'From' => new Horde_Mail_Rfc822_Address($message->getFrom()), 'To' => new Horde_Mail_Rfc822_Address($message->getTo()), @@ -155,32 +153,43 @@ public function saveMessage(string $mailbox, SimpleMessage $message, MailAccount $raw = $mail->getRaw(); $data = stream_get_contents($raw); - return $client->append($mailbox, [ - [ - 'data' => $data, - ] - ])->ids[0]; + $client = $this->getClient($account); + try { + return $client->append($mailbox, [ + [ + 'data' => $data, + ] + ])->ids[0]; + } finally { + $client->logout(); + } } public function flagMessage($mailbox, $id, MailAccount $account = null) { $client = $this->getClient($account); - - $client->store($mailbox, [ - 'ids' => new Horde_Imap_Client_Ids([$id]), - 'add' => [ - Horde_Imap_Client::FLAG_FLAGGED, - ], - ]); + try { + $client->store($mailbox, [ + 'ids' => new Horde_Imap_Client_Ids([$id]), + 'add' => [ + Horde_Imap_Client::FLAG_FLAGGED, + ], + ]); + } finally { + $client->logout(); + } } public function deleteMessage($mailbox, $id, MailAccount $account = null) { $client = $this->getClient($account); - $ids = new Horde_Imap_Client_Ids([$id]); - $client->expunge($mailbox, [ - 'ids' => $ids, - 'delete' => true, - ]); + try { + $client->expunge($mailbox, [ + 'ids' => $ids, + 'delete' => true, + ]); + } finally { + $client->logout(); + } } /** diff --git a/tests/Integration/IMAP/AbstractTest.php b/tests/Integration/IMAP/AbstractTest.php index bbaaddf46a..a5119efc5e 100644 --- a/tests/Integration/IMAP/AbstractTest.php +++ b/tests/Integration/IMAP/AbstractTest.php @@ -77,23 +77,6 @@ public static function tearDownAfterClass(): void { } } - /** - * @param string $name - * @return Mailbox - */ - public function createMailBox($name) { - try { - $this->getTestAccount()->getMailbox($name); - $this->deleteMailbox($name); - } catch (Exception $e) { - // Ignore mailbox not found - } - - $mailbox = $this->getTestAccount()->createMailbox($name); - self::$createdMailboxes[$name] = $mailbox; - return $mailbox; - } - /** * @return Account */ diff --git a/tests/Integration/IMAP/MessageMapperTest.php b/tests/Integration/IMAP/MessageMapperTest.php index 20e93e688a..d537bcaa1c 100644 --- a/tests/Integration/IMAP/MessageMapperTest.php +++ b/tests/Integration/IMAP/MessageMapperTest.php @@ -77,10 +77,13 @@ public function testTagging(): void { $newUid = $this->saveMessage($inbox->getName(), $message, $account); // now we tag this message! + $client = $this->getClient($account); try { - $imapMessageMapper->addFlag($this->getClient($account), $mailBox, [$newUid], '$label1'); + $imapMessageMapper->addFlag($client, $mailBox, [$newUid], '$label1'); } catch (Horde_Imap_Client_Exception $e) { self::fail('Could not tag message'); + } finally { + $client->logout(); } // sync @@ -103,10 +106,13 @@ public function testTagging(): void { // now we untag this message! + $client = $this->getClient($account); try { - $imapMessageMapper->removeFlag($this->getClient($account), $mailBox, [$newUid], '$label1'); + $imapMessageMapper->removeFlag($client, $mailBox, [$newUid], '$label1'); } catch (Horde_Imap_Client_Exception $e) { self::fail('Could not untag message'); + } finally { + $client->logout(); } // sync again @@ -151,7 +157,6 @@ public function testGetFlagged(): void { ->finish(); $newUid = $this->saveMessage($inbox->getName(), $message, $account); - // Put another new message into the mailbox $message = $this->getMessageBuilder() ->from('fluffington@domain.tld') @@ -166,38 +171,34 @@ public function testGetFlagged(): void { ->finish(); $this->saveMessage($inbox->getName(), $message, $account); - // now we tag this message with $label1 + $client = $this->getClient($account); try { - $imapMessageMapper->addFlag($this->getClient($account), $mailBox, [$newUid], '$label1'); - } catch (Horde_Imap_Client_Exception $e) { - self::fail('Could not tag message'); + // now we tag this message with $label1 + $imapMessageMapper->addFlag($client, $mailBox, [$newUid], '$label1'); + // now we tag this and the previous message with $label2 + $imapMessageMapper->addFlag($client, $mailBox, [$newUid, $newUid2], '$label2'); + + // test for labels + $tagged = $imapMessageMapper->getFlagged($client, $mailBox, '$label1'); + self::assertNotEmpty($tagged); + // are the counts correct? + self::assertCount(1, $tagged); + + $tagged = $imapMessageMapper->getFlagged($client, $mailBox, '$label2'); + self::assertNotEmpty($tagged); + self::assertCount(2, $tagged); + + // test for labels that wasn't set + $tagged = $imapMessageMapper->getFlagged($client, $mailBox, '$notAvailable'); + self::assertEmpty($tagged); + + // test for regular flag - recent + $tagged = $imapMessageMapper->getFlagged($client, $mailBox, Horde_Imap_Client::FLAG_RECENT); + self::assertNotEmpty($tagged); + // should return all messages + self::assertCount(3, $tagged); + } finally { + $client->logout(); } - - // now we tag this and the previous message with $label2 - try { - $imapMessageMapper->addFlag($this->getClient($account), $mailBox, [$newUid, $newUid2], '$label2'); - } catch (Horde_Imap_Client_Exception $e) { - self::fail('Could not tag message'); - } - - // test for labels - $tagged = $imapMessageMapper->getFlagged($this->getClient($account), $mailBox, '$label1'); - self::assertNotEmpty($tagged); - // are the counts correct? - self::assertCount(1, $tagged); - - $tagged = $imapMessageMapper->getFlagged($this->getClient($account), $mailBox, '$label2'); - self::assertNotEmpty($tagged); - self::assertCount(2, $tagged); - - // test for labels that wasn't set - $tagged = $imapMessageMapper->getFlagged($this->getClient($account), $mailBox, '$notAvailable'); - self::assertEmpty($tagged); - - // test for regular flag - recent - $tagged = $imapMessageMapper->getFlagged($this->getClient($account), $mailBox, Horde_Imap_Client::FLAG_RECENT); - self::assertNotEmpty($tagged); - // should return all messages - self::assertCount(3, $tagged); } }