Validate purpose mappings against real mailboxes, and clear stale verification on identity changes
savePurposes() only checked that each key was a nonempty string, so a stale or tampered mailbox key (reachable by posting straight to /livewire/update) saved successfully while MailboxResolver silently returned null for it. Every purpose's key must now name a mailbox that exists; system's must additionally be active, since every unmapped or inactive OTHER purpose falls through to it. EditMailbox::save() also cleared last_verified_at only when the password changed, but smtpUsername() depends on address and username too (falling back to address when username is blank) — changing either while leaving the password untouched kept showing a successful verification for an identity nothing had actually tested.feat/mailboxes
parent
c55a5d2b49
commit
3278f063ea
|
|
@ -97,12 +97,46 @@ class Mail extends Component
|
|||
{
|
||||
$this->authorize('mail.manage');
|
||||
|
||||
// system is the fallback, so it is the one that may not be empty —
|
||||
// there is nothing left to fall back to.
|
||||
$this->validate(
|
||||
['purposes.system' => ['required', 'string']],
|
||||
['purposes.system.required' => __('mail_settings.system_required')],
|
||||
);
|
||||
// Reachable by posting straight to /livewire/update, past whatever
|
||||
// the <select> in the view would ever submit — so "is a nonempty
|
||||
// string" is not enough. Every purpose's key must name a mailbox
|
||||
// that actually exists; system's additionally must be ACTIVE, because
|
||||
// MailboxResolver::for() falls every unmapped (or inactive) OTHER
|
||||
// purpose through to system, and there is nothing left to fall back
|
||||
// to when system itself is the broken one.
|
||||
$activeKeys = Mailbox::query()->where('active', true)->pluck('key')->all();
|
||||
$allKeys = Mailbox::query()->pluck('key')->all();
|
||||
|
||||
$rules = [
|
||||
// system is the fallback, so it is the one that may not be empty
|
||||
// — there is nothing left to fall back to.
|
||||
'purposes.system' => ['required', 'string', function ($attribute, $value, $fail) use ($activeKeys) {
|
||||
if ($value !== '' && ! in_array($value, $activeKeys, true)) {
|
||||
$fail(__('mail_settings.system_inactive'));
|
||||
}
|
||||
}],
|
||||
];
|
||||
|
||||
foreach (MailPurpose::ALL as $purpose) {
|
||||
if ($purpose === MailPurpose::SYSTEM) {
|
||||
continue;
|
||||
}
|
||||
|
||||
// Blank is a legitimate choice for every OTHER purpose —
|
||||
// MailboxResolver already falls an unmapped (or inactive)
|
||||
// purpose through to system on its own, so only a key that names
|
||||
// NO mailbox at all (a deleted row, or a tampered payload) is
|
||||
// refused here.
|
||||
$rules["purposes.{$purpose}"] = ['nullable', 'string', function ($attribute, $value, $fail) use ($allKeys) {
|
||||
if ($value !== '' && ! in_array($value, $allKeys, true)) {
|
||||
$fail(__('mail_settings.purpose_unknown_mailbox'));
|
||||
}
|
||||
}];
|
||||
}
|
||||
|
||||
$this->validate($rules, [
|
||||
'purposes.system.required' => __('mail_settings.system_required'),
|
||||
]);
|
||||
|
||||
foreach (MailPurpose::ALL as $purpose) {
|
||||
Settings::set(MailPurpose::settingKey($purpose), $this->purposes[$purpose] ?? '');
|
||||
|
|
|
|||
|
|
@ -100,12 +100,25 @@ class EditMailbox extends ModalComponent
|
|||
}
|
||||
}
|
||||
|
||||
$oldAddress = $box->address;
|
||||
$oldUsername = $box->username;
|
||||
|
||||
$box->address = $this->address;
|
||||
$box->display_name = $this->displayName ?: null;
|
||||
$box->username = $this->username ?: null;
|
||||
$box->no_reply = $this->noReply;
|
||||
$box->active = $this->active;
|
||||
|
||||
// Either one changes the identity smtpUsername() authenticates as:
|
||||
// username directly, or address as username's fallback when none is
|
||||
// set (including username being CLEARED back to that fallback).
|
||||
// Leaving last_verified_at standing after either would keep showing
|
||||
// a successful verification for credentials nothing has actually
|
||||
// tested against the new identity.
|
||||
if ($box->address !== $oldAddress || $box->username !== $oldUsername) {
|
||||
$box->last_verified_at = null;
|
||||
}
|
||||
|
||||
// Empty means "leave it alone", not "delete it" — otherwise every edit
|
||||
// of an address would silently drop the password.
|
||||
if ($this->password !== '') {
|
||||
|
|
|
|||
|
|
@ -45,6 +45,8 @@ return [
|
|||
],
|
||||
'purposes_saved' => 'Zuordnung gespeichert.',
|
||||
'system_required' => '„System" muss ein Postfach haben — es ist der Rückfall für alle anderen.',
|
||||
'system_inactive' => '„System" braucht ein aktives Postfach — es ist der Rückfall, auf den alles andere angewiesen ist.',
|
||||
'purpose_unknown_mailbox' => 'Dieses Postfach existiert nicht mehr. Bitte eines aus der Liste wählen.',
|
||||
|
||||
'test' => 'Testmail senden',
|
||||
'test_recipient' => 'Testempfänger',
|
||||
|
|
|
|||
|
|
@ -45,6 +45,8 @@ return [
|
|||
],
|
||||
'purposes_saved' => 'Mapping saved.',
|
||||
'system_required' => '"System" must have a mailbox — it is the fallback for all the others.',
|
||||
'system_inactive' => '"System" needs a mailbox that is active — it is the fallback everything else depends on.',
|
||||
'purpose_unknown_mailbox' => 'That mailbox no longer exists. Please choose one from the list.',
|
||||
|
||||
'test' => 'Send test mail',
|
||||
'test_recipient' => 'Test recipient',
|
||||
|
|
|
|||
|
|
@ -280,6 +280,88 @@ it('saves the purpose-to-mailbox mapping', function () {
|
|||
->and(Settings::get(MailPurpose::settingKey(MailPurpose::SYSTEM)))->toBe('no-reply');
|
||||
});
|
||||
|
||||
// --- P2: a Livewire request can post any string to purposes.*, including one
|
||||
// posted straight to /livewire/update that never went through the <select>
|
||||
// at all — the fallback is only useful if it actually names a mailbox that
|
||||
// can send.
|
||||
|
||||
it('refuses to map system to a mailbox key that names no mailbox at all', function () {
|
||||
Mailbox::factory()->create(['key' => 'support']);
|
||||
|
||||
Livewire::actingAs(User::factory()->operator('Owner')->create())
|
||||
->test(MailPage::class)
|
||||
->set('purposes.system', 'a-deleted-or-tampered-key')
|
||||
->call('savePurposes')
|
||||
->assertHasErrors('purposes.system');
|
||||
|
||||
expect(Settings::get(MailPurpose::settingKey(MailPurpose::SYSTEM)))->toBeNull();
|
||||
});
|
||||
|
||||
it('refuses to map system to a mailbox that exists but is not active', function () {
|
||||
// Existing is not enough for system specifically: MailboxResolver::active()
|
||||
// filters an inactive mailbox out at BOTH the direct lookup and the system
|
||||
// fallback, so an inactive "system" leaves every unmapped purpose with
|
||||
// nothing behind it — not just system's own mail.
|
||||
Mailbox::factory()->create(['key' => 'no-reply', 'active' => false]);
|
||||
|
||||
Livewire::actingAs(User::factory()->operator('Owner')->create())
|
||||
->test(MailPage::class)
|
||||
->set('purposes.system', 'no-reply')
|
||||
->call('savePurposes')
|
||||
->assertHasErrors('purposes.system');
|
||||
|
||||
expect(Settings::get(MailPurpose::settingKey(MailPurpose::SYSTEM)))->toBeNull();
|
||||
});
|
||||
|
||||
it('refuses to map a non-system purpose to a mailbox key that names no mailbox at all', function () {
|
||||
Mailbox::factory()->create(['key' => 'no-reply']);
|
||||
Settings::set(MailPurpose::settingKey(MailPurpose::SYSTEM), 'no-reply');
|
||||
|
||||
Livewire::actingAs(User::factory()->operator('Owner')->create())
|
||||
->test(MailPage::class)
|
||||
->set('purposes.support', 'a-deleted-or-tampered-key')
|
||||
->call('savePurposes')
|
||||
->assertHasErrors('purposes.support');
|
||||
|
||||
expect(Settings::get(MailPurpose::settingKey(MailPurpose::SUPPORT)))->toBeNull();
|
||||
});
|
||||
|
||||
it('still allows mapping a non-system purpose to a mailbox that exists but is inactive', function () {
|
||||
// The control case for the two refusals above: MailboxResolver::active()
|
||||
// already falls an inactive NON-system mapping through to system on its
|
||||
// own (see its own docblock), so only system needs the stricter active
|
||||
// check — a non-system purpose merely needs the key to name a real row.
|
||||
Mailbox::factory()->create(['key' => 'no-reply']);
|
||||
Mailbox::factory()->create(['key' => 'support', 'active' => false]);
|
||||
Settings::set(MailPurpose::settingKey(MailPurpose::SYSTEM), 'no-reply');
|
||||
|
||||
Livewire::actingAs(User::factory()->operator('Owner')->create())
|
||||
->test(MailPage::class)
|
||||
->set('purposes.support', 'support')
|
||||
->call('savePurposes')
|
||||
->assertHasNoErrors();
|
||||
|
||||
expect(Settings::get(MailPurpose::settingKey(MailPurpose::SUPPORT)))->toBe('support');
|
||||
});
|
||||
|
||||
it('still allows leaving a non-system purpose blank, even after it once had a mailbox', function () {
|
||||
// Blank is the legitimate "fall back to system" choice, not a tampered
|
||||
// value — the fix must not turn every unmapped purpose into a validation
|
||||
// error the moment it is resaved.
|
||||
Mailbox::factory()->create(['key' => 'no-reply']);
|
||||
Mailbox::factory()->create(['key' => 'support']);
|
||||
Settings::set(MailPurpose::settingKey(MailPurpose::SYSTEM), 'no-reply');
|
||||
Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support');
|
||||
|
||||
Livewire::actingAs(User::factory()->operator('Owner')->create())
|
||||
->test(MailPage::class)
|
||||
->set('purposes.support', '')
|
||||
->call('savePurposes')
|
||||
->assertHasNoErrors();
|
||||
|
||||
expect(Settings::get(MailPurpose::settingKey(MailPurpose::SUPPORT)))->toBe('');
|
||||
});
|
||||
|
||||
it('re-checks mail.manage on every page action, not only at mount', function () {
|
||||
// mount() alone would pass every test above: an Owner has the capability
|
||||
// for the whole test, so nothing so far proves saveServer()/savePurposes()
|
||||
|
|
@ -357,6 +439,73 @@ it('clears last_verified_at when the password changes, since it no longer proves
|
|||
expect($box->fresh()->last_verified_at)->toBeNull();
|
||||
});
|
||||
|
||||
// --- P2: last_verified_at proves credentials were tested, not just that a
|
||||
// password exists — changing EITHER half of the identity smtpUsername()
|
||||
// authenticates as (an explicit username, or address as its fallback) must
|
||||
// invalidate that proof exactly like a new password does. None of these set
|
||||
// a password at all: the point is that the guard fires without one.
|
||||
|
||||
it('clears last_verified_at when the address changes, even though the password stays blank', function () {
|
||||
$box = Mailbox::factory()->create([
|
||||
'address' => 'alt@clupilot.com', 'username' => null, 'last_verified_at' => now(),
|
||||
]);
|
||||
|
||||
Livewire::actingAs(User::factory()->operator('Owner')->create())
|
||||
->test(EditMailbox::class, ['uuid' => $box->uuid])
|
||||
->set('address', 'neu@clupilot.com')
|
||||
->call('save');
|
||||
|
||||
expect($box->fresh()->last_verified_at)->toBeNull();
|
||||
});
|
||||
|
||||
it('clears last_verified_at when an explicit username changes, even though the password stays blank', function () {
|
||||
$box = Mailbox::factory()->create([
|
||||
'username' => 'alt-smtp-user', 'last_verified_at' => now(),
|
||||
]);
|
||||
|
||||
Livewire::actingAs(User::factory()->operator('Owner')->create())
|
||||
->test(EditMailbox::class, ['uuid' => $box->uuid])
|
||||
->set('username', 'neu-smtp-user')
|
||||
->call('save');
|
||||
|
||||
expect($box->fresh()->last_verified_at)->toBeNull();
|
||||
});
|
||||
|
||||
it('clears last_verified_at when the username is cleared, since smtpUsername() then falls back to the address', function () {
|
||||
// The fallback the brief names explicitly: username going from an
|
||||
// explicit value to blank changes what smtpUsername() returns just as
|
||||
// much as typing a new one would, even though the raw address field
|
||||
// never moves.
|
||||
$box = Mailbox::factory()->create([
|
||||
'address' => 'bleibt@clupilot.com', 'username' => 'alt-smtp-user', 'last_verified_at' => now(),
|
||||
]);
|
||||
|
||||
Livewire::actingAs(User::factory()->operator('Owner')->create())
|
||||
->test(EditMailbox::class, ['uuid' => $box->uuid])
|
||||
->set('username', '')
|
||||
->call('save');
|
||||
|
||||
expect($box->fresh()->username)->toBeNull()
|
||||
->and($box->fresh()->last_verified_at)->toBeNull();
|
||||
});
|
||||
|
||||
it('keeps last_verified_at when neither the address nor the username actually changes', function () {
|
||||
// The control case for the three tests above: the guard must react to an
|
||||
// actual CHANGE, not fire on every save regardless of what moved —
|
||||
// otherwise it would stay green even if it degenerated into always
|
||||
// clearing the column.
|
||||
$box = Mailbox::factory()->create([
|
||||
'address' => 'bleibt@clupilot.com', 'username' => 'bleibt-auch', 'last_verified_at' => now(),
|
||||
]);
|
||||
|
||||
Livewire::actingAs(User::factory()->operator('Owner')->create())
|
||||
->test(EditMailbox::class, ['uuid' => $box->uuid])
|
||||
->set('displayName', 'Neuer Anzeigename')
|
||||
->call('save');
|
||||
|
||||
expect($box->fresh()->last_verified_at)->not->toBeNull();
|
||||
});
|
||||
|
||||
it('refuses to set a new mailbox password without a recently confirmed password', function () {
|
||||
// The credential-interception primitive the brief names: a signed-in
|
||||
// mail.manage session alone must not be enough to repoint where a
|
||||
|
|
|
|||
Loading…
Reference in New Issue