Detect missing mailbox cache and block access to messages

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

12 files changed, 133 insertions, 85 deletions

diff --git a/lib/Contracts/IMailSearch.php b/lib/Contracts/IMailSearch.php
index 43299907c..ac2a82d3b 100644
--- a/lib/Contracts/IMailSearch.php
+++ b/lib/Contracts/IMailSearch.php
@@ -24,6 +24,7 @@
 namespace OCA\Mail\Contracts;
 
 use OCA\Mail\Account;
+use OCA\Mail\Exception\ClientException;
 use OCA\Mail\Exception\ServiceException;
 use OCA\Mail\Model\IMAPMessage;
 
@@ -36,6 +37,7 @@ interface IMailSearch {
 	 * @param string|null $cursor
 	 *
 	 * @return IMAPMessage[]
+	 * @throws ClientException
 	 * @throws ServiceException
 	 */
 	public function findMessages(Account $account, string $mailboxName, ?string $filter, ?int $cursor): array;
diff --git a/lib/Controller/FoldersController.php b/lib/Controller/FoldersController.php
index 3caa7c051..96442b68f 100644
--- a/lib/Controller/FoldersController.php
+++ b/lib/Controller/FoldersController.php
@@ -26,6 +26,7 @@ declare(strict_types=1);
 namespace OCA\Mail\Controller;
 
 use Horde_Imap_Client;
+use OCA\Mail\Exception\ClientException;
 use OCA\Mail\Exception\MailboxNotCachedException;
 use OCA\Mail\Exception\ServiceException;
 use OCA\Mail\Service\SyncService;
@@ -102,10 +103,12 @@ class FoldersController extends Controller {
 	 * @param string $folderId
 	 * @param string $syncToken
 	 * @param int[] $uids
+	 *
 	 * @return JSONResponse
+	 * @throws ClientException
 	 * @throws ServiceException
 	 */
-	public function sync(int $accountId, string $folderId, array $uids = []): JSONResponse {
+	public function sync(int $accountId, string $folderId, array $uids = [], bool $init = false): JSONResponse {
 		$account = $this->accountService->find($this->currentUserId, $accountId);
 
 		if (empty($accountId) || empty($folderId) || !is_array($uids)) {
@@ -120,7 +123,7 @@ class FoldersController extends Controller {
 				array_map(function($uid) {
 					return (int) $uid;
 				}, $uids),
-				true
+				!$init
 			);
 		} catch (MailboxNotCachedException $e) {
 			return new JSONResponse(null, Http::STATUS_PRECONDITION_REQUIRED);
diff --git a/lib/Controller/MessagesController.php b/lib/Controller/MessagesController.php
index 7e075621e..9a6fc82de 100755
--- a/lib/Controller/MessagesController.php
+++ b/lib/Controller/MessagesController.php
@@ -33,6 +33,7 @@ namespace OCA\Mail\Controller;
 use Exception;
 use OCA\Mail\Contracts\IMailManager;
 use OCA\Mail\Contracts\IMailSearch;
+use OCA\Mail\Exception\ClientException;
 use OCA\Mail\Exception\ServiceException;
 use OCA\Mail\Http\AttachmentDownloadResponse;
 use OCA\Mail\Http\HtmlResponse;
@@ -40,7 +41,6 @@ use OCA\Mail\Model\IMAPMessage;
 use OCA\Mail\Service\AccountService;
 use OCA\Mail\Service\IMailBox;
 use OCA\Mail\Service\ItineraryService;
-use OCA\Mail\Service\SyncService;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Http;
@@ -70,9 +70,6 @@ class MessagesController extends Controller {
 	/** @var ItineraryService */
 	private $itineraryService;
 
-	/** @var SyncService */
-	private $syncService;
-
 	/** @var string */
 	private $currentUserId;
 
@@ -108,7 +105,6 @@ class MessagesController extends Controller {
 								IMailManager $mailManager,
 								IMailSearch $mailSearch,
 								ItineraryService $itineraryService,
-								SyncService $syncService,
 								string $UserId,
 								$userFolder,
 								ILogger $logger,
@@ -121,7 +117,6 @@ class MessagesController extends Controller {
 		$this->mailManager = $mailManager;
 		$this->mailSearch = $mailSearch;
 		$this->itineraryService = $itineraryService;
-		$this->syncService = $syncService;
 		$this->currentUserId = $UserId;
 		$this->userFolder = $userFolder;
 		$this->logger = $logger;
@@ -141,6 +136,7 @@ class MessagesController extends Controller {
 	 * @param string $filter
 	 *
 	 * @return JSONResponse
+	 * @throws ClientException
 	 * @throws ServiceException
 	 */
 	public function index(int $accountId, string $folderId, int $cursor = null, string $filter = null): JSONResponse {
@@ -150,11 +146,6 @@ class MessagesController extends Controller {
 			return new JSONResponse(null, Http::STATUS_FORBIDDEN);
 		}
 
-		$this->syncService->ensurePopulated(
-			$account,
-			base64_decode($folderId)
-		);
-
 		$this->logger->debug("loading messages of folder <$folderId>");
 
 		return new JSONResponse(
diff --git a/lib/Db/Mailbox.php b/lib/Db/Mailbox.php
index c09b3981a..772ad641e 100644
--- a/lib/Db/Mailbox.php
+++ b/lib/Db/Mailbox.php
@@ -127,4 +127,16 @@ class Mailbox extends Entity {
 		return $this->getName();
 	}
 
+	public function isCached(): bool {
+		return $this->getSyncNewToken() !== null
+			&& $this->getSyncChangedToken() !== null
+			&& $this->getSyncVanishedToken() !== null;
+	}
+
+	public function hasLocks(): bool {
+		return $this->getSyncNewLock() !== null
+			|| $this->getSyncChangedLock() !== null
+			|| $this->getSyncVanishedLock() !== null;
+	}
+
 }
diff --git a/lib/Db/MailboxMapper.php b/lib/Db/MailboxMapper.php
index 3acbde53b..6fcb527c5 100644
--- a/lib/Db/MailboxMapper.php
+++ b/lib/Db/MailboxMapper.php
@@ -24,7 +24,7 @@
 namespace OCA\Mail\Db;
 
 use OCA\Mail\Account;
-use OCA\Mail\Exception\ConcurrentSyncException;
+use OCA\Mail\Exception\MailboxLockedException;
 use OCA\Mail\Exception\ServiceException;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Db\MultipleObjectsReturnedException;
@@ -109,7 +109,7 @@ class MailboxMapper extends QBMapper {
 	}
 
 	/**
-	 * @throws ConcurrentSyncException
+	 * @throws MailboxLockedException
 	 */
 	private function lockForSync(Mailbox $mailbox, string $attr, ?int $lock): int {
 		$now = $this->timeFactory->getTime();
@@ -117,7 +117,7 @@ class MailboxMapper extends QBMapper {
 		if ($lock !== null
 			&& $lock > ($now - 5 * 60)) {
 			// Another process is syncing
-			throw new ConcurrentSyncException($mailbox->getId() . ' is already being synced');
+			throw MailboxLockedException::from($mailbox);
 		}
 
 		$query = $this->db->getQueryBuilder();
@@ -130,14 +130,14 @@ class MailboxMapper extends QBMapper {
 		if ($query->execute() === 0) {
 			// Another process just started syncing
 
-			throw new ConcurrentSyncException();
+			throw MailboxLockedException::from($mailbox);
 		}
 
 		return $now;
 	}
 
 	/**
-	 * @throws ConcurrentSyncException
+	 * @throws MailboxLockedException
 	 */
 	public function lockForNewSync(Mailbox $mailbox): void {
 		$mailbox->setSyncNewLock(
@@ -146,7 +146,7 @@ class MailboxMapper extends QBMapper {
 	}
 
 	/**
-	 * @throws ConcurrentSyncException
+	 * @throws MailboxLockedException
 	 */
 	public function lockForChangeSync(Mailbox $mailbox): void {
 		$mailbox->setSyncChangedLock(
@@ -155,7 +155,7 @@ class MailboxMapper extends QBMapper {
 	}
 
 	/**
-	 * @throws ConcurrentSyncException
+	 * @throws MailboxLockedException
 	 */
 	public function lockForVanishedSync(Mailbox $mailbox): void {
 		$mailbox->setSyncVanishedLock(
diff --git a/lib/Exception/ClientException.php b/lib/Exception/ClientException.php
index b2f212512..3619ee04d 100644
--- a/lib/Exception/ClientException.php
+++ b/lib/Exception/ClientException.php
@@ -27,7 +27,12 @@ declare(strict_types=1);
 namespace OCA\Mail\Exception;
 
 use Exception;
+use OCP\AppFramework\Http;
 
 class ClientException extends Exception {
 
+	public function getHttpCode(): int {
+		return Http::STATUS_BAD_REQUEST;
+	}
+
 }
diff --git a/lib/Exception/ConcurrentSyncException.php b/lib/Exception/MailboxLockedException.php
index 7cc91136d..8f66e50af 100644
--- a/lib/Exception/ConcurrentSyncException.php
+++ b/lib/Exception/MailboxLockedException.php
@@ -23,8 +23,17 @@
 
 namespace OCA\Mail\Exception;
 
-use Exception;
+use OCA\Mail\Db\Mailbox;
+use OCP\AppFramework\Http;
 
-class ConcurrentSyncException extends Exception {
+class MailboxLockedException extends ClientException {
+
+	public static function from(Mailbox $mailbox): self {
+		return new self($mailbox->getId() . ' is already being synced');
+	}
+
+	public function getHttpCode(): int {
+		return Http::STATUS_CONFLICT;
+	}
 
 }
diff --git a/lib/Exception/MailboxNotCachedException.php b/lib/Exception/MailboxNotCachedException.php
index 37242b1ad..728700c92 100644
--- a/lib/Exception/MailboxNotCachedException.php
+++ b/lib/Exception/MailboxNotCachedException.php
@@ -2,6 +2,12 @@
 
 namespace OCA\Mail\Exception;
 
-class MailboxNotCachedException extends ServiceException {
+use OCA\Mail\Db\Mailbox;
+
+class MailboxNotCachedException extends ClientException {
+
+	public static function from(Mailbox $mailbox): self {
+		return new self("mailbox {$mailbox->getId()} is not cached");
+	}
 
 }
diff --git a/lib/Http/JsonResponse.php b/lib/Http/JsonResponse.php
index c647e5089..8c5cc9396 100644
--- a/lib/Http/JsonResponse.php
+++ b/lib/Http/JsonResponse.php
@@ -25,6 +25,7 @@ declare(strict_types=1);
 
 namespace OCA\Mail\Http;
 
+use OCA\Mail\Exception\ClientException;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\JSONResponse as Base;
 use Throwable;
@@ -40,7 +41,7 @@ use function get_class;
 class JsonResponse extends Base {
 
 	public function __construct($data = [],
-						 int $statusCode = Http::STATUS_OK) {
+								int $statusCode = Http::STATUS_OK) {
 		parent::__construct($data, $statusCode);
 
 		$this->addHeader('x-mail-response', 'true');
@@ -68,6 +69,16 @@ class JsonResponse extends Base {
 		);
 	}
 
+	public static function failWith(ClientException $exception): self {
+		return self::fail(
+			[
+				'message' => $exception->getMessage(),
+				'type' => get_class($exception),
+			],
+			$exception->getHttpCode()
+		);
+	}
+
 	public static function error(string $message,
 								 int $status = Http::STATUS_INTERNAL_SERVER_ERROR,
 								 array $data = [],
diff --git a/lib/Http/Middleware/ErrorMiddleware.php b/lib/Http/Middleware/ErrorMiddleware.php
index 07fd4130e..3890faaa6 100644
--- a/lib/Http/Middleware/ErrorMiddleware.php
+++ b/lib/Http/Middleware/ErrorMiddleware.php
@@ -76,9 +76,7 @@ class ErrorMiddleware extends Middleware {
 		}
 
 		if ($exception instanceof ClientException) {
-			return JsonResponse::fail([
-				'message' => $exception->getMessage()
-			]);
+			return JsonResponse::failWith($exception);
 		}
 
 		if ($exception instanceof DoesNotExistException) {
diff --git a/lib/Service/Search/MailSearch.php b/lib/Service/Search/MailSearch.php
index 1927af9aa..290f1fb1c 100644
--- a/lib/Service/Search/MailSearch.php
+++ b/lib/Service/Search/MailSearch.php
@@ -30,6 +30,9 @@ use OCA\Mail\Db\Mailbox;
 use OCA\Mail\Db\MailboxMapper;
 use OCA\Mail\Db\Message;
 use OCA\Mail\Db\MessageMapper;
+use OCA\Mail\Exception\ClientException;
+use OCA\Mail\Exception\MailboxLockedException;
+use OCA\Mail\Exception\MailboxNotCachedException;
 use OCA\Mail\Exception\ServiceException;
 use OCA\Mail\IMAP\Search\Provider as ImapSearchProvider;
 use OCP\AppFramework\Db\DoesNotExistException;
@@ -71,6 +74,7 @@ class MailSearch implements IMailSearch {
 	 * @param string|null $cursor
 	 *
 	 * @return Message[]
+	 * @throws ClientException
 	 * @throws ServiceException
 	 */
 	public function findMessages(Account $account,
@@ -83,6 +87,13 @@ class MailSearch implements IMailSearch {
 			throw new ServiceException('Mailbox does not exist', 0, $e);
 		}
 
+		if ($mailbox->hasLocks()) {
+			throw MailboxLockedException::from($mailbox);
+		}
+		if (!$mailbox->isCached()) {
+			throw MailboxNotCachedException::from($mailbox);
+		}
+
 		$query = $this->filterStringParser->parse($filter);
 		if ($cursor !== null) {
 			$query->setCursor($cursor);
diff --git a/lib/Service/SyncService.php b/lib/Service/SyncService.php
index 6a084a575..b99caf1c3 100644
--- a/lib/Service/SyncService.php
+++ b/lib/Service/SyncService.php
@@ -32,7 +32,8 @@ use OCA\Mail\Db\MailboxMapper;
 use OCA\Mail\Db\Message;
 use OCA\Mail\Db\MessageMapper;
 use OCA\Mail\Db\MessageMapper as DatabaseMessageMapper;
-use OCA\Mail\Exception\ConcurrentSyncException;
+use OCA\Mail\Exception\ClientException;
+use OCA\Mail\Exception\MailboxLockedException;
 use OCA\Mail\Exception\MailboxNotCachedException;
 use OCA\Mail\Exception\ServiceException;
 use OCA\Mail\IMAP\IMAPClientFactory;
@@ -51,6 +52,57 @@ use function array_map;
 
 class SyncService {
 
+	/**
+	 * @param int[] $knownUids
+	 *
+	 * @throws ClientException
+	 * @throws ServiceException
+	 */
+	private function sync(Account $account,
+						  Mailbox $mailbox,
+						  int $criteria,
+						  array $knownUids = null,
+						  bool $force = false): Response {
+		if ($mailbox->getSelectable() === false) {
+			return new Response();
+		}
+
+		if ($criteria & Horde_Imap_Client::SYNC_NEWMSGSUIDS) {
+			$this->mailboxMapper->lockForNewSync($mailbox);
+		}
+		if ($criteria & Horde_Imap_Client::SYNC_FLAGSUIDS) {
+			$this->mailboxMapper->lockForChangeSync($mailbox);
+		}
+		if ($criteria & Horde_Imap_Client::SYNC_VANISHEDUIDS) {
+			$this->mailboxMapper->lockForVanishedSync($mailbox);
+		}
+
+		try {
+			if ($force
+				|| $mailbox->getSyncNewToken() === null
+				|| $mailbox->getSyncChangedToken() === null
+				|| $mailbox->getSyncVanishedToken() === null) {
+				$response = $this->runInitialSync($account, $mailbox);
+			} else {
+				$response = $this->runPartialSync($account, $mailbox, $criteria, $knownUids);
+			}
+		} catch (Throwable $e) {
+			throw new ServiceException('Sync failed', 0, $e);
+		} finally {
+			if ($criteria & Horde_Imap_Client::SYNC_VANISHEDUIDS) {
+				$this->mailboxMapper->unlockFromVanishedSync($mailbox);
+			}
+			if ($criteria & Horde_Imap_Client::SYNC_FLAGSUIDS) {
+				$this->mailboxMapper->unlockFromChangedSync($mailbox);
+			}
+			if ($criteria & Horde_Imap_Client::SYNC_NEWMSGSUIDS) {
+				$this->mailboxMapper->unlockFromNewSync($mailbox);
+			}
+		}
+
+		return $response;
+	}
+
 	/** @var DatabaseMessageMapper */
 	private $dbMapper;
 
@@ -94,6 +146,7 @@ class SyncService {
 	}
 
 	/**
+	 * @throws ClientException
 	 * @throws ServiceException
 	 */
 	public function syncAccount(Account $account,
@@ -113,6 +166,7 @@ class SyncService {
 	/**
 	 * @param int[] $knownUids
 	 *
+	 * @throws ClientException
 	 * @throws ServiceException
 	 */
 	public function syncMailbox(Account $account,
@@ -123,8 +177,8 @@ class SyncService {
 		try {
 			$mb = $this->mailboxMapper->find($account, $mailbox);
 
-			if ($partialOnly && $mb->getSyncNewToken() === null) {
-				throw new MailboxNotCachedException();
+			if ($partialOnly && !$mb->isCached()) {
+				throw MailboxNotCachedException::from($mb);
 			}
 
 			return $this->sync(
@@ -158,7 +212,7 @@ class SyncService {
 			$this->mailboxMapper->lockForVanishedSync($mb);
 
 			$this->runInitialSync($account, $mb);
-		} catch (ConcurrentSyncException $e) {
+		} catch (MailboxLockedException $e) {
 			// Fine, then we don't have to do it
 		} finally {
 			$this->mailboxMapper->unlockFromNewSync($mb);
@@ -168,60 +222,6 @@ class SyncService {
 	}
 
 	/**
-	 * @param int[] $knownUids
-	 *
-	 * @throws ServiceException
-	 */
-	private function sync(Account $account,
-						  Mailbox $mailbox,
-						  int $criteria,
-						  array $knownUids = null,
-						  bool $force = false): Response {
-		if ($mailbox->getSelectable() === false) {
-			return new Response();
-		}
-
-		try {
-			if ($criteria & Horde_Imap_Client::SYNC_NEWMSGSUIDS) {
-				$this->mailboxMapper->lockForNewSync($mailbox);
-			}
-			if ($criteria & Horde_Imap_Client::SYNC_FLAGSUIDS) {
-				$this->mailboxMapper->lockForChangeSync($mailbox);
-			}
-			if ($criteria & Horde_Imap_Client::SYNC_VANISHEDUIDS) {
-				$this->mailboxMapper->lockForVanishedSync($mailbox);
-			}
-		} catch (ConcurrentSyncException $e) {
-			throw new ServiceException('Another sync is in progress for ' . $mailbox->getId(), 0, $e);
-		}
-
-		try {
-			if ($force
-				|| $mailbox->getSyncNewToken() === null
-				|| $mailbox->getSyncChangedToken() === null
-				|| $mailbox->getSyncVanishedToken() === null) {
-				$response = $this->runInitialSync($account, $mailbox);
-			} else {
-				$response = $this->runPartialSync($account, $mailbox, $criteria, $knownUids);
-			}
-		} catch (Throwable $e) {
-			throw new ServiceException('Sync failed', 0, $e);
-		} finally {
-			if ($criteria & Horde_Imap_Client::SYNC_VANISHEDUIDS) {
-				$this->mailboxMapper->unlockFromVanishedSync($mailbox);
-			}
-			if ($criteria & Horde_Imap_Client::SYNC_FLAGSUIDS) {
-				$this->mailboxMapper->unlockFromChangedSync($mailbox);
-			}
-			if ($criteria & Horde_Imap_Client::SYNC_NEWMSGSUIDS) {
-				$this->mailboxMapper->unlockFromNewSync($mailbox);
-			}
-		}
-
-		return $response;
-	}
-
-	/**
 	 * @throws ServiceException
 	 */
 	private function runInitialSync(Account $account, Mailbox $mailbox): Response {