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 5 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
3 changes: 2 additions & 1 deletion appinfo/info.xml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ The rating depends on the installed text processing backend. See [the rating ove

Learn more about the Nextcloud Ethical AI Rating [in our blog](https://nextcloud.com/blog/nextcloud-ethical-ai-rating/).
]]></description>
<version>4.1.0-alpha.2</version>
<version>4.2.0-alpha.1</version>
<licence>agpl</licence>
<author homepage="https://github.com/ChristophWurst">Christoph Wurst</author>
<author homepage="https://github.com/GretaD">GretaD</author>
Expand Down 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
10 changes: 10 additions & 0 deletions lib/Account.php
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,16 @@ public function getUserId() {
return $this->account->getUserId();
}

/**
* @return array
*/
public function getDebug(): array {
if (!empty($this->account->getDebug())) {
return explode('|', $this->account->getDebug());
}
return [];
}

/**
* Set the quota percentage
* @param Quota $quota
Expand Down
75 changes: 75 additions & 0 deletions lib/Command/DebugAccount.php
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';
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 = [];

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

View check run for this annotation

Codecov / codecov/patch

lib/Command/DebugAccount.php#L46-L51

Added lines #L46 - L51 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 57 in lib/Command/DebugAccount.php

View check run for this annotation

Codecov / codecov/patch

lib/Command/DebugAccount.php#L54-L57

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

if ($imapDefault) {
$debug[] = 'imap';
} elseif ($imapFull) {
$debug[] = 'imap-full';

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

View check run for this annotation

Codecov / codecov/patch

lib/Command/DebugAccount.php#L60-L63

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

if ($smtpDefault) {
$debug[] = 'smtp';

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

View check run for this annotation

Codecov / codecov/patch

lib/Command/DebugAccount.php#L66-L67

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

$account->setDebug(implode('|', $debug));
$this->accountService->save($account);

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

View check run for this annotation

Codecov / codecov/patch

lib/Command/DebugAccount.php#L70-L71

Added lines #L70 - L71 were not covered by tests

return 0;

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

View check run for this annotation

Codecov / codecov/patch

lib/Command/DebugAccount.php#L73

Added line #L73 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 string getDebug()
* @method void setDebug(string $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 string $debug = '';

/**
* @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', 'string');
}

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
9 changes: 7 additions & 2 deletions lib/IMAP/IMAPClientFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,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 (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;

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

View check run for this annotation

Codecov / codecov/patch

lib/IMAP/IMAPClientFactory.php#L125

Added line #L125 was not covered by tests
}
}

$client = new HordeImapClient($params);
Expand Down
39 changes: 39 additions & 0 deletions lib/Migration/Version4100Date20241028000000.php
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();

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::STRING, [
'length' => 32,
'notnull' => true,
'default' => '',
]);

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

View check run for this annotation

Codecov / codecov/patch

lib/Migration/Version4100Date20241028000000.php#L29-L35

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

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

View check run for this annotation

Codecov / codecov/patch

lib/Migration/Version4100Date20241028000000.php#L37

Added line #L37 was not covered by tests
}
}
6 changes: 4 additions & 2 deletions lib/SMTP/SmtpClientFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,10 @@
$decryptedAccessToken,
);
}
if ($this->config->getSystemValue('debug', false)) {
$params['debug'] = $this->config->getSystemValue('datadirectory') . '/horde_smtp.log';
$debug = $account->getDebug();
if (in_array('smtp', $debug)) {
$fn = 'mail-' . $account->getUserId() . '-' . $account->getId() . '-smtp.log';
$params['debug'] = $this->config->getSystemValue('datadirectory') . '/' . $fn;

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

View check run for this annotation

Codecov / codecov/patch

lib/SMTP/SmtpClientFactory.php#L82-L83

Added lines #L82 - L83 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' => '',
], $a->toJson());
}

Expand Down Expand Up @@ -107,6 +108,7 @@ public function testMailAccountConstruct() {
'snoozeMailboxId' => null,
'searchBody' => false,
'outOfOfficeFollowsSystem' => false,
'debug' => '',
];
$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('imap');
$acc = $accountService->save($mailAccount);

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