feat(provisioning): Add support for Dovecot master user authentication #12442
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,7 +67,7 @@ public function validate(array $data): Provisioning { | |
| $exception->setField('imapHost', false); | ||
| } | ||
| if (!isset($data['imapPort']) || (int)$data['imapPort'] === 0) { | ||
| $exception->setField('imapPort', false); | ||
|
Contributor
Author
There was a problem hiding this comment. Unrelated, yet I fixed it while on it. It's extra commit; we can pull that out if necessary. |
||
| } | ||
| if (!isset($data['imapSslMode']) || $data['imapSslMode'] === '') { | ||
| $exception->setField('imapSslMode', false); | ||
|
|
@@ -92,6 +92,20 @@ public function validate(array $data): Provisioning { | |
| $exception->setField('ldapAliasesAttribute', false); | ||
| } | ||
|
|
||
| $masterPasswordEnabled = (bool)($data['masterPasswordEnabled'] ?? false); | ||
| $masterPassword = $data['masterPassword'] ?? ''; | ||
| $masterUser = $data['masterUser'] ?? ''; | ||
| $masterUserSeparator = $data['masterUserSeparator'] ?? ''; | ||
|
Contributor
Author
There was a problem hiding this comment. The initial PR had a check "if masterUser not empty, and masterUser is not the placeholder, and masterPasswordEnabled is false", then make masterPasswordEnabled required. I've reworked it to only show the inputs for password, username, and separator when the checkbox is toggled. Backend-wise, the validation should follow the checkbox. If master password enabled, then we need a password. If non-empty username is given, also the separator is needed. In addition, the current values are now cleared if the master password is disabled. |
||
|
|
||
| if ($masterPasswordEnabled) { | ||
| if ($masterPassword === '') { | ||
| $exception->setField('masterPassword', false); | ||
| } | ||
| if ($masterUser !== '' && $masterUserSeparator === '') { | ||
| $exception->setField('masterUserSeparator', false); | ||
| } | ||
| } | ||
|
Comment on lines
+95
to
+107
There was a problem hiding this comment.
|
||
|
|
||
| if (!empty($exception->getFields())) { | ||
| throw $exception; | ||
| } | ||
|
|
@@ -108,12 +122,6 @@ public function validate(array $data): Provisioning { | |
| $provisioning->setSmtpHost($data['smtpHost']); | ||
| $provisioning->setSmtpPort((int)$data['smtpPort']); | ||
| $provisioning->setSmtpSslMode($data['smtpSslMode']); | ||
| $provisioning->setSieveEnabled((bool)$data['sieveEnabled']); | ||
| $provisioning->setSieveHost($data['sieveHost'] ?? ''); | ||
| $provisioning->setSieveUser($data['sieveUser'] ?? ''); | ||
|
|
@@ -123,6 +131,20 @@ public function validate(array $data): Provisioning { | |
| $provisioning->setLdapAliasesProvisioning($ldapAliasesProvisioning); | ||
| $provisioning->setLdapAliasesAttribute($ldapAliasesAttribute); | ||
|
|
||
| if ($masterPasswordEnabled) { | ||
| $provisioning->setMasterPasswordEnabled(true); | ||
| if ($masterPassword !== Provisioning::MASTER_PASSWORD_PLACEHOLDER) { | ||
| $provisioning->setMasterPassword($masterPassword); | ||
| } | ||
| $provisioning->setMasterUser($masterUser); | ||
| $provisioning->setMasterUserSeparator($masterUserSeparator); | ||
| } else { | ||
| $provisioning->setMasterPasswordEnabled(false); | ||
| $provisioning->setMasterPassword(null); | ||
| $provisioning->setMasterUser(null); | ||
| $provisioning->setMasterUserSeparator(null); | ||
| } | ||
|
|
||
| return $provisioning; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
| use Horde_Imap_Client_Socket; | ||
| use OCA\Mail\Account; | ||
| use OCA\Mail\Cache\HordeCacheFactory; | ||
| use OCA\Mail\Db\ProvisioningMapper; | ||
| use OCA\Mail\Events\BeforeImapClientCreated; | ||
| use OCA\Mail\Exception\ServiceException; | ||
| use OCP\AppFramework\Utility\ITimeFactory; | ||
|
|
@@ -42,12 +43,15 @@ class IMAPClientFactory { | |
| private ITimeFactory $timeFactory; | ||
| private HordeCacheFactory $hordeCacheFactory; | ||
|
|
||
| public function __construct( | ||
| ICrypto $crypto, | ||
| IConfig $config, | ||
| ICacheFactory $cacheFactory, | ||
| IEventDispatcher $eventDispatcher, | ||
| ITimeFactory $timeFactory, | ||
| HordeCacheFactory $hordeCacheFactory, | ||
| private ProvisioningMapper $provisioningMapper | ||
| ) { | ||
| $this->crypto = $crypto; | ||
| $this->config = $config; | ||
| $this->cacheFactory = $cacheFactory; | ||
|
|
@@ -85,6 +89,16 @@ public function getClient(Account $account, bool $useCache = true): Horde_Imap_C | |
| $sslMode = false; | ||
| } | ||
|
|
||
| // Check for Dovecot master user authentication | ||
| $provisioningId = $account->getMailAccount()->getProvisioningId(); | ||
| if ($provisioningId !== null) { | ||
| $provisioning = $this->provisioningMapper->get($provisioningId); | ||
| if ($provisioning !== null && !empty($provisioning->getMasterUser())) { | ||
| $separator = $provisioning->getMasterUserSeparator() ?? '*'; | ||
|
Contributor
Author
There was a problem hiding this comment. @ChristophWurst if $provisioning = null, throw (like for oauth)? |
||
| $user = $user . $separator . $provisioning->getMasterUser(); | ||
|
Contributor
Author
There was a problem hiding this comment. @ChristophWurst wdyt about moving that logic to a trait? |
||
| } | ||
| } | ||
|
Comment on lines
+92
to
+100
There was a problem hiding this comment. The newly introduced Dovecot master-user behavior isn’t covered by tests in this file. Add a test case where the account has a provisioning with
Comment on lines
+92
to
+100
There was a problem hiding this comment. This adds an extra DB lookup (
Comment on lines
+92
to
+100
There was a problem hiding this comment. The new master-user username formatting logic is duplicated across IMAP/SMTP/Sieve factories. To reduce drift (e.g., default separator handling, enablement checks), consider extracting this into a shared helper (e.g., on |
||
|
|
||
| $params = [ | ||
| 'username' => $user, | ||
| 'password' => $decryptedPassword, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| /** | ||
| * SPDX-FileCopyrightText: 2026 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; | ||
|
|
||
| /** | ||
| * Add master_user and master_user_separator columns to mail_provisionings table | ||
| * for Dovecot Master User authentication support. | ||
| * | ||
| * @codeCoverageIgnore | ||
| */ | ||
| class Version5007Date20260124120000 extends SimpleMigrationStep { | ||
| /** | ||
| * @param IOutput $output | ||
| * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` | ||
| * @param array $options | ||
| * @return null|ISchemaWrapper | ||
| */ | ||
| #[\Override] | ||
| public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { | ||
| $schema = $schemaClosure(); | ||
|
|
||
| $provisioningTable = $schema->getTable('mail_provisionings'); | ||
| if (!$provisioningTable->hasColumn('master_user')) { | ||
| $provisioningTable->addColumn('master_user', Types::STRING, [ | ||
| 'notnull' => false, | ||
| 'length' => 256, | ||
| ]); | ||
| } | ||
| if (!$provisioningTable->hasColumn('master_user_separator')) { | ||
| $provisioningTable->addColumn('master_user_separator', Types::STRING, [ | ||
| 'notnull' => false, | ||
| 'length' => 8, | ||
| ]); | ||
| } | ||
|
|
||
| return $schema; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
| use Horde_Mail_Transport_Smtphorde; | ||
| use Horde_Smtp_Password_Xoauth2; | ||
| use OCA\Mail\Account; | ||
| use OCA\Mail\Db\ProvisioningMapper; | ||
| use OCA\Mail\Exception\ServiceException; | ||
| use OCA\Mail\Support\HostNameFactory; | ||
| use OCP\IConfig; | ||
|
|
@@ -29,12 +30,16 @@ class SmtpClientFactory { | |
| /** @var HostNameFactory */ | ||
| private $hostNameFactory; | ||
|
|
||
| private ProvisioningMapper $provisioningMapper; | ||
|
|
||
| public function __construct(IConfig $config, | ||
| ICrypto $crypto, | ||
| HostNameFactory $hostNameFactory, | ||
| ProvisioningMapper $provisioningMapper) { | ||
| $this->config = $config; | ||
| $this->crypto = $crypto; | ||
| $this->hostNameFactory = $hostNameFactory; | ||
| $this->provisioningMapper = $provisioningMapper; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -50,12 +55,25 @@ public function create(Account $account): Horde_Mail_Transport { | |
| $decryptedPassword = $this->crypto->decrypt($mailAccount->getOutboundPassword()); | ||
| } | ||
| $security = $mailAccount->getOutboundSslMode(); | ||
|
|
||
| $username = $mailAccount->getOutboundUser(); | ||
|
|
||
| // Check for Dovecot master user authentication | ||
| $provisioningId = $mailAccount->getProvisioningId(); | ||
| if ($provisioningId !== null) { | ||
| $provisioning = $this->provisioningMapper->get($provisioningId); | ||
| if ($provisioning !== null && !empty($provisioning->getMasterUser())) { | ||
| $separator = $provisioning->getMasterUserSeparator() ?? '*'; | ||
| $username = $username . $separator . $provisioning->getMasterUser(); | ||
| } | ||
| } | ||
|
Comment on lines
+61
to
+69
There was a problem hiding this comment. This adds an additional DB lookup (
Comment on lines
+61
to
+69
There was a problem hiding this comment. The newly introduced Dovecot master-user behavior isn’t covered by unit tests. Consider adding a test that sets a provisioning ID on the MailAccount, mocks |
||
|
|
||
| $params = [ | ||
| 'localhost' => $this->hostNameFactory->getHostName(), | ||
| 'host' => $mailAccount->getOutboundHost(), | ||
| 'password' => $decryptedPassword, | ||
| 'port' => $mailAccount->getOutboundPort(), | ||
| 'username' => $username, | ||
| 'secure' => $security === 'none' ? false : $security, | ||
| 'timeout' => (int)$this->config->getSystemValue('app.mail.smtp.timeout', 20), | ||
| 'context' => [ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,17 +11,20 @@ | |
|
|
||
| use Horde\ManageSieve; | ||
| use OCA\Mail\Account; | ||
| use OCA\Mail\Db\ProvisioningMapper; | ||
| use OCP\IConfig; | ||
| use OCP\Security\ICrypto; | ||
|
|
||
| class SieveClientFactory { | ||
| private ICrypto $crypto; | ||
| private IConfig $config; | ||
| private ProvisioningMapper $provisioningMapper; | ||
| private array $cache = []; | ||
|
|
||
| public function __construct(ICrypto $crypto, IConfig $config, ProvisioningMapper $provisioningMapper) { | ||
| $this->crypto = $crypto; | ||
| $this->config = $config; | ||
| $this->provisioningMapper = $provisioningMapper; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -38,6 +41,16 @@ public function getClient(Account $account): ManageSieve { | |
| $password = $account->getMailAccount()->getInboundPassword(); | ||
| } | ||
|
|
||
| // Check for Dovecot master user authentication | ||
| $provisioningId = $account->getMailAccount()->getProvisioningId(); | ||
| if ($provisioningId !== null) { | ||
| $provisioning = $this->provisioningMapper->get($provisioningId); | ||
| if ($provisioning !== null && !empty($provisioning->getMasterUser())) { | ||
| $separator = $provisioning->getMasterUserSeparator() ?? '*'; | ||
| $user = $user . $separator . $provisioning->getMasterUser(); | ||
| } | ||
| } | ||
|
Comment on lines
+44
to
+52
There was a problem hiding this comment. The newly introduced Dovecot master-user behavior isn’t covered by tests. Consider adding an integration/unit test that sets a provisioning ID on the MailAccount, mocks |
||
|
|
||
| if ($account->getMailAccount()->getDebug() || $this->config->getSystemValueBool('app.mail.debug')) { | ||
| $logFile = $this->config->getSystemValue('datadirectory') . '/mail-' . $account->getUserId() . '-' . $account->getId() . '-sieve.log'; | ||
| } else { | ||
|
|
||
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.
The initial PR flagged the master user as confidential (like the masterPasword). The username doesn't sound too critical to me, so I've dropped it.