diff --git a/VERSION b/VERSION index 155d478..38670bb 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.3.44 +1.3.45 diff --git a/app/Livewire/Admin/Inbox.php b/app/Livewire/Admin/Inbox.php index 0204def..f6443f4 100644 --- a/app/Livewire/Admin/Inbox.php +++ b/app/Livewire/Admin/Inbox.php @@ -6,6 +6,7 @@ use App\Models\Customer; use App\Models\InboundMail; use App\Services\Mail\IngestInboundMail; use App\Services\Mail\InboundMailbox; +use App\Services\Mail\InboundMailStatus; use Livewire\Attributes\Layout; use Livewire\Attributes\Url; use Livewire\Component; @@ -127,6 +128,11 @@ class Inbox extends Component // explain: a mailbox that was never configured looks exactly like a // mailbox with no mail in it. 'configured' => app(InboundMailbox::class)->isConfigured(), + // The same line the settings page shows. This is where somebody + // notices nothing is arriving, so this is where "last reached" has + // to be readable — otherwise an empty inbox and a broken mailbox + // look identical. + 'status' => app(InboundMailStatus::class)->last(), ]); } } diff --git a/app/Livewire/Admin/Integrations.php b/app/Livewire/Admin/Integrations.php index bd0f8e7..8b17cbc 100644 --- a/app/Livewire/Admin/Integrations.php +++ b/app/Livewire/Admin/Integrations.php @@ -6,6 +6,8 @@ use App\Livewire\Concerns\ConfirmsPassword; use App\Services\Deployment\UpdateChannel; use App\Services\Env\EnvFileEditor; use App\Services\Env\InvalidEnvContentException; +use App\Services\Mail\InboundMailbox; +use App\Services\Mail\InboundMailStatus; use App\Services\Secrets\SecretVault; use App\Support\ProvisioningSettings; use App\Support\Settings; @@ -400,6 +402,32 @@ class Integrations extends Component abort_unless($this->passwordRecentlyConfirmed(), 403); } + /** + * Reach the support mailbox now and say what came back. + * + * The one thing an operator wants after typing a host, a user and a + * password: did any of it work. Waiting for the next scheduled run to find + * out is not an answer, and "der Posteingang ist leer" is not one either. + * + * Saved first, deliberately: testing what is on screen while the server + * still holds the previous values would report on a mailbox nobody + * configured. The unsaved form is the question being asked. + */ + public function testInbound(): void + { + $this->guardInfra(); + $this->saveInfra(); + + $check = app(InboundMailbox::class)->check(); + app(InboundMailStatus::class)->record($check['ok'], $check['message'], $check['unseen']); + + $this->dispatch('notify', message: $check['ok'] + ? ($check['unseen'] === null + ? __('integrations.inbound_ok') + : trans_choice('integrations.inbound_ok_waiting', $check['unseen'], ['count' => $check['unseen']])) + : __('integrations.inbound_failed_'.$check['message'])); + } + public function render() { $vault = app(SecretVault::class); @@ -458,6 +486,9 @@ class Integrations extends Component self::TABS, fn (string $tab) => $tab !== 'env' || $canSecrets, )), + // When the mailbox was last reached, and what came back. Null until + // somebody — or the scheduler — has asked once. + 'inboundStatus' => app(InboundMailStatus::class)->last(), 'canSecrets' => $canSecrets, 'canInfra' => $canInfra, 'unlocked' => $unlocked, diff --git a/app/Services/Mail/FakeMailbox.php b/app/Services/Mail/FakeMailbox.php index 05b1500..fd7afd6 100644 --- a/app/Services/Mail/FakeMailbox.php +++ b/app/Services/Mail/FakeMailbox.php @@ -25,6 +25,22 @@ class FakeMailbox implements InboundMailbox return $this->configured; } + /** + * What a test wants the check to answer, if not the obvious. + * + * @var array{ok: bool, message: string, unseen: int|null}|null + */ + public array|null $checkAnswer = null; + + public function check(): array + { + return $this->checkAnswer ?? [ + 'ok' => $this->configured, + 'message' => $this->configured ? '' : 'not configured', + 'unseen' => $this->configured ? count($this->mails) : null, + ]; + } + public function fetch(int $limit = 50): array { return array_slice($this->mails, 0, $limit); diff --git a/app/Services/Mail/ImapMailbox.php b/app/Services/Mail/ImapMailbox.php index c7f1cea..ab3b384 100644 --- a/app/Services/Mail/ImapMailbox.php +++ b/app/Services/Mail/ImapMailbox.php @@ -2,6 +2,7 @@ namespace App\Services\Mail; +use App\Services\Secrets\SecretVault; use App\Support\ProvisioningSettings; use Illuminate\Support\Facades\Log; use RuntimeException; @@ -44,9 +45,20 @@ class ImapMailbox implements InboundMailbox * Configured means: somebody entered a host, a user and a password. * * The first two come from the console (ProvisioningSettings), the password - * from the vault — read through config, which SecretVault overrides at - * boot. No separate "enabled" switch to forget: a mailbox with credentials - * IS the switch, and one without them cannot be polled anyway. + * from the VAULT — read here, at the point of use. It used to read + * config('services.inbound_mail.password'), which is only ever the .env + * value: this project deliberately does not overlay stored secrets onto + * config at boot (SecretVault's docblock, rule 3). So a password saved in + * the console counted for nothing, isConfigured() stayed false, and the + * inbox went on saying no mailbox was set up while the password sat right + * there on the settings page. Reported exactly that way. + * + * SecretVault::get() falls back to the configured value when nothing is + * stored, so an installation carrying INBOUND_MAIL_PASSWORD in .env keeps + * working unchanged. + * + * No separate "enabled" switch to forget: a mailbox with credentials IS the + * switch, and one without them cannot be polled anyway. */ public function isConfigured(): bool { @@ -54,7 +66,63 @@ class ImapMailbox implements InboundMailbox return filled($mailbox['host']) && filled($mailbox['username']) - && filled(config('services.inbound_mail.password')); + && filled($this->password()); + } + + /** The mailbox password, from the vault, falling back to .env. */ + private function password(): string + { + return (string) app(SecretVault::class)->get('inbound_mail.password'); + } + + /** + * Reach the mailbox and count what is waiting. + * + * Deliberately the same four verbs the fetch uses — LOGIN, SELECT, SEARCH — + * so a green check means the thing that actually runs would work. A check + * that proves something easier than the real job is worse than none: it was + * exactly that mistake (a test computing its expectation with the code under + * test) the timezone rule was written about. + * + * @return array{ok: bool, message: string, unseen: int|null} + */ + public function check(): array + { + if (! $this->isConfigured()) { + return ['ok' => false, 'message' => 'incomplete', 'unseen' => null]; + } + + try { + $this->connect(); + $unseen = count($this->search()); + + return ['ok' => true, 'message' => '', 'unseen' => $unseen]; + } catch (Throwable $e) { + // The message, not the trace, and never the command: LOGIN carries + // the password and this string is shown on a page. + return ['ok' => false, 'message' => $this->reason($e), 'unseen' => null]; + } finally { + $this->disconnect(); + } + } + + /** + * One line an operator can act on, with nothing secret in it. + * + * IMAP servers answer a failed LOGIN with their own prose, and this class + * puts the command in the exception message — which for LOGIN would be the + * password. So the text is matched, never passed through. + */ + private function reason(Throwable $e): string + { + $text = strtolower($e->getMessage()); + + return match (true) { + str_contains($text, 'could not reach') => 'unreachable', + str_contains($text, 'authenticationfailed') || str_contains($text, 'login') => 'credentials', + str_contains($text, 'nonexistent') || str_contains($text, 'select') => 'folder', + default => 'failed', + }; } /** @return array */ @@ -147,7 +215,7 @@ class ImapMailbox implements InboundMailbox $this->command(sprintf( 'LOGIN %s %s', $this->quote($mailbox['username']), - $this->quote((string) config('services.inbound_mail.password')), + $this->quote($this->password()), )); $this->command('SELECT '.$this->quote($mailbox['folder'])); diff --git a/app/Services/Mail/InboundMailStatus.php b/app/Services/Mail/InboundMailStatus.php new file mode 100644 index 0000000..c55a458 --- /dev/null +++ b/app/Services/Mail/InboundMailStatus.php @@ -0,0 +1,59 @@ + Carbon::now()->toIso8601String(), + 'ok' => $ok, + // Never the password, never the whole exception: this ends up on a + // page, and an IMAP error can quote the command it failed on. + 'message' => mb_substr($message, 0, 300), + 'unseen' => $unseen, + ]); + } + + /** + * The last answer, or null if nobody has ever asked. + * + * @return array{at: Carbon, ok: bool, message: string, unseen: int|null}|null + */ + public function last(): ?array + { + $row = Settings::get(self::KEY); + + if (! is_array($row) || ! isset($row['at'])) { + return null; + } + + return [ + // Parsed back into a Carbon so the view can put it on the wall clock + // (R19) rather than printing an ISO string at somebody. + 'at' => Carbon::parse((string) $row['at']), + 'ok' => (bool) ($row['ok'] ?? false), + 'message' => (string) ($row['message'] ?? ''), + 'unseen' => isset($row['unseen']) ? (int) $row['unseen'] : null, + ]; + } +} diff --git a/app/Services/Mail/InboundMailbox.php b/app/Services/Mail/InboundMailbox.php index 3e240fb..ef63fa1 100644 --- a/app/Services/Mail/InboundMailbox.php +++ b/app/Services/Mail/InboundMailbox.php @@ -15,6 +15,20 @@ interface InboundMailbox /** Is a mailbox configured at all? */ public function isConfigured(): bool; + /** + * Reach the mailbox now and say what happened. + * + * For the button in the console: an operator who has just typed a host, a + * user and a password should not have to wait for the next scheduled run to + * find out whether any of it was right — and "the inbox is empty" is not an + * answer to that question. + * + * Never throws. A failed check IS the answer. + * + * @return array{ok: bool, message: string, unseen: int|null} + */ + public function check(): array; + /** * Messages that have arrived, oldest first. * diff --git a/app/Services/Mail/IngestInboundMail.php b/app/Services/Mail/IngestInboundMail.php index 15b5fc1..22e54f6 100644 --- a/app/Services/Mail/IngestInboundMail.php +++ b/app/Services/Mail/IngestInboundMail.php @@ -22,7 +22,10 @@ use Illuminate\Support\Facades\Log; */ class IngestInboundMail { - public function __construct(private readonly InboundMailbox $mailbox) {} + public function __construct( + private readonly InboundMailbox $mailbox, + private readonly InboundMailStatus $status, + ) {} /** * Fetch, store, and flag what was stored. @@ -35,6 +38,18 @@ class IngestInboundMail return 0; } + // The scheduled run records its own outcome, so the "last checked" line + // in the console means the last time the mailbox was actually reached — + // not the last time somebody pressed a button. A mailbox that stopped + // answering at three in the morning is the one worth seeing, and nobody + // presses a button at three in the morning. + $check = $this->mailbox->check(); + $this->status->record($check['ok'], $check['message'], $check['unseen']); + + if (! $check['ok']) { + return 0; + } + $taken = 0; foreach ($this->mailbox->fetch($limit) as $mail) { diff --git a/lang/de/inbox.php b/lang/de/inbox.php index 94f2e58..36ad498 100644 --- a/lang/de/inbox.php +++ b/lang/de/inbox.php @@ -8,6 +8,9 @@ return [ 'fetch_now' => 'Jetzt abrufen', 'fetched' => '{1} :count neue Nachricht|[2,*] :count neue Nachrichten', 'nothing_new' => 'Nichts Neues im Postfach.', + 'never_checked' => 'Das Postfach wurde noch nie abgerufen — der nächste Lauf ist in wenigen Minuten.', + 'last_ok' => 'Postfach zuletzt erreicht: :when (:ago)', + 'last_failed' => 'Postfach zuletzt nicht erreichbar: :when (:ago).', 'show_archived' => 'Erledigte zeigen', 'show_open' => 'Offene zeigen', diff --git a/lang/de/integrations.php b/lang/de/integrations.php index 4551151..2d582ba 100644 --- a/lang/de/integrations.php +++ b/lang/de/integrations.php @@ -88,6 +88,17 @@ return [ 'inbound_port_hint' => '993 ist IMAP über TLS. Alles andere läuft unverschlüsselt — dieser Zugang trägt ein Passwort und die Worte Ihrer Kunden.', 'inbound_username' => 'Postfach', 'inbound_folder' => 'Ordner', + 'inbound_test' => 'Verbindung prüfen', + 'inbound_never' => 'Noch nie geprüft.', + 'inbound_last_ok' => 'Zuletzt erreichbar: :when (:ago)', + 'inbound_last_failed' => 'Zuletzt fehlgeschlagen: :when (:ago)', + 'inbound_ok' => 'Postfach erreichbar.', + 'inbound_ok_waiting' => '{0} Postfach erreichbar, nichts Ungelesenes.|{1} Postfach erreichbar, :count ungelesene Nachricht.|[2,*] Postfach erreichbar, :count ungelesene Nachrichten.', + 'inbound_failed_incomplete' => 'Es fehlen noch Angaben: Server, Postfach oder Passwort.', + 'inbound_failed_unreachable' => 'Der Server ist nicht erreichbar. Stimmen Adresse und Port?', + 'inbound_failed_credentials' => 'Der Server hat die Anmeldung abgelehnt. Stimmen Postfach und Passwort?', + 'inbound_failed_folder' => 'Anmeldung hat funktioniert, den Ordner gibt es aber nicht.', + 'inbound_failed_failed' => 'Die Verbindung ist fehlgeschlagen. Näheres steht im Log.', // Die Reiter. Getrennt nach dem, wofür ein Abschnitt da ist — nicht danach, // welcher Speichermechanismus einen Wert zufällig hält. diff --git a/lang/en/inbox.php b/lang/en/inbox.php index 01f742a..54fbb16 100644 --- a/lang/en/inbox.php +++ b/lang/en/inbox.php @@ -8,6 +8,9 @@ return [ 'fetch_now' => 'Fetch now', 'fetched' => '{1} :count new message|[2,*] :count new messages', 'nothing_new' => 'Nothing new in the mailbox.', + 'never_checked' => 'The mailbox has never been fetched yet — the next run is a few minutes away.', + 'last_ok' => 'Mailbox last reached: :when (:ago)', + 'last_failed' => 'Mailbox last unreachable: :when (:ago).', 'show_archived' => 'Show filed', 'show_open' => 'Show open', diff --git a/lang/en/integrations.php b/lang/en/integrations.php index 24ea100..cbf1014 100644 --- a/lang/en/integrations.php +++ b/lang/en/integrations.php @@ -88,6 +88,17 @@ return [ 'inbound_port_hint' => '993 is IMAP over TLS. Anything else is unencrypted — and this connection carries a password and your customers own words.', 'inbound_username' => 'Mailbox', 'inbound_folder' => 'Folder', + 'inbound_test' => 'Test the connection', + 'inbound_never' => 'Never checked.', + 'inbound_last_ok' => 'Last reached: :when (:ago)', + 'inbound_last_failed' => 'Last failed: :when (:ago)', + 'inbound_ok' => 'Mailbox reachable.', + 'inbound_ok_waiting' => '{0} Mailbox reachable, nothing unread.|{1} Mailbox reachable, :count unread message.|[2,*] Mailbox reachable, :count unread messages.', + 'inbound_failed_incomplete' => 'Something is still missing: server, mailbox or password.', + 'inbound_failed_unreachable' => 'The server cannot be reached. Are the address and port right?', + 'inbound_failed_credentials' => 'The server refused the sign-in. Are the mailbox and password right?', + 'inbound_failed_folder' => 'The sign-in worked, but that folder does not exist.', + 'inbound_failed_failed' => 'The connection failed. There is more in the log.', // The tabs. Split by what a section is for — not by which storage // mechanism happens to hold a given value. diff --git a/resources/views/livewire/admin/inbox.blade.php b/resources/views/livewire/admin/inbox.blade.php index ea57daf..b5ca7fd 100644 --- a/resources/views/livewire/admin/inbox.blade.php +++ b/resources/views/livewire/admin/inbox.blade.php @@ -24,6 +24,31 @@ @endif + {{-- When the mailbox was last reached. Shown whether or not anything came of + it: an empty inbox and a mailbox that stopped answering look exactly the + same, and the difference is this line. --}} + @if ($configured) +

