diff --git a/src/Client/Auth/OAuthAuthenticator.php b/src/Client/Auth/OAuthAuthenticator.php index ac44f7ae..74abf7b7 100644 --- a/src/Client/Auth/OAuthAuthenticator.php +++ b/src/Client/Auth/OAuthAuthenticator.php @@ -139,7 +139,7 @@ public function handleChallenge(RequestInterface $request, ResponseInterface $re } $this->logger->info('The authorization server no longer knows this client; registering it again.', ['issuer' => $metadata->issuer]); - $this->storage->forget($metadata->issuer); + $this->forgetRefusedClient($metadata->issuer, $client); $token = null; $client = $this->resolveClient($metadata, $scopes, $scopesSupported); } @@ -150,7 +150,7 @@ public function handleChallenge(RequestInterface $request, ResponseInterface $re } catch (AuthorizationException $e) { // The user already went through the browser once, so only the next attempt registers again. if (self::isForgottenClient($e, $client)) { - $this->storage->forget($metadata->issuer); + $this->forgetRefusedClient($metadata->issuer, $client); } throw $e; @@ -206,6 +206,16 @@ private function discover(string $endpoint, AuthorizationChallenge $challenge): return [$metadata->resource, $server, $metadata->scopesSupported]; } + private function forgetRefusedClient(string $issuer, ClientRegistration $client): void + { + // Under the registration lock, so a client that another worker registered after this one is kept. + $this->storage->synchronize(function () use ($issuer, $client): void { + if ($client->clientId === $this->storage->getClientRegistration($issuer)?->clientId) { + $this->storage->forget($issuer); + } + }); + } + private static function isForgottenClient(AuthorizationException $e, ClientRegistration $client): bool { return $client->dynamic && self::isRefusedClient($e); diff --git a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php index 6ba3d41a..8935e236 100644 --- a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php +++ b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php @@ -827,6 +827,38 @@ public function testDropsAnUnknownClientAfterTheCodeExchange(): void $this->assertSame('registered-client', $server->token['client_id'] ?? null); } + #[TestDox('an unknown client on a refresh keeps the client another worker registered meanwhile')] + public function testRefreshKeepsAClientRegisteredByAnotherWorker(): void + { + $server = new FakeOAuthServer(refreshTokens: true); + $server->unknownClient = 'forgotten-client'; + $storage = $this->storageWithAnotherWorkerRegistering(); + $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('expired', 'Bearer', time() - 60, 'a-refresh-token')); + + $this->authenticator($server, storage: $storage)->handleChallenge($this->request(), $server->challenge()); + + $this->assertSame([], $server->registration); + $this->assertSame('other-worker-client', $server->token['client_id'] ?? null); + $this->assertSame('other-worker-client', $storage->getClientRegistration('https://auth.example.com')?->clientId); + } + + #[TestDox('an unknown client on the code exchange keeps the client another worker registered meanwhile')] + public function testCodeExchangeKeepsAClientRegisteredByAnotherWorker(): void + { + $server = new FakeOAuthServer(); + $server->unknownClient = 'forgotten-client'; + $storage = $this->storageWithAnotherWorkerRegistering(); + + try { + $this->authenticator($server, storage: $storage)->handleChallenge($this->request(), $server->challenge()); + $this->fail('The unknown client should have failed.'); + } catch (AuthorizationException $e) { + $this->assertStringContainsString('invalid_client', $e->getMessage()); + } + + $this->assertSame('other-worker-client', $storage->getClientRegistration('https://auth.example.com')?->clientId); + } + #[TestDox('a client that was not registered dynamically is not replaced when it is unknown')] public function testDoesNotReplaceAConfiguredClient(): void { @@ -1762,6 +1794,61 @@ public function testRefusesAServerWithoutPkce(?array $methods): void $this->assertNotContains('https://auth.example.com/register', $server->requested); } + /** + * Holds a registration that another worker replaces once the authenticator has picked it up. + */ + private function storageWithAnotherWorkerRegistering(): CredentialStorageInterface + { + return new class implements CredentialStorageInterface { + private readonly InMemoryCredentialStorage $storage; + + private bool $replaced = false; + + public function __construct() + { + $this->storage = new InMemoryCredentialStorage(); + $this->storage->saveClientRegistration('https://auth.example.com', new ClientRegistration('forgotten-client', dynamic: true)); + } + + public function getClientRegistration(string $issuer): ?ClientRegistration + { + return $this->storage->getClientRegistration($issuer); + } + + public function saveClientRegistration(string $issuer, ClientRegistration $registration): void + { + $this->storage->saveClientRegistration($issuer, $registration); + } + + public function getToken(string $issuer, string $resource): ?AccessToken + { + return $this->storage->getToken($issuer, $resource); + } + + public function saveToken(string $issuer, string $resource, AccessToken $token): void + { + $this->storage->saveToken($issuer, $resource, $token); + } + + public function forget(?string $issuer = null): void + { + $this->storage->forget($issuer); + } + + public function synchronize(callable $operation): mixed + { + $result = $operation(); + + if (!$this->replaced) { + $this->replaced = true; + $this->storage->saveClientRegistration('https://auth.example.com', new ClientRegistration('other-worker-client', null, TokenEndpointAuthMethod::None, true, null, ['http://127.0.0.1:8765/callback'])); + } + + return $result; + } + }; + } + private function request(): RequestInterface { return (new Psr17Factory())->createRequest('POST', self::ENDPOINT);