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([