Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: per account imap and smtp debugging #10301

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ jobs:
run: echo "SELECT * FROM mysql.slow_log WHERE sql_text LIKE '%oc_mail%' AND sql_text NOT LIKE '%information_schema%'" | mysql -h 127.0.0.1 -u root -pmy-secret-pw
- name: Print debug logs
if: ${{ always() }}
run: cat nextcloud/data/horde_*.log
run: cat nextcloud/data/mail-*-*-imap.log
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
run: cat nextcloud/data/mail-*-*-imap.log
run: cat nextcloud/data/mail*.log

To also print smtp and sieve like before?

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

- name: Report coverage
uses: codecov/codecov-action@b9fd7d16f6d7d1b5d2bec1a2887e65ceed900238 # v4.6.0
if: ${{ always() && matrix.db == 'mysql' }}
Expand Down
1 change: 1 addition & 0 deletions appinfo/info.xml
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ Learn more about the Nextcloud Ethical AI Rating [in our blog](https://nextcloud
<command>OCA\Mail\Command\CleanUp</command>
<command>OCA\Mail\Command\CreateAccount</command>
<command>OCA\Mail\Command\CreateTagMigrationJobEntry</command>
<command>OCA\Mail\Command\DebugAccount</command>
<command>OCA\Mail\Command\DeleteAccount</command>
<command>OCA\Mail\Command\DiagnoseAccount</command>
<command>OCA\Mail\Command\ExportAccount</command>
Expand Down
7 changes: 7 additions & 0 deletions lib/Account.php
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,13 @@ public function getUserId() {
return $this->account->getUserId();
}

/**
* @return int
*/
public function getDebug(): int {
return $this->account->getDebug();
}

/**
* Set the quota percentage
* @param Quota $quota
Expand Down
78 changes: 78 additions & 0 deletions lib/Command/DebugAccount.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<?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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about Sieve? ;)

It's using the SieveClientFactory

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Check warning on line 27 in lib/Command/DebugAccount.php

View check run for this annotation

Codecov / codecov/patch

lib/Command/DebugAccount.php#L27

Added line #L27 was not covered by tests
private AccountService $accountService,
private LoggerInterface $logger,
) {
parent::__construct();

Check warning on line 31 in lib/Command/DebugAccount.php

View check run for this annotation

Codecov / codecov/patch

lib/Command/DebugAccount.php#L31

Added line #L31 was not covered by tests
}

/**
* @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);

Check warning on line 43 in lib/Command/DebugAccount.php

View check run for this annotation

Codecov / codecov/patch

lib/Command/DebugAccount.php#L37-L43

Added lines #L37 - L43 were not covered by tests
}

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 = 0;
$debugImapDefault = 1;
$debugImapFull = 2;
$debugSmtpDefault = 16;

Check warning on line 54 in lib/Command/DebugAccount.php

View check run for this annotation

Codecov / codecov/patch

lib/Command/DebugAccount.php#L46-L54

Added lines #L46 - L54 were not covered by tests

try {
$account = $this->accountService->findById($accountId)->getMailAccount();
} catch (DoesNotExistException $e) {
$output->writeln("<error>Account $accountId does not exist</error>");
return 1;

Check warning on line 60 in lib/Command/DebugAccount.php

View check run for this annotation

Codecov / codecov/patch

lib/Command/DebugAccount.php#L57-L60

Added lines #L57 - L60 were not covered by tests
}

if ($imapDefault) {
$debug += $debugImapDefault;
} elseif ($imapFull) {
$debug += $debugImapFull;

Check warning on line 66 in lib/Command/DebugAccount.php

View check run for this annotation

Codecov / codecov/patch

lib/Command/DebugAccount.php#L63-L66

Added lines #L63 - L66 were not covered by tests
}

if ($smtpDefault) {
$debug += $debugSmtpDefault;

Check warning on line 70 in lib/Command/DebugAccount.php

View check run for this annotation

Codecov / codecov/patch

lib/Command/DebugAccount.php#L69-L70

Added lines #L69 - L70 were not covered by tests
}

$account->setDebug($debug);
$this->accountService->save($account);

Check warning on line 74 in lib/Command/DebugAccount.php

View check run for this annotation

Codecov / codecov/patch

lib/Command/DebugAccount.php#L73-L74

Added lines #L73 - L74 were not covered by tests

return 0;

Check warning on line 76 in lib/Command/DebugAccount.php

View check run for this annotation

Codecov / codecov/patch

lib/Command/DebugAccount.php#L76

Added line #L76 was not covered by tests
}
}
9 changes: 9 additions & 0 deletions lib/Db/MailAccount.php
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@
* @method void setSearchBody(bool $searchBody)
* @method bool|null getOooFollowsSystem()
* @method void setOooFollowsSystem(bool $oooFollowsSystem)
* @method int getDebug()
* @method void setDebug(int $debug)
*/
class MailAccount extends Entity {
public const SIGNATURE_MODE_PLAIN = 0;
Expand Down Expand Up @@ -183,6 +185,8 @@ class MailAccount extends Entity {
/** @var bool|null */
protected $oooFollowsSystem;

protected int $debug = 0;

/**
* @param array $params
*/
Expand Down Expand Up @@ -240,6 +244,9 @@ public function __construct(array $params = []) {
if (isset($params['outOfOfficeFollowsSystem'])) {
$this->setOutOfOfficeFollowsSystem($params['outOfOfficeFollowsSystem']);
}
if (isset($params['debug'])) {
$this->setDebug($params['debug']);
}

$this->addType('inboundPort', 'integer');
$this->addType('outboundPort', 'integer');
Expand All @@ -263,6 +270,7 @@ public function __construct(array $params = []) {
$this->addType('junkMailboxId', 'integer');
$this->addType('searchBody', 'boolean');
$this->addType('oooFollowsSystem', 'boolean');
$this->addType('debug', 'integer');
}

public function getOutOfOfficeFollowsSystem(): bool {
Expand Down Expand Up @@ -310,6 +318,7 @@ public function toJson() {
'junkMailboxId' => $this->getJunkMailboxId(),
'searchBody' => $this->getSearchBody(),
'outOfOfficeFollowsSystem' => $this->getOutOfOfficeFollowsSystem(),
'debug' => $this->getDebug(),
];

if (!is_null($this->getOutboundHost())) {
Expand Down
12 changes: 10 additions & 2 deletions lib/IMAP/IMAPClientFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@
private ITimeFactory $timeFactory;
private HordeCacheFactory $hordeCacheFactory;

private int $debugDefault = 1;
private int $debugFull = 2;

public function __construct(ICrypto $crypto,
IConfig $config,
ICacheFactory $cacheFactory,
Expand Down Expand Up @@ -117,8 +120,13 @@
'backend' => $this->hordeCacheFactory->newCache($account),
];
}
if ($this->config->getSystemValue('debug', false)) {
$params['debug'] = $this->config->getSystemValue('datadirectory') . '/horde_imap.log';
$debug = $account->getDebug();
if ($debug & $this->debugDefault || $debug & $this->debugFull) {
$fn = 'mail-' . $account->getUserId() . '-' . $account->getId() . '-imap.log';
$params['debug'] = $this->config->getSystemValue('datadirectory') . '/' . $fn;
if ($debug & $this->debugFull) {
$params['debug_literal'] = true;

Check warning on line 128 in lib/IMAP/IMAPClientFactory.php

View check run for this annotation

Codecov / codecov/patch

lib/IMAP/IMAPClientFactory.php#L128

Added line #L128 was not covered by tests
}
}

$client = new HordeImapClient($params);
Expand Down
38 changes: 38 additions & 0 deletions lib/Migration/Version4100Date20241028000000.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<?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();

Check warning on line 27 in lib/Migration/Version4100Date20241028000000.php

View check run for this annotation

Codecov / codecov/patch

lib/Migration/Version4100Date20241028000000.php#L26-L27

Added lines #L26 - L27 were not covered by tests

$accountsTable = $schema->getTable('mail_accounts');
if (!$accountsTable->hasColumn('debug')) {
$accountsTable->addColumn('debug', Types::SMALLINT, [
'notnull' => false,
'default' => 0,
]);

Check warning on line 34 in lib/Migration/Version4100Date20241028000000.php

View check run for this annotation

Codecov / codecov/patch

lib/Migration/Version4100Date20241028000000.php#L29-L34

Added lines #L29 - L34 were not covered by tests
}
return $schema;

Check warning on line 36 in lib/Migration/Version4100Date20241028000000.php

View check run for this annotation

Codecov / codecov/patch

lib/Migration/Version4100Date20241028000000.php#L36

Added line #L36 was not covered by tests
}
}
7 changes: 5 additions & 2 deletions lib/SMTP/SmtpClientFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
/** @var HostNameFactory */
private $hostNameFactory;

private int $debugDefault = 16;

public function __construct(IConfig $config,
ICrypto $crypto,
HostNameFactory $hostNameFactory) {
Expand Down Expand Up @@ -77,8 +79,9 @@
$decryptedAccessToken,
);
}
if ($this->config->getSystemValue('debug', false)) {
$params['debug'] = $this->config->getSystemValue('datadirectory') . '/horde_smtp.log';
if ($account->getDebug() & $this->debugDefault) {
$fn = 'mail-' . $account->getUserId() . '-' . $account->getId() . '-smtp.log';
$params['debug'] = $this->config->getSystemValue('datadirectory') . '/' . $fn;

Check warning on line 84 in lib/SMTP/SmtpClientFactory.php

View check run for this annotation

Codecov / codecov/patch

lib/SMTP/SmtpClientFactory.php#L83-L84

Added lines #L83 - L84 were not covered by tests
}
return new Horde_Mail_Transport_Smtphorde($params);
}
Expand Down
4 changes: 3 additions & 1 deletion tests/Integration/Db/MailAccountTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public function testToAPI() {
'editorMode' => 'html',
'provisioningId' => null,
'order' => 13,
'showSubscribedOnly' => null,
'showSubscribedOnly' => false,
'personalNamespace' => null,
'draftsMailboxId' => null,
'sentMailboxId' => null,
Expand All @@ -70,6 +70,7 @@ public function testToAPI() {
'snoozeMailboxId' => null,
'searchBody' => false,
'outOfOfficeFollowsSystem' => true,
'debug' => 0,
], $a->toJson());
}

Expand Down Expand Up @@ -107,6 +108,7 @@ public function testMailAccountConstruct() {
'snoozeMailboxId' => null,
'searchBody' => false,
'outOfOfficeFollowsSystem' => false,
'debug' => 0,
];
$a = new MailAccount($expected);
// TODO: fix inconsistency
Expand Down
1 change: 1 addition & 0 deletions tests/Integration/Framework/ImapTestAccount.php
Original file line number Diff line number Diff line change
Expand Up @@ -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(1);
$acc = $accountService->save($mailAccount);

/** @var MailboxSync $mbSync */
Expand Down
Loading