From 44c53152689880c5ed8cfd977ffe9381b61d2530 Mon Sep 17 00:00:00 2001 From: Richard Steinmetz Date: Wed, 21 Aug 2024 14:18:22 +0200 Subject: [PATCH] fix(imap): persist vanished messages immediately on EXAMINE commands Signed-off-by: Richard Steinmetz --- lib/Cache/Cache.php | 93 ++++++++++-------- lib/Cache/CacheFactory.php | 32 ++++++ lib/IMAP/IMAPClientFactory.php | 18 +++- tests/Integration/Framework/Caching.php | 50 ++++++++++ tests/Integration/Framework/ImapTest.php | 25 +++++ .../MailboxSynchronizationTest.php | 97 +++++++++++++++++++ 6 files changed, 270 insertions(+), 45 deletions(-) create mode 100644 lib/Cache/CacheFactory.php create mode 100644 tests/Integration/Framework/Caching.php diff --git a/lib/Cache/Cache.php b/lib/Cache/Cache.php index fc1cbc8c5c..9ad9d55cd1 100644 --- a/lib/Cache/Cache.php +++ b/lib/Cache/Cache.php @@ -13,7 +13,9 @@ use Exception; use Horde_Imap_Client_Cache_Backend; use Horde_Imap_Client_Exception; -use InvalidArgumentException; +use OCA\Mail\Account; +use OCA\Mail\Db\MailboxMapper; +use OCA\Mail\Db\MessageMapper; use OCP\ICache; /** @@ -21,12 +23,7 @@ */ class Cache extends Horde_Imap_Client_Cache_Backend { /** Cache structure version. */ - public const VERSION = 3; - - /** - * The cache object. - */ - protected ICache $_cache; + public const VERSION = 4; /** * The working data for the current pageload. All changes take place to @@ -52,26 +49,24 @@ class Cache extends Horde_Imap_Client_Cache_Backend { */ protected array $_update = []; - /** - * Constructor. - * - * @param array $params Configuration parameters: - */ - public function __construct(array $params = []) { - // Default parameters. - $params = array_merge([ - 'lifetime' => 604800, - 'slicesize' => 50 - ], array_filter($params)); - - if (!isset($params['cacheob'])) { - throw new InvalidArgumentException('Missing cacheob parameter.'); - } - - foreach (['lifetime', 'slicesize'] as $val) { - $params[$val] = intval($params[$val]); - } - + /** @var int[]|null */ + private ?array $cachedUids = null; + + private ?int $uidvalid = null; + + public function __construct( + private MessageMapper $messageMapper, + private MailboxMapper $mailboxMapper, + private Account $account, + private ICache $_cache, + int $lifetime = 604800, + int $slicesize = 50, + ) { + $params = [ + 'lifetime' => $lifetime, + 'slicesize' => $slicesize, + 'cacheob' => $this->_cache, + ]; parent::__construct($params); } @@ -164,13 +159,23 @@ public function get($mailbox, $uids, $fields, $uidvalid) { return $ret; } - /** {@inheritDoc} */ - public function getCachedUids($mailbox, $uidvalid) { - $this->_loadSliceMap($mailbox, $uidvalid); - return array_unique(array_merge( - array_keys($this->_slicemap[$mailbox]['s']), - (isset($this->_update[$mailbox]) ? $this->_update[$mailbox]['add'] : []) - )); + public function getCachedUids($mailbox, $uidvalid): array { + // Delete cached data of mailbox if uidvalid has changed + if ($this->uidvalid !== null && $uidvalid !== null && $this->uidvalid !== (int)$uidvalid) { + $this->_deleteMailbox($mailbox); + } + + // Refresh cached uids lazily + if ($this->cachedUids === null) { + $mailboxEntity = $this->mailboxMapper->find($this->account, $mailbox); + $this->cachedUids = $this->messageMapper->findAllUids($mailboxEntity); + } + + if ($this->uidvalid === null && $uidvalid !== null) { + $this->uidvalid = (int)$uidvalid; + } + + return array_merge([], $this->cachedUids); } /** @@ -235,12 +240,15 @@ public function setMetaData($mailbox, $data) { $this->_toUpdate($mailbox, 'slicemap', true); } - /** - * {@inheritDoc} - * - * @return void - */ - public function deleteMsgs($mailbox, $uids) { + public function deleteMsgs($mailbox, $uids): void { + // Delete uids from the db cache + $mailboxEntity = $this->mailboxMapper->find($this->account, $mailbox); + $this->messageMapper->deleteByUid($mailboxEntity, ...$uids); + if ($this->cachedUids !== null) { + $this->cachedUids = array_diff($this->cachedUids, $uids); + } + + // Delete uids from the memory cache $this->_loadSliceMap($mailbox); $slicemap = &$this->_slicemap[$mailbox]; @@ -298,6 +306,8 @@ public function deleteMailbox($mailbox) { public function clear($lifetime) { $this->_cache->clear(); $this->_data = $this->_loaded = $this->_slicemap = $this->_update = []; + $this->cachedUids = null; + $this->uidvalid = null; } /** @@ -339,6 +349,9 @@ protected function _deleteMailbox($mbox): void { $this->_slicemap[$mbox], $this->_update[$mbox] ); + + $this->cachedUids = null; + $this->uidvalid = null; } /** diff --git a/lib/Cache/CacheFactory.php b/lib/Cache/CacheFactory.php new file mode 100644 index 0000000000..3a5f3e32af --- /dev/null +++ b/lib/Cache/CacheFactory.php @@ -0,0 +1,32 @@ +messageMapper, + $this->mailboxMapper, + $account, + $cache, + ); + } +} diff --git a/lib/IMAP/IMAPClientFactory.php b/lib/IMAP/IMAPClientFactory.php index 14e96a942f..1560228397 100644 --- a/lib/IMAP/IMAPClientFactory.php +++ b/lib/IMAP/IMAPClientFactory.php @@ -13,7 +13,7 @@ use Horde_Imap_Client_Password_Xoauth2; use Horde_Imap_Client_Socket; use OCA\Mail\Account; -use OCA\Mail\Cache\Cache; +use OCA\Mail\Cache\CacheFactory; use OCA\Mail\Events\BeforeImapClientCreated; use OCP\AppFramework\Utility\ITimeFactory; use OCP\EventDispatcher\IEventDispatcher; @@ -40,17 +40,20 @@ class IMAPClientFactory { private $eventDispatcher; private ITimeFactory $timeFactory; + private CacheFactory $hordeCacheFactory; public function __construct(ICrypto $crypto, IConfig $config, ICacheFactory $cacheFactory, IEventDispatcher $eventDispatcher, - ITimeFactory $timeFactory) { + ITimeFactory $timeFactory, + CacheFactory $hordeCacheFactory) { $this->crypto = $crypto; $this->config = $config; $this->cacheFactory = $cacheFactory; $this->eventDispatcher = $eventDispatcher; $this->timeFactory = $timeFactory; + $this->hordeCacheFactory = $hordeCacheFactory; } /** @@ -113,10 +116,15 @@ public function getClient(Account $account, bool $useCache = true): Horde_Imap_C ); if ($useCache && $this->cacheFactory->isAvailable()) { $params['cache'] = [ - 'backend' => new Cache([ - 'cacheob' => $this->cacheFactory->createDistributed(md5((string)$account->getId())), - ])]; + 'backend' => $this->hordeCacheFactory->newCache( + $account, + $this->cacheFactory->createDistributed(md5((string)$account->getId())), + ), + ]; } else { + // WARNING: This is very dangerous! We **will** miss changes when using QRESYNC without + // actually persisting changes to the cache. Especially vanished messages will + // be missed. /** * If we don't use a cache we use a null cache to trick Horde into * using QRESYNC/CONDSTORE if they are available diff --git a/tests/Integration/Framework/Caching.php b/tests/Integration/Framework/Caching.php new file mode 100644 index 0000000000..2647175cc3 --- /dev/null +++ b/tests/Integration/Framework/Caching.php @@ -0,0 +1,50 @@ +getSystemValue('memcache.local', null), + $config->getSystemValue('memcache.distributed', null), + $config->getSystemValue('memcache.locking', null), + $config->getSystemValueString('redis_log_file') + ); + $imapClient = new IMAPClientFactory( + $crypto ?? Server::get(ICrypto::class), + $config, + $cacheFactory, + Server::get(IEventDispatcher::class), + Server::get(ITimeFactory::class), + Server::get(CacheFactory::class), + ); + return [$imapClient, $cacheFactory]; + } +} diff --git a/tests/Integration/Framework/ImapTest.php b/tests/Integration/Framework/ImapTest.php index 03f98e0c53..589b5d6d41 100644 --- a/tests/Integration/Framework/ImapTest.php +++ b/tests/Integration/Framework/ImapTest.php @@ -211,6 +211,31 @@ public function deleteMessage($mailbox, $id, ?MailAccount $account = null) { } } + /** + * Delete a message without informing Horde or the db cache. This simulates another client + * deleting a message on IMAP. + * + * @param int[] $uids + */ + public function deleteMessagesExternally(string $mailbox, array $uids): void { + $client = new Horde_Imap_Client_Socket([ + 'username' => 'user@domain.tld', + 'password' => 'mypassword', + 'hostspec' => '127.0.0.1', + 'port' => 993, + 'secure' => 'ssl', + ]); + $ids = new Horde_Imap_Client_Ids($uids); + try { + $client->expunge($mailbox, [ + 'ids' => $ids, + 'delete' => true, + ]); + } finally { + $client->logout(); + } + } + /** * @param Horde_Imap_Client_Socket $client * @param string $mailbox diff --git a/tests/Integration/MailboxSynchronizationTest.php b/tests/Integration/MailboxSynchronizationTest.php index 9d9d45589e..86581ba9b6 100644 --- a/tests/Integration/MailboxSynchronizationTest.php +++ b/tests/Integration/MailboxSynchronizationTest.php @@ -11,15 +11,26 @@ use Horde_Imap_Client; use Horde_Imap_Client_Socket; +use OC\Memcache\Redis; use OCA\Mail\Account; use OCA\Mail\Contracts\IMailManager; use OCA\Mail\Controller\MailboxesController; +use OCA\Mail\Db\MailboxMapper; +use OCA\Mail\Db\MessageMapper as DbMessageMapper; +use OCA\Mail\IMAP\MessageMapper as ImapMessageMapper; +use OCA\Mail\IMAP\Sync\Synchronizer; use OCA\Mail\Service\AccountService; +use OCA\Mail\Service\Sync\ImapToDbSynchronizer; use OCA\Mail\Service\Sync\SyncService; +use OCA\Mail\Support\PerformanceLogger; +use OCA\Mail\Tests\Integration\Framework\Caching; use OCA\Mail\Tests\Integration\Framework\ImapTest; use OCA\Mail\Tests\Integration\Framework\ImapTestAccount; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\IConfig; use OCP\IRequest; use OCP\Server; +use Psr\Log\LoggerInterface; class MailboxSynchronizationTest extends TestCase { use ImapTest, @@ -220,4 +231,90 @@ public function testSyncVanishedMessage() { // self::assertCount(0, $syncJson['changedMessages']); // self::assertCount(1, $syncJson['vanishedMessages'], 'Message does not show as vanished, possibly because UID and ID are mixed up above.'); } + + public function testUnsolicitedVanishedMessage() { + $config = Server::get(IConfig::class); + $cacheClass = $config->getSystemValueString('memcache.distributed'); + if (ltrim($cacheClass, '\\') !== Redis::class) { + $this->markTestSkipped('Redis not available. Found ' . $cacheClass); + } + + [$imapClientFactory, $cacheFactory] = Caching::getImapClientFactoryAndConfiguredCacheFactory(); + + // Just to be sure ... + $cache = $cacheFactory->createDistributed(); + $this->assertInstanceOf(Redis::class, $cache); + + $cache->clear(); + + $mailbox = 'INBOX'; + $message = $this->getMessageBuilder() + ->from('ralph@buffington@domain.tld') + ->to('user@domain.tld') + ->subject('Msg 1') + ->finish(); + $uid1 = $this->saveMessage($mailbox, $message, $this->account); + $message = $this->getMessageBuilder() + ->from('ralph@buffington@domain.tld') + ->to('user@domain.tld') + ->subject('Msg 2') + ->finish(); + $uid2 = $this->saveMessage($mailbox, $message, $this->account); + /** @var IMailManager $mailManager */ + $mailManager = Server::get(IMailManager::class); + $mailBoxes = $mailManager->getMailboxes(new Account($this->account)); + $inbox = null; + foreach ($mailBoxes as $mailBox) { + if ($mailBox->getName() === 'INBOX') { + $inbox = $mailBox; + break; + } + } + /** @var SyncService $syncService */ + $syncService = Server::get(SyncService::class); + $syncService->syncMailbox( + new Account($this->account), + $inbox, + Horde_Imap_Client::SYNC_NEWMSGSUIDS | Horde_Imap_Client::SYNC_FLAGSUIDS | Horde_Imap_Client::SYNC_VANISHEDUIDS, + false, + null, + [] + ); + + // Trigger partial sync to warm the cache. + // Construct a new ImapToDbSynchronizer from scratch to prevent getting a cached instance + // without the proper ICacheFactory that still builds instances of ArrayCache despite Redis + // being configured. + $dbMessageMapper = Server::get(DbMessageMapper::class); + $logger = Server::get(LoggerInterface::class); + $synchronizer = new ImapToDbSynchronizer( + $dbMessageMapper, + $imapClientFactory, + Server::get(ImapMessageMapper::class), + Server::get(MailboxMapper::class), + $dbMessageMapper, + Server::get(Synchronizer::class), + Server::get(IEventDispatcher::class), + Server::get(PerformanceLogger::class), + $logger, + Server::get(IMailManager::class), + ); + $synchronizer->syncAccount(new Account($this->account), $logger); + + // Assert that there are 2 messages and nothing changes when deleting a message externally + self::assertCount(2, $dbMessageMapper->findAllUids($inbox)); + $this->deleteMessagesExternally($mailbox, [$uid1]); + self::assertCount(2, $dbMessageMapper->findAllUids($inbox)); + + // Receive unsolicited vanished uid + $mailManager->getSource( + $imapClientFactory->getClient(new Account($this->account)), + new Account($this->account), + $mailbox, + $uid2, + ); + + // Assert that the unsolicited change was synced to the db + self::assertCount(1, $dbMessageMapper->findAllUids($inbox)); + } }