bug #50470 [SecurityBundle] Fix configuring OIDC user info token hand…

…ler client (vincentchalamon) This PR was squashed before being merged into the 6.3 branch. Discussion ---------- [SecurityBundle] Fix configuring OIDC user info token handler client | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | N/A | License | MIT | Doc PR | symfony/symfony-docs#18354 Cf. #50453 (comment) Commits ------- 23c9e17 [SecurityBundle] Fix configuring OIDC user info token handler client
symfony · May 30, 2023 · 5c46e7e · 5c46e7e
2 parents 5bab536 + 23c9e17
commit 5c46e7e
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 32 deletions.
diff --git a/...curityBundle/DependencyInjection/Security/AccessToken/OidcUserInfoTokenHandlerFactory.php b/...curityBundle/DependencyInjection/Security/AccessToken/OidcUserInfoTokenHandlerFactory.php
@@ -15,7 +15,7 @@
 use Symfony\Component\DependencyInjection\ChildDefinition;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Reference;
-use Symfony\Component\HttpClient\HttpClient;
+use Symfony\Contracts\HttpClient\HttpClientInterface;
 
 /**
  * Configures a token handler for an OIDC server.
@@ -26,24 +26,20 @@ class OidcUserInfoTokenHandlerFactory implements TokenHandlerFactoryInterface
 {
     public function create(ContainerBuilder $container, string $id, array|string $config): void
     {
-        $tokenHandlerDefinition = $container->setDefinition($id, new ChildDefinition('security.access_token_handler.oidc_user_info'));
-        $tokenHandlerDefinition->replaceArgument(2, $config['claim']);
+        $clientDefinition = (new ChildDefinition('security.access_token_handler.oidc_user_info.http_client'))
+            ->replaceArgument(0, ['base_uri' => $config['base_uri']]);
 
-        // Create the client service
-        if (!isset($config['client']['id'])) {
-            $clientDefinitionId = 'http_client.security.access_token_handler.oidc_user_info';
-            if (!ContainerBuilder::willBeAvailable('symfony/http-client', HttpClient::class, ['symfony/security-bundle'])) {
-                $container->register($clientDefinitionId, 'stdClass')
-                    ->addError('You cannot use the "oidc_user_info" token handler since the HttpClient component is not installed. Try running "composer require symfony/http-client".');
-            } else {
-                $container->register($clientDefinitionId, HttpClient::class)
-                    ->setFactory([HttpClient::class, 'create'])
-                    ->setArguments([$config['client']])
-                    ->addTag('http_client.client');
-            }
+        if (isset($config['client'])) {
+            $clientDefinition->setFactory([new Reference($config['client']), 'withOptions']);
+        } elseif (!ContainerBuilder::willBeAvailable('symfony/http-client', HttpClientInterface::class, ['symfony/security-bundle'])) {
+            $clientDefinition
+                ->setFactory(null)
+                ->addError('You cannot use the "oidc_user_info" token handler since the HttpClient component is not installed. Try running "composer require symfony/http-client".');
         }
 
-        $tokenHandlerDefinition->replaceArgument(0, new Reference($config['client']['id'] ?? $clientDefinitionId));
+        $container->setDefinition($id, new ChildDefinition('security.access_token_handler.oidc_user_info'))
+            ->replaceArgument(0, $clientDefinition)
+            ->replaceArgument(2, $config['claim']);
     }
 
     public function getKey(): string
@@ -56,19 +52,23 @@ public function addConfiguration(NodeBuilder $node): void
         $node
             ->arrayNode($this->getKey())
                 ->fixXmlConfig($this->getKey())
+                ->beforeNormalization()
+                    ->ifString()
+                    ->then(static fn ($v) => ['claim' => 'sub', 'base_uri' => $v])
+                ->end()
                 ->children()
+                    ->scalarNode('base_uri')
+                        ->info('Base URI of the userinfo endpoint on the OIDC server.')
+                        ->isRequired()
+                        ->cannotBeEmpty()
+                    ->end()
                     ->scalarNode('claim')
-                        ->info('Claim which contains the user identifier (e.g.: sub, email..).')
+                        ->info('Claim which contains the user identifier (e.g. sub, email, etc.).')
                         ->defaultValue('sub')
+                        ->cannotBeEmpty()
                     ->end()
-                    ->arrayNode('client')
-                        ->info('HttpClient to call the OIDC server.')
-                        ->isRequired()
-                        ->beforeNormalization()
-                            ->ifString()
-                            ->then(static function ($v): array { return ['id' => $v]; })
-                        ->end()
-                        ->prototype('scalar')->end()
+                    ->scalarNode('client')
+                        ->info('HttpClient service id to use to call the OIDC server.')
                     ->end()
                 ->end()
             ->end()

diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator_access_token.php b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator_access_token.php
@@ -18,6 +18,7 @@
 use Symfony\Component\Security\Http\AccessToken\Oidc\OidcUserInfoTokenHandler;
 use Symfony\Component\Security\Http\AccessToken\QueryAccessTokenExtractor;
 use Symfony\Component\Security\Http\Authenticator\AccessTokenAuthenticator;
+use Symfony\Contracts\HttpClient\HttpClientInterface;
 
 return static function (ContainerConfigurator $container) {
     $container->services()
@@ -44,12 +45,17 @@
             ])
 
         // OIDC
+        ->set('security.access_token_handler.oidc_user_info.http_client', HttpClientInterface::class)
+            ->abstract()
+            ->factory([service('http_client'), 'withOptions'])
+            ->args([abstract_arg('http client options')])
+
         ->set('security.access_token_handler.oidc_user_info', OidcUserInfoTokenHandler::class)
             ->abstract()
             ->args([
                 abstract_arg('http client'),
                 service('logger')->nullOnInvalid(),
-                'sub',
+                abstract_arg('claim'),
             ])
 
         ->set('security.access_token_handler.oidc', OidcTokenHandler::class)

diff --git a/...ndle/SecurityBundle/Tests/DependencyInjection/Security/Factory/AccessTokenFactoryTest.php b/...ndle/SecurityBundle/Tests/DependencyInjection/Security/Factory/AccessTokenFactoryTest.php
@@ -18,7 +18,9 @@
 use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\AccessTokenFactory;
 use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition;
 use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
+use Symfony\Component\DependencyInjection\ChildDefinition;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Reference;
 
 class AccessTokenFactoryTest extends TestCase
 {
@@ -76,7 +78,12 @@ public function testOidcUserInfoTokenHandlerConfigurationWithExistingClient()
     {
         $container = new ContainerBuilder();
         $config = [
-            'token_handler' => ['oidc_user_info' => ['client' => 'oidc.client']],
+            'token_handler' => [
+                'oidc_user_info' => [
+                    'base_uri' => 'https://www.example.com/realms/demo/protocol/openid-connect/userinfo',
+                    'client' => 'oidc.client',
+                ],
+            ],
         ];
 
         $factory = new AccessTokenFactory($this->createTokenHandlerFactories());
@@ -86,14 +93,24 @@ public function testOidcUserInfoTokenHandlerConfigurationWithExistingClient()
 
         $this->assertTrue($container->hasDefinition('security.authenticator.access_token.firewall1'));
         $this->assertTrue($container->hasDefinition('security.access_token_handler.firewall1'));
-        $this->assertFalse($container->hasDefinition('http_client.security.access_token_handler.oidc_user_info'));
+
+        $expected = [
+            'index_0' => (new ChildDefinition('security.access_token_handler.oidc_user_info.http_client'))
+                ->setFactory([new Reference('oidc.client'), 'withOptions'])
+                ->replaceArgument(0, ['base_uri' => 'https://www.example.com/realms/demo/protocol/openid-connect/userinfo']),
+            'index_2' => 'sub',
+        ];
+        $this->assertEquals($expected, $container->getDefinition('security.access_token_handler.firewall1')->getArguments());
     }
 
-    public function testOidcUserInfoTokenHandlerConfigurationWithClientCreation()
+    /**
+     * @dataProvider getOidcUserInfoConfiguration
+     */
+    public function testOidcUserInfoTokenHandlerConfigurationWithBaseUri(array|string $configuration)
     {
         $container = new ContainerBuilder();
         $config = [
-            'token_handler' => ['oidc_user_info' => ['client' => ['base_uri' => 'https://www.example.com/realms/demo/protocol/openid-connect/userinfo']]],
+            'token_handler' => ['oidc_user_info' => $configuration],
         ];
 
         $factory = new AccessTokenFactory($this->createTokenHandlerFactories());
@@ -103,7 +120,19 @@ public function testOidcUserInfoTokenHandlerConfigurationWithClientCreation()
 
         $this->assertTrue($container->hasDefinition('security.authenticator.access_token.firewall1'));
         $this->assertTrue($container->hasDefinition('security.access_token_handler.firewall1'));
-        $this->assertTrue($container->hasDefinition('http_client.security.access_token_handler.oidc_user_info'));
+
+        $expected = [
+            'index_0' => (new ChildDefinition('security.access_token_handler.oidc_user_info.http_client'))
+                ->replaceArgument(0, ['base_uri' => 'https://www.example.com/realms/demo/protocol/openid-connect/userinfo']),
+            'index_2' => 'sub',
+        ];
+        $this->assertEquals($expected, $container->getDefinition('security.access_token_handler.firewall1')->getArguments());
+    }
+
+    public static function getOidcUserInfoConfiguration(): iterable
+    {
+        yield [['base_uri' => 'https://www.example.com/realms/demo/protocol/openid-connect/userinfo']];
+        yield ['https://www.example.com/realms/demo/protocol/openid-connect/userinfo'];
     }
 
     public function testMultipleTokenHandlersSet()
@@ -114,7 +143,7 @@ public function testMultipleTokenHandlersSet()
         $config = [
             'token_handler' => [
                 'id' => 'in_memory_token_handler_service_id',
-                'oidc_user_info' => ['client' => 'oidc.client'],
+                'oidc_user_info' => 'https://www.example.com/realms/demo/protocol/openid-connect/userinfo',
             ],
         ];
 

diff --git a/src/Symfony/Bundle/SecurityBundle/composer.json b/src/Symfony/Bundle/SecurityBundle/composer.json
@@ -60,6 +60,7 @@
         "symfony/browser-kit": "<5.4",
         "symfony/console": "<5.4",
         "symfony/framework-bundle": "<5.4",
+        "symfony/http-client": "<5.4",
         "symfony/ldap": "<5.4",
         "symfony/twig-bundle": "<5.4"
     },