diff --git a/lib/Command/DebugAccount.php b/lib/Command/DebugAccount.php index 6ea36ce394..cfde2428d4 100644 --- a/lib/Command/DebugAccount.php +++ b/lib/Command/DebugAccount.php @@ -9,7 +9,9 @@ namespace OCA\Mail\Command; +use OCA\Mail\Account; use OCA\Mail\Service\AccountService; +use OCA\Mail\Support\DebugLogPathFactory; use OCP\AppFramework\Db\DoesNotExistException; use Psr\Log\LoggerInterface; use Symfony\Component\Console\Command\Command; @@ -26,6 +28,7 @@ final class DebugAccount extends Command { public function __construct( private AccountService $accountService, private LoggerInterface $logger, + private DebugLogPathFactory $debugLogPathFactory, ) { parent::__construct(); } @@ -48,7 +51,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $debug = false; try { - $account = $this->accountService->findById($accountId)->getMailAccount(); + $mailAccount = $this->accountService->findById($accountId)->getMailAccount(); } catch (DoesNotExistException $e) { $output->writeln("Account $accountId does not exist"); return 1; @@ -63,8 +66,23 @@ protected function execute(InputInterface $input, OutputInterface $output): int $debug = true; } - $account->setDebug($debug); - $this->accountService->save($account); + $mailAccount->setDebug($debug); + $this->accountService->save($mailAccount); + + $account = new Account($mailAccount); + + if ($debug) { + $output->writeln("Debug mode enabled for account $accountId"); + } else { + $output->writeln("Debug mode disabled for account $accountId"); + if ($this->debugLogPathFactory->isActive($account)) { + $output->writeln('Note: the system-wide "app.mail.debug" config value is enabled, so debug logging for this account is still active.'); + } + $output->writeln('Existing log files are not deleted automatically:'); + } + $output->writeln('IMAP log: ' . $this->debugLogPathFactory->buildPath($account, 'imap')); + $output->writeln('SMTP log: ' . $this->debugLogPathFactory->buildPath($account, 'smtp')); + $output->writeln('Sieve log: ' . $this->debugLogPathFactory->buildPath($account, 'sieve')); return 0; } diff --git a/lib/IMAP/IMAPClientFactory.php b/lib/IMAP/IMAPClientFactory.php index 40b86bfcca..f6919705d2 100644 --- a/lib/IMAP/IMAPClientFactory.php +++ b/lib/IMAP/IMAPClientFactory.php @@ -16,6 +16,7 @@ use OCA\Mail\Cache\HordeCacheFactory; use OCA\Mail\Events\BeforeImapClientCreated; use OCA\Mail\Exception\ServiceException; +use OCA\Mail\Support\DebugLogPathFactory; use OCP\AppFramework\Utility\ITimeFactory; use OCP\EventDispatcher\IEventDispatcher; use OCP\ICacheFactory; @@ -51,6 +52,7 @@ public function __construct( IEventDispatcher $eventDispatcher, ITimeFactory $timeFactory, private HordeCacheFactory $hordeCacheFactory, + private DebugLogPathFactory $debugLogPathFactory, ) { $this->crypto = $crypto; $this->config = $config; @@ -132,9 +134,9 @@ public function getClient(Account $account, bool $useCache = true): Horde_Imap_C 'backend' => $this->hordeCacheFactory->newCache($account), ]; } - if ($account->getMailAccount()->getDebug() || $this->config->getSystemValueBool('app.mail.debug')) { - $fn = "mail-{$account->getUserId()}-{$account->getId()}-imap.log"; - $params['debug'] = $this->config->getSystemValue('datadirectory') . '/' . $fn; + $debugLogPath = $this->debugLogPathFactory->getPath($account, 'imap'); + if ($debugLogPath !== null) { + $params['debug'] = $debugLogPath; } $client = new HordeImapClient($params, $this); diff --git a/lib/SMTP/SmtpClientFactory.php b/lib/SMTP/SmtpClientFactory.php index 7d8c34f2f5..b78e4fc32c 100644 --- a/lib/SMTP/SmtpClientFactory.php +++ b/lib/SMTP/SmtpClientFactory.php @@ -15,6 +15,7 @@ use Horde_Smtp_Password_Xoauth2; use OCA\Mail\Account; use OCA\Mail\Exception\ServiceException; +use OCA\Mail\Support\DebugLogPathFactory; use OCA\Mail\Support\HostNameFactory; use OCP\IConfig; use OCP\Security\ICrypto; @@ -30,6 +31,7 @@ public function __construct( IConfig $config, ICrypto $crypto, private HostNameFactory $hostNameFactory, + private DebugLogPathFactory $debugLogPathFactory, ) { $this->config = $config; $this->crypto = $crypto; @@ -80,9 +82,9 @@ public function create(Account $account): Horde_Mail_Transport { $decryptedAccessToken, ); } - if ($account->getMailAccount()->getDebug() || $this->config->getSystemValueBool('app.mail.debug')) { - $fn = "mail-{$account->getUserId()}-{$account->getId()}-smtp.log"; - $params['debug'] = $this->config->getSystemValue('datadirectory') . '/' . $fn; + $debugLogPath = $this->debugLogPathFactory->getPath($account, 'smtp'); + if ($debugLogPath !== null) { + $params['debug'] = $debugLogPath; } return new Horde_Mail_Transport_Smtphorde($params); } diff --git a/lib/Sieve/SieveClientFactory.php b/lib/Sieve/SieveClientFactory.php index f39867271b..8c971d82fe 100644 --- a/lib/Sieve/SieveClientFactory.php +++ b/lib/Sieve/SieveClientFactory.php @@ -11,6 +11,7 @@ use Horde\ManageSieve; use OCA\Mail\Account; +use OCA\Mail\Support\DebugLogPathFactory; use OCP\IConfig; use OCP\Security\ICrypto; @@ -19,7 +20,11 @@ class SieveClientFactory { private IConfig $config; private array $cache = []; - public function __construct(ICrypto $crypto, IConfig $config) { + public function __construct( + ICrypto $crypto, + IConfig $config, + private DebugLogPathFactory $debugLogPathFactory, + ) { $this->crypto = $crypto; $this->config = $config; } @@ -38,11 +43,7 @@ public function getClient(Account $account): ManageSieve { $password = $account->getMailAccount()->getInboundPassword(); } - if ($account->getMailAccount()->getDebug() || $this->config->getSystemValueBool('app.mail.debug')) { - $logFile = $this->config->getSystemValue('datadirectory') . "/mail-{$account->getUserId()}-{$account->getId()}-sieve.log"; - } else { - $logFile = null; - } + $logFile = $this->debugLogPathFactory->getPath($account, 'sieve'); $this->cache[$account->getId()] = $this->createClient( $account->getMailAccount()->getSieveHost(), diff --git a/lib/Support/DebugLogPathFactory.php b/lib/Support/DebugLogPathFactory.php new file mode 100644 index 0000000000..64f870ee9f --- /dev/null +++ b/lib/Support/DebugLogPathFactory.php @@ -0,0 +1,47 @@ +getMailAccount()->getDebug() || $this->config->getSystemValueBool('app.mail.debug'); + } + + /** + * @return string|null the log file path, or null if debug logging is not active + */ + public function getPath(Account $account, string $protocol): ?string { + return $this->isActive($account) ? $this->buildPath($account, $protocol) : null; + } + + /** + * Build the log file path regardless of whether debug logging is currently active. + * + * The account might not have been persisted yet (e.g. during the connectivity + * check when setting up a new account), in which case its id is not known yet. + */ + public function buildPath(Account $account, string $protocol): string { + $id = $account->getId() ?? 'new'; + $fn = "mail-{$account->getUserId()}-{$id}-{$protocol}.log"; + return $this->config->getSystemValue('datadirectory') . '/' . $fn; + } +} diff --git a/tests/Integration/Framework/Caching.php b/tests/Integration/Framework/Caching.php index d54d74714f..3efd46e1db 100644 --- a/tests/Integration/Framework/Caching.php +++ b/tests/Integration/Framework/Caching.php @@ -12,6 +12,7 @@ use OC\Memcache\Factory; use OCA\Mail\Cache\HordeCacheFactory; use OCA\Mail\IMAP\IMAPClientFactory; +use OCA\Mail\Support\DebugLogPathFactory; use OCP\AppFramework\Utility\ITimeFactory; use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; @@ -60,6 +61,7 @@ public static function getImapClientFactoryAndConfiguredCacheFactory(?ICrypto $c Server::get(IEventDispatcher::class), Server::get(ITimeFactory::class), Server::get(HordeCacheFactory::class), + new DebugLogPathFactory($config), ); return [$imapClient, $cacheFactory]; } diff --git a/tests/Integration/IMAP/IMAPClientFactoryTest.php b/tests/Integration/IMAP/IMAPClientFactoryTest.php index 0d60515db8..8578043cc9 100644 --- a/tests/Integration/IMAP/IMAPClientFactoryTest.php +++ b/tests/Integration/IMAP/IMAPClientFactoryTest.php @@ -19,6 +19,7 @@ use OCA\Mail\Db\MailAccount; use OCA\Mail\IMAP\HordeImapClient; use OCA\Mail\IMAP\IMAPClientFactory; +use OCA\Mail\Support\DebugLogPathFactory; use OCA\Mail\Tests\Integration\Framework\Caching; use OCP\AppFramework\Utility\ITimeFactory; use OCP\EventDispatcher\IEventDispatcher; @@ -62,6 +63,7 @@ protected function setUp(): void { $this->eventDispatcher, $this->timeFactory, $this->hordeCacheFactory, + new DebugLogPathFactory($this->config), ); } diff --git a/tests/Integration/Sieve/SieveClientFactoryTest.php b/tests/Integration/Sieve/SieveClientFactoryTest.php index 8218228251..8cb97b37e5 100644 --- a/tests/Integration/Sieve/SieveClientFactoryTest.php +++ b/tests/Integration/Sieve/SieveClientFactoryTest.php @@ -14,6 +14,7 @@ use OCA\Mail\Account; use OCA\Mail\Db\MailAccount; use OCA\Mail\Sieve\SieveClientFactory; +use OCA\Mail\Support\DebugLogPathFactory; use OCP\IConfig; use OCP\Security\ICrypto; use OCP\Server; @@ -46,7 +47,7 @@ protected function setUp(): void { ['app.mail.debug', false, false], ]); - $this->factory = new SieveClientFactory($this->crypto, $this->config); + $this->factory = new SieveClientFactory($this->crypto, $this->config, new DebugLogPathFactory($this->config)); } /** diff --git a/tests/Unit/Command/DebugAccountTest.php b/tests/Unit/Command/DebugAccountTest.php new file mode 100644 index 0000000000..96690bdb9a --- /dev/null +++ b/tests/Unit/Command/DebugAccountTest.php @@ -0,0 +1,137 @@ +accountService = $this->createMock(AccountService::class); + $this->config = $this->createMock(IConfig::class); + $this->config->method('getSystemValue') + ->with('datadirectory') + ->willReturn('/data'); + $this->config->method('getSystemValueBool') + ->with('app.mail.debug') + ->willReturn(false); + + $this->command = new DebugAccount( + $this->accountService, + $this->createMock(LoggerInterface::class), + new DebugLogPathFactory($this->config), + ); + } + + private function mockAccount(): MailAccount { + $mailAccount = new MailAccount(); + $mailAccount->setId(1); + $mailAccount->setUserId('bob'); + + $this->accountService->method('findById') + ->with(1) + ->willReturn(new Account($mailAccount)); + + return $mailAccount; + } + + public function testEnablesDebugAndPrintsLogPaths(): void { + $mailAccount = $this->mockAccount(); + $this->accountService->expects($this->once()) + ->method('save') + ->with($this->callback(fn (MailAccount $a) => $a->getDebug() === true)); + + $tester = new CommandTester($this->command); + $exitCode = $tester->execute(['account-id' => '1', '--on' => true]); + + $this->assertSame(0, $exitCode); + $display = $tester->getDisplay(); + $this->assertStringContainsString('Debug mode enabled', $display); + $this->assertStringContainsString('/data/mail-bob-1-imap.log', $display); + $this->assertStringContainsString('/data/mail-bob-1-smtp.log', $display); + $this->assertStringContainsString('/data/mail-bob-1-sieve.log', $display); + } + + public function testDisablesDebugAndPrintsLogPathsWithoutDeletingThem(): void { + $this->mockAccount(); + $this->accountService->expects($this->once()) + ->method('save') + ->with($this->callback(fn (MailAccount $a) => $a->getDebug() === false)); + + $tester = new CommandTester($this->command); + $exitCode = $tester->execute(['account-id' => '1', '--off' => true]); + + $this->assertSame(0, $exitCode); + $display = $tester->getDisplay(); + $this->assertStringContainsString('Debug mode disabled', $display); + $this->assertStringContainsString('not deleted automatically', $display); + $this->assertStringNotContainsString('still active', $display); + $this->assertStringContainsString('/data/mail-bob-1-imap.log', $display); + $this->assertStringContainsString('/data/mail-bob-1-smtp.log', $display); + $this->assertStringContainsString('/data/mail-bob-1-sieve.log', $display); + } + + public function testWarnsWhenDisablingButSystemWideDebugIsStillOn(): void { + $this->config = $this->createMock(IConfig::class); + $this->config->method('getSystemValue') + ->with('datadirectory') + ->willReturn('/data'); + $this->config->method('getSystemValueBool') + ->with('app.mail.debug') + ->willReturn(true); + $this->command = new DebugAccount( + $this->accountService, + $this->createMock(LoggerInterface::class), + new DebugLogPathFactory($this->config), + ); + $this->mockAccount(); + + $tester = new CommandTester($this->command); + $exitCode = $tester->execute(['account-id' => '1', '--off' => true]); + + $this->assertSame(0, $exitCode); + $this->assertStringContainsString('still active', $tester->getDisplay()); + } + + public function testRejectsOnAndOffTogether(): void { + $this->mockAccount(); + $this->accountService->expects($this->never())->method('save'); + + $tester = new CommandTester($this->command); + $exitCode = $tester->execute(['account-id' => '1', '--on' => true, '--off' => true]); + + $this->assertSame(1, $exitCode); + } + + public function testFailsWhenAccountDoesNotExist(): void { + $this->accountService->method('findById') + ->with(1) + ->willThrowException(new DoesNotExistException('not found')); + + $tester = new CommandTester($this->command); + $exitCode = $tester->execute(['account-id' => '1']); + + $this->assertSame(1, $exitCode); + } +} diff --git a/tests/Unit/SMTP/SmtpClientFactoryTest.php b/tests/Unit/SMTP/SmtpClientFactoryTest.php index 1e9e918874..40ec5c18c1 100644 --- a/tests/Unit/SMTP/SmtpClientFactoryTest.php +++ b/tests/Unit/SMTP/SmtpClientFactoryTest.php @@ -14,6 +14,7 @@ use OCA\Mail\Account; use OCA\Mail\Db\MailAccount; use OCA\Mail\SMTP\SmtpClientFactory; +use OCA\Mail\Support\DebugLogPathFactory; use OCA\Mail\Support\HostNameFactory; use OCP\IConfig; use OCP\Security\ICrypto; @@ -29,6 +30,9 @@ class SmtpClientFactoryTest extends TestCase { /** @var HostNameFactory|MockObject */ private $hostNameFactory; + /** @var DebugLogPathFactory|MockObject */ + private $debugLogPathFactory; + /** @var SmtpClientFactory */ private $factory; @@ -38,8 +42,11 @@ protected function setUp(): void { $this->config = $this->createMock(IConfig::class); $this->crypto = $this->createMock(ICrypto::class); $this->hostNameFactory = $this->createMock(HostNameFactory::class); + $this->debugLogPathFactory = $this->createMock(DebugLogPathFactory::class); + $this->debugLogPathFactory->method('getPath') + ->willReturn(null); - $this->factory = new SmtpClientFactory($this->config, $this->crypto, $this->hostNameFactory); + $this->factory = new SmtpClientFactory($this->config, $this->crypto, $this->hostNameFactory, $this->debugLogPathFactory); } public function testSmtpTransport() { diff --git a/tests/Unit/Support/DebugLogPathFactoryTest.php b/tests/Unit/Support/DebugLogPathFactoryTest.php new file mode 100644 index 0000000000..1f571431e0 --- /dev/null +++ b/tests/Unit/Support/DebugLogPathFactoryTest.php @@ -0,0 +1,91 @@ +config = $this->createMock(IConfig::class); + $this->config->method('getSystemValue') + ->with('datadirectory') + ->willReturn('/data'); + + $this->factory = new DebugLogPathFactory($this->config); + } + + private function account(bool $accountDebug, ?int $id = 5): Account { + $mailAccount = new MailAccount(); + if ($id !== null) { + $mailAccount->setId($id); + } + $mailAccount->setUserId('bob'); + $mailAccount->setDebug($accountDebug); + + return new Account($mailAccount); + } + + public function testIsActiveWhenAccountDebugIsOn(): void { + $this->config->method('getSystemValueBool') + ->with('app.mail.debug') + ->willReturn(false); + + $this->assertTrue($this->factory->isActive($this->account(true))); + } + + public function testIsActiveWhenSystemWideDebugIsOn(): void { + $this->config->method('getSystemValueBool') + ->with('app.mail.debug') + ->willReturn(true); + + $this->assertTrue($this->factory->isActive($this->account(false))); + } + + public function testNotActiveWhenNeitherFlagIsOn(): void { + $this->config->method('getSystemValueBool') + ->with('app.mail.debug') + ->willReturn(false); + + $this->assertFalse($this->factory->isActive($this->account(false))); + } + + public function testGetPathReturnsNullWhenNotActive(): void { + $this->config->method('getSystemValueBool') + ->with('app.mail.debug') + ->willReturn(false); + + $this->assertNull($this->factory->getPath($this->account(false), 'imap')); + } + + public function testGetPathReturnsPathWhenActive(): void { + $this->config->method('getSystemValueBool') + ->with('app.mail.debug') + ->willReturn(false); + + $this->assertSame('/data/mail-bob-5-imap.log', $this->factory->getPath($this->account(true), 'imap')); + } + + public function testBuildPathUsesPlaceholderWhenAccountHasNoIdYet(): void { + $this->assertSame('/data/mail-bob-new-smtp.log', $this->factory->buildPath($this->account(false, null), 'smtp')); + } + + public function testBuildPathUsesTheAccountId(): void { + $this->assertSame('/data/mail-bob-5-sieve.log', $this->factory->buildPath($this->account(false), 'sieve')); + } +}