feat: per account imap and smtp debugging #10301
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| /** | ||
| * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors | ||
| * SPDX-License-Identifier: AGPL-3.0-or-later | ||
| */ | ||
|
|
||
| namespace OCA\Mail\Command; | ||
|
|
||
| use OCA\Mail\Service\AccountService; | ||
| use OCP\AppFramework\Db\DoesNotExistException; | ||
| use Psr\Log\LoggerInterface; | ||
| use Symfony\Component\Console\Command\Command; | ||
| use Symfony\Component\Console\Input\InputArgument; | ||
| use Symfony\Component\Console\Input\InputInterface; | ||
| use Symfony\Component\Console\Input\InputOption; | ||
| use Symfony\Component\Console\Output\OutputInterface; | ||
|
|
||
| class DebugAccount extends Command { | ||
| protected const ARGUMENT_ACCOUNT_ID = 'account-id'; | ||
| protected const OPTION_IMAP_DEFAULT = 'imap'; | ||
| protected const OPTION_IMAP_FULL = 'imap-full'; | ||
| protected const OPTION_SMTP_DEFAULT = 'smtp'; | ||
|
There was a problem hiding this comment. What about Sieve? ;) It's using the SieveClientFactory There was a problem hiding this comment. I looked at the SieveClientFactory, it will need to be refactored first, so I thought it would be best to add sieve in a separate PR. |
||
|
|
||
| public function __construct( | ||
| private AccountService $accountService, | ||
| private LoggerInterface $logger, | ||
| ) { | ||
| parent::__construct(); | ||
| } | ||
|
|
||
| /** | ||
| * @return void | ||
| */ | ||
| protected function configure() { | ||
| $this->setName('mail:account:debug'); | ||
| $this->setDescription('Enable or Disable IMAP/SMTP debugging on a account'); | ||
| $this->addArgument(self::ARGUMENT_ACCOUNT_ID, InputArgument::REQUIRED); | ||
| $this->addOption(self::OPTION_IMAP_DEFAULT, null, InputOption::VALUE_NONE); | ||
| $this->addOption(self::OPTION_IMAP_FULL, null, InputOption::VALUE_NONE); | ||
| $this->addOption(self::OPTION_SMTP_DEFAULT, null, InputOption::VALUE_NONE); | ||
| } | ||
|
|
||
| protected function execute(InputInterface $input, OutputInterface $output): int { | ||
| $accountId = (int)$input->getArgument(self::ARGUMENT_ACCOUNT_ID); | ||
| $imapDefault = $input->getOption(self::OPTION_IMAP_DEFAULT); | ||
| $imapFull = $input->getOption(self::OPTION_IMAP_FULL); | ||
| $smtpDefault = $input->getOption(self::OPTION_SMTP_DEFAULT); | ||
| $debug = []; | ||
|
|
||
| try { | ||
| $account = $this->accountService->findById($accountId)->getMailAccount(); | ||
| } catch (DoesNotExistException $e) { | ||
| $output->writeln("<error>Account $accountId does not exist</error>"); | ||
| return 1; | ||
| } | ||
|
|
||
| if ($imapDefault) { | ||
| $debug[] = 'imap'; | ||
| } elseif ($imapFull) { | ||
| $debug[] = 'imap-full'; | ||
| } | ||
|
|
||
| if ($smtpDefault) { | ||
| $debug[] = 'smtp'; | ||
| } | ||
|
|
||
| $account->setDebug(implode('|', $debug)); | ||
| $this->accountService->save($account); | ||
|
|
||
| return 0; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,8 +117,16 @@ | |
| 'backend' => $this->hordeCacheFactory->newCache($account), | ||
| ]; | ||
| } | ||
| $debug = array_merge( | ||
|
There was a problem hiding this comment. If we decide to keep the protocol option, then this code should go into a small service or into the account object. classDiagram
class DebugHelper
DebugHelper : +debugImap(account) bool
DebugHelper : +debugSmtp(account) bool
DebugHelper : +debugSieve(account) bool
DebugHelper : +fileNameImap(account) string
DebugHelper : +fileNameSmtp(account) string
DebugHelper : +fileNameSieve(account) string
There was a problem hiding this comment. Sure, that can be done. Let see what the rest think. |
||
| explode('|', $this->config->getSystemValueString('MAIL_DEBUG')), | ||
|
There was a problem hiding this comment. I'm aware that getSystemValueString is used here as a workaround for reading environment variables, but I have some concerns:
There was a problem hiding this comment.
There was a problem hiding this comment.
I was referring to the symfony mailer configuration in Nextcloud using There was a problem hiding this comment.
In a previous version, you were using $_ENV / getenv, why did you withdraw this approach? I think that would be cleaner than hijacking SystemConfig 🏴☠️ There was a problem hiding this comment.
Ah, Okay, yeah we can still change it to mail_app_debug or anything else.
I agree it was cleaner but abandoned the approach, ironically because I thought it would get flagged during the review process, for one of two reasons,
|
||
| explode('|', $account->getDebug()) | ||
| ); | ||
| if (in_array('imap', $debug) || in_array('imap-full', $debug)) { | ||
| $fn = 'mail-' . $account->getUserId() . '-' . $account->getId() . '-imap.log'; | ||
| $params['debug'] = $this->config->getSystemValue('datadirectory') . '/' . $fn; | ||
| if (in_array('imap-full', $debug)) { | ||
| $params['debug_literal'] = true; | ||
| } | ||
| } | ||
|
|
||
| $client = new HordeImapClient($params); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| /** | ||
| * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors | ||
| * SPDX-License-Identifier: AGPL-3.0-or-later | ||
| */ | ||
|
|
||
| namespace OCA\Mail\Migration; | ||
|
|
||
| use Closure; | ||
| use OCP\DB\ISchemaWrapper; | ||
| use OCP\DB\Types; | ||
| use OCP\Migration\IOutput; | ||
| use OCP\Migration\SimpleMigrationStep; | ||
|
|
||
| class Version4100Date20241028000000 extends SimpleMigrationStep { | ||
|
|
||
| /** | ||
| * @param IOutput $output | ||
| * @param Closure(): ISchemaWrapper $schemaClosure | ||
| * @param array $options | ||
| * @return null|ISchemaWrapper | ||
| */ | ||
| public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { | ||
| $schema = $schemaClosure(); | ||
|
|
||
| $accountsTable = $schema->getTable('mail_accounts'); | ||
| if (!$accountsTable->hasColumn('debug')) { | ||
| $accountsTable->addColumn('debug', Types::STRING, [ | ||
| 'length' => 32, | ||
| 'notnull' => false, | ||
| 'default' => '', | ||
| ]); | ||
| } | ||
| return $schema; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,7 @@ public function createTestAccount(?string $userId = null) { | |
| $mailAccount->setOutboundUser('[email protected]'); | ||
| $mailAccount->setOutboundPassword(OC::$server->getCrypto()->encrypt('mypassword')); | ||
| $mailAccount->setOutboundSslMode('none'); | ||
| $mailAccount->setDebug('imap'); | ||
| $acc = $accountService->save($mailAccount); | ||
|
|
||
| /** @var MailboxSync $mbSync */ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To also print smtp and sieve like before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would kind of defeat the purpose of per account logging, as all account smtp transmissions would end up in the same file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NVM. I miss understood this, sure I can change this.