diff --git a/app/Modules/Platform/Mail/Console/RefreshMailOAuthTokensCommand.php b/app/Modules/Platform/Mail/Console/RefreshMailOAuthTokensCommand.php index 91976c5b..42382be2 100644 --- a/app/Modules/Platform/Mail/Console/RefreshMailOAuthTokensCommand.php +++ b/app/Modules/Platform/Mail/Console/RefreshMailOAuthTokensCommand.php @@ -50,7 +50,10 @@ class RefreshMailOAuthTokensCommand extends Command $hadError = $connection->last_error !== null; try { - $brokers->for($connection->provider)->refresh($connection); + // Serialised against sends: refresh() on its own is the + // other half of the race freshAccessToken()'s lock is + // there to stop. + $brokers->for($connection->provider)->refreshSerially($connection); $this->info("Refreshed {$connection->provider->value} ({$connection->account_email})."); diff --git a/app/Modules/Platform/Mail/MailOAuthBroker.php b/app/Modules/Platform/Mail/MailOAuthBroker.php index 3958d2bc..cde14387 100644 --- a/app/Modules/Platform/Mail/MailOAuthBroker.php +++ b/app/Modules/Platform/Mail/MailOAuthBroker.php @@ -37,6 +37,15 @@ interface MailOAuthBroker */ public function refresh(MailOAuthConnection $connection): void; + /** + * A refresh that is not racing a send: the scheduled health check's + * way in, serialised against freshAccessToken() on the same + * connection. + * + * @throws MailOAuthException + */ + public function refreshSerially(MailOAuthConnection $connection): void; + /** * An access token currently valid for at least a small safety margin, * refreshing first when needed — what transports call at send time. diff --git a/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php b/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php index 7a7b6d9b..cdfc2f54 100644 --- a/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php +++ b/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Modules\Platform\Mail; +use Illuminate\Contracts\Cache\Lock; use Illuminate\Contracts\Cache\LockTimeoutException; use Illuminate\Http\Client\Response; use Illuminate\Support\Facades\Cache; @@ -84,6 +85,48 @@ abstract class OAuthCodeFlowBroker implements MailOAuthBroker $this->storeTokens($connection, $response); } + /** + * The scheduled refresh, holding the same lock a send would. + * + * freshAccessToken() takes that lock because a refresh token is good + * for exactly one use, and it names this command as one of the racers: + * "a worker racing the nightly refresh command means the slower one + * spends a token the faster one has already replaced", which the + * provider answers with an invalid_grant indistinguishable from a + * revoked grant. The command was doing its refresh outside the lock, + * so it was the other half of that race rather than a party to it. + * + * Unlike freshAccessToken() this refreshes a token that is still + * usable, which is the point of the daily run: a delegated refresh + * token dies of disuse, and the refresh keeps the window sliding. + * + * Taken rather than waited for, unlike the send path: nobody is + * standing at a screen here, and a held lock means somebody is + * refreshing this very connection right now — which slides the window + * and establishes its health just as well as doing it again would. + * Spending the token behind them is the false alarm the lock exists to + * prevent. + */ + public function refreshSerially(MailOAuthConnection $connection): void + { + $lock = $this->refreshLock($connection); + + if (! $lock->get()) { + return; + } + + try { + // Re-read first: the winner may have stored tokens while this + // call was waiting, and refreshing the copy walked in with + // would spend a refresh token that is no longer current. + $connection->refresh(); + + $this->refresh($connection); + } finally { + $lock->release(); + } + } + public function freshAccessToken(MailOAuthConnection $connection): string { if ($this->stillUsable($connection)) { @@ -104,7 +147,7 @@ abstract class OAuthCodeFlowBroker implements MailOAuthBroker // 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); + $lock = $this->refreshLock($connection); try { $lock->block(15); @@ -135,6 +178,16 @@ abstract class OAuthCodeFlowBroker implements MailOAuthBroker } /** Whether the stored access token has enough life left to send with. */ + /** + * One refresh at a time per connection, whoever is asking. The TTL + * outlives a token request and releases the claim if the holder dies + * mid-flight. + */ + private function refreshLock(MailOAuthConnection $connection): Lock + { + return Cache::lock('mail-oauth-refresh:'.$connection->provider->value, 30); + } + private function stillUsable(MailOAuthConnection $connection): bool { $token = $connection->access_token; diff --git a/tests/Feature/Platform/MailOAuthTest.php b/tests/Feature/Platform/MailOAuthTest.php index 6dc99de5..0811a8db 100644 --- a/tests/Feature/Platform/MailOAuthTest.php +++ b/tests/Feature/Platform/MailOAuthTest.php @@ -11,6 +11,7 @@ use App\Modules\Platform\Settings\MailConfigApplier; use App\Modules\Platform\Settings\MailProvider; use App\Modules\Platform\Settings\MailProviderSettings; use Illuminate\Support\Facades\Artisan; +use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Mail; @@ -425,6 +426,39 @@ test('the scheduled refresh keeps a healthy connection fresh', function () { ->and($connection->last_refreshed_at)->not->toBeNull(); }); +// freshAccessToken() takes a per-connection lock because a refresh token +// is good for one use, and its comment names this command as one of the +// racers. The command refreshed outside that lock, so it was the other +// half of the race rather than a party to it. +test('the scheduled refresh stands aside for a send that is already refreshing', function () { + Http::fake([ + 'login.microsoftonline.com/*' => Http::response(fakeTokenResponse([ + 'access_token' => 'access-token-2', + 'refresh_token' => 'refresh-token-2', + ])), + ]); + + connectMicrosoftMailbox(); + + $before = MailOAuthConnection::for(MailProvider::Microsoft365); + + // Somebody else is mid-refresh on this connection. + $held = Cache::lock('mail-oauth-refresh:microsoft365', 30); + expect($held->get())->toBeTrue(); + + Artisan::call('projectsend:refresh-mail-oauth-tokens'); + + // The token the holder is spending was not spent a second time. + Http::assertNothingSent(); + + $after = MailOAuthConnection::for(MailProvider::Microsoft365); + expect($after->access_token)->toBe($before->access_token) + ->and($after->refresh_token)->toBe($before->refresh_token) + ->and($after->last_error)->toBeNull(); + + $held->release(); +}); + test('a dead grant records the error and notifies settings admins exactly once', function () { Http::fake([ 'login.microsoftonline.com/*' => Http::response([