Fix X token chain breaking from over-rotation
X OAuth2 refresh tokens are single-use: each refresh rotates the pair and invalidates the previous refresh_token, and reusing a rotated one kills the whole family. Three things made this fragile and disconnected accounts far more often than necessary: - The proactive refresh job called refreshToken() directly, bypassing the access-token-first guard in verify() and rotating on every run. - RefreshExpiringTokens used a 2h window on an hourly schedule — equal to the 2h access-token lifetime — so every X account was rotated every hour even while its token was still valid. - A single 4xx refresh failure disconnected the account without checking whether a concurrent refresh had already persisted a working token. Changes: - RefreshSocialToken now routes through verify() (access-token-first), so it only rotates when the access_token is actually invalid. - Shrink the proactive window to 30m and run the command every 15m, so the window still covers the run interval but rotation happens near real expiry. - verify() tolerates the lost-rotation race: on a 4xx refresh, reload and verify with a concurrently-refreshed token before marking TokenExpired. Refs #126
This commit is contained in:
parent
1284c46960
commit
2f4b974130
7 changed files with 102 additions and 23 deletions
|
|
@ -13,7 +13,7 @@ class RefreshExpiringTokens extends Command
|
|||
{
|
||||
protected $signature = 'social:refresh-expiring-tokens';
|
||||
|
||||
protected $description = 'Proactively refresh tokens expiring in the next 2 hours (or already expired)';
|
||||
protected $description = 'Proactively refresh tokens expiring in the next 30 minutes (or already expired)';
|
||||
|
||||
public function handle(): void
|
||||
{
|
||||
|
|
@ -22,7 +22,7 @@ public function handle(): void
|
|||
SocialAccount::query()
|
||||
->where('status', Status::Connected)
|
||||
->whereNotNull('token_expires_at')
|
||||
->where('token_expires_at', '<=', now()->addHours(2))
|
||||
->where('token_expires_at', '<=', now()->addMinutes(30))
|
||||
->chunk(50, function ($accounts) use (&$count) {
|
||||
foreach ($accounts as $account) {
|
||||
RefreshSocialToken::dispatch($account);
|
||||
|
|
|
|||
|
|
@ -24,7 +24,7 @@ public function __construct(public SocialAccount $account) {}
|
|||
public function handle(ConnectionVerifier $verifier): void
|
||||
{
|
||||
try {
|
||||
$verifier->refreshToken($this->account);
|
||||
$verifier->verify($this->account);
|
||||
} catch (PlatformUnavailableException $e) {
|
||||
Log::warning('Token refresh skipped: platform unavailable', [
|
||||
'account_id' => $this->account->id,
|
||||
|
|
|
|||
|
|
@ -31,9 +31,7 @@ public function verify(SocialAccount $account): bool
|
|||
// refresh, so proactive refreshes during races cause false-positive
|
||||
// disconnects even though the access_token still works fine.
|
||||
if ($account->is_token_expired) {
|
||||
$this->refreshToken($account);
|
||||
|
||||
return $this->callVerifyEndpoint($account);
|
||||
return $this->refreshThenVerify($account);
|
||||
}
|
||||
|
||||
try {
|
||||
|
|
@ -41,14 +39,39 @@ public function verify(SocialAccount $account): bool
|
|||
} catch (TokenExpiredException $e) {
|
||||
// Verify returned 401: the access_token is actually invalid.
|
||||
// Refresh and retry once with the new token.
|
||||
try {
|
||||
$this->refreshToken($account);
|
||||
} catch (TokenExpiredException) {
|
||||
throw $e;
|
||||
return $this->refreshThenVerify($account, $e);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Refresh the token, then verify with the new one.
|
||||
*
|
||||
* If the refresh is rejected (4xx) but a concurrent refresh has already
|
||||
* rotated the account and persisted a fresh access_token, reload and
|
||||
* verify with that token instead of giving up — X (and other providers
|
||||
* that single-use their refresh_token) otherwise disconnect a still-usable
|
||||
* account whenever two refreshes race and one loses the rotation.
|
||||
*
|
||||
* @throws TokenExpiredException
|
||||
* @throws PlatformUnavailableException
|
||||
*/
|
||||
private function refreshThenVerify(SocialAccount $account, ?TokenExpiredException $original = null): bool
|
||||
{
|
||||
$accessTokenBeforeRefresh = $account->access_token;
|
||||
|
||||
try {
|
||||
$this->refreshToken($account);
|
||||
} catch (TokenExpiredException $e) {
|
||||
$account->refresh();
|
||||
|
||||
if ($account->access_token !== $accessTokenBeforeRefresh) {
|
||||
return $this->callVerifyEndpoint($account);
|
||||
}
|
||||
|
||||
return $this->callVerifyEndpoint($account);
|
||||
throw $original ?? $e;
|
||||
}
|
||||
|
||||
return $this->callVerifyEndpoint($account);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -14,7 +14,7 @@
|
|||
|
||||
Schedule::command(ProcessScheduledPosts::class)->everyMinute()->withoutOverlapping()->onOneServer();
|
||||
Schedule::command(CheckSocialConnections::class)->daily()->withoutOverlapping()->onOneServer();
|
||||
Schedule::command(RefreshExpiringTokens::class)->hourly()->withoutOverlapping()->onOneServer();
|
||||
Schedule::command(RefreshExpiringTokens::class)->everyFifteenMinutes()->withoutOverlapping()->onOneServer();
|
||||
Schedule::command(RecoverStuckPosts::class)->everyThirtyMinutes()->withoutOverlapping()->onOneServer();
|
||||
Schedule::command(FireScheduleTriggers::class)->everyMinute()->withoutOverlapping()->onOneServer();
|
||||
Schedule::command(ProcessAutomationDelays::class)->everyMinute()->withoutOverlapping()->onOneServer();
|
||||
|
|
|
|||
|
|
@ -9,25 +9,25 @@
|
|||
use App\Models\Workspace;
|
||||
use Illuminate\Support\Facades\Queue;
|
||||
|
||||
test('it dispatches refresh jobs for tokens expiring within 2 hours or already expired', function () {
|
||||
test('it dispatches refresh jobs for tokens expiring within 30 minutes or already expired', function () {
|
||||
Queue::fake();
|
||||
|
||||
$workspace = Workspace::factory()->create();
|
||||
|
||||
// Should be refreshed (expires in 1 hour)
|
||||
// Should be refreshed (expires in 15 minutes — inside the proactive window)
|
||||
$expiringSoon = SocialAccount::factory()->create([
|
||||
'workspace_id' => $workspace->id,
|
||||
'platform' => Platform::LinkedIn,
|
||||
'status' => Status::Connected,
|
||||
'token_expires_at' => now()->addHour(),
|
||||
'token_expires_at' => now()->addMinutes(15),
|
||||
]);
|
||||
|
||||
// Should NOT be refreshed (expires in 5 hours — outside the proactive window)
|
||||
// Should NOT be refreshed (expires in 1 hour — outside the proactive window)
|
||||
SocialAccount::factory()->create([
|
||||
'workspace_id' => $workspace->id,
|
||||
'platform' => Platform::Instagram,
|
||||
'status' => Status::Connected,
|
||||
'token_expires_at' => now()->addHours(5),
|
||||
'token_expires_at' => now()->addHour(),
|
||||
]);
|
||||
|
||||
// SHOULD be refreshed (already expired — last-chance attempt before the
|
||||
|
|
|
|||
|
|
@ -11,6 +11,7 @@
|
|||
use App\Models\User;
|
||||
use App\Models\Workspace;
|
||||
use App\Services\Social\ConnectionVerifier;
|
||||
use Illuminate\Support\Facades\Http;
|
||||
use Illuminate\Support\Facades\Log;
|
||||
use Illuminate\Support\Facades\Queue;
|
||||
|
||||
|
|
@ -24,22 +25,46 @@
|
|||
]);
|
||||
});
|
||||
|
||||
test('refresh job calls refreshToken (not verify) on the verifier', function () {
|
||||
test('refresh job routes through verify (access-token-first) not refreshToken', function () {
|
||||
$verifier = mock(ConnectionVerifier::class);
|
||||
$verifier->shouldReceive('refreshToken')->once()->with(
|
||||
$verifier->shouldReceive('verify')->once()->with(
|
||||
Mockery::on(fn ($account) => $account->id === $this->account->id)
|
||||
);
|
||||
$verifier->shouldNotReceive('verify');
|
||||
$verifier->shouldNotReceive('refreshToken');
|
||||
app()->instance(ConnectionVerifier::class, $verifier);
|
||||
|
||||
(new RefreshSocialToken($this->account))->handle($verifier);
|
||||
});
|
||||
|
||||
test('proactive refresh does NOT rotate the X refresh token while the access token still works', function () {
|
||||
Http::fake([
|
||||
config('trypost.platforms.x.api').'/users/me' => Http::response(['data' => ['id' => '123']], 200),
|
||||
config('trypost.platforms.x.api').'/oauth2/token' => Http::response([
|
||||
'access_token' => 'should-not-be-used',
|
||||
'refresh_token' => 'should-not-be-used',
|
||||
'expires_in' => 7200,
|
||||
], 200),
|
||||
]);
|
||||
|
||||
// Token is "expiring soon" (inside the proactive window) but still valid.
|
||||
$this->account->update([
|
||||
'token_expires_at' => now()->addMinutes(20),
|
||||
'refresh_token' => 'original-refresh-token',
|
||||
]);
|
||||
|
||||
(new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class));
|
||||
|
||||
Http::assertSent(fn ($request) => str_contains($request->url(), '/users/me'));
|
||||
Http::assertNotSent(fn ($request) => str_contains($request->url(), '/oauth2/token'));
|
||||
expect($this->account->fresh()->refresh_token)->toBe('original-refresh-token');
|
||||
expect($this->account->fresh()->status)->toBe(Status::Connected);
|
||||
});
|
||||
|
||||
test('refresh job marks account as TokenExpired when refresh_token is rejected', function () {
|
||||
Queue::fake();
|
||||
|
||||
$verifier = mock(ConnectionVerifier::class);
|
||||
$verifier->shouldReceive('refreshToken')->once()->andThrow(
|
||||
$verifier->shouldReceive('verify')->once()->andThrow(
|
||||
new TokenExpiredException('refresh_token revoked')
|
||||
);
|
||||
app()->instance(ConnectionVerifier::class, $verifier);
|
||||
|
|
@ -61,7 +86,7 @@
|
|||
});
|
||||
|
||||
$verifier = mock(ConnectionVerifier::class);
|
||||
$verifier->shouldReceive('refreshToken')->once()->andThrow(new RuntimeException('network blip'));
|
||||
$verifier->shouldReceive('verify')->once()->andThrow(new RuntimeException('network blip'));
|
||||
app()->instance(ConnectionVerifier::class, $verifier);
|
||||
|
||||
(new RefreshSocialToken($this->account))->handle($verifier);
|
||||
|
|
@ -79,7 +104,7 @@
|
|||
});
|
||||
|
||||
$verifier = mock(ConnectionVerifier::class);
|
||||
$verifier->shouldReceive('refreshToken')->once()->andThrow(
|
||||
$verifier->shouldReceive('verify')->once()->andThrow(
|
||||
new PlatformUnavailableException('X API returned 503 during token refresh', 503)
|
||||
);
|
||||
app()->instance(ConnectionVerifier::class, $verifier);
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@
|
|||
|
||||
declare(strict_types=1);
|
||||
|
||||
use App\Enums\SocialAccount\Status;
|
||||
use App\Exceptions\PlatformUnavailableException;
|
||||
use App\Exceptions\TokenExpiredException;
|
||||
use App\Models\SocialAccount;
|
||||
|
|
@ -414,6 +415,36 @@
|
|||
expect(fn () => $verifier->refreshToken($account))->toThrow(PlatformUnavailableException::class);
|
||||
});
|
||||
|
||||
test('does not disconnect when a concurrent refresh already rotated the token (lost-rotation race)', function () {
|
||||
Http::fake([
|
||||
// Our stale refresh_token is rejected — a concurrent process already used it.
|
||||
config('trypost.platforms.x.api').'/oauth2/token' => Http::response(['error' => 'invalid_grant'], 400),
|
||||
// But the access_token the winning refresh persisted still works.
|
||||
config('trypost.platforms.x.api').'/users/me' => Http::response(['data' => ['id' => '123']], 200),
|
||||
]);
|
||||
|
||||
$account = SocialAccount::factory()->x()->create([
|
||||
'status' => Status::Connected,
|
||||
'access_token' => 'stale-token',
|
||||
'refresh_token' => 'already-rotated',
|
||||
'token_expires_at' => now()->subHour(),
|
||||
]);
|
||||
|
||||
// Simulate the concurrent refresh: a separate instance persists a fresh,
|
||||
// valid token (through the encrypted cast) while our in-memory copy stays
|
||||
// the stale, expired one.
|
||||
SocialAccount::find($account->id)->update([
|
||||
'access_token' => 'fresh-token',
|
||||
'token_expires_at' => now()->addHours(2),
|
||||
]);
|
||||
|
||||
expect((new ConnectionVerifier)->verify($account))->toBeTrue();
|
||||
expect($account->fresh()->status)->toBe(Status::Connected);
|
||||
|
||||
Http::assertSent(fn ($request) => str_contains($request->url(), '/users/me')
|
||||
&& $request->header('Authorization')[0] === 'Bearer fresh-token');
|
||||
});
|
||||
|
||||
test('4xx during refresh keeps raising TokenExpiredException', function () {
|
||||
Http::fake([
|
||||
config('trypost.platforms.x.api').'/oauth2/token' => Http::response(['error' => 'invalid_grant'], 400),
|
||||
|
|
|
|||
Loading…
Reference in a new issue