$status === null || $status['ok'], + 'text-danger' => $status !== null && ! $status['ok'], + ])> + @if ($status === null) + {{ __('inbox.never_checked') }} + @else + + {{-- R19: stored in UTC, read on the wall clock. --}} + {{ __($status['ok'] ? 'inbox.last_ok' : 'inbox.last_failed', [ + 'when' => $status['at']->local()->isoFormat('D. MMM YYYY, HH:mm'), + 'ago' => $status['at']->diffForHumans(), + ]) }} + @if (! $status['ok']) + {{ __('inbox.to_integrations') }} + @endif + @endif +

+ @endif + @if ($mails->isEmpty())

{{ $archived ? __('inbox.empty_archived') : __('inbox.empty') }}

diff --git a/resources/views/livewire/admin/integrations.blade.php b/resources/views/livewire/admin/integrations.blade.php index 2a16057..34ec41d 100644 --- a/resources/views/livewire/admin/integrations.blade.php +++ b/resources/views/livewire/admin/integrations.blade.php @@ -125,6 +125,35 @@ @if ($canInfra)
@endif @endif + + {{-- The one thing an operator wants after typing all of it: did any + of it work. The button saves first, because testing what is on + screen while the server still holds the old values would report + on a mailbox nobody configured. --}} + @if ($canInfra) +
+ + {{ __('integrations.inbound_test') }} + + + {{-- Always said, including "never": a line that appears only + after a success leaves an operator unable to tell a + mailbox that has not been checked from one that is fine. + R19: stored in UTC, read on the wall clock. --}} + @if ($inboundStatus === null) +

