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 29ae8358..c1d19652 100644 --- a/app/Modules/Platform/Mail/Console/RefreshMailOAuthTokensCommand.php +++ b/app/Modules/Platform/Mail/Console/RefreshMailOAuthTokensCommand.php @@ -80,7 +80,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()); @@ -89,6 +99,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 fee8b6be..5e94d5c0 100644 --- a/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php +++ b/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php @@ -230,7 +230,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 4a277353..3f5201c5 100644 --- a/tests/Feature/Platform/MailOAuthTest.php +++ b/tests/Feature/Platform/MailOAuthTest.php @@ -494,6 +494,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),