Make a deactivated mailbox fall back like a deleted one

Review of Task 4: MailboxResolver::for() only fell back on a missing
mapping or a missing row. Deactivating a mailbox — the operator's
explicit "do not use this" — left it resolvable anyway, so
MailboxTransport reached it, found isConfigured() false, and threw
instead of quietly using the next candidate the way every other
"this mailbox is unusable" case does. Worse, __toString() still
reported the deactivated address as though it were healthy.

named() now runs through a new active() filter at BOTH lookup sites —
the purpose's own mapping and the system fallback — so an inactive
system mailbox is refused exactly like a missing one, not handed out
as the last resort. A missing password stays deliberately unfiltered
here: that is a MailboxTransport concern (loud failure naming the
mailbox), not a resolver fallback trigger — silently rerouting mail
because a password hasn't been typed yet would hide the gap instead
of surfacing it.

Added two tests and mutated active() to a no-op filter to confirm they
catch it: both failed (one expected the system mailbox, got null; the
other expected null, got the inactive mailbox's full attribute dump).
Reverted, reran, both pass again.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
feat/mailboxes
nexxo 2026-07-27 22:57:22 +02:00
parent 095d8e694a
commit 0be9c61484
2 changed files with 43 additions and 2 deletions

View File

@ -21,7 +21,7 @@ final class MailboxResolver
throw new InvalidArgumentException("Unknown mail purpose [{$purpose}].");
}
$box = $this->named(Settings::get(MailPurpose::settingKey($purpose)));
$box = $this->active($this->named(Settings::get(MailPurpose::settingKey($purpose))));
if ($box !== null) {
return $box;
@ -31,11 +31,27 @@ final class MailboxResolver
// address is recoverable, one that is never sent may not be noticed.
return $purpose === MailPurpose::SYSTEM
? null
: $this->named(Settings::get(MailPurpose::settingKey(MailPurpose::SYSTEM)));
: $this->active($this->named(Settings::get(MailPurpose::settingKey(MailPurpose::SYSTEM))));
}
private function named(mixed $key): ?Mailbox
{
return is_string($key) && $key !== '' ? Mailbox::findByKey($key) : null;
}
/**
* Deactivating a mailbox is an explicit operator action meaning "do not
* use this" — it must fall back exactly like a mapping that points
* nowhere or a row that is gone, so it is filtered here, at BOTH lookup
* sites, rather than left for a caller to notice.
*
* A mailbox with no password is deliberately NOT filtered here: silently
* sending support mail from billing@ because support@ has no password
* yet is worse than a loud failure naming the mailbox that needs one
* that check stays a MailboxTransport concern, not this one.
*/
private function active(?Mailbox $box): ?Mailbox
{
return $box?->active === true ? $box : null;
}
}

View File

@ -35,6 +35,31 @@ it('falls back to system when the named mailbox no longer exists', function () {
->toBe('no-reply@clupilot.com');
});
it('falls back to system when the mailbox is deactivated, not deleted', function () {
// Deactivating is an explicit operator action. Reaching this mailbox by
// its row surviving would silently send from an address the operator
// just told the console to stop using.
Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com', 'active' => false]);
Mailbox::factory()->create(['key' => 'no-reply', 'address' => 'no-reply@clupilot.com']);
Settings::set('mail.purpose.support', 'support');
Settings::set('mail.purpose.system', 'no-reply');
expect(app(MailboxResolver::class)->for(MailPurpose::SUPPORT)?->address)
->toBe('no-reply@clupilot.com');
});
it('returns null rather than an inactive mailbox when the fallback is deactivated too', function () {
// The fallback lookup is filtered the SAME way as the direct one — an
// inactive system mailbox must not be handed out just because it is the
// last candidate.
Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com', 'active' => false]);
Mailbox::factory()->create(['key' => 'no-reply', 'address' => 'no-reply@clupilot.com', 'active' => false]);
Settings::set('mail.purpose.support', 'support');
Settings::set('mail.purpose.system', 'no-reply');
expect(app(MailboxResolver::class)->for(MailPurpose::SUPPORT))->toBeNull();
});
it('returns null rather than guessing when even system is unset', function () {
expect(app(MailboxResolver::class)->for(MailPurpose::SYSTEM))->toBeNull();
});