From 7be81d3586f7d5149e9343b7f4c9e493c400b28a Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:41:38 +0200 Subject: [PATCH] Tell the admins the mailbox is dead, even when a send noticed first The daily refresh doubles as the health check for a connected OAuth mailbox, and its own docblock says why that matters: a grant can die silently, "which for a portal whose password-reset mails ride on this connection must surface as a warning, not as a support ticket weeks later". It decided whether to warn by reading last_error -- but the send path writes that column too. OAuthCodeFlowBroker::refresh() records the failure and notifies nobody, and freshAccessToken() reaches it from every send. So on an installation that actually sends mail, the send lands first, the command reads the column as "already told them", and the warning never goes out. last_error is cleared only by a successful refresh, which a dead grant never has, so it never goes out again either. Measured on main, one dead grant, two orders: nobody sends, command first 1 notification, then quiet correct a password-reset mail first 0 ... 0 ... 0 never The alarm worked on installations that were not using the mailbox and failed on the ones that were. The anti-nag rule is not the problem and does not change. The problem is that last_error answers "is this broken", which any writer may set, while the command needs "have the admins been told", which only the notifier can. The table's own comment shows the conflation -- one column described as "what the settings page's warning and the admin notification read". So the notification gets its own column. broken_notified_at is stamped when the command notifies, and cleared wherever last_error is cleared: a successful refresh, a disconnect, a changed client id. The three call sites go through MailOAuthConnection::clearFailure() rather than nulling two columns each, because a connection left marked "already told them" while healthy would go quiet the next time it died -- the same bug in a new place. --- .../Http/Controllers/EmailOAuthController.php | 5 +- .../Controllers/EmailSettingsController.php | 2 +- .../Console/RefreshMailOAuthTokensCommand.php | 15 +++- .../Platform/Mail/MailOAuthConnection.php | 19 +++++ .../Platform/Mail/OAuthCodeFlowBroker.php | 2 +- ...ied_at_to_mail_oauth_connections_table.php | 38 ++++++++++ tests/Feature/Platform/MailOAuthTest.php | 76 +++++++++++++++++++ 7 files changed, 152 insertions(+), 5 deletions(-) create mode 100644 database/migrations/2026_09_07_090000_add_broken_notified_at_to_mail_oauth_connections_table.php diff --git a/app/Modules/Platform/Http/Controllers/EmailOAuthController.php b/app/Modules/Platform/Http/Controllers/EmailOAuthController.php index 218e9edf..f0cf09f3 100644 --- a/app/Modules/Platform/Http/Controllers/EmailOAuthController.php +++ b/app/Modules/Platform/Http/Controllers/EmailOAuthController.php @@ -162,8 +162,9 @@ class EmailOAuthController extends Controller 'refresh_token' => null, 'token_expires_at' => null, 'account_email' => null, - 'last_error' => null, - ])->save(); + ]); + $connection->clearFailure(); + $connection->save(); $this->activateConnection(); diff --git a/app/Modules/Platform/Http/Controllers/EmailSettingsController.php b/app/Modules/Platform/Http/Controllers/EmailSettingsController.php index efa512aa..be069fdf 100644 --- a/app/Modules/Platform/Http/Controllers/EmailSettingsController.php +++ b/app/Modules/Platform/Http/Controllers/EmailSettingsController.php @@ -185,8 +185,8 @@ class EmailSettingsController extends Controller 'refresh_token' => null, 'token_expires_at' => null, 'account_email' => null, - 'last_error' => null, ]); + $connection->clearFailure(); } $connection->save(); diff --git a/app/Modules/Platform/Mail/Console/RefreshMailOAuthTokensCommand.php b/app/Modules/Platform/Mail/Console/RefreshMailOAuthTokensCommand.php index 91976c5b..4e3f06d9 100644 --- a/app/Modules/Platform/Mail/Console/RefreshMailOAuthTokensCommand.php +++ b/app/Modules/Platform/Mail/Console/RefreshMailOAuthTokensCommand.php @@ -71,7 +71,17 @@ class RefreshMailOAuthTokensCommand extends Command // notification would otherwise repeat daily for as long // as nobody reconnects, and a nagging alert trains // people to ignore the one that matters. - if (! $hadError) { + // + // Asked of broken_notified_at, not of last_error. The + // question is "have the admins been told", and last_error + // cannot answer it: the send path writes that column too + // (OAuthCodeFlowBroker::refresh, reached from + // freshAccessToken) and notifies nobody. On an + // installation that actually sends mail, that write lands + // first — so reading it as "already told them" left this + // silent for good, on exactly the installations whose + // password-reset mail rides on the connection. + if ($connection->broken_notified_at === null) { $recipients = array_values(User::query()->where('type', UserType::Staff)->get() ->filter(fn (User $staff): bool => $permissions->allows($staff, Permission::EditSettings)) ->all()); @@ -80,6 +90,9 @@ class RefreshMailOAuthTokensCommand extends Command 'provider' => $connection->provider->label(), 'account' => (string) $connection->account_email, ]); + + $connection->broken_notified_at = now(); + $connection->save(); } $mailConfig->flush(); diff --git a/app/Modules/Platform/Mail/MailOAuthConnection.php b/app/Modules/Platform/Mail/MailOAuthConnection.php index ba7ddb3c..29219cc6 100644 --- a/app/Modules/Platform/Mail/MailOAuthConnection.php +++ b/app/Modules/Platform/Mail/MailOAuthConnection.php @@ -35,6 +35,7 @@ use Illuminate\Support\Carbon; * @property Carbon|null $token_expires_at * @property Carbon|null $last_refreshed_at * @property string|null $last_error + * @property Carbon|null $broken_notified_at */ class MailOAuthConnection extends Model { @@ -51,9 +52,27 @@ class MailOAuthConnection extends Model 'refresh_token' => 'encrypted', 'token_expires_at' => 'datetime', 'last_refreshed_at' => 'datetime', + 'broken_notified_at' => 'datetime', ]; } + /** + * The failure is over: the error and the record of having alarmed + * about it go together, because they describe one state. + * + * One method rather than two nulls at each call site. The three + * places that end a failure — a successful refresh, a disconnect, a + * changed client id — must never clear one and keep the other: a + * connection that is healthy but still marked "already told them" + * would go quiet the next time it dies, which is the shape of the + * bug this column was added to close. + */ + public function clearFailure(): void + { + $this->last_error = null; + $this->broken_notified_at = null; + } + public static function for(MailProvider $provider): self { return static::query()->firstOrNew(['provider' => $provider->value]); diff --git a/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php b/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php index 7a7b6d9b..2e4d77e6 100644 --- a/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php +++ b/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php @@ -172,7 +172,7 @@ abstract class OAuthCodeFlowBroker implements MailOAuthBroker } $connection->last_refreshed_at = now(); - $connection->last_error = null; + $connection->clearFailure(); $connection->save(); } diff --git a/database/migrations/2026_09_07_090000_add_broken_notified_at_to_mail_oauth_connections_table.php b/database/migrations/2026_09_07_090000_add_broken_notified_at_to_mail_oauth_connections_table.php new file mode 100644 index 00000000..ca3d94b7 --- /dev/null +++ b/database/migrations/2026_09_07_090000_add_broken_notified_at_to_mail_oauth_connections_table.php @@ -0,0 +1,38 @@ +timestamp('broken_notified_at')->nullable()->after('last_error'); + }); + } + + public function down(): void + { + Schema::table('mail_oauth_connections', function (Blueprint $table) { + $table->dropColumn('broken_notified_at'); + }); + } +}; diff --git a/tests/Feature/Platform/MailOAuthTest.php b/tests/Feature/Platform/MailOAuthTest.php index 6dc99de5..0c5fa847 100644 --- a/tests/Feature/Platform/MailOAuthTest.php +++ b/tests/Feature/Platform/MailOAuthTest.php @@ -454,6 +454,82 @@ test('a dead grant records the error and notifies settings admins exactly once', expect(InAppNotification::query()->where('type', 'mail_oauth_connection_broken')->count())->toBe(1); }); +/** + * The same grant, dying in the order it actually dies on an installation + * that sends mail: a message goes out, the transport refreshes, and the + * failure is recorded by the send path — which notifies nobody. Reading + * that record as "already told them" is what kept the daily command + * silent for good. + */ +test('a send that reaches the dead grant first does not swallow the alarm', function () { + Http::fake([ + 'login.microsoftonline.com/*' => Http::response([ + 'error' => 'invalid_grant', + 'error_description' => 'AADSTS50173: The provided grant has expired.', + ], 400), + ]); + + $connection = connectMicrosoftMailbox(); + $connection->fill(['token_expires_at' => now()->subMinute()])->save(); + + // A password-reset mail — the case the command's docblock is about. + try { + Mail::mailer('microsoft-graph')->raw('Reset your password', function ($message) { + $message->to('client@example.com')->subject('Password reset'); + }); + } catch (TransportException) { + // The send fails; that half already worked. + } + + // The send path records the failure and tells nobody, as before. + expect(MailOAuthConnection::for(MailProvider::Microsoft365)->last_error)->toContain('AADSTS50173') + ->and(InAppNotification::query()->where('type', 'mail_oauth_connection_broken')->count())->toBe(0); + + Artisan::call('projectsend:refresh-mail-oauth-tokens'); + + expect(InAppNotification::query()->where('type', 'mail_oauth_connection_broken')->count())->toBe(1); + + // And still exactly once: the anti-nag rule is unchanged. + Artisan::call('projectsend:refresh-mail-oauth-tokens'); + + expect(InAppNotification::query()->where('type', 'mail_oauth_connection_broken')->count())->toBe(1); +}); + +test('a connection that recovers can raise the alarm a second time', function () { + Http::fake([ + 'login.microsoftonline.com/*' => Http::sequence() + ->push(['error' => 'invalid_grant'], 400) + ->push(fakeTokenResponse()) + ->push(['error' => 'invalid_grant'], 400), + ]); + + connectMicrosoftMailbox(); + + Artisan::call('projectsend:refresh-mail-oauth-tokens'); // dies, alarms + Artisan::call('projectsend:refresh-mail-oauth-tokens'); // recovers + Artisan::call('projectsend:refresh-mail-oauth-tokens'); // dies again + + expect(InAppNotification::query()->where('type', 'mail_oauth_connection_broken')->count())->toBe(2); +}); + +test('ending the failure any other way also clears the record of having alarmed', function () { + Http::fake([ + 'login.microsoftonline.com/*' => Http::response(['error' => 'invalid_grant'], 400), + ]); + + connectMicrosoftMailbox(); + Artisan::call('projectsend:refresh-mail-oauth-tokens'); + + expect(MailOAuthConnection::for(MailProvider::Microsoft365)->broken_notified_at)->not->toBeNull(); + + $this->actingAs($this->admin) + ->from('/system/settings/email') + ->delete('/system/settings/email/oauth') + ->assertRedirect(); + + expect(MailOAuthConnection::for(MailProvider::Microsoft365)->broken_notified_at)->toBeNull(); +}); + test('a transient token endpoint failure neither flags the connection nor notifies anyone', function () { Http::fake([ 'login.microsoftonline.com/*' => Http::response(['error' => 'temporarily_unavailable'], 503),