fix(OpenId): Drop Guzzle client for Symfony Http client and silent op…

…en-id misconfiguration errors on Discovery
roadiz · May 27, 2024 · 4fe292f · 4fe292f
1 parent 399bd91
commit 4fe292f
Show file tree

Hide file tree

Showing 7 changed files with 78 additions and 62 deletions.
diff --git a/.env b/.env
@@ -21,10 +21,10 @@ VARNISH_DOMAIN=varnish
 HTTP_CACHE_MAX_AGE=60
 HTTP_CACHE_SHARED_MAX_AGE=600
 
-OPEN_ID_DISCOVERY_URL=https://accounts.google.com/.well-known/openid-configuration
-OPEN_ID_HOSTED_DOMAIN=
-OPEN_ID_CLIENT_ID=
-OPEN_ID_CLIENT_SECRET=
+#OPEN_ID_DISCOVERY_URL=https://accounts.google.com/.well-known/openid-configuration
+#OPEN_ID_HOSTED_DOMAIN=
+#OPEN_ID_CLIENT_ID=
+#OPEN_ID_CLIENT_SECRET=
 
 SOLR_HOST=solr
 SOLR_PORT=8983

diff --git a/config/packages/framework.yaml b/config/packages/framework.yaml
@@ -58,3 +58,9 @@ when@test:
         test: true
         session:
             storage_factory_id: session.storage.factory.mock_file
+
+when@prod:
+    framework:
+        php_errors:
+            # E_ALL & ~E_NOTICE & ~E_STRICT & ~E_DEPRECATED
+            log: 22519
diff --git a/config/packages/monolog.yaml b/config/packages/monolog.yaml
@@ -10,14 +10,6 @@ when@dev:
                 path: "%kernel.logs_dir%/%kernel.environment%.log"
                 level: debug
                 channels: ["!event"]
-            # uncomment to get logging in your browser
-            # you may have to allow bigger header sizes in your Web server configuration
-            #firephp:
-            #    type: firephp
-            #    level: info
-            #chromephp:
-            #    type: chromephp
-            #    level: info
             messenger:
                 type: stream
                 path: "%kernel.logs_dir%/messenger.%kernel.environment%.log"
@@ -26,7 +18,12 @@ when@dev:
             console:
                 type: console
                 process_psr_3_messages: false
-                channels: ["!event", "!doctrine", "!console"]
+                channels: ["!event", "!doctrine", "!console", "!deprecation"]
+            custom:
+                type: service
+                id: RZ\Roadiz\CoreBundle\Logger\DoctrineHandler
+                level: info
+                channels: [ "app" ]
 
 when@test:
     monolog:
@@ -51,12 +48,14 @@ when@prod:
                 handler: nested
                 excluded_http_codes: [404, 405]
                 buffer_size: 50 # How many messages should be saved? Prevent memory leaks
+                channels: ["!event", "!doctrine", "!deprecation"]
             nested:
                 type: stream
                 path: php://stderr
                 level: info
                 formatter: monolog.formatter.json
+                channels: ["!event", "!doctrine", "!deprecation"]
             console:
                 type: console
                 process_psr_3_messages: false
-                channels: ["!event", "!doctrine"]
+                channels: ["!event", "!doctrine", "!deprecation"]
diff --git a/lib/OpenId/composer.json b/lib/OpenId/composer.json
@@ -19,11 +19,11 @@
         "roadiz/jwt": "2.3.*",
         "roadiz/random": "2.3.*",
         "roadiz/models": "2.3.*",
-        "guzzlehttp/guzzle": "^7.2.0",
         "symfony/http-foundation": "6.4.*",
         "symfony/security-core": "6.4.*",
         "symfony/security-http": "6.4.*",
         "symfony/security-csrf": "6.4.*",
+        "symfony/http-client": "6.4.*",
         "psr/cache": ">=1.0.1",
         "symfony/event-dispatcher-contracts": "^2.4.0",
         "codercat/jwk-to-pem": "^1.0",

diff --git a/lib/OpenId/src/Authentication/OpenIdAuthenticator.php b/lib/OpenId/src/Authentication/OpenIdAuthenticator.php
@@ -4,10 +4,6 @@
 
 namespace RZ\Roadiz\OpenId\Authentication;
 
-use GuzzleHttp\Client;
-use GuzzleHttp\Exception\GuzzleException;
-use GuzzleHttp\Exception\RequestException;
-use GuzzleHttp\Psr7\Query;
 use Lcobucci\JWT\Token\Plain;
 use Lcobucci\JWT\Validation\RequiredConstraintsViolated;
 use RZ\Roadiz\OpenId\Authentication\Provider\JwtRoleStrategy;
@@ -31,19 +27,23 @@
 use Symfony\Component\Security\Http\HttpUtils;
 use Symfony\Component\Security\Http\SecurityRequestAttributes;
 use Symfony\Component\Security\Http\Util\TargetPathTrait;
+use Symfony\Contracts\HttpClient\Exception\ExceptionInterface;
+use Symfony\Contracts\HttpClient\Exception\HttpExceptionInterface;
+use Symfony\Contracts\HttpClient\HttpClientInterface;
 
 final class OpenIdAuthenticator extends AbstractAuthenticator
 {
     use TargetPathTrait;
 
-    private Client $client;
+    private HttpClientInterface $client;
 
     public function __construct(
         private readonly HttpUtils $httpUtils,
         private readonly ?Discovery $discovery,
         private readonly JwtRoleStrategy $roleStrategy,
         private readonly OpenIdJwtConfigurationFactory $jwtConfigurationFactory,
         private readonly UrlGeneratorInterface $urlGenerator,
+        HttpClientInterface $client,
         private readonly string $returnPath,
         private readonly string $defaultRoute,
         private readonly ?string $oauthClientId,
@@ -54,7 +54,7 @@ public function __construct(
         private readonly string $targetPathParameter = '_target_path',
         private readonly array $defaultRoles = []
     ) {
-        $this->client = new Client([
+        $this->client = $client->withOptions([
             // You can set any number of default request options.
             'timeout'  => 2.0,
         ]);
@@ -96,7 +96,8 @@ public function authenticate(Request $request): Passport
         if (null === $request->query->get('state')) {
             throw new OpenIdAuthenticationException('State is not valid');
         }
-        $state = Query::parse((string) $request->query->get('state'));
+
+        \parse_str((string) $request->query->get('state'), $state);
 
         /*
          * Fetch _target_path parameter from OAuth2 state
@@ -121,8 +122,8 @@ public function authenticate(Request $request): Passport
             if (!\is_string($tokenEndpoint) || empty($tokenEndpoint)) {
                 throw new OpenIdConfigurationException('Discovery does not provide a valid token_endpoint.');
             }
-            $response = $this->client->post($tokenEndpoint, [
-                'form_params' => [
+            $response = $this->client->request('POST', $tokenEndpoint, [
+                'body' => [
                     'code' => $request->query->get('code'),
                     'client_id' => $this->oauthClientId ?? '',
                     'client_secret' => $this->oauthClientSecret ?? '',
@@ -131,27 +132,19 @@ public function authenticate(Request $request): Passport
                 ]
             ]);
             /** @var array $jsonResponse */
-            $jsonResponse = \json_decode($response->getBody()->getContents(), true);
-        } catch (RequestException $e) {
-            if (null !== $e->getResponse()) {
-                /** @var array $jsonResponse */
-                $jsonResponse = \json_decode($e->getResponse()->getBody()->getContents(), true);
-                $errorTitle = $jsonResponse['error'] ?? $e->getMessage();
-                $errorDescription = $jsonResponse['error_description'] ?? '';
-
-                throw new OpenIdAuthenticationException(
-                    $errorTitle . ': ' . $errorDescription,
-                    $e->getCode(),
-                    $e
-                );
-            }
+            $jsonResponse = \json_decode(json: $response->getContent(), associative: true, flags: JSON_THROW_ON_ERROR);
+        } catch (HttpExceptionInterface $e) {
+            /** @var array $jsonResponse */
+            $jsonResponse = \json_decode(json: $e->getResponse()->getContent(false), associative: true, flags: JSON_THROW_ON_ERROR);
+            $errorTitle = $jsonResponse['error'] ?? $e->getMessage();
+            $errorDescription = $jsonResponse['error_description'] ?? '';
 
             throw new OpenIdAuthenticationException(
-                $e->getMessage(),
+                $errorTitle . ': ' . $errorDescription,
                 $e->getCode(),
                 $e
             );
-        } catch (GuzzleException $e) {
+        } catch (ExceptionInterface $e) {
             throw new OpenIdAuthenticationException(
                 $e->getMessage(),
                 $e->getCode(),

diff --git a/lib/OpenId/src/Discovery.php b/lib/OpenId/src/Discovery.php
@@ -7,11 +7,16 @@
 use CoderCat\JWKToPEM\Exception\Base64DecodeException;
 use CoderCat\JWKToPEM\Exception\JWKConverterException;
 use CoderCat\JWKToPEM\JWKConverter;
-use GuzzleHttp\Client;
-use GuzzleHttp\Exception\GuzzleException;
-use GuzzleHttp\Exception\RequestException;
 use Psr\Cache\CacheItemPoolInterface;
+use Psr\Cache\InvalidArgumentException;
+use Psr\Log\LoggerInterface;
 use RZ\Roadiz\Bag\LazyParameterBag;
+use Symfony\Contracts\HttpClient\Exception\ClientExceptionInterface;
+use Symfony\Contracts\HttpClient\Exception\ExceptionInterface;
+use Symfony\Contracts\HttpClient\Exception\RedirectionExceptionInterface;
+use Symfony\Contracts\HttpClient\Exception\ServerExceptionInterface;
+use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface;
+use Symfony\Contracts\HttpClient\HttpClientInterface;
 
 /**
  * @package RZ\Roadiz\OpenId
@@ -22,13 +27,11 @@ class Discovery extends LazyParameterBag
     public const CACHE_KEY = 'rz_openid_discovery_parameters';
     protected ?array $jwksData = null;
 
-    /**
-     * @param string  $discoveryUri
-     * @param CacheItemPoolInterface $cacheAdapter
-     */
     public function __construct(
         protected readonly string $discoveryUri,
-        protected readonly CacheItemPoolInterface $cacheAdapter
+        protected readonly CacheItemPoolInterface $cacheAdapter,
+        protected readonly HttpClientInterface $client,
+        protected readonly LoggerInterface $logger,
     ) {
         parent::__construct();
     }
@@ -46,16 +49,19 @@ protected function populateParameters(): void
             $parameters = $cacheItem->get();
         } else {
             try {
-                $client = new Client([
-                    // You can set any number of default request options.
-                    'timeout'  => 2.0,
+                $client = $this->client->withOptions([
+                    'timeout' => 2.0,
                 ]);
-                $response = $client->get($this->discoveryUri);
+                $response = $client->request('GET', $this->discoveryUri);
                 /** @var array $parameters */
-                $parameters = \json_decode($response->getBody()->getContents(), true);
+                $parameters = \json_decode(json: $response->getContent(), associative: true, flags: JSON_THROW_ON_ERROR);
                 $cacheItem->set($parameters);
                 $this->cacheAdapter->save($cacheItem);
-            } catch (RequestException $exception) {
+            } catch (ExceptionInterface $exception) {
+                $this->logger->warning('Cannot fetch OpenID discovery parameters: ' . $exception->getMessage());
+                return;
+            } catch (\JsonException $exception) {
+                $this->logger->warning('Cannot fetch OpenID discovery parameters: ' . $exception->getMessage());
                 return;
             }
         }
@@ -78,8 +84,13 @@ public function canVerifySignature(): bool
     /**
      * @return array<string>|null
      * @throws Base64DecodeException
+     * @throws ClientExceptionInterface
+     * @throws InvalidArgumentException
      * @throws JWKConverterException
-     * @throws GuzzleException
+     * @throws RedirectionExceptionInterface
+     * @throws ServerExceptionInterface
+     * @throws TransportExceptionInterface
+     * @throws \JsonException
      * @see https://auth0.com/docs/tokens/json-web-tokens/json-web-key-sets
      */
     public function getPems(): ?array
@@ -94,7 +105,12 @@ public function getPems(): ?array
 
     /**
      * @return array|null
-     * @throws GuzzleException
+     * @throws ClientExceptionInterface
+     * @throws InvalidArgumentException
+     * @throws RedirectionExceptionInterface
+     * @throws ServerExceptionInterface
+     * @throws TransportExceptionInterface
+     * @throws \JsonException
      */
     protected function getJwksData(): ?array
     {
@@ -112,12 +128,11 @@ protected function getJwksData(): ?array
                     $this->jwksData = null;
                 }
             } else {
-                $client = new Client([
-                    // You can set any number of default request options.
-                    'timeout'  => 3.0,
+                $client = $this->client->withOptions([
+                    'timeout' => 2.0,
                 ]);
-                $response = $client->get($jwksUri);
-                $data = \json_decode($response->getBody()->getContents(), true);
+                $response = $client->request('GET', $jwksUri);
+                $data = \json_decode(json: $response->getContent(), associative: true, flags: JSON_THROW_ON_ERROR);
                 if (is_array($data)) {
                     $this->jwksData = $data;
                 } else {

diff --git a/lib/RoadizRozierBundle/src/DependencyInjection/RoadizRozierExtension.php b/lib/RoadizRozierBundle/src/DependencyInjection/RoadizRozierExtension.php
@@ -66,7 +66,9 @@ private function registerOpenId(array $config, ContainerBuilder $container): voi
                     ->setPublic(true)
                     ->setArguments([
                         $config['open_id']['discovery_url'],
-                        new Reference(\Psr\Cache\CacheItemPoolInterface::class)
+                        new Reference(\Psr\Cache\CacheItemPoolInterface::class),
+                        new Reference(\Symfony\Contracts\HttpClient\HttpClientInterface::class),
+                        new Reference(\Psr\Log\LoggerInterface::class)
                     ])
             );
         }
@@ -98,6 +100,7 @@ private function registerOpenId(array $config, ContainerBuilder $container): voi
                     new Reference(\RZ\Roadiz\OpenId\Authentication\Provider\ChainJwtRoleStrategy::class),
                     new Reference('roadiz_rozier.open_id.jwt_configuration_factory'),
                     new Reference(\Symfony\Component\Routing\Generator\UrlGeneratorInterface::class),
+                    new Reference(\Symfony\Contracts\HttpClient\HttpClientInterface::class),
                     'loginPage',
                     'adminHomePage',
                     $config['open_id']['oauth_client_id'],