{{ __('integrations.inbound_never') }}

+ @else +

$inboundStatus['ok'], 'text-danger' => ! $inboundStatus['ok']])> + + {{ __($inboundStatus['ok'] ? 'integrations.inbound_last_ok' : 'integrations.inbound_last_failed', [ + 'when' => $inboundStatus['at']->local()->isoFormat('D. MMM YYYY, HH:mm'), + 'ago' => $inboundStatus['at']->diffForHumans(), + ]) }} +

+ @endif +
+ @endif
@endif diff --git a/tests/Feature/Admin/InboundMailTest.php b/tests/Feature/Admin/InboundMailTest.php index c25eb86..3a1fc00 100644 --- a/tests/Feature/Admin/InboundMailTest.php +++ b/tests/Feature/Admin/InboundMailTest.php @@ -9,6 +9,7 @@ use App\Services\Mail\FetchedMail; use App\Services\Mail\IngestInboundMail; use App\Services\Mail\InboundMailbox; use App\Services\Mail\MailParser; +use Illuminate\Support\Facades\File; use Livewire\Livewire; /** @@ -332,3 +333,108 @@ it('keeps the inbox to operators who may see customers', function () { ->test(Inbox::class) ->assertForbidden(); }); + +// ---- Configured means configured, and the check that says so ---- + +it('counts a password from the vault as configured', function () { + // The bug this closes: isConfigured() read config('services.inbound_mail + // .password'), which is only ever the .env value — this project deliberately + // does not overlay stored secrets onto config at boot. So a password saved in + // the console counted for nothing, and the inbox went on saying no mailbox + // was set up while the password sat right there on the settings page. + App\Support\Settings::set('inbound_mail.host', 'mail.example.test'); + App\Support\Settings::set('inbound_mail.username', 'support@example.test'); + config()->set('services.inbound_mail.password', null); + + $mailbox = new App\Services\Mail\ImapMailbox; + + expect($mailbox->isConfigured())->toBeFalse(); + + app(App\Services\Secrets\SecretVault::class)->put('inbound_mail.password', 'geheim', operator('Owner')); + + expect($mailbox->isConfigured())->toBeTrue(); +}); + +it('still takes the password out of the environment where that is where it is', function () { + // An installation carrying INBOUND_MAIL_PASSWORD keeps working: the vault + // falls back to the configured value when nothing is stored. + App\Support\Settings::set('inbound_mail.host', 'mail.example.test'); + App\Support\Settings::set('inbound_mail.username', 'support@example.test'); + config()->set('services.inbound_mail.password', 'aus-der-env'); + + expect((new App\Services\Mail\ImapMailbox)->isConfigured())->toBeTrue(); +}); + +it('records when the mailbox was last reached, and what came back', function () { + $status = app(App\Services\Mail\InboundMailStatus::class); + + expect($status->last())->toBeNull(); + + $status->record(true, '', 3); + $last = $status->last(); + + expect($last['ok'])->toBeTrue() + ->and($last['unseen'])->toBe(3) + // A Carbon, so the view can put it on the wall clock rather than + // printing an ISO string at somebody (R19). + ->and($last['at'])->toBeInstanceOf(Illuminate\Support\Carbon::class); +}); + +it('records the scheduled run too, not only the button', function () { + // "Last checked" has to mean the last time the mailbox was actually reached. + // A mailbox that stopped answering at three in the morning is the one worth + // seeing, and nobody presses a button at three in the morning. + $this->mailbox->mails = [fetched()]; + + app(IngestInboundMail::class)(); + + expect(app(App\Services\Mail\InboundMailStatus::class)->last())->not->toBeNull(); +}); + +it('takes nothing in when the mailbox cannot be reached, and says so', function () { + // Fetching against a mailbox that just refused the login would report + // "nothing new", which is true and useless. + $this->mailbox->mails = [fetched()]; + $this->mailbox->checkAnswer = ['ok' => false, 'message' => 'credentials', 'unseen' => null]; + + expect(app(IngestInboundMail::class)())->toBe(0) + ->and(InboundMail::query()->count())->toBe(0); + + $last = app(App\Services\Mail\InboundMailStatus::class)->last(); + + expect($last['ok'])->toBeFalse() + ->and($last['message'])->toBe('credentials'); +}); + +it('tests the connection from the settings page and shows when it last answered', function () { + $page = Livewire::actingAs(operator('Owner'), 'operator') + ->test(App\Livewire\Admin\Integrations::class) + ->set('inboundHost', 'mail.example.test') + ->set('inboundUsername', 'support@example.test') + ->call('testInbound'); + + $page->assertDispatched('notify'); + + // And the page now carries the line, whichever way it went. + Livewire::actingAs(operator('Owner'), 'operator') + ->test(App\Livewire\Admin\Integrations::class) + ->assertViewHas('inboundStatus', fn ($status) => $status !== null); +}); + +it('never puts the password or the command into the message it shows', function () { + // IMAP servers answer a failed LOGIN with their own prose, and this class + // puts the failing command in the exception — which for LOGIN is the + // password. The reasons are matched, never passed through. + $reasons = ['incomplete', 'unreachable', 'credentials', 'folder', 'failed']; + + foreach ($reasons as $reason) { + expect(__('integrations.inbound_failed_'.$reason))->not->toBe('integrations.inbound_failed_'.$reason); + } + + $code = File::get(app_path('Services/Mail/ImapMailbox.php')); + + // The reason() method returns one of the fixed words above, and nothing + // derived from the exception text. + expect($code)->toContain("return match (true) {") + ->and($code)->not->toContain('$e->getMessage()]'); +}); diff --git a/tests/Feature/Provisioning/HostStepsTest.php b/tests/Feature/Provisioning/HostStepsTest.php index 7207806..812fdbc 100644 --- a/tests/Feature/Provisioning/HostStepsTest.php +++ b/tests/Feature/Provisioning/HostStepsTest.php @@ -1044,6 +1044,14 @@ it('marks the host active on completion', function () { }); it('gives the host a name that points at the tunnel, not at its public address', function () { + // The zone is pinned here rather than read from the machine's own .env. It + // used to assert the literal clupilot.com, which was true until somebody + // set CLUPILOT_DNS_ZONE to clupilot.cloud on their installation — and then + // this test failed on a correct configuration. What it is about is the + // SHAPE of the name and which address it carries, not which domain this + // particular box is configured for. + config()->set('provisioning.dns.zone', 'clupilot.com'); + $s = fakeServices(); $host = Host::factory()->active()->create([ 'datacenter' => 'fsn',