From 729f57755a62c84595571143d6601142bc43dcdb Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 28 Jul 2026 06:36:06 +0200 Subject: [PATCH] Clear every mailbox's verification when the shared server config changes saveServer() wrote host/port/encryption without touching any mailbox, so the console kept showing every mailbox as verified against a server nothing had tested since. Mirrors EditMailbox::save()'s existing guard on a single mailbox's own identity, just at server scope: only clears when a field actually changed, so re-saving unedited values leaves a real verification alone. Both places now delegate the actual clear to new Mailbox:: invalidateVerification()/invalidateAllVerifications() methods rather than touching last_verified_at directly, sharing the mutation. The "did this change" comparison stays separate in each caller - one diffs a loaded model's attributes, the other diffs persisted settings against incoming scalars, and forcing them through one function would add machinery no actual duplication justifies. Co-Authored-By: Claude Opus 5 --- app/Livewire/Admin/Mail.php | 23 +++++ app/Livewire/EditMailbox.php | 10 +- app/Models/Mailbox.php | 26 +++++ tests/Feature/Mail/MailSettingsPageTest.php | 109 ++++++++++++++++++++ 4 files changed, 165 insertions(+), 3 deletions(-) diff --git a/app/Livewire/Admin/Mail.php b/app/Livewire/Admin/Mail.php index ec44542..2bacd7b 100644 --- a/app/Livewire/Admin/Mail.php +++ b/app/Livewire/Admin/Mail.php @@ -86,10 +86,33 @@ class Mail extends Component 'encryption' => ['required', 'in:tls,ssl,none'], ]); + // Codex R15#6, P2: last_verified_at on EVERY mailbox proves a test + // against THIS server config specifically — the shared-server mirror + // of EditMailbox::save()'s guard on a single mailbox's own address/ + // username/authenticates. Compared against the STORED values, not + // just "did the operator touch the field": re-opening the card and + // saving without editing anything must not wipe a legitimate + // verification. Read before Settings::set() overwrites them below, + // and deliberately not merged with EditMailbox's own comparison — that + // one diffs a single loaded model's in-memory attributes, this one + // diffs persisted Settings against incoming scalars for a config that + // has no single row to load at all; forcing one comparison function + // over both shapes would add an abstraction with nothing genuinely + // shared to justify it. What IS shared is the actual clear itself: + // both this method and EditMailbox::save() delegate to a Mailbox:: + // invalidate*() method rather than touching the column directly. + $serverChanged = $this->host !== (string) Settings::get('mail.host', '') + || (int) $this->port !== (int) Settings::get('mail.port', 587) + || $this->encryption !== (string) Settings::get('mail.encryption', 'tls'); + Settings::set('mail.host', $this->host); Settings::set('mail.port', (int) $this->port); Settings::set('mail.encryption', $this->encryption); + if ($serverChanged) { + Mailbox::invalidateAllVerifications(); + } + $this->dispatch('notify', message: __('mail_settings.server_saved')); } diff --git a/app/Livewire/EditMailbox.php b/app/Livewire/EditMailbox.php index b4843c2..28909e9 100644 --- a/app/Livewire/EditMailbox.php +++ b/app/Livewire/EditMailbox.php @@ -121,16 +121,20 @@ class EditMailbox extends ModalComponent // authenticates itself is the fourth: whether that identity gets // sent at all. Leaving last_verified_at standing after any of the // three would keep showing a successful verification for a - // connection nothing has actually tested since. + // connection nothing has actually tested since. Mailbox:: + // invalidateVerification() is the same clear Admin\Mail::saveServer() + // uses for the server card, in its single-row shape — see that + // method's own comment for why the two do NOT also share the "did + // this actually change" comparison above this block. if ($box->address !== $oldAddress || $box->username !== $oldUsername || $box->authenticates !== $oldAuthenticates) { - $box->last_verified_at = null; + $box->invalidateVerification(); } // Empty means "leave it alone", not "delete it" — otherwise every edit // of an address would silently drop the password. if ($this->password !== '') { $box->password = $this->password; - $box->last_verified_at = null; + $box->invalidateVerification(); } $box->save(); diff --git a/app/Models/Mailbox.php b/app/Models/Mailbox.php index 0edaf15..363e4c0 100644 --- a/app/Models/Mailbox.php +++ b/app/Models/Mailbox.php @@ -62,6 +62,32 @@ class Mailbox extends Model : $this->address; } + /** + * last_verified_at proves a successful test against the config THIS + * mailbox held at the time — an address, username or authenticates + * change (or a new password) invalidates only this row. Sets the + * attribute without saving: EditMailbox::save() calls this alongside + * other attribute changes so the whole edit lands in one write, not two. + */ + public function invalidateVerification(): void + { + $this->last_verified_at = null; + } + + /** + * The mirror of invalidateVerification() for the server card + * (Admin\Mail::saveServer()): host, port and encryption are shared by + * every mailbox at once, not a per-row concept, so a change there + * invalidates all of them in one write rather than each row deciding + * independently — Codex R15#6, P2. Scoped to rows that still have + * something to clear, so saving unrelated server settings does not bump + * every mailbox's updated_at for nothing. + */ + public static function invalidateAllVerifications(): int + { + return static::query()->whereNotNull('last_verified_at')->update(['last_verified_at' => null]); + } + /** * Enough to send with: an address, and a password ONLY when this mailbox * actually authenticates. diff --git a/tests/Feature/Mail/MailSettingsPageTest.php b/tests/Feature/Mail/MailSettingsPageTest.php index 5de7ff3..5a71fa4 100644 --- a/tests/Feature/Mail/MailSettingsPageTest.php +++ b/tests/Feature/Mail/MailSettingsPageTest.php @@ -263,6 +263,115 @@ it('rejects an empty mail server host', function () { ->assertHasErrors('host'); }); +// --- Codex R15#6, P2: last_verified_at proves a mailbox was tested against a +// SPECIFIC server config. Changing host, port or encryption here silently +// invalidates that proof for every mailbox at once — the shared-server mirror +// of EditMailbox::save()'s guard on a single mailbox's own address/username/ +// authenticates. Each test below pins host/port/encryption to a KNOWN old +// value via Settings::set() before mounting, rather than trusting whatever +// RefreshDatabase's baseline seed happens to contain — the point is to control +// exactly which one field moves. + +it("clears every mailbox's last_verified_at when the server host changes", function () { + Settings::set('mail.host', 'old-relay.example.com'); + Settings::set('mail.port', 587); + Settings::set('mail.encryption', 'tls'); + + $a = Mailbox::factory()->create(['last_verified_at' => now()]); + $b = Mailbox::factory()->create(['last_verified_at' => now()]); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(MailPage::class) + ->set('confirmablePassword', 'password') + ->call('confirmPassword') + ->set('host', 'new-relay.example.com') + ->call('saveServer') + ->assertHasNoErrors(); + + expect($a->fresh()->last_verified_at)->toBeNull() + ->and($b->fresh()->last_verified_at)->toBeNull(); +}); + +it("clears every mailbox's last_verified_at when the server port changes", function () { + Settings::set('mail.host', 'relay.example.com'); + Settings::set('mail.port', 587); + Settings::set('mail.encryption', 'tls'); + + $box = Mailbox::factory()->create(['last_verified_at' => now()]); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(MailPage::class) + ->set('confirmablePassword', 'password') + ->call('confirmPassword') + ->set('port', 2525) + ->call('saveServer') + ->assertHasNoErrors(); + + expect($box->fresh()->last_verified_at)->toBeNull(); +}); + +it("clears every mailbox's last_verified_at when the server encryption changes", function () { + Settings::set('mail.host', 'relay.example.com'); + Settings::set('mail.port', 587); + Settings::set('mail.encryption', 'tls'); + + $box = Mailbox::factory()->create(['last_verified_at' => now()]); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(MailPage::class) + ->set('confirmablePassword', 'password') + ->call('confirmPassword') + ->set('encryption', 'ssl') + ->call('saveServer') + ->assertHasNoErrors(); + + expect($box->fresh()->last_verified_at)->toBeNull(); +}); + +it('keeps last_verified_at when the server card is saved with no actual change', function () { + // The control case for the three tests above: an operator who opens the + // card and saves without editing anything has changed nothing, and must + // not wipe a legitimate verification. Sets every field to the SAME value + // mount() already read, rather than never calling set() at all — this is + // what "re-saving identical values" actually looks like through the form. + Settings::set('mail.host', 'relay.example.com'); + Settings::set('mail.port', 587); + Settings::set('mail.encryption', 'tls'); + + $box = Mailbox::factory()->create(['last_verified_at' => now()]); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(MailPage::class) + ->set('confirmablePassword', 'password') + ->call('confirmPassword') + ->set('host', 'relay.example.com') + ->set('port', 587) + ->set('encryption', 'tls') + ->call('saveServer') + ->assertHasNoErrors(); + + expect($box->fresh()->last_verified_at)->not->toBeNull(); +}); + +it('shows "not yet verified" instead of a blank cell once the server change clears it', function () { + // The console-list-facing half of the finding: a cleared timestamp must + // read as an honest, explicit "not yet verified", not a blank cell that + // could be mistaken for a rendering error. + Settings::set('mail.host', 'relay.example.com'); + Settings::set('mail.port', 587); + Settings::set('mail.encryption', 'tls'); + + Mailbox::factory()->create(['key' => 'support', 'last_verified_at' => now()]); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(MailPage::class) + ->set('confirmablePassword', 'password') + ->call('confirmPassword') + ->set('host', 'new-relay.example.com') + ->call('saveServer') + ->assertSee(__('mail_settings.never_verified')); +}); + it('refuses to change the mail server without a recently confirmed password, capability notwithstanding', function () { // saveServer() writes mail.host — the platform's outbound relay for every // purpose mailbox at once. An Owner has mail.manage for the whole test;