From ae76e3946dfcaf9f240cacee785b5b92acbb993c Mon Sep 17 00:00:00 2001 From: Ralf Lang Date: Wed, 26 Aug 2026 18:34:26 +0200 Subject: [PATCH] fix(mail): keep master SMTP credentials for CLI without session password Co-authored-by: Torben Dannhauer --- lib/Horde/Core/Factory/Mail.php | 49 +++++-- test/Unit/Factory/MailFactoryTest.php | 185 ++++++++++++++++++++++++++ 2 files changed, 223 insertions(+), 11 deletions(-) create mode 100644 test/Unit/Factory/MailFactoryTest.php diff --git a/lib/Horde/Core/Factory/Mail.php b/lib/Horde/Core/Factory/Mail.php index cc6c3114..eccee8b0 100644 --- a/lib/Horde/Core/Factory/Mail.php +++ b/lib/Horde/Core/Factory/Mail.php @@ -97,9 +97,21 @@ public function getConfig() /* Add username/password options now, regardless of current value of * 'auth'. Will remove in create() if final config doesn't require - * authentication. Need isAuthenticated() check since we may be - * running from CLI with 'user_admin' registry flag, which sets - * the authentication name but not the credentials. */ + * authentication. Need the isAuthenticated() check since we may be + * running from CLI with the 'user_admin' registry flag. That flag + * sets the authentication name but not the credentials. + * + * password_auth means "use the logged-in user's password for SMTP". + * CLI tools like horde-alarms authenticate by name only. They have + * no session password. So getAuthCredential('password') comes back + * empty. When password_auth wants that missing password, we derive + * neither the username nor the password and keep both master values. + * Pairing the auth username with the master password would mismatch + * the SMTP account. This keeps the fix in the mail factory. It does + * not fabricate a session. See the review on horde/Core#219. + * + * Problem originally reported and fixed by Torben Dannhauer + * in horde/Core#219. */ if (strcasecmp($transport, 'smtp') === 0) { if ($registry->isAuthenticated() && strlen((string) ($auth = $registry->getAuth()))) { @@ -116,26 +128,41 @@ public function getConfig() } // Don't set password when using XOAUTH2 } else { - // Hook returned regular credentials + // Hook returned regular credentials. Resolve the + // session password once. When password_auth wants + // it but the session has none, derive neither field + // and keep both master values. Pairing the auth + // username with the master password would mismatch + // the SMTP account. + $cred = $registry->getAuthCredential('password'); + $hasCred = strlen((string) $cred) > 0; + if (isset($smtp_creds['username'])) { $params['username'] = $smtp_creds['username']; - } elseif (!empty($params['username_auth'])) { + } elseif (!empty($params['username_auth']) + && (empty($params['password_auth']) || $hasCred)) { $params['username'] = $auth; } if (isset($smtp_creds['password'])) { $params['password'] = $smtp_creds['password']; - } elseif (!empty($params['password_auth'])) { - $params['password'] = $registry->getAuthCredential('password'); + } elseif (!empty($params['password_auth']) && $hasCred) { + $params['password'] = $cred; } } } catch (Horde_Exception_HookNotSet $e) { - // No hook defined, use default username/password - if (!empty($params['username_auth'])) { + // No hook defined, use default username/password. Same + // pairing rule as above: skip the auth username when + // password_auth wants a session password we don't have. + $cred = $registry->getAuthCredential('password'); + $hasCred = strlen((string) $cred) > 0; + + if (!empty($params['username_auth']) + && (empty($params['password_auth']) || $hasCred)) { $params['username'] = $auth; } - if (!empty($params['password_auth'])) { - $params['password'] = $registry->getAuthCredential('password'); + if (!empty($params['password_auth']) && $hasCred) { + $params['password'] = $cred; } } } diff --git a/test/Unit/Factory/MailFactoryTest.php b/test/Unit/Factory/MailFactoryTest.php new file mode 100644 index 00000000..77bc0b41 --- /dev/null +++ b/test/Unit/Factory/MailFactoryTest.php @@ -0,0 +1,185 @@ + + * @category Horde + * @copyright 2026 The Horde Project + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + * @package Core + */ + +namespace Horde\Core\Test\Unit\Factory; + +use Horde_Core_Factory_Mail; +use Horde_Core_Hooks; +use Horde_Exception_HookNotSet; +use Horde_Injector; +use Horde_Injector_TopLevel; +use Horde_Registry; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\TestCase; + +/** + * Ensures SMTP username_auth / password_auth fall back to configured master + * credentials when the session has an auth name but no password — the + * horde-alarms CLI user_admin case that previously sent empty SMTP passwords. + */ +#[CoversClass(Horde_Core_Factory_Mail::class)] +class MailFactoryTest extends TestCase +{ + private array $confBackup = []; + private $registryBackup; + + protected function setUp(): void + { + $this->confBackup = $GLOBALS['conf'] ?? []; + $this->registryBackup = $GLOBALS['registry'] ?? null; + } + + protected function tearDown(): void + { + $GLOBALS['conf'] = $this->confBackup; + if ($this->registryBackup === null) { + unset($GLOBALS['registry']); + } else { + $GLOBALS['registry'] = $this->registryBackup; + } + } + + #[Test] + public function testCliUserAdminKeepsMasterSmtpCredentials(): void + { + $this->configureMailer([ + 'username' => 'smtp-master@example.com', + 'password' => 'master-secret', + 'username_auth' => true, + 'password_auth' => true, + 'auth' => true, + ]); + + $registry = $this->createMock(Horde_Registry::class); + $registry->method('isAuthenticated')->willReturn(true); + $registry->method('getAuth')->willReturn('admin@example.com'); + $registry->method('getAuthCredential') + ->with('password') + ->willReturn(false); + $GLOBALS['registry'] = $registry; + + [$transport, $params] = $this->factoryWithoutSmtpHook()->getConfig(); + + $this->assertSame('smtp', $transport); + $this->assertSame('smtp-master@example.com', $params['username']); + $this->assertSame('master-secret', $params['password']); + $this->assertArrayNotHasKey('username_auth', $params); + $this->assertArrayNotHasKey('password_auth', $params); + } + + #[Test] + public function testSessionCredentialsReplaceMasterWhenPresent(): void + { + $this->configureMailer([ + 'username' => 'smtp-master@example.com', + 'password' => 'master-secret', + 'username_auth' => true, + 'password_auth' => true, + 'auth' => true, + ]); + + $registry = $this->createMock(Horde_Registry::class); + $registry->method('isAuthenticated')->willReturn(true); + $registry->method('getAuth')->willReturn('alice@example.com'); + $registry->method('getAuthCredential') + ->with('password') + ->willReturn('alice-secret'); + $GLOBALS['registry'] = $registry; + + [, $params] = $this->factoryWithoutSmtpHook()->getConfig(); + + $this->assertSame('alice@example.com', $params['username']); + $this->assertSame('alice-secret', $params['password']); + } + + #[Test] + public function testPasswordAuthAloneKeepsMasterUsername(): void + { + $this->configureMailer([ + 'username' => 'smtp-master@example.com', + 'password' => 'master-secret', + 'username_auth' => false, + 'password_auth' => true, + 'auth' => true, + ]); + + $registry = $this->createMock(Horde_Registry::class); + $registry->method('isAuthenticated')->willReturn(true); + $registry->method('getAuth')->willReturn('alice@example.com'); + $registry->method('getAuthCredential') + ->with('password') + ->willReturn('alice-secret'); + $GLOBALS['registry'] = $registry; + + [, $params] = $this->factoryWithoutSmtpHook()->getConfig(); + + $this->assertSame('smtp-master@example.com', $params['username']); + $this->assertSame('alice-secret', $params['password']); + } + + #[Test] + public function testEmptySessionPasswordDoesNotClearMasterPassword(): void + { + $this->configureMailer([ + 'username' => 'smtp-master@example.com', + 'password' => 'master-secret', + 'username_auth' => true, + 'password_auth' => true, + 'auth' => true, + ]); + + $registry = $this->createMock(Horde_Registry::class); + $registry->method('isAuthenticated')->willReturn(true); + $registry->method('getAuth')->willReturn('admin@example.com'); + $registry->method('getAuthCredential') + ->with('password') + ->willReturn(''); + $GLOBALS['registry'] = $registry; + + [, $params] = $this->factoryWithoutSmtpHook()->getConfig(); + + $this->assertSame('smtp-master@example.com', $params['username']); + $this->assertSame('master-secret', $params['password']); + } + + /** + * @param array $params + */ + private function configureMailer(array $params): void + { + $GLOBALS['conf'] = [ + 'mailer' => [ + 'type' => 'smtp', + 'params' => $params, + ], + ]; + } + + private function factoryWithoutSmtpHook(): Horde_Core_Factory_Mail + { + $hooks = $this->createMock(Horde_Core_Hooks::class); + $hooks->method('callHook') + ->with('smtp_credentials', 'horde', $this->anything()) + ->willThrowException(new Horde_Exception_HookNotSet()); + + $injector = new Horde_Injector(new Horde_Injector_TopLevel()); + $injector->setInstance('Horde_Core_Hooks', $hooks); + + return new Horde_Core_Factory_Mail($injector); + } +}