Skip to content

Commit 561d8b4

Browse files
Ali-HENDAAli-HENDA
committed
[Security] Fix OIDC discovery when using multiple HttpClient instances
1 parent a518bc2 commit 561d8b4

File tree

2 files changed

+95
-17
lines changed

2 files changed

+95
-17
lines changed

‎src/Symfony/Component/Security/Http/AccessToken/Oidc/OidcTokenHandler.php‎

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -149,25 +149,23 @@ public function getUserBadgeFrom(string $accessToken): UserBadge
149149
public function computeDiscoveryKeys(ItemInterface $item): array
150150
{
151151
$clients = $this->discoveryClients;
152+
if (!$clients) {
153+
throw new \LogicException('No OIDC discovery client configured.');
154+
}
152155
$logger = $this->logger;
153-
154156
try {
155-
$configResponses = [];
157+
$keys = [];
158+
$minTtl = null;
159+
156160
foreach ($clients as $client) {
157-
$configResponses[] = $client->request('GET', '.well-known/openid-configuration', [
158-
'user_data' => $client,
159-
]);
160-
}
161+
$config = $client->request('GET', '.well-known/openid-configuration')->toArray();
161162

162-
$jwkSetResponses = [];
163-
foreach ($client->stream($configResponses) as $response => $chunk) {
164-
if ($chunk->isLast()) {
165-
$jwkSetResponses[] = $response->getInfo('user_data')->request('GET', $response->toArray()['jwks_uri']);
163+
$jwksUri = $config['jwks_uri'] ?? null;
164+
if (!\is_string($jwksUri) || '' === $jwksUri) {
165+
throw new \RuntimeException('The "jwks_uri" is missing from the OIDC discovery document.');
166166
}
167-
}
168-
$keys = [];
169-
$minTtl = null;
170-
foreach ($jwkSetResponses as $response) {
167+
168+
$response = $client->request('GET', $jwksUri);
171169
$headers = $response->getHeaders();
172170
if (preg_match('/max-age=(\d+)/', $headers['cache-control'][0] ?? '', $m)) {
173171
$currentTtl = (int) $m[1];
@@ -181,7 +179,7 @@ public function computeDiscoveryKeys(ItemInterface $item): array
181179
}
182180

183181
foreach ($response->toArray()['keys'] as $key) {
184-
if ('sig' === $key['use']) {
182+
if ('sig' === ($key['use'] ?? null)) {
185183
$keys[] = $key;
186184
}
187185
}
@@ -198,6 +196,7 @@ public function computeDiscoveryKeys(ItemInterface $item): array
198196
'error' => $e->getMessage(),
199197
'trace' => $e->getTraceAsString(),
200198
]);
199+
201200
throw new BadCredentialsException('Invalid credentials.', $e->getCode(), $e);
202201
}
203202
}

‎src/Symfony/Component/Security/Http/Tests/AccessToken/Oidc/OidcTokenHandlerTest.php‎

Lines changed: 81 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
use Symfony\Component\Security\Core\User\OidcUser;
2929
use Symfony\Component\Security\Http\AccessToken\Oidc\OidcTokenHandler;
3030
use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
31+
use Symfony\Contracts\Cache\ItemInterface;
3132

3233
#[RequiresPhpExtension('openssl')]
3334
class OidcTokenHandlerTest extends TestCase
@@ -316,7 +317,8 @@ public function testDiscoveryCachesJwksAccordingToCacheControl()
316317
{
317318
$time = time();
318319
$claims = [
319-
'iat' => $time, 'nbf' => $time,
320+
'iat' => $time,
321+
'nbf' => $time,
320322
'exp' => $time + 3600,
321323
'iss' => 'https://www.example.com',
322324
'aud' => self::AUDIENCE,
@@ -355,7 +357,8 @@ public function testDiscoveryCachesJwksAccordingToExpires()
355357
{
356358
$time = time();
357359
$claims = [
358-
'iat' => $time, 'nbf' => $time,
360+
'iat' => $time,
361+
'nbf' => $time,
359362
'exp' => $time + 3600,
360363
'iss' => 'https://www.example.com',
361364
'aud' => self::AUDIENCE,
@@ -390,4 +393,80 @@ public function testDiscoveryCachesJwksAccordingToExpires()
390393
$this->assertSame('user-expires', $handler->getUserBadgeFrom($token)->getUserIdentifier());
391394
$this->assertSame(2, $requestCount);
392395
}
396+
397+
public function testComputeDiscoveryKeysReturnsEmptyWhenNoClients()
398+
{
399+
$cache = new ArrayAdapter();
400+
$handler = new OidcTokenHandler(
401+
new AlgorithmManager([new ES256()]),
402+
null,
403+
self::AUDIENCE,
404+
['https://www.example.com']
405+
);
406+
407+
$handler->enableDiscovery($cache, [], 'oidc_empty_clients');
408+
409+
$item = $this->createMock(ItemInterface::class);
410+
$item->expects($this->never())->method('expiresAfter');
411+
412+
$this->expectException(\LogicException::class);
413+
$this->expectExceptionMessage('No OIDC discovery client configured.');
414+
$handler->computeDiscoveryKeys($item);
415+
}
416+
417+
public function testDiscoveryThrowsWhenJwksUriIsMissing()
418+
{
419+
$time = time();
420+
$claims = [
421+
'iat' => $time,
422+
'nbf' => $time,
423+
'exp' => $time + 3600,
424+
'iss' => 'https://www.example.com',
425+
'aud' => self::AUDIENCE,
426+
'sub' => 'user-missing-jwks-uri',
427+
];
428+
$token = self::buildJWS(json_encode($claims));
429+
430+
$httpClient = new MockHttpClient([
431+
new JsonMockResponse(['issuer' => 'https://www.example.com']),
432+
]);
433+
434+
$cache = new ArrayAdapter();
435+
$handler = new OidcTokenHandler(
436+
new AlgorithmManager([new ES256()]),
437+
null,
438+
self::AUDIENCE,
439+
['https://www.example.com']
440+
);
441+
$handler->enableDiscovery($cache, $httpClient, 'oidc_missing_jwks_uri');
442+
443+
$this->expectException(BadCredentialsException::class);
444+
$handler->getUserBadgeFrom($token);
445+
}
446+
447+
public function testDiscoveryIgnoresNonSignatureKeys()
448+
{
449+
$httpClient = new MockHttpClient([
450+
new JsonMockResponse(['jwks_uri' => 'https://www.example.com/jwks.json']),
451+
new JsonMockResponse([
452+
'keys' => [
453+
array_merge(self::getJWK()->all(), ['use' => 'enc']),
454+
array_merge(self::getSecondJWK()->all(), []),
455+
],
456+
]),
457+
]);
458+
459+
$cache = new ArrayAdapter();
460+
$handler = new OidcTokenHandler(
461+
new AlgorithmManager([new ES256()]),
462+
null,
463+
self::AUDIENCE,
464+
['https://www.example.com']
465+
);
466+
$handler->enableDiscovery($cache, $httpClient, 'oidc_non_sig_keys');
467+
468+
$item = $this->createMock(ItemInterface::class);
469+
$item->expects($this->never())->method('expiresAfter');
470+
$this->assertSame([], $handler->computeDiscoveryKeys($item));
471+
}
393472
}

0 commit comments

Comments
 (0)