From eda8091aefb15fd64e3c548712d1ed16766aee4e Mon Sep 17 00:00:00 2001 From: ignacionelson Date: Tue, 25 Aug 2026 00:24:41 -0300 Subject: [PATCH] Serialise OAuth token refreshes, and don't offer Connect for an unsaved provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both fixes are follow-ups to the mail providers @denkfabrik-li added in #1679. A refresh token is good for exactly one use — Microsoft and Google both retire it as they issue the next one. Two queue workers finding the same expired access token would therefore both spend it, and the loser gets invalid_grant back. That is the same answer a revoked grant gives, so a healthy connection would be marked broken, painted red on the settings page and mailed to every admin. Refreshes now hold a per-connection lock and whoever waits re-reads the row, which normally means finding a token the winner already stored and not refreshing at all. The Connect button read the provider dropdown, but the flow it starts uses the saved provider. On an installation with both vendors registered, switching without saving would open the wrong consent screen. The dropdown now counts as an unsaved change like any other field, which also gives it the right "save first" hint for free. --- .../Platform/Mail/OAuthCodeFlowBroker.php | 64 +++++++++++++++++-- resources/js/pages/system/settings/email.tsx | 10 ++- tests/Feature/Platform/MailOAuthTest.php | 45 +++++++++++++ 3 files changed, 110 insertions(+), 9 deletions(-) diff --git a/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php b/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php index 03582c7f..7a7b6d9b 100644 --- a/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php +++ b/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php @@ -4,7 +4,9 @@ declare(strict_types=1); namespace App\Modules\Platform\Mail; +use Illuminate\Contracts\Cache\LockTimeoutException; use Illuminate\Http\Client\Response; +use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Http; /** @@ -83,17 +85,65 @@ abstract class OAuthCodeFlowBroker implements MailOAuthBroker } public function freshAccessToken(MailOAuthConnection $connection): string + { + if ($this->stillUsable($connection)) { + return (string) $connection->access_token; + } + + // Both providers rotate the refresh token as they hand out a new + // access token, so a refresh token is good for exactly one use. + // Two queue workers reaching an expired token at the same moment — + // or a worker racing the nightly refresh command — means the slower + // one spends a token the faster one has already replaced. The + // provider answers that with invalid_grant, which is the same thing + // it says about a genuinely revoked grant: last_error gets written, + // the settings page turns red, and every admin is told to go and + // re-consent a connection that was never broken. + // + // So refresh one at a time per connection, and make whoever waited + // re-read the row instead of trusting the copy it walked in with: by + // the time the lock is theirs, the winner has already stored a token + // they can just use. + $lock = Cache::lock('mail-oauth-refresh:'.$connection->provider->value, 30); + + try { + $lock->block(15); + } catch (LockTimeoutException) { + // Fifteen seconds means something is wrong with the lock rather + // than with the provider. Racing is a false alarm; not sending is + // a lost message. Take the race. + $this->refresh($connection); + + return (string) $connection->access_token; + } + + try { + // Eloquent's refresh(), re-reading the row — not this class's, + // which is the thing the lock exists to serialise. + $connection->refresh(); + + if ($this->stillUsable($connection)) { + return (string) $connection->access_token; + } + + $this->refresh($connection); + + return (string) $connection->access_token; + } finally { + $lock->release(); + } + } + + /** Whether the stored access token has enough life left to send with. */ + private function stillUsable(MailOAuthConnection $connection): bool { $token = $connection->access_token; $expiresAt = $connection->token_expires_at; - if (is_string($token) && $token !== '' && $expiresAt !== null && $expiresAt->gt(now()->addSeconds(self::EXPIRY_MARGIN_SECONDS))) { - return $token; - } - - $this->refresh($connection); - - return (string) $connection->access_token; + return is_string($token) + && $token !== '' + && $expiresAt !== null + && $expiresAt->gt(now()->addSeconds(self::EXPIRY_MARGIN_SECONDS)); } private function storeTokens(MailOAuthConnection $connection, Response $response): void diff --git a/resources/js/pages/system/settings/email.tsx b/resources/js/pages/system/settings/email.tsx index 8827fb92..b01a72b7 100644 --- a/resources/js/pages/system/settings/email.tsx +++ b/resources/js/pages/system/settings/email.tsx @@ -108,9 +108,15 @@ export default function EmailSettings({ // Connecting sends the browser to the provider with whatever is in // the database — a registration typed into the form but not yet // saved would silently not be the one used, so the button waits. + // + // The dropdown itself counts as part of that. The flow starts against + // the *saved* provider, so on an installation with both vendors + // registered, switching without saving and pressing Connect would open + // the other one's consent screen while this page says otherwise. const oauthDirty = - connection !== undefined && - (data.client_id !== connection.client_id || data.client_secret !== '' || data.tenant_id !== connection.tenant_id); + data.provider !== mail_provider.provider || + (connection !== undefined && + (data.client_id !== connection.client_id || data.client_secret !== '' || data.tenant_id !== connection.tenant_id)); const canConnect = connection !== undefined && connection.client_id !== '' && connection.has_client_secret && !oauthDirty; const addRecipient = () => { diff --git a/tests/Feature/Platform/MailOAuthTest.php b/tests/Feature/Platform/MailOAuthTest.php index 53ebfc48..6dc99de5 100644 --- a/tests/Feature/Platform/MailOAuthTest.php +++ b/tests/Feature/Platform/MailOAuthTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); use App\Models\User; use App\Modules\Notifications\InAppNotification; use App\Modules\Platform\Capabilities\Edition; +use App\Modules\Platform\Mail\MailOAuthBrokers; use App\Modules\Platform\Mail\MailOAuthConnection; use App\Modules\Platform\Settings\MailConfigApplier; use App\Modules\Platform\Settings\MailProvider; @@ -360,6 +361,50 @@ test('a Graph refusal surfaces as a send failure, not a silent success', functio }))->toThrow(TransportException::class, 'Not allowed to send as this user'); }); +test('a second sender waits for the refresh in flight instead of spending the rotated token', function () { + // Both providers hand back a new refresh token every time and retire + // the old one, so a refresh token is good for exactly one use. Without + // the lock, two senders that both find an expired access token would + // both POST the same refresh token; the loser gets invalid_grant, which + // is indistinguishable from a revoked grant and would wrongly mark the + // connection broken. The winner's stored token is what the other one + // should end up sending with. + Http::fake([ + 'login.microsoftonline.com/*' => Http::response(fakeTokenResponse([ + 'access_token' => 'access-token-2', + 'refresh_token' => 'refresh-token-2', + ])), + ]); + + $connection = connectMicrosoftMailbox(); + $connection->fill(['token_expires_at' => now()->subMinute()])->save(); + + $broker = app(MailOAuthBrokers::class)->for(MailProvider::Microsoft365); + + // Two separate model instances, exactly as two queue workers would each + // have read the row for themselves a moment before it was rotated. + $first = $broker->freshAccessToken(MailOAuthConnection::for(MailProvider::Microsoft365)); + $second = $broker->freshAccessToken(MailOAuthConnection::for(MailProvider::Microsoft365)); + + expect($first)->toBe('access-token-2') + ->and($second)->toBe('access-token-2'); + + // One refresh between them, not two — the second re-read the row under + // the lock and found a token it could just use. + $refreshes = 0; + Http::assertSent(function ($request) use (&$refreshes): bool { + if (str_contains($request->url(), '/token') && $request['grant_type'] === 'refresh_token') { + $refreshes++; + } + + return true; + }); + + expect($refreshes)->toBe(1); + + expect(MailOAuthConnection::for(MailProvider::Microsoft365)->last_error)->toBeNull(); +}); + test('the scheduled refresh keeps a healthy connection fresh', function () { Http::fake([ 'login.microsoftonline.com/*' => Http::response(fakeTokenResponse([