From 3e4d5472a71362d0fcfdf6a7fe7448365269ec97 Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 28 Jul 2026 00:24:54 +0200 Subject: [PATCH] Move the one SMTP account into the mailbox table it outgrew The seed migration also guards against SECRETS_KEY being unset at migrate time (encrypting the .env password would otherwise throw and take the whole migration down, on a fresh install as much as in the test suite), and the four tests/Feature/Mail files written before this migration reset the mailbox table in their beforeEach so they keep testing what they did before mailboxes.key collided with the seeded rows. --- app/Services/Secrets/SecretVault.php | 11 +- ...100000_seed_mailboxes_from_environment.php | 105 ++++++++++++++++++ lang/de/secrets.php | 1 - lang/en/secrets.php | 1 - tests/Feature/Mail/MailboxModelTest.php | 1 + tests/Feature/Mail/MailboxResolverTest.php | 1 + tests/Feature/Mail/MailboxTakeoverTest.php | 26 +++++ tests/Feature/Mail/MailboxTransportTest.php | 1 + tests/Feature/Mail/SenderAddressTest.php | 1 + tests/Pest.php | 23 ++++ 10 files changed, 164 insertions(+), 7 deletions(-) create mode 100644 database/migrations/2026_07_28_100000_seed_mailboxes_from_environment.php create mode 100644 tests/Feature/Mail/MailboxTakeoverTest.php diff --git a/app/Services/Secrets/SecretVault.php b/app/Services/Secrets/SecretVault.php index 97d4292..d04d01b 100644 --- a/app/Services/Secrets/SecretVault.php +++ b/app/Services/Secrets/SecretVault.php @@ -40,10 +40,15 @@ final class SecretVault * * The list is the whole point of the area. With one entry it was a page * that existed to hold a single Stripe key, which is not worth a password - * gate; these four are the credentials that actually stop the business when + * gate; these three are the credentials that actually stop the business when * they expire, and none of them can otherwise be rotated without a shell on * the server. * + * mail.password lived here too, until the mailboxes table (see its own + * migration) took over sending credentials — a single SMTP password could + * not say that an invoice comes from billing@ while a maintenance notice + * comes from no-reply@. + * * `check` is optional and names a class that can verify the value before it * is stored. Where there is none, no test button is offered — a button that * silently checks something else is worse than no button. @@ -66,10 +71,6 @@ final class SecretVault 'config' => 'services.monitoring.token', 'label' => 'secrets.item.monitoring_token', ], - 'mail.password' => [ - 'config' => 'mail.mailers.smtp.password', - 'label' => 'secrets.item.mail_password', - ], ]; public function has(string $key): bool diff --git a/database/migrations/2026_07_28_100000_seed_mailboxes_from_environment.php b/database/migrations/2026_07_28_100000_seed_mailboxes_from_environment.php new file mode 100644 index 0000000..7ed0db4 --- /dev/null +++ b/database/migrations/2026_07_28_100000_seed_mailboxes_from_environment.php @@ -0,0 +1,105 @@ +}> */ + private const SEED = [ + 'no-reply' => ['no_reply' => true, 'purposes' => [MailPurpose::MAINTENANCE, MailPurpose::PROVISIONING, MailPurpose::SYSTEM]], + 'support' => ['no_reply' => false, 'purposes' => [MailPurpose::SUPPORT]], + 'billing' => ['no_reply' => false, 'purposes' => [MailPurpose::BILLING]], + 'office' => ['no_reply' => false, 'purposes' => []], + 'info' => ['no_reply' => false, 'purposes' => []], + ]; + + public function up(): void + { + // The server, once. Whatever the .env already proved workable. + Settings::set('mail.host', (string) config('mail.mailers.smtp.host', '')); + $port = (int) config('mail.mailers.smtp.port', 587); + Settings::set('mail.port', $port); + + // NOT copied from MAIL_SCHEME. That variable currently reads "tls", + // which is not a Symfony mailer scheme — Symfony knows smtp and smtps, + // and Laravel 13's MailManager passes scheme straight through with no + // encryption fallback. It has never failed only because MAIL_MAILER is + // log, so nothing has opened a real connection. + // + // Stored as a human-facing choice instead, translated to a scheme where + // it is used: 465 means implicit TLS, everything else means STARTTLS. + Settings::set('mail.encryption', $port === 465 ? 'ssl' : 'tls'); + + $configuredUser = (string) config('mail.mailers.smtp.username', ''); + $domain = str_contains($configuredUser, '@') + ? substr($configuredUser, strpos($configuredUser, '@') + 1) + : 'clupilot.com'; + + // A password may already be stored in the vault; it moves rather than + // being left behind in a registry entry that is about to disappear. + $storedPassword = DB::table('app_secrets')->where('key', 'mail.password')->value('value'); + $envPassword = (string) config('mail.mailers.smtp.password', ''); + + // A fresh install may not have generated SECRETS_KEY yet — encrypting + // is what needs it, not migrating. Without this check, an install with + // MAIL_PASSWORD set in .env but no SECRETS_KEY would fail to migrate + // at all: setPasswordAttribute() throws through SecretCipher rather + // than leave the row unwritten. The mailbox still gets created, just + // without a password, exactly like the four addresses below it. + $canEncrypt = app(SecretCipher::class)->isUsable(); + + foreach (self::SEED as $key => $meta) { + $isConfigured = $key === 'no-reply' && $configuredUser !== ''; + + $box = new Mailbox([ + 'key' => $key, + 'address' => $isConfigured ? $configuredUser : $key.'@'.$domain, + 'display_name' => 'CluPilot', + 'no_reply' => $meta['no_reply'], + 'active' => true, + ]); + + // Only the account that actually exists gets credentials. + if ($isConfigured && $envPassword !== '' && $canEncrypt) { + $box->password = $envPassword; + } + + $box->save(); + + // The ciphertext from the vault is already under SECRETS_KEY, so it + // is carried across as-is rather than decrypted and re-encrypted. + if ($isConfigured && $storedPassword !== null) { + DB::table('mailboxes')->where('id', $box->id)->update(['password' => $storedPassword]); + } + + foreach ($meta['purposes'] as $purpose) { + Settings::set(MailPurpose::settingKey($purpose), $key); + } + } + + DB::table('app_secrets')->where('key', 'mail.password')->delete(); + } + + public function down(): void + { + DB::table('mailboxes')->whereIn('key', array_keys(self::SEED))->delete(); + + foreach (MailPurpose::ALL as $purpose) { + DB::table('app_settings')->where('key', MailPurpose::settingKey($purpose))->delete(); + } + + DB::table('app_settings')->whereIn('key', ['mail.host', 'mail.port', 'mail.encryption'])->delete(); + } +}; diff --git a/lang/de/secrets.php b/lang/de/secrets.php index f746580..a27cda0 100644 --- a/lang/de/secrets.php +++ b/lang/de/secrets.php @@ -6,7 +6,6 @@ return [ 'stripe_secret' => 'Stripe Secret Key', 'dns_token' => 'Hetzner-DNS-API-Token', 'monitoring_token' => 'Uptime-Kuma-API-Token', - 'mail_password' => 'SMTP-Passwort (E-Mail-Versand)', ], 'title' => 'Zugangsdaten', diff --git a/lang/en/secrets.php b/lang/en/secrets.php index e5b167f..0b964bd 100644 --- a/lang/en/secrets.php +++ b/lang/en/secrets.php @@ -6,7 +6,6 @@ return [ 'stripe_secret' => 'Stripe secret key', 'dns_token' => 'Hetzner DNS API token', 'monitoring_token' => 'Uptime Kuma API token', - 'mail_password' => 'SMTP password (outgoing mail)', ], 'title' => 'Credentials', diff --git a/tests/Feature/Mail/MailboxModelTest.php b/tests/Feature/Mail/MailboxModelTest.php index 455e4c4..3e16eb8 100644 --- a/tests/Feature/Mail/MailboxModelTest.php +++ b/tests/Feature/Mail/MailboxModelTest.php @@ -6,6 +6,7 @@ use Illuminate\Support\Facades\DB; beforeEach(function () { config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32))); + clearMailboxSeed(); }); it('stores the password encrypted and never in the clear', function () { diff --git a/tests/Feature/Mail/MailboxResolverTest.php b/tests/Feature/Mail/MailboxResolverTest.php index 151c8b2..839682e 100644 --- a/tests/Feature/Mail/MailboxResolverTest.php +++ b/tests/Feature/Mail/MailboxResolverTest.php @@ -7,6 +7,7 @@ use App\Support\Settings; beforeEach(function () { config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32))); + clearMailboxSeed(); }); it('resolves a purpose to the mailbox it points at', function () { diff --git a/tests/Feature/Mail/MailboxTakeoverTest.php b/tests/Feature/Mail/MailboxTakeoverTest.php new file mode 100644 index 0000000..1971677 --- /dev/null +++ b/tests/Feature/Mail/MailboxTakeoverTest.php @@ -0,0 +1,26 @@ +pluck('key')->sort()->values()->all()) + ->toBe(['billing', 'info', 'no-reply', 'office', 'support']); +}); + +it('marks no-reply as unanswerable and the rest as answerable', function () { + expect(Mailbox::findByKey('no-reply')->no_reply)->toBeTrue() + ->and(Mailbox::findByKey('support')->no_reply)->toBeFalse(); +}); + +it('points every purpose at a mailbox, so nothing sends from nowhere', function () { + foreach (MailPurpose::ALL as $purpose) { + expect(Settings::get(MailPurpose::settingKey($purpose)))->not->toBeNull(); + } +}); + +it('no longer offers mail.password as a secret, because it is superseded', function () { + expect(SecretVault::REGISTRY)->not->toHaveKey('mail.password'); +}); diff --git a/tests/Feature/Mail/MailboxTransportTest.php b/tests/Feature/Mail/MailboxTransportTest.php index 0e30f64..3751326 100644 --- a/tests/Feature/Mail/MailboxTransportTest.php +++ b/tests/Feature/Mail/MailboxTransportTest.php @@ -12,6 +12,7 @@ use Symfony\Component\Mailer\Transport\TransportInterface; beforeEach(function () { config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32))); + clearMailboxSeed(); Settings::set('mail.host', 'mail.example.test'); Settings::set('mail.port', 587); Settings::set('mail.encryption', 'tls'); diff --git a/tests/Feature/Mail/SenderAddressTest.php b/tests/Feature/Mail/SenderAddressTest.php index bf09e45..37426f5 100644 --- a/tests/Feature/Mail/SenderAddressTest.php +++ b/tests/Feature/Mail/SenderAddressTest.php @@ -13,6 +13,7 @@ use Illuminate\Support\Facades\Crypt; beforeEach(function () { config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32))); + clearMailboxSeed(); }); it('sends from the mailbox its purpose points at', function () { diff --git a/tests/Pest.php b/tests/Pest.php index 4bf4c0b..c41cd98 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -87,3 +87,26 @@ function admin(): App\Models\User { return App\Models\User::factory()->create(['is_admin' => true]); } + +/** + * Undo the mailbox-seeding migration's baseline, for tests written before it. + * + * RefreshDatabase migrates once per run, not once per test, so every test + * starts from a transaction rolled back to that migrated state — including the + * five mailboxes (no-reply, support, billing, office, info) and all five + * mail.purpose.* settings the mailbox-seeding migration writes. The tests in + * tests/Feature/Mail predate that migration: several create their own mailbox + * under one of those same five keys, which now collides with mailboxes.key's + * unique constraint; a couple rely on a purpose being genuinely unset to prove + * MailboxResolver's system fallback, which the seeded mapping would otherwise + * pre-empt. Call this from such a file's beforeEach() to restore the blank + * slate it was written against. + */ +function clearMailboxSeed(): void +{ + App\Models\Mailbox::query()->delete(); + + foreach (App\Services\Mail\MailPurpose::ALL as $purpose) { + App\Support\Settings::set(App\Services\Mail\MailPurpose::settingKey($purpose), null); + } +}