diff --git a/lib/BackgroundJob/SyncJob.php b/lib/BackgroundJob/SyncJob.php index e3c1676271..260b6cf66f 100644 --- a/lib/BackgroundJob/SyncJob.php +++ b/lib/BackgroundJob/SyncJob.php @@ -8,10 +8,9 @@ namespace OCA\Mail\BackgroundJob; -use Horde_Imap_Client_Exception; use OCA\Mail\AppInfo\Application; +use OCA\Mail\Exception\ImapAuthenticationFailedException; use OCA\Mail\Exception\IncompleteSyncException; -use OCA\Mail\Exception\ServiceException; use OCA\Mail\IMAP\MailboxSync; use OCA\Mail\Service\AccountService; use OCA\Mail\Service\Sync\ImapToDbSynchronizer; @@ -123,9 +122,7 @@ protected function run($argument) { 'exception' => $e, ]); } catch (Throwable $e) { - if ($e instanceof ServiceException - && $e->getPrevious() instanceof Horde_Imap_Client_Exception - && $e->getPrevious()->getCode() === Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED) { + if (ImapAuthenticationFailedException::findHordeAuthFailure($e) !== null) { $this->logger->info('Cron mail sync authentication failed for account {accountId}', [ 'accountId' => $accountId, 'exception' => $e, diff --git a/lib/Controller/MessagesController.php b/lib/Controller/MessagesController.php index 022964f703..60384e978b 100755 --- a/lib/Controller/MessagesController.php +++ b/lib/Controller/MessagesController.php @@ -297,6 +297,7 @@ public function getItineraries(int $id): JSONResponse { * @param int $id * @return JSONResponse */ + #[TrapError] public function getDkim(int $id): JSONResponse { if ($this->userId === null) { return new JSONResponse([], Http::STATUS_UNAUTHORIZED); diff --git a/lib/Exception/ImapAuthenticationFailedException.php b/lib/Exception/ImapAuthenticationFailedException.php new file mode 100644 index 0000000000..2c0a7e8e4d --- /dev/null +++ b/lib/Exception/ImapAuthenticationFailedException.php @@ -0,0 +1,65 @@ +reason = $reason; + } + + public static function fromHorde(Horde_Imap_Client_Exception $e): self { + $reason = $e->getMessage() === 'Too many auth attempts' + ? self::REASON_RATE_LIMITED + : self::REASON_AUTHENTICATION_FAILED; + return new self('IMAP authentication failed', $e->getCode(), $e, $reason); + } + + /** + * Find an IMAP authentication failure anywhere in an exception chain + */ + public static function findHordeAuthFailure(?Throwable $e): ?Horde_Imap_Client_Exception { + while ($e !== null) { + if ($e instanceof Horde_Imap_Client_Exception + && $e->getCode() === Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED) { + return $e; + } + $e = $e->getPrevious(); + } + return null; + } + + public function getReason(): string { + return $this->reason; + } + + #[\Override] + public function getHttpCode(): int { + return Http::STATUS_FAILED_DEPENDENCY; + } +} diff --git a/lib/Http/Middleware/ErrorMiddleware.php b/lib/Http/Middleware/ErrorMiddleware.php index 8e0999759d..6e507effed 100644 --- a/lib/Http/Middleware/ErrorMiddleware.php +++ b/lib/Http/Middleware/ErrorMiddleware.php @@ -12,6 +12,7 @@ use Exception; use Horde_Imap_Client_Exception; use OCA\Mail\Exception\ClientException; +use OCA\Mail\Exception\ImapAuthenticationFailedException; use OCA\Mail\Exception\NotImplemented; use OCA\Mail\Exception\ServiceException; use OCA\Mail\Http\JsonResponse; @@ -69,6 +70,23 @@ public function afterException($controller, $methodName, Exception $exception) { return JSONResponse::fail([], Http::STATUS_NOT_IMPLEMENTED); } + $authFailure = ImapAuthenticationFailedException::findHordeAuthFailure($exception); + if ($authFailure !== null) { + $wrapped = ImapAuthenticationFailedException::fromHorde($authFailure); + // The client is informed and can react, no need to flood the log + $this->logger->info('IMAP authentication failed: ' . $authFailure->getMessage(), [ + 'exception' => $exception, + ]); + return JsonResponse::fail( + [ + 'message' => $wrapped->getMessage(), + 'type' => get_class($wrapped), + 'reason' => $wrapped->getReason(), + ], + $wrapped->getHttpCode() + ); + } + $temporary = $this->isTemporaryException($exception); if ($temporary) { $this->logger->warning($exception->getMessage(), [ diff --git a/lib/IMAP/HordeImapClient.php b/lib/IMAP/HordeImapClient.php index b9701becce..6756e5de96 100644 --- a/lib/IMAP/HordeImapClient.php +++ b/lib/IMAP/HordeImapClient.php @@ -15,7 +15,6 @@ use OCP\AppFramework\Utility\ITimeFactory; use OCP\IMemcache; use OCP\IMemcacheTTL; -use function floor; /** * "Decorator" around Horde's IMAP client to add auth error rate limiting. @@ -62,7 +61,14 @@ public function login() { } } - private const RATE_LIMIT_WINDOW = 3 * 60 * 60; + /** Failed attempts are forgotten after this many seconds */ + private const FAILURE_TTL = 3 * 60 * 60; + /** Unthrottled attempts before the leaky bucket engages */ + private const THROTTLE_THRESHOLD = 3; + /** Throttled attempts allowed at the fast interval before slowing down */ + private const FAST_RETRIES = 3; + private const FAST_RETRY_INTERVAL = 30; + private const SLOW_RETRY_INTERVAL = 5 * 60; protected function imapLogin() { $result = parent::_login(); @@ -76,27 +82,45 @@ protected function _login() { return $this->imapLogin(); } - $now = $this->timeFactory->getTime(); - $window = floor($now / self::RATE_LIMIT_WINDOW); - $cacheKey = $this->hash . (string)$window; + $failureKey = $this->hash . '_failures'; + $attemptKey = $this->hash . '_last_attempt'; - $counter = $this->rateLimiterCache->get($cacheKey); - if ($counter !== null && $counter >= 3) { - // Enough errors. Let's fail without involving IMAP - throw new Horde_Imap_Client_Exception( - 'Too many auth attempts', - Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED - ); + $failures = (int)$this->rateLimiterCache->get($failureKey); + if ($failures >= self::THROTTLE_THRESHOLD) { + // Leaky bucket instead of a hard block: keep allowing one real + // attempt per interval so transient server-side failures heal on + // their own — a few quick retries first, then a slow trickle. + $interval = $failures < self::THROTTLE_THRESHOLD + self::FAST_RETRIES + ? self::FAST_RETRY_INTERVAL + : self::SLOW_RETRY_INTERVAL; + $now = $this->timeFactory->getTime(); + $lastAttempt = (int)$this->rateLimiterCache->get($attemptKey); + if ($now - $lastAttempt < $interval) { + // Enough errors. Let's fail without involving IMAP + throw new Horde_Imap_Client_Exception( + 'Too many auth attempts', + Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED + ); + } + // Not atomic: a parallel burst may pass together, but the next + // interval only opens once the timestamp is stored + $this->rateLimiterCache->set($attemptKey, $now, self::FAILURE_TTL); } try { - return $this->imapLogin(); + $result = $this->imapLogin(); + if ($failures > 0) { + // The credentials work (again): reset the throttle state + $this->rateLimiterCache->remove($failureKey); + $this->rateLimiterCache->remove($attemptKey); + } + return $result; } catch (Horde_Imap_Client_Exception $e) { if ($e->getCode() === Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED && in_array($e->getMessage(), ['Authentication failed.', 'Mail server denied authentication.'], true)) { - $this->rateLimiterCache->inc($cacheKey); + $this->rateLimiterCache->inc($failureKey); if ($this->rateLimiterCache instanceof IMemcacheTTL) { - $this->rateLimiterCache->setTTL($cacheKey, self::RATE_LIMIT_WINDOW); + $this->rateLimiterCache->setTTL($failureKey, self::FAILURE_TTL); } } throw $e; diff --git a/lib/Service/Sync/ImapToDbSynchronizer.php b/lib/Service/Sync/ImapToDbSynchronizer.php index 2cf929bc43..1065c8be43 100644 --- a/lib/Service/Sync/ImapToDbSynchronizer.php +++ b/lib/Service/Sync/ImapToDbSynchronizer.php @@ -186,7 +186,15 @@ public function sync(Account $account, return $rebuildThreads; } - $client->login(); // Need to login before fetching capabilities. + try { + $client->login(); // Need to login before fetching capabilities. + } catch (Horde_Imap_Client_Exception $e) { + throw new ServiceException( + 'Sync failed for account ' . $account->getId() . ' mailbox ' . $mailbox->getId() . ': ' . $e->getMessage(), + 0, + $e, + ); + } // There is no partial sync when using QRESYNC. As per RFC the client will always pull // all changes. This is a cheap operation when using QRESYNC as the server keeps track diff --git a/src/components/Mailbox.vue b/src/components/Mailbox.vue index 77dd09c082..edab2383f3 100644 --- a/src/components/Mailbox.vue +++ b/src/components/Mailbox.vue @@ -233,7 +233,7 @@ export default { this.error = false logger.debug(`syncing folder ${this.mailbox.databaseId} (${this.query}) during cache initalization`) - this.sync(true) + return this.sync(true) .then(() => { this.loadingCacheInitialization = false diff --git a/src/components/ThreadEnvelope.vue b/src/components/ThreadEnvelope.vue index 7484e623aa..82f87de93c 100644 --- a/src/components/ThreadEnvelope.vue +++ b/src/components/ThreadEnvelope.vue @@ -872,22 +872,32 @@ export default { this.handleThreadScrolling() }) } catch (error) { - this.error = error + const account = this.mainStore.getAccount(this.envelope.accountId) + if (account?.error) { + // The account cannot authenticate against IMAP: show an + // actionable message instead of a generic 'Not found' + this.error = new Error(t('mail', 'Authentication failed for account {email}. Please update your credentials in the account settings.', { email: account.emailAddress })) + } else { + this.error = error + } this.loading = Loading.Done logger.error('Could not fetch message', { error }) } + // Skip IMAP-backed background lookups while the account cannot authenticate + const accountHasAuthError = !!this.mainStore.getAccount(this.envelope.accountId)?.error + // Fetch itineraries if they haven't been included in the message data - if (this.message && !this.message.itineraries) { + if (this.message && !this.message.itineraries && !accountHasAuthError) { this.fetchItineraries() } // Fetch dkim - if (this.message && this.message.dkimValid === undefined) { + if (this.message && this.message.dkimValid === undefined && !accountHasAuthError) { this.fetchDkim() } // Fetch smart replies - if (this.enabledFreePrompt && this.message && !['trash', 'junk'].includes(this.mailbox.specialRole) && !this.showFollowUpHeader) { + if (this.enabledFreePrompt && this.message && !accountHasAuthError && !['trash', 'junk'].includes(this.mailbox.specialRole) && !this.showFollowUpHeader) { try { this.smartReplies = await smartReply(this.envelope.databaseId) } catch (error) { diff --git a/src/errors/ImapAuthenticationFailedError.js b/src/errors/ImapAuthenticationFailedError.js new file mode 100644 index 0000000000..61d1d71364 --- /dev/null +++ b/src/errors/ImapAuthenticationFailedError.js @@ -0,0 +1,16 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +export default class ImapAuthenticationFailedError extends Error { + constructor(message) { + super(message) + this.name = ImapAuthenticationFailedError.getName() + this.message = message + } + + static getName() { + return 'ImapAuthenticationFailedError' + } +} diff --git a/src/errors/convert.js b/src/errors/convert.js index e52921417c..fceb010c81 100644 --- a/src/errors/convert.js +++ b/src/errors/convert.js @@ -4,6 +4,7 @@ */ import CouldNotConnectError from './CouldNotConnectError.js' +import ImapAuthenticationFailedError from './ImapAuthenticationFailedError.js' import MailboxLockedError from './MailboxLockedError.js' import MailboxNotCachedError from './MailboxNotCachedError.js' import ManageSieveError from './ManageSieveError.js' @@ -19,6 +20,7 @@ const map = { 'OCA\\Mail\\Exception\\SentMailboxNotSetException': NoSentMailboxConfiguredError, 'OCA\\Mail\\Exception\\TrashMailboxNotSetException': NoTrashMailboxConfiguredError, 'OCA\\Mail\\Exception\\CouldNotConnectException': CouldNotConnectError, + 'OCA\\Mail\\Exception\\ImapAuthenticationFailedException': ImapAuthenticationFailedError, 'OCA\\Mail\\Exception\\ManyRecipientsException': ManyRecipientsError, 'Horde\\ManageSieve\\Exception': ManageSieveError, } @@ -44,5 +46,8 @@ export function convertAxiosError(axiosError) { return axiosError } - return new map[response.data.data.type](response.data.data.message) + const error = new map[response.data.data.type](response.data.data.message) + // Attach the structured payload so handlers can inspect extra fields (e.g. reason) + error.data = response.data.data + return error } diff --git a/src/store/mainStore/actions.js b/src/store/mainStore/actions.js index 825551d174..9a03e8d53f 100644 --- a/src/store/mainStore/actions.js +++ b/src/store/mainStore/actions.js @@ -3,7 +3,7 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ -import { showError, showWarning, TOAST_DEFAULT_TIMEOUT } from '@nextcloud/dialogs' +import { showError, showWarning, TOAST_DEFAULT_TIMEOUT, TOAST_PERMANENT_TIMEOUT } from '@nextcloud/dialogs' import { translate as t } from '@nextcloud/l10n' import DOMPurify from 'dompurify' import escapeRegExp from 'lodash/fp/escapeRegExp.js' @@ -32,6 +32,7 @@ import { where, } from 'ramda' import Vue from 'vue' +import ImapAuthenticationFailedError from '../../errors/ImapAuthenticationFailedError.js' import MailboxLockedError from '../../errors/MailboxLockedError.js' import { matchError } from '../../errors/match.js' import SyncIncompleteError from '../../errors/SyncIncompleteError.js' @@ -142,6 +143,49 @@ import useOutboxStore from '../outboxStore.js' const sliceToPage = slice(0, PAGE_SIZE) +// Accounts flagged with an authentication error are excluded from background +// sync, but probe requests are let through so transient failures (e.g. a mail +// server hiccup or an OAuth token refresh problem) heal without a page reload. +// Mirrors the backend rate limiter: a few quick retries, then a slow trickle. +const AUTH_ERROR_FAST_RETRIES = 3 +const AUTH_ERROR_FAST_RETRY_INTERVAL = 30 * 1000 +const AUTH_ERROR_SLOW_RETRY_INTERVAL = 5 * 60 * 1000 +const AUTH_ERROR_PROBE_WINDOW = 15 * 1000 +const authErrorProbes = new Map() + +/** + * @param {object} account the account to check + * @return {boolean} whether syncing the account should be skipped + */ +function shouldSkipAuthErroredAccount(account) { + if (!account.error) { + return false + } + + const now = Date.now() + const probe = authErrorProbes.get(account.id) + if (!probe) { + // Unknown state (e.g. flagged at page load): probe right away + authErrorProbes.set(account.id, { openedAt: now, count: 1 }) + return false + } + if (now - probe.openedAt < AUTH_ERROR_PROBE_WINDOW) { + // A probe was just allowed — let the related requests through + return false + } + + const interval = probe.count < AUTH_ERROR_FAST_RETRIES + ? AUTH_ERROR_FAST_RETRY_INTERVAL + : AUTH_ERROR_SLOW_RETRY_INTERVAL + if (now - probe.openedAt < interval) { + return true + } + + // Open a new probe window + authErrorProbes.set(account.id, { openedAt: now, count: probe.count + 1 }) + return false +} + const findIndividualMailboxes = curry((getMailboxes, specialRole) => pipe( filter(complement(prop('isUnified'))), map(prop('id')), @@ -266,6 +310,7 @@ export default function mainStoreActions() { const account = await updateAccount(config) logger.debug('account updated', { account }) this.editAccountMutation({ ...account, error: false }) + authErrorProbes.delete(account.id) await this.syncMailboxesForAccount(this.accountsUnmapped[account.id]) return account }) @@ -900,9 +945,19 @@ export default function mainStoreActions() { const mailbox = this.getMailbox(mailboxId) - // Skip superfluous requests if using passwordless authentication. They will fail anyway. + // Skip superfluous requests if using passwordless authentication or if the + // account has an authentication error. They will fail anyway. const passwordIsUnavailable = this.getPreference('password-is-unavailable', false) - const isDisabled = (account) => passwordIsUnavailable && !!account.provisioningId + const isDisabled = (account) => (passwordIsUnavailable && !!account.provisioningId) || shouldSkipAuthErroredAccount(account) + + if (init && (mailbox.isUnified || mailbox.isPriorityInbox) + && this.getAccounts.some((account) => !account.isUnified && !!account.error)) { + // An initial sync must fail loudly when an account is skipped for an + // authentication error. Filtering it out silently would resolve the + // fan-out below successfully without ever caching the account's + // mailboxes, sending Mailbox.vue's initializeCache into a request loop. + throw new ImapAuthenticationFailedError('An account has an authentication error') + } if (mailbox.isUnified) { return Promise.all(this.getAccounts @@ -930,6 +985,19 @@ export default function mainStoreActions() { })) } + const account = this.getAccount(mailbox.accountId) + if (account && !account.isUnified && isDisabled(account)) { + logger.debug(`Skipping sync of mailbox ${mailboxId}: account ${mailbox.accountId} is disabled or has an authentication error`) + if (init) { + // An initial sync must fail loudly. Resolving would make + // Mailbox.vue poll for envelopes that can never be cached. + throw account.error + ? new ImapAuthenticationFailedError('Account has an authentication error') + : new Error('Account sync is disabled') + } + return [] + } + const ids = this.getEnvelopes(mailboxId, query).map((env) => env.databaseId) const lastTimestamp = this.getPreference('sort-order') === 'newest' ? null : this.getEnvelopes(mailboxId, query)[0]?.dateInt logger.debug(`mailbox sync of ${mailboxId} (${query}) has ${ids.length} known IDs. ${lastTimestamp} is the last known message timestamp`, { mailbox }) @@ -937,6 +1005,14 @@ export default function mainStoreActions() { .then((syncData) => { logger.debug(`mailbox ${mailboxId} (${query}) synchronized, ${syncData.newMessages.length} new, ${syncData.changedMessages.length} changed and ${syncData.vanishedMessages.length} vanished messages`) + const syncedAccount = this.getAccount(mailbox.accountId) + if (syncedAccount && syncedAccount.error) { + // A probe request went through: the account works again + logger.info(`Account ${mailbox.accountId} can connect again, resuming sync`) + this.editAccountMutation({ ...syncedAccount, error: false }) + authErrorProbes.delete(mailbox.accountId) + } + const unifiedMailbox = this.getUnifiedMailbox(mailbox.specialRole) this.addEnvelopesMutation({ @@ -980,6 +1056,27 @@ export default function mainStoreActions() { init, }) }, + [ImapAuthenticationFailedError.getName()]: (error) => { + const failedAccount = this.getAccount(mailbox.accountId) + if (failedAccount && !failedAccount.error) { + logger.warn(`Authentication failed for account ${mailbox.accountId} (reason: ${error.data?.reason ?? 'unknown'}), pausing sync for this account`, { error }) + this.editAccountMutation({ ...failedAccount, error: true }) + // The failed request counts as attempt zero; the first + // probe is due one fast interval later + authErrorProbes.set(mailbox.accountId, { openedAt: Date.now(), count: 0 }) + showError( + t('mail', 'Authentication failed for account {email}. Please update your credentials in the account settings.', { email: failedAccount.emailAddress }), + { timeout: TOAST_PERMANENT_TIMEOUT }, + ) + } + + if (init) { + // Mirror MailboxLockedError: an initial sync must fail + // loudly or Mailbox.vue keeps polling for envelopes + // that can never be cached. + throw error + } + }, [MailboxLockedError.getName()]: (error) => { if (init) { logger.info('Sync failed because the mailbox is locked, stopping here because this is an initial sync', { error }) @@ -1002,9 +1099,10 @@ export default function mainStoreActions() { }) }, async syncInboxes() { - // Skip superfluous requests if using passwordless authentication. They will fail anyway. + // Skip superfluous requests if using passwordless authentication or if the + // account has an authentication error. They will fail anyway. const passwordIsUnavailable = this.getPreference('password-is-unavailable', false) - const isDisabled = (account) => passwordIsUnavailable && !!account.provisioningId + const isDisabled = (account) => (passwordIsUnavailable && !!account.provisioningId) || shouldSkipAuthErroredAccount(account) return handleHttpAuthErrors(async () => { const results = await Promise.all(this.getAccounts diff --git a/tests/Integration/IMAP/IMAPClientFactoryTest.php b/tests/Integration/IMAP/IMAPClientFactoryTest.php index 0d60515db8..443565ab95 100644 --- a/tests/Integration/IMAP/IMAPClientFactoryTest.php +++ b/tests/Integration/IMAP/IMAPClientFactoryTest.php @@ -139,7 +139,9 @@ public function testRateLimiting(): void { $client = $imapClientFactory->getClient($account); self::assertInstanceOf(HordeImapClient::class, $client); - foreach ([1, 2, 3] as $attempts) { + // Three free attempts arm the throttle, the fourth is the immediately + // allowed leaky-bucket retry - all four reach IMAP and fail there + foreach ([1, 2, 3, 4] as $attempts) { try { $client->login(); $this->fail("Login #$attempts should cause an exception"); @@ -151,6 +153,8 @@ public function testRateLimiting(): void { // 🔥 This is fine 🔥 } } + // The fifth attempt within the same 30 second interval is throttled + // locally without contacting IMAP $this->expectException(Horde_Imap_Client_Exception::class); $this->expectExceptionCode(Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED); $this->expectExceptionMessage('Too many auth attempts'); diff --git a/tests/Unit/Http/Middleware/ErrorMiddlewareTest.php b/tests/Unit/Http/Middleware/ErrorMiddlewareTest.php index a0902fa1ab..6a52ff6e0d 100644 --- a/tests/Unit/Http/Middleware/ErrorMiddlewareTest.php +++ b/tests/Unit/Http/Middleware/ErrorMiddlewareTest.php @@ -12,6 +12,7 @@ use ChristophWurst\Nextcloud\Testing\TestCase; use Exception; use Horde_Imap_Client_Exception; +use OCA\Mail\Exception\ImapAuthenticationFailedException; use OCA\Mail\Exception\NotImplemented; use OCA\Mail\Exception\ServiceException; use OCA\Mail\Http\Middleware\ErrorMiddleware; @@ -129,6 +130,56 @@ public function foo() { $this->assertInstanceOf(JSONResponse::class, $response); } + public function imapAuthFailureData(): array { + return [ + 'raw horde exception' => [ + new Horde_Imap_Client_Exception('Authentication failed.', Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED), + ImapAuthenticationFailedException::REASON_AUTHENTICATION_FAILED, + ], + 'rate limiter engaged' => [ + new Horde_Imap_Client_Exception('Too many auth attempts', Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED), + ImapAuthenticationFailedException::REASON_RATE_LIMITED, + ], + 'wrapped in service exception' => [ + new ServiceException('Could not load message', 0, new Horde_Imap_Client_Exception('Authentication failed.', Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED)), + ImapAuthenticationFailedException::REASON_AUTHENTICATION_FAILED, + ], + 'doubly wrapped' => [ + new ServiceException('Sync failed', 0, new ServiceException('login failed', 0, new Horde_Imap_Client_Exception('Authentication failed.', Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED))), + ImapAuthenticationFailedException::REASON_AUTHENTICATION_FAILED, + ], + ]; + } + + /** + * @dataProvider imapAuthFailureData + */ + public function testTranslatesImapAuthenticationFailures(Throwable $ex, string $expectedReason): void { + $request = $this->createStub(IRequest::class); + $controller = new class($request) extends Controller { + public function __construct(IRequest $request) { + parent::__construct('myapp', $request); + } + + #[TrapError] + public function foo() { + } + }; + $this->logger->expects($this->once()) + ->method('info'); + $this->logger->expects($this->never()) + ->method('error'); + + $response = $this->middleware->afterException($controller, 'foo', $ex); + + $this->assertInstanceOf(JSONResponse::class, $response); + $this->assertSame(Http::STATUS_FAILED_DEPENDENCY, $response->getStatus()); + $data = $response->getData(); + $this->assertSame('fail', $data['status']); + $this->assertSame(ImapAuthenticationFailedException::class, $data['data']['type']); + $this->assertSame($expectedReason, $data['data']['reason']); + } + public function temporaryExceptionsData(): array { return [ [new ServiceException('not temporary'), false], diff --git a/tests/Unit/IMAP/HordeImapClientTest.php b/tests/Unit/IMAP/HordeImapClientTest.php index fe7668f721..d05583be61 100644 --- a/tests/Unit/IMAP/HordeImapClientTest.php +++ b/tests/Unit/IMAP/HordeImapClientTest.php @@ -11,6 +11,7 @@ use ChristophWurst\Nextcloud\Testing\TestCase; use Horde_Imap_Client_Exception; +use OC\Memcache\ArrayCache; use OCA\Mail\IMAP\HordeImapClient; use OCP\AppFramework\Utility\ITimeFactory; use OCP\IMemcache; @@ -24,41 +25,151 @@ interface IMemcacheWithTTL extends IMemcache, IMemcacheTTL { * Testable subclass that stubs out the real IMAP connection. */ class TestableHordeImapClient extends HordeImapClient { + /** @var list<'ok'|'fail'> */ + public array $outcomes = []; + public int $realAttempts = 0; + public function __construct() { // Skip Horde constructor — we only test rate limiter logic. } + public function doLogin() { + return $this->_login(); + } + protected function imapLogin() { + $this->realAttempts++; + $outcome = array_shift($this->outcomes) ?? 'fail'; + if ($outcome === 'ok') { + return true; + } throw new Horde_Imap_Client_Exception( - 'Authentication failed.', + $outcome === 'denied' ? 'Mail server denied authentication.' : 'Authentication failed.', Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED, ); } } class HordeImapClientTest extends TestCase { - private IMemcacheWithTTL|MockObject $cache; + private ArrayCache $cache; private ITimeFactory|MockObject $timeFactory; private TestableHordeImapClient $client; + private int $now = 100000; protected function setUp(): void { parent::setUp(); - $this->cache = $this->createMock(IMemcacheWithTTL::class); + $this->cache = new ArrayCache('horde_test'); $this->timeFactory = $this->createMock(ITimeFactory::class); + $this->timeFactory->method('getTime') + ->willReturnCallback(fn () => $this->now); $this->client = new TestableHordeImapClient(); $this->client->enableRateLimiter($this->cache, 'testhash', $this->timeFactory); } + private function loginExpectingAuthFailure(): void { + try { + $this->client->doLogin(); + $this->fail('login should have failed with an authentication error'); + } catch (Horde_Imap_Client_Exception $e) { + $this->assertSame('Authentication failed.', $e->getMessage()); + } + } + + private function loginExpectingThrottle(): void { + try { + $this->client->doLogin(); + $this->fail('login should have been throttled'); + } catch (Horde_Imap_Client_Exception $e) { + $this->assertSame('Too many auth attempts', $e->getMessage()); + } + } + + public function testFirstThreeFailuresAreNotThrottled(): void { + $this->loginExpectingAuthFailure(); + $this->loginExpectingAuthFailure(); + $this->loginExpectingAuthFailure(); + + $this->assertSame(3, $this->client->realAttempts); + } + + public function testFastRetriesEveryThirtySecondsThenSlowTrickle(): void { + // Arm the throttle with three failures + $this->loginExpectingAuthFailure(); + $this->loginExpectingAuthFailure(); + $this->loginExpectingAuthFailure(); + + // One real attempt is allowed right away (failure #4)… + $this->loginExpectingAuthFailure(); + $this->assertSame(4, $this->client->realAttempts); + + // …but not a second one within the same 30 seconds + $this->now += 10; + $this->loginExpectingThrottle(); + $this->assertSame(4, $this->client->realAttempts); + + // Failures #5 and #6 at 30 second spacing + $this->now += 20; + $this->loginExpectingAuthFailure(); + $this->now += 30; + $this->loginExpectingAuthFailure(); + $this->assertSame(6, $this->client->realAttempts); + + // From now on only one attempt per 5 minutes + $this->now += 30; + $this->loginExpectingThrottle(); + $this->now += 240; + $this->loginExpectingThrottle(); + $this->assertSame(6, $this->client->realAttempts); + + // 300 seconds after the last real attempt the next one is allowed + $this->now += 30; + $this->loginExpectingAuthFailure(); + $this->assertSame(7, $this->client->realAttempts); + } + + public function testSuccessfulLoginResetsTheThrottle(): void { + $this->loginExpectingAuthFailure(); + $this->loginExpectingAuthFailure(); + $this->loginExpectingAuthFailure(); + + // The immediate throttled attempt succeeds (server recovered) + $this->client->outcomes = ['ok', 'ok', 'ok']; + $this->assertTrue($this->client->doLogin()); + + // State is cleared: subsequent logins are not throttled at all + $this->assertTrue($this->client->doLogin()); + $this->assertTrue($this->client->doLogin()); + $this->assertSame(6, $this->client->realAttempts); + } + + public function testDeniedAuthenticationAlsoCounts(): void { + /** @var IMemcacheWithTTL|MockObject $cache */ + $cache = $this->createMock(IMemcacheWithTTL::class); + $cache->method('get')->willReturn(null); + $cache->expects($this->once())->method('inc')->with('testhash_failures'); + + $client = new TestableHordeImapClient(); + $client->outcomes = ['denied']; + $client->enableRateLimiter($cache, 'testhash', $this->timeFactory); + + $this->expectException(Horde_Imap_Client_Exception::class); + $this->expectExceptionMessage('Mail server denied authentication.'); + $client->doLogin(); + } + public function testSetsTtlOnAuthFailure(): void { - $this->timeFactory->method('getTime')->willReturn(100000); - $expectedKey = 'testhash9'; - $this->cache->method('get')->with($expectedKey)->willReturn(null); - $this->cache->expects($this->once())->method('inc')->with($expectedKey); - $this->cache->expects($this->once())->method('setTTL')->with($expectedKey, 3 * 60 * 60); + /** @var IMemcacheWithTTL|MockObject $cache */ + $cache = $this->createMock(IMemcacheWithTTL::class); + $cache->method('get')->willReturn(null); + $cache->expects($this->once())->method('inc')->with('testhash_failures'); + $cache->expects($this->once())->method('setTTL')->with('testhash_failures', 3 * 60 * 60); + + $client = new TestableHordeImapClient(); + $client->enableRateLimiter($cache, 'testhash', $this->timeFactory); $this->expectException(Horde_Imap_Client_Exception::class); - $this->client->login(); + $client->doLogin(); } }