From c48691fca75a731e4acedaa9e20e295c6fedd88a Mon Sep 17 00:00:00 2001 From: FreddleSpl0it <75116288+FreddleSpl0it@users.noreply.github.com> Date: Mon, 9 Nov 2026 01:09:43 +0100 Subject: [PATCH] [Web][Rspamd] Keep the global archive switch in Redis only --- data/conf/rspamd/dynmaps/archive.php | 2 +- data/conf/rspamd/lua/rspamd.local.lua | 10 +++------- .../inc/lib/mailcow/Archive/ArchiveModule.php | 2 +- .../lib/mailcow/Archive/Redis/ArchiveState.php | 13 ++++++++++++- .../Archive/Repository/SettingsRepository.php | 2 -- .../mailcow/Archive/Service/JournalService.php | 5 ++--- .../Archive/Service/MailboxListService.php | 6 +++--- .../Archive/Service/SettingsService.php | 14 ++++++-------- .../mailcow/Archive/Service/StatusService.php | 2 +- tests/php/Archive/Model/MailboxTest.php | 16 +++++++--------- tests/php/Archive/Redis/ArchiveStateTest.php | 15 ++++++++++++++- .../php/Archive/Service/JournalServiceTest.php | 7 +++---- .../Archive/Service/MailboxListServiceTest.php | 13 ++++++------- .../Archive/Service/SettingsServiceTest.php | 18 ++++++++++++------ .../php/Archive/Service/StatusServiceTest.php | 8 +++++++- 15 files changed, 78 insertions(+), 55 deletions(-) diff --git a/data/conf/rspamd/dynmaps/archive.php b/data/conf/rspamd/dynmaps/archive.php index afb24900e..fc41f3e89 100644 --- a/data/conf/rspamd/dynmaps/archive.php +++ b/data/conf/rspamd/dynmaps/archive.php @@ -21,7 +21,7 @@ catch (PDOException $e) { http_response_code(501); exit; } -// Init Redis, only to mirror the switch rspamd reads +// Init Redis, it keeps the global switch; without it nothing is archived $redis = new Redis(); try { $redis->connect('redis-mailcow', 6379); diff --git a/data/conf/rspamd/lua/rspamd.local.lua b/data/conf/rspamd/lua/rspamd.local.lua index d75c513a1..b0903a372 100644 --- a/data/conf/rspamd/lua/rspamd.local.lua +++ b/data/conf/rspamd/lua/rspamd.local.lua @@ -1072,13 +1072,12 @@ rspamd_config:register_symbol({ end local function enabled_cb(err, data) - if not err and type(data) == 'string' and data == '0' then - return + if not err and type(data) == 'string' and data == '1' then + archive() end - archive() end - local ret = rspamd_redis_make_request(task, + rspamd_redis_make_request(task, redis_params, -- connect params 'ARCHIVE_ENABLED', -- hash key false, -- is write @@ -1086,8 +1085,5 @@ rspamd_config:register_symbol({ 'GET', -- command {'ARCHIVE_ENABLED'} -- arguments ) - if not ret then - archive() - end end }) diff --git a/data/web/inc/lib/mailcow/Archive/ArchiveModule.php b/data/web/inc/lib/mailcow/Archive/ArchiveModule.php index a9033d819..96896d598 100644 --- a/data/web/inc/lib/mailcow/Archive/ArchiveModule.php +++ b/data/web/inc/lib/mailcow/Archive/ArchiveModule.php @@ -51,7 +51,7 @@ class ArchiveModule $settings = new SettingsService($settingsRepo, $lookup, $state, $smtp, $context); $journal = new JournalService($settingsRepo, new OwnerResolver($lookup), new HeaderBuilder(), $smtp, $state, $context); $status = new StatusService($settingsRepo, $state, new LegacyQueue(), $context); - $mailboxes = new MailboxListService($lookup, $settingsRepo, $context); + $mailboxes = new MailboxListService($lookup, $state, $context); $api = new ArchiveApiController($settings, $status, $journal, $mailboxes, new ApiResponse(self::SECRET_KEYS)); return new self($settings, $journal, $status, $mailboxes, $api); } diff --git a/data/web/inc/lib/mailcow/Archive/Redis/ArchiveState.php b/data/web/inc/lib/mailcow/Archive/Redis/ArchiveState.php index 85ebe4c96..ea6846009 100644 --- a/data/web/inc/lib/mailcow/Archive/Redis/ArchiveState.php +++ b/data/web/inc/lib/mailcow/Archive/Redis/ArchiveState.php @@ -5,10 +5,11 @@ declare(strict_types=1); namespace Mailcow\Archive\Redis; use Mailcow\Archive\Model\Settings; +use RuntimeException; /** * Redis side of the archive: - * - ARCHIVE_ENABLED: the switch rspamd reads before it asks for owners, mirrored from the settings. + * - ARCHIVE_ENABLED: the global switch, kept here only; rspamd reads it before it asks for owners. * - ARCHIVE_DROPPED / ARCHIVE_DROPPED_LOG: copies that could not be handed to Postfix (rspamd.local.lua, release * hooks), counted and the last 100 listed. * - POSTFIX_MAILLOG: the Postfix log, read for the archive log: deliveries of the archive_smtp service and expired @@ -33,6 +34,16 @@ class ArchiveState $this->redis?->set(self::ENABLED, $enabled ? '1' : '0'); } + /** The global switch: on only while ARCHIVE_ENABLED is '1', as rspamd reads it (missing or unreadable is off) */ + public function enabled(): bool + { + try { + return $this->redis?->get(self::ENABLED) === '1'; + } catch (RuntimeException) { + return false; + } + } + /** A mail was not archived: its copy could not be handed to Postfix */ public function dropped(string $qid, string $reason, string $messageId = ''): void { diff --git a/data/web/inc/lib/mailcow/Archive/Repository/SettingsRepository.php b/data/web/inc/lib/mailcow/Archive/Repository/SettingsRepository.php index 1848dcced..4b375d0e3 100644 --- a/data/web/inc/lib/mailcow/Archive/Repository/SettingsRepository.php +++ b/data/web/inc/lib/mailcow/Archive/Repository/SettingsRepository.php @@ -19,7 +19,6 @@ class SettingsRepository extends Repository { $rows = $this->query("SELECT `name`, `value` FROM `archive_settings`")->fetchAll(PDO::FETCH_KEY_PAIR); $settings = new Settings(); - $settings->enabled = ($rows['enabled'] ?? '0') === '1'; $settings->address = (string)($rows['address'] ?? ''); $settings->host = (string)($rows['host'] ?? ''); $settings->port = (int)($rows['port'] ?? $settings->port); @@ -40,7 +39,6 @@ class SettingsRepository extends Repository public function save(Settings $settings): void { $values = [ - 'enabled' => $settings->enabled ? '1' : '0', 'address' => $settings->address, 'host' => $settings->host, 'port' => (string)$settings->port, diff --git a/data/web/inc/lib/mailcow/Archive/Service/JournalService.php b/data/web/inc/lib/mailcow/Archive/Service/JournalService.php index eb444e6b8..08de41556 100644 --- a/data/web/inc/lib/mailcow/Archive/Service/JournalService.php +++ b/data/web/inc/lib/mailcow/Archive/Service/JournalService.php @@ -48,8 +48,7 @@ class JournalService public function prepare(array $request): array { $settings = $this->settings->load(); - $this->state->setEnabled($settings->enabled); - if (!$settings->enabled || !$settings->canDeliver()) { + if (!$this->state->enabled() || !$settings->canDeliver()) { return ['archive' => false]; } $from = self::text($request['from'] ?? ''); @@ -75,7 +74,7 @@ class JournalService { try { $settings = $this->settings->load(); - if (!$settings->enabled || !$settings->canDeliver()) { + if (!$this->state->enabled() || !$settings->canDeliver()) { return false; } $resolved = $this->resolver->resolve($from, $rcpts, '', false); diff --git a/data/web/inc/lib/mailcow/Archive/Service/MailboxListService.php b/data/web/inc/lib/mailcow/Archive/Service/MailboxListService.php index ae4ecf47f..0603f2d1b 100644 --- a/data/web/inc/lib/mailcow/Archive/Service/MailboxListService.php +++ b/data/web/inc/lib/mailcow/Archive/Service/MailboxListService.php @@ -5,8 +5,8 @@ declare(strict_types=1); namespace Mailcow\Archive\Service; use Mailcow\Archive\Model\Mailbox; +use Mailcow\Archive\Redis\ArchiveState; use Mailcow\Archive\Repository\LookupRepository; -use Mailcow\Archive\Repository\SettingsRepository; use Mailcow\Core\AccessDenied; use Mailcow\Core\Context; @@ -16,7 +16,7 @@ use Mailcow\Core\Context; */ class MailboxListService { - public function __construct(private LookupRepository $lookup, private SettingsRepository $settings, private Context $context) + public function __construct(private LookupRepository $lookup, private ArchiveState $state, private Context $context) { } @@ -29,7 +29,7 @@ class MailboxListService public function archived(): array { $this->context->requireAdmin(false); - $enabled = $this->settings->load()->enabled; + $enabled = $this->state->enabled(); return [ 'enabled' => (int)$enabled, 'mailboxes' => $enabled ? array_map(fn(Mailbox $mailbox): array => $mailbox->toArray(), $this->lookup->archivedMailboxes()) : [], diff --git a/data/web/inc/lib/mailcow/Archive/Service/SettingsService.php b/data/web/inc/lib/mailcow/Archive/Service/SettingsService.php index 8edc70af7..6a8bbe636 100644 --- a/data/web/inc/lib/mailcow/Archive/Service/SettingsService.php +++ b/data/web/inc/lib/mailcow/Archive/Service/SettingsService.php @@ -14,7 +14,6 @@ use Mailcow\Core\AccessDenied; use Mailcow\Core\Context; use Mailcow\Core\Input; use Mailcow\Core\UserError; -use PDOException; /** * Admin use cases of the archive target. Read-only API keys may read the settings (never password or key). @@ -34,17 +33,15 @@ class SettingsService public function get(): Settings { $this->context->requireAdmin(false); - return $this->repo->load(); + $settings = $this->repo->load(); + $settings->enabled = $this->state->enabled(); + return $settings; } - /** Global switch for internal callers (page templates, mailbox details), no permission check; off while the table is missing */ + /** Global switch for internal callers (page templates, mailbox details), no permission check */ public function enabled(): bool { - try { - return $this->repo->load()->enabled; - } catch (PDOException) { - return false; - } + return $this->state->enabled(); } /** @@ -57,6 +54,7 @@ class SettingsService { $this->context->requireAdmin(); $settings = $this->repo->load(); + $settings->enabled = $this->state->enabled(); if (array_key_exists('address', $input)) { $address = strtolower((string)Input::text($input['address'])); if (!is_scalar($input['address']) || ($address !== '' && !filter_var($address, FILTER_VALIDATE_EMAIL))) { diff --git a/data/web/inc/lib/mailcow/Archive/Service/StatusService.php b/data/web/inc/lib/mailcow/Archive/Service/StatusService.php index 2439db9a9..c240c2482 100644 --- a/data/web/inc/lib/mailcow/Archive/Service/StatusService.php +++ b/data/web/inc/lib/mailcow/Archive/Service/StatusService.php @@ -58,7 +58,7 @@ class StatusService } } return [ - 'enabled' => (int)$settings->enabled, + 'enabled' => (int)$this->state->enabled(), 'configured' => (int)$settings->canDeliver(), 'queued' => count($copies), 'queued_bytes' => $bytes, diff --git a/tests/php/Archive/Model/MailboxTest.php b/tests/php/Archive/Model/MailboxTest.php index 49d0937d6..e20aff27d 100644 --- a/tests/php/Archive/Model/MailboxTest.php +++ b/tests/php/Archive/Model/MailboxTest.php @@ -5,13 +5,13 @@ declare(strict_types=1); namespace Mailcow\Tests\Archive\Model; use Mailcow\Archive\Model\Mailbox; +use Mailcow\Archive\Redis\ArchiveState; use Mailcow\Archive\Service\MailboxListService; -use Mailcow\Archive\Repository\SettingsRepository; -use Mailcow\Archive\Model\Settings; use Mailcow\Core\Context; use Mailcow\Core\UserError; use Mailcow\Tests\Support\FakeArchiveLookup; use Mailcow\Tests\Support\FakeContext; +use Mailcow\Tests\Support\FakeRedis; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; @@ -43,20 +43,18 @@ class MailboxTest extends TestCase { $lookup = new FakeArchiveLookup(); $lookup->mailboxes = ['b@firma.de' => ['inherit', 1], 'a@other.de' => ['1', 0], 'c@firma.de' => ['0', 1]]; - $settings = new Settings(); - $repo = $this->createMock(SettingsRepository::class); - $repo->method('load')->willReturnCallback(fn(): Settings => $settings); + $redis = new FakeRedis(); $readOnly = new Context('admin', 'API', [], false, 'ro'); - $this->assertSame(['enabled' => 0, 'mailboxes' => []], (new MailboxListService($lookup, $repo, $readOnly))->archived()); + $this->assertSame(['enabled' => 0, 'mailboxes' => []], (new MailboxListService($lookup, new ArchiveState($redis), $readOnly))->archived()); - $settings->enabled = true; + $redis->strings['ARCHIVE_ENABLED'] = '1'; $this->assertSame(['enabled' => 1, 'mailboxes' => [ ['username' => 'b@firma.de', 'domain' => 'firma.de', 'name' => '', 'active' => 1, 'source' => 'domain'], ['username' => 'a@other.de', 'domain' => 'other.de', 'name' => '', 'active' => 1, 'source' => 'mailbox'], - ]], (new MailboxListService($lookup, $repo, $readOnly))->archived()); + ]], (new MailboxListService($lookup, new ArchiveState($redis), $readOnly))->archived()); $this->expectException(UserError::class); - (new MailboxListService($lookup, $repo, FakeContext::domainAdmin()))->archived(); + (new MailboxListService($lookup, new ArchiveState($redis), FakeContext::domainAdmin()))->archived(); } } diff --git a/tests/php/Archive/Redis/ArchiveStateTest.php b/tests/php/Archive/Redis/ArchiveStateTest.php index e68088ba1..0fdcaf682 100644 --- a/tests/php/Archive/Redis/ArchiveStateTest.php +++ b/tests/php/Archive/Redis/ArchiveStateTest.php @@ -25,12 +25,25 @@ class ArchiveStateTest extends TestCase $this->redis->lPush('POSTFIX_MAILLOG', json_encode(['time' => $time, 'program' => $program, 'message' => $message])); } - public function testSwitchIsMirroredForRspamd(): void + public function testGlobalSwitchIsKeptHere(): void { + // no key yet: off + $this->assertFalse($this->state->enabled()); $this->state->setEnabled(true); $this->assertSame('1', $this->redis->strings['ARCHIVE_ENABLED']); + $this->assertTrue($this->state->enabled()); $this->state->setEnabled(false); $this->assertSame('0', $this->redis->strings['ARCHIVE_ENABLED']); + $this->assertFalse($this->state->enabled()); + // no connection or a failing one: off, rspamd cannot read the switch either + $this->assertFalse((new ArchiveState(null))->enabled()); + $failing = new class { + public function get(string $key): string + { + throw new \RuntimeException('connection lost'); + } + }; + $this->assertFalse((new ArchiveState($failing))->enabled()); } public function testDroppedCopiesAreCountedAndTheLast100Listed(): void diff --git a/tests/php/Archive/Service/JournalServiceTest.php b/tests/php/Archive/Service/JournalServiceTest.php index 50faa3099..31db81fb5 100644 --- a/tests/php/Archive/Service/JournalServiceTest.php +++ b/tests/php/Archive/Service/JournalServiceTest.php @@ -29,7 +29,6 @@ class JournalServiceTest extends TestCase protected function setUp(): void { $this->stored = new Settings(); - $this->stored->enabled = true; $this->stored->address = 'journal@archive.example'; $this->stored->signingKey = 'key'; $this->lookup = new FakeArchiveLookup(); @@ -38,6 +37,7 @@ class JournalServiceTest extends TestCase $this->lookup->aliases[$username] = $username; } $this->redis = new FakeRedis(); + $this->redis->strings['ARCHIVE_ENABLED'] = '1'; $this->smtp = new FakeSmtpClient(); } @@ -87,9 +87,8 @@ class JournalServiceTest extends TestCase $this->stored->signingKey = ''; $this->assertSame(['archive' => false], $this->service()->prepare(['from' => 'x@extern.example', 'rcpt' => ['anna@firma.de']])); - $this->stored->enabled = false; + $this->redis->strings['ARCHIVE_ENABLED'] = '0'; $this->assertSame(['archive' => false], $this->service()->prepare(['from' => 'x@extern.example', 'rcpt' => ['anna@firma.de']])); - $this->assertSame('0', $this->redis->strings['ARCHIVE_ENABLED']); $this->assertSame([], $this->smtp->submitted); } @@ -140,7 +139,7 @@ class JournalServiceTest extends TestCase public function testTestMail(): void { - $this->stored->enabled = false; + $this->redis->strings['ARCHIVE_ENABLED'] = '0'; $this->assertSame('Q1', $this->service(FakeContext::admin())->testMail()); diff --git a/tests/php/Archive/Service/MailboxListServiceTest.php b/tests/php/Archive/Service/MailboxListServiceTest.php index f1a11ebeb..cef77cbf6 100644 --- a/tests/php/Archive/Service/MailboxListServiceTest.php +++ b/tests/php/Archive/Service/MailboxListServiceTest.php @@ -4,21 +4,23 @@ declare(strict_types=1); namespace Mailcow\Tests\Archive\Service; -use Mailcow\Archive\Model\Settings; -use Mailcow\Archive\Repository\SettingsRepository; +use Mailcow\Archive\Redis\ArchiveState; use Mailcow\Archive\Service\MailboxListService; use Mailcow\Core\AccessDenied; use Mailcow\Core\Context; use Mailcow\Tests\Support\FakeArchiveLookup; use Mailcow\Tests\Support\FakeContext; +use Mailcow\Tests\Support\FakeRedis; use PHPUnit\Framework\TestCase; class MailboxListServiceTest extends TestCase { private FakeArchiveLookup $lookup; + private FakeRedis $redis; protected function setUp(): void { + $this->redis = new FakeRedis(); $this->lookup = new FakeArchiveLookup(); // username => [mailbox setting, domain default] $this->lookup->mailboxes = [ @@ -31,11 +33,8 @@ class MailboxListServiceTest extends TestCase private function service(bool $enabled, ?Context $context = null): MailboxListService { - $settings = new Settings(); - $settings->enabled = $enabled; - $repo = $this->createStub(SettingsRepository::class); - $repo->method('load')->willReturn($settings); - return new MailboxListService($this->lookup, $repo, $context ?? FakeContext::admin()); + $this->redis->strings['ARCHIVE_ENABLED'] = $enabled ? '1' : '0'; + return new MailboxListService($this->lookup, new ArchiveState($this->redis), $context ?? FakeContext::admin()); } public function testArchivedMailboxesWithTheirSource(): void diff --git a/tests/php/Archive/Service/SettingsServiceTest.php b/tests/php/Archive/Service/SettingsServiceTest.php index 2842d0ccb..3af90c2f8 100644 --- a/tests/php/Archive/Service/SettingsServiceTest.php +++ b/tests/php/Archive/Service/SettingsServiceTest.php @@ -15,7 +15,6 @@ use Mailcow\Tests\Support\FakeArchiveLookup; use Mailcow\Tests\Support\FakeContext; use Mailcow\Tests\Support\FakeRedis; use Mailcow\Tests\Support\FakeSmtpClient; -use PDOException; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -74,12 +73,19 @@ class SettingsServiceTest extends TestCase $this->assertSame('', $this->service()->save(['password' => '', 'password_remove' => '1'])->password); } - public function testEnabledIsOffWithoutTheTable(): void + public function testSwitchIsKeptInRedisOnly(): void { - $repo = $this->createMock(SettingsRepository::class); - $repo->method('load')->willThrowException(new PDOException('no such table')); - - $this->assertFalse((new SettingsService($repo, $this->lookup, new ArchiveState($this->redis), $this->smtp, FakeContext::admin()))->enabled()); + $this->assertFalse($this->service()->enabled()); + $this->assertSame(0, $this->service()->get()->toArray()['enabled']); + $this->service()->save(['enabled' => '1']); + $this->assertSame('1', $this->redis->strings['ARCHIVE_ENABLED']); + $this->assertSame(1, $this->service()->get()->toArray()['enabled']); + // saving other settings keeps the switch + $this->service()->save(['port' => '2525']); + $this->assertTrue($this->service()->enabled()); + // a lost Redis key shows as off, as rspamd sees it + unset($this->redis->strings['ARCHIVE_ENABLED']); + $this->assertFalse($this->service()->enabled()); } public function testAddressOutsideTheMailcowDomains(): void diff --git a/tests/php/Archive/Service/StatusServiceTest.php b/tests/php/Archive/Service/StatusServiceTest.php index f7458d6e7..535761488 100644 --- a/tests/php/Archive/Service/StatusServiceTest.php +++ b/tests/php/Archive/Service/StatusServiceTest.php @@ -24,12 +24,12 @@ class StatusServiceTest extends TestCase protected function setUp(): void { $this->redis = new FakeRedis(); + $this->redis->strings['ARCHIVE_ENABLED'] = '1'; } private function service(?Context $context = null): StatusService { $settings = new Settings(); - $settings->enabled = true; $settings->address = 'journal@archive.example'; $settings->profile = 'none'; $repo = $this->createStub(SettingsRepository::class); @@ -70,6 +70,12 @@ class StatusServiceTest extends TestCase ], $this->service()->status()); } + public function testSwitchFromRedis(): void + { + $this->redis->strings['ARCHIVE_ENABLED'] = '0'; + $this->assertSame(0, $this->service()->status()['enabled']); + } + public function testLogWithMessageIds(): void { (new ArchiveState($this->redis))->dropped('4XyZ1', 'owner lookup failed', 'm1@firma.de');