From 4fe292f66932d98b22057df6b341afa4e2735fab Mon Sep 17 00:00:00 2001 From: Ambroise Maupate Date: Mon, 27 May 2024 16:52:11 +0200 Subject: [PATCH] fix(OpenId): Drop Guzzle client for Symfony Http client and silent open-id misconfiguration errors on Discovery --- .env | 8 +-- config/packages/framework.yaml | 6 ++ config/packages/monolog.yaml | 19 +++---- lib/OpenId/composer.json | 2 +- .../Authentication/OpenIdAuthenticator.php | 43 ++++++-------- lib/OpenId/src/Discovery.php | 57 ++++++++++++------- .../RoadizRozierExtension.php | 5 +- 7 files changed, 78 insertions(+), 62 deletions(-) diff --git a/.env b/.env index 2f533be8..32ed0b4e 100644 --- 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 index d3cb6c34..776ead45 100644 --- 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 index e060bdf9..bd795ce7 100644 --- 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 index 1a2d1570..9f991cd0 100644 --- 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 index 42c7095e..122f2b7c 100644 --- 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,12 +27,15 @@ 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, @@ -44,6 +43,7 @@ public function __construct( 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 index 8e0ac68f..961420f2 100644 --- 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|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 index 1ed1bcc6..ca22e1c2 100644 --- 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'],