diff --git a/.env b/.env index 106b135e..16868827 100644 --- a/.env +++ b/.env @@ -146,6 +146,11 @@ RELATIONS_CHECKSUM_ENABLED=true ###> itk-dev/openid-connect-bundle ### # See docs/feed/openid-connect.md for variable explanations. +# Total HTTP request timeout (seconds) for calls to the OIDC providers +# (discovery, JWKS, token exchange). Bounds a hung/slow IdP so it cannot +# tie up a php-fpm worker indefinitely. Applies to both providers. +OIDC_HTTP_TIMEOUT=5 + # internal provider (admin login) # URL to OIDC provider's metadata/discovery endpoint. INTERNAL_OIDC_METADATA_URL=INTERNAL_OIDC_METADATA_URL diff --git a/CHANGELOG.md b/CHANGELOG.md index 4faff68f..90d4ff32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,15 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +- Upgraded `itk-dev/openid-connect-bundle` to 5.0 (and `itk-dev/openid-connect` to 5.0). Migrated the + OIDC exception catches in `AuthOidcController` and `AzureOidcAuthenticator` to the new + `OpenIdConnectExceptionInterface` marker, since concrete exceptions no longer extend the deprecated + `ItkOpenIdConnectException`. Added regression tests covering the exception-mapping branches and the + authenticator's claim-to-tenant-role provisioning and de-provisioning logic. +- Bounded OIDC provider HTTP calls (discovery, JWKS, token exchange) with an explicit + `http_client_options.timeout` on both providers, configurable via the new `OIDC_HTTP_TIMEOUT` env var + (default 5s). Previously no timeout was set anywhere in the chain, so Guzzle waited indefinitely and a + hung/slow IdP could tie up a php-fpm worker. - Removed the deprecated feed types `SparkleIOFeedType`, `EventDatabaseApiFeedType` and `KobaFeedType`. Made the unknown-feed-type handling consistent: **reads degrade, writes are rejected.** Feed sources (and feeds) that reference a removed type keep loading — item and collection reads return them with no diff --git a/composer.json b/composer.json index e8f7427f..cc666430 100644 --- a/composer.json +++ b/composer.json @@ -21,7 +21,7 @@ "doctrine/doctrine-migrations-bundle": "^3.1", "doctrine/orm": "^2.9", "gesdinet/jwt-refresh-token-bundle": "^1.0", - "itk-dev/openid-connect-bundle": "^4.1", + "itk-dev/openid-connect-bundle": "^5.0", "justinrainbow/json-schema": "^5.2", "kubawerlos/php-cs-fixer-custom-fixers": "^3.11", "lexik/jwt-authentication-bundle": "^2.14", diff --git a/composer.lock b/composer.lock index d0865985..2f746076 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "0ff4d5d4340e850928105cac42ff0c81", + "content-hash": "bdd63ae5b088abd9c17af1c7314d434e", "packages": [ { "name": "api-platform/core", @@ -2490,25 +2490,26 @@ }, { "name": "guzzlehttp/guzzle", - "version": "7.10.4", + "version": "7.11.0", "source": { "type": "git", "url": "https://github.com/guzzle/guzzle.git", - "reference": "aec528da477062d3af11f51e6b33402be233b21f" + "reference": "c987f8ce84b8434fa430795eca0f3430663da72b" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/guzzle/guzzle/zipball/aec528da477062d3af11f51e6b33402be233b21f", - "reference": "aec528da477062d3af11f51e6b33402be233b21f", + "url": "https://api.github.com/repos/guzzle/guzzle/zipball/c987f8ce84b8434fa430795eca0f3430663da72b", + "reference": "c987f8ce84b8434fa430795eca0f3430663da72b", "shasum": "" }, "require": { "ext-json": "*", - "guzzlehttp/promises": "^2.3", - "guzzlehttp/psr7": "^2.8", + "guzzlehttp/promises": "^2.5", + "guzzlehttp/psr7": "^2.11", "php": "^7.2.5 || ^8.0", "psr/http-client": "^1.0", - "symfony/deprecation-contracts": "^2.2 || ^3.0" + "symfony/deprecation-contracts": "^2.5 || ^3.0", + "symfony/polyfill-php80": "^1.24" }, "provide": { "psr/http-client-implementation": "1.0" @@ -2517,7 +2518,7 @@ "bamarni/composer-bin-plugin": "^1.8.2", "ext-curl": "*", "guzzle/client-integration-tests": "3.0.2", - "guzzlehttp/test-server": "^0.3.2", + "guzzlehttp/test-server": "^0.4", "php-http/message-factory": "^1.1", "phpunit/phpunit": "^8.5.52 || ^9.6.34", "psr/log": "^1.1 || ^2.0 || ^3.0" @@ -2597,7 +2598,7 @@ ], "support": { "issues": "https://github.com/guzzle/guzzle/issues", - "source": "https://github.com/guzzle/guzzle/tree/7.10.4" + "source": "https://github.com/guzzle/guzzle/tree/7.11.0" }, "funding": [ { @@ -2613,24 +2614,25 @@ "type": "tidelift" } ], - "time": "2026-05-22T19:00:53+00:00" + "time": "2026-06-02T12:40:51+00:00" }, { "name": "guzzlehttp/promises", - "version": "2.4.1", + "version": "2.5.0", "source": { "type": "git", "url": "https://github.com/guzzle/promises.git", - "reference": "09e8a212562fb1fb6a512c4156ed71525969d6c2" + "reference": "4360e982f87f5f258bf872d094647791db2f4c8e" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/guzzle/promises/zipball/09e8a212562fb1fb6a512c4156ed71525969d6c2", - "reference": "09e8a212562fb1fb6a512c4156ed71525969d6c2", + "url": "https://api.github.com/repos/guzzle/promises/zipball/4360e982f87f5f258bf872d094647791db2f4c8e", + "reference": "4360e982f87f5f258bf872d094647791db2f4c8e", "shasum": "" }, "require": { - "php": "^7.2.5 || ^8.0" + "php": "^7.2.5 || ^8.0", + "symfony/deprecation-contracts": "^2.5 || ^3.0" }, "require-dev": { "bamarni/composer-bin-plugin": "^1.8.2", @@ -2680,7 +2682,7 @@ ], "support": { "issues": "https://github.com/guzzle/promises/issues", - "source": "https://github.com/guzzle/promises/tree/2.4.1" + "source": "https://github.com/guzzle/promises/tree/2.5.0" }, "funding": [ { @@ -2696,27 +2698,29 @@ "type": "tidelift" } ], - "time": "2026-05-20T22:57:30+00:00" + "time": "2026-06-02T12:23:43+00:00" }, { "name": "guzzlehttp/psr7", - "version": "2.10.2", + "version": "2.11.0", "source": { "type": "git", "url": "https://github.com/guzzle/psr7.git", - "reference": "a1bbdc172f32a25fe999965b65b6e71fd87da9ed" + "reference": "bbb5e61349fa5cb822b3e87842b951088b76b81f" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/guzzle/psr7/zipball/a1bbdc172f32a25fe999965b65b6e71fd87da9ed", - "reference": "a1bbdc172f32a25fe999965b65b6e71fd87da9ed", + "url": "https://api.github.com/repos/guzzle/psr7/zipball/bbb5e61349fa5cb822b3e87842b951088b76b81f", + "reference": "bbb5e61349fa5cb822b3e87842b951088b76b81f", "shasum": "" }, "require": { "php": "^7.2.5 || ^8.0", "psr/http-factory": "^1.0", "psr/http-message": "^1.1 || ^2.0", - "ralouphie/getallheaders": "^3.0" + "ralouphie/getallheaders": "^3.0", + "symfony/deprecation-contracts": "^2.5 || ^3.0", + "symfony/polyfill-php80": "^1.24" }, "provide": { "psr/http-factory-implementation": "1.0", @@ -2797,7 +2801,7 @@ ], "support": { "issues": "https://github.com/guzzle/psr7/issues", - "source": "https://github.com/guzzle/psr7/tree/2.10.2" + "source": "https://github.com/guzzle/psr7/tree/2.11.0" }, "funding": [ { @@ -2813,7 +2817,7 @@ "type": "tidelift" } ], - "time": "2026-05-25T22:58:15+00:00" + "time": "2026-06-02T12:30:48+00:00" }, { "name": "imagine/imagine", @@ -2879,16 +2883,16 @@ }, { "name": "itk-dev/openid-connect", - "version": "4.1.2", + "version": "5.0.0", "source": { "type": "git", "url": "https://github.com/itk-dev/openid-connect.git", - "reference": "f19b9a39e7f1beae231d0b06e5b7a78a9efd8eb5" + "reference": "f241f6794a2e74eab8c4808bc22f341a98e96f0b" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/itk-dev/openid-connect/zipball/f19b9a39e7f1beae231d0b06e5b7a78a9efd8eb5", - "reference": "f19b9a39e7f1beae231d0b06e5b7a78a9efd8eb5", + "url": "https://api.github.com/repos/itk-dev/openid-connect/zipball/f241f6794a2e74eab8c4808bc22f341a98e96f0b", + "reference": "f241f6794a2e74eab8c4808bc22f341a98e96f0b", "shasum": "" }, "require": { @@ -2905,7 +2909,8 @@ "ergebnis/composer-normalize": "^2.50", "friendsofphp/php-cs-fixer": "^3.75", "mockery/mockery": "^1.6.12", - "phpstan/phpstan": "^2.1.40", + "phpstan/phpstan": "^2.1.41", + "phpstan/phpstan-mockery": "^2.0", "phpunit/php-code-coverage": "^12", "phpunit/phpunit": "^12" }, @@ -2936,29 +2941,29 @@ "description": "OpenID connect configuration package", "support": { "issues": "https://github.com/itk-dev/openid-connect/issues", - "source": "https://github.com/itk-dev/openid-connect/tree/4.1.2" + "source": "https://github.com/itk-dev/openid-connect/tree/5.0.0" }, - "time": "2026-05-11T12:03:06+00:00" + "time": "2026-06-02T09:01:56+00:00" }, { "name": "itk-dev/openid-connect-bundle", - "version": "4.2.0", + "version": "5.0.0", "source": { "type": "git", "url": "https://github.com/itk-dev/openid-connect-bundle.git", - "reference": "7964acbd630ba0a6b4a362f714bfed255ad88e05" + "reference": "af57823b41629ad5bc1abc50eeb388c7b4b6dbb4" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/itk-dev/openid-connect-bundle/zipball/7964acbd630ba0a6b4a362f714bfed255ad88e05", - "reference": "7964acbd630ba0a6b4a362f714bfed255ad88e05", + "url": "https://api.github.com/repos/itk-dev/openid-connect-bundle/zipball/af57823b41629ad5bc1abc50eeb388c7b4b6dbb4", + "reference": "af57823b41629ad5bc1abc50eeb388c7b4b6dbb4", "shasum": "" }, "require": { "doctrine/orm": "^2.8 || ^3.0", "ext-json": "*", "ext-openssl": "*", - "itk-dev/openid-connect": "^4.0", + "itk-dev/openid-connect": "^5.0", "php": "^8.3", "symfony/cache": "^6.4 || ^7.0 || ^8.0", "symfony/framework-bundle": "^6.4.13 || ^7.0 || ^8.0", @@ -2969,7 +2974,11 @@ "require-dev": { "ergebnis/composer-normalize": "^2.28", "friendsofphp/php-cs-fixer": "^3.11", - "phpstan/phpstan": "^2.1", + "phpstan/phpstan": "^2.1.41", + "phpstan/phpstan-deprecation-rules": "^2.0", + "phpstan/phpstan-phpunit": "^2.0", + "phpstan/phpstan-strict-rules": "^2.0", + "phpstan/phpstan-symfony": "^2.0", "phpunit/phpunit": "^12.0", "rector/rector": "^2.0", "symfony/runtime": "^6.4.13 || ^7.0 || ^8.0" @@ -2997,9 +3006,9 @@ "description": "Symfony bundle for openid-connect", "support": { "issues": "https://github.com/itk-dev/openid-connect-bundle/issues", - "source": "https://github.com/itk-dev/openid-connect-bundle/tree/4.2.0" + "source": "https://github.com/itk-dev/openid-connect-bundle/tree/5.0.0" }, - "time": "2026-05-11T12:11:09+00:00" + "time": "2026-06-02T11:15:20+00:00" }, { "name": "jms/metadata", @@ -5793,16 +5802,16 @@ }, { "name": "symfony/cache", - "version": "v6.4.40", + "version": "v6.4.41", "source": { "type": "git", "url": "https://github.com/symfony/cache.git", - "reference": "8f9b022e63fa02bd984c06dc886039936ea17714" + "reference": "5490a577195422c3c9cda09c64823580858af854" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/symfony/cache/zipball/8f9b022e63fa02bd984c06dc886039936ea17714", - "reference": "8f9b022e63fa02bd984c06dc886039936ea17714", + "url": "https://api.github.com/repos/symfony/cache/zipball/5490a577195422c3c9cda09c64823580858af854", + "reference": "5490a577195422c3c9cda09c64823580858af854", "shasum": "" }, "require": { @@ -5869,7 +5878,7 @@ "psr6" ], "support": { - "source": "https://github.com/symfony/cache/tree/v6.4.40" + "source": "https://github.com/symfony/cache/tree/v6.4.41" }, "funding": [ { @@ -5889,7 +5898,7 @@ "type": "tidelift" } ], - "time": "2026-05-19T20:33:22+00:00" + "time": "2026-05-24T08:42:40+00:00" }, { "name": "symfony/cache-contracts", @@ -8180,16 +8189,16 @@ }, { "name": "symfony/polyfill-intl-idn", - "version": "v1.37.0", + "version": "v1.38.1", "source": { "type": "git", "url": "https://github.com/symfony/polyfill-intl-idn.git", - "reference": "9614ac4d8061dc257ecc64cba1b140873dce8ad3" + "reference": "dc21118016c039a66235cf93d96b435ffb282412" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/symfony/polyfill-intl-idn/zipball/9614ac4d8061dc257ecc64cba1b140873dce8ad3", - "reference": "9614ac4d8061dc257ecc64cba1b140873dce8ad3", + "url": "https://api.github.com/repos/symfony/polyfill-intl-idn/zipball/dc21118016c039a66235cf93d96b435ffb282412", + "reference": "dc21118016c039a66235cf93d96b435ffb282412", "shasum": "" }, "require": { @@ -8243,7 +8252,7 @@ "shim" ], "support": { - "source": "https://github.com/symfony/polyfill-intl-idn/tree/v1.37.0" + "source": "https://github.com/symfony/polyfill-intl-idn/tree/v1.38.1" }, "funding": [ { @@ -8263,7 +8272,7 @@ "type": "tidelift" } ], - "time": "2024-09-10T14:38:51+00:00" + "time": "2026-05-25T15:22:23+00:00" }, { "name": "symfony/polyfill-intl-normalizer", @@ -8352,16 +8361,16 @@ }, { "name": "symfony/polyfill-mbstring", - "version": "v1.38.0", + "version": "v1.38.1", "source": { "type": "git", "url": "https://github.com/symfony/polyfill-mbstring.git", - "reference": "6b177d03d2eb04a6c9d01bab9818fb93a30ce7fd" + "reference": "14c5439eec4ccff081ac14eca2dc57feb2a66d92" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/symfony/polyfill-mbstring/zipball/6b177d03d2eb04a6c9d01bab9818fb93a30ce7fd", - "reference": "6b177d03d2eb04a6c9d01bab9818fb93a30ce7fd", + "url": "https://api.github.com/repos/symfony/polyfill-mbstring/zipball/14c5439eec4ccff081ac14eca2dc57feb2a66d92", + "reference": "14c5439eec4ccff081ac14eca2dc57feb2a66d92", "shasum": "" }, "require": { @@ -8413,7 +8422,7 @@ "shim" ], "support": { - "source": "https://github.com/symfony/polyfill-mbstring/tree/v1.38.0" + "source": "https://github.com/symfony/polyfill-mbstring/tree/v1.38.1" }, "funding": [ { @@ -8433,7 +8442,7 @@ "type": "tidelift" } ], - "time": "2026-05-25T14:08:27+00:00" + "time": "2026-05-26T12:51:13+00:00" }, { "name": "symfony/polyfill-php56", diff --git a/config/packages/itkdev_openid_connect.yaml b/config/packages/itkdev_openid_connect.yaml index 28d4fe3b..e60b69cd 100644 --- a/config/packages/itkdev_openid_connect.yaml +++ b/config/packages/itkdev_openid_connect.yaml @@ -12,6 +12,8 @@ itkdev_openid_connect: client_secret: '%env(string:INTERNAL_OIDC_CLIENT_SECRET)%' redirect_uri: '%env(string:INTERNAL_OIDC_REDIRECT_URI)%' leeway: '%env(int:INTERNAL_OIDC_LEEWAY)%' + http_client_options: + timeout: '%env(float:OIDC_HTTP_TIMEOUT)%' # Bound IdP HTTP calls so a hung provider can't tie up a worker. external: options: metadata_url: '%env(string:EXTERNAL_OIDC_METADATA_URL)%' @@ -19,3 +21,5 @@ itkdev_openid_connect: client_secret: '%env(string:EXTERNAL_OIDC_CLIENT_SECRET)%' redirect_uri: '%env(string:EXTERNAL_OIDC_REDIRECT_URI)%' leeway: '%env(int:EXTERNAL_OIDC_LEEWAY)%' + http_client_options: + timeout: '%env(float:OIDC_HTTP_TIMEOUT)%' # Bound IdP HTTP calls so a hung provider can't tie up a worker. diff --git a/docs/configuration/openid-connect.md b/docs/configuration/openid-connect.md index edadb6d6..689db6e6 100644 --- a/docs/configuration/openid-connect.md +++ b/docs/configuration/openid-connect.md @@ -2,6 +2,9 @@ ```dotenv ###> itk-dev/openid-connect-bundle ### +# Total HTTP request timeout (seconds) for calls to the OIDC providers. +OIDC_HTTP_TIMEOUT=5 + # internal provider INTERNAL_OIDC_METADATA_URL=INTERNAL_OIDC_METADATA_URL INTERNAL_OIDC_CLIENT_ID=INTERNAL_OIDC_CLIENT_ID diff --git a/src/Controller/Api/AuthOidcController.php b/src/Controller/Api/AuthOidcController.php index 21c6e795..4296c0ac 100644 --- a/src/Controller/Api/AuthOidcController.php +++ b/src/Controller/Api/AuthOidcController.php @@ -5,7 +5,7 @@ namespace App\Controller\Api; use App\Security\AzureOidcAuthenticator; -use ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException; +use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; use Lexik\Bundle\JWTAuthenticationBundle\Security\Http\Authentication\AuthenticationFailureHandler; @@ -75,7 +75,7 @@ public function getUrls(Request $request): Response // We allow end session endpoint to not be set. try { $endSessionUrl = $provider->getEndSessionUrl(); - } catch (ItkOpenIdConnectException) { + } catch (OpenIdConnectExceptionInterface) { $endSessionUrl = null; } @@ -92,7 +92,7 @@ public function getUrls(Request $request): Response return new JsonResponse($data); } catch (InvalidProviderException) { throw $this->createNotFoundException('Unknown provider: '.$providerKey); - } catch (ItkOpenIdConnectException $e) { + } catch (OpenIdConnectExceptionInterface $e) { throw new HttpException(500, $e->getMessage()); } } diff --git a/src/Security/AzureOidcAuthenticator.php b/src/Security/AzureOidcAuthenticator.php index e3f9497a..07e1a3de 100644 --- a/src/Security/AzureOidcAuthenticator.php +++ b/src/Security/AzureOidcAuthenticator.php @@ -11,7 +11,7 @@ use App\Service\UserService; use App\Utils\Roles; use Doctrine\ORM\EntityManagerInterface; -use ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException; +use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; use ItkDev\OpenIdConnectBundle\Security\OpenIdLoginAuthenticator; @@ -120,7 +120,7 @@ public function authenticate(Request $request): Passport $this->entityManager->flush(); return new SelfValidatingPassport(new UserBadge($user->getUserIdentifier(), $this->getUser(...))); - } catch (CustomUserMessageAuthenticationException|InvalidProviderException|ItkOpenIdConnectException $exception) { + } catch (CustomUserMessageAuthenticationException|OpenIdConnectExceptionInterface $exception) { $this->logger->error($exception); throw new CustomUserMessageAuthenticationException($exception->getMessage()); diff --git a/tests/Controller/Api/AuthOidcControllerTest.php b/tests/Controller/Api/AuthOidcControllerTest.php index 6825a682..37a1e987 100644 --- a/tests/Controller/Api/AuthOidcControllerTest.php +++ b/tests/Controller/Api/AuthOidcControllerTest.php @@ -6,7 +6,9 @@ use App\Controller\Api\AuthOidcController; use App\Security\AzureOidcAuthenticator; +use ItkDev\OpenIdConnect\Exception\MetadataException; use ItkDev\OpenIdConnect\Security\OpenIdConfigurationProvider; +use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; use Lexik\Bundle\JWTAuthenticationBundle\Security\Http\Authentication\AuthenticationFailureHandler; use Lexik\Bundle\JWTAuthenticationBundle\Security\Http\Authentication\AuthenticationSuccessHandler; @@ -16,6 +18,8 @@ use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpFoundation\Session\Session; use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage; +use Symfony\Component\HttpKernel\Exception\HttpException; +use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; /** * Regression test for the switch from autowired SessionInterface to @@ -87,4 +91,110 @@ public function testGetUrlsFallsBackToFirstProviderKeyWhenNoneRequested(): void $this->assertSame('first', $session->get('oauth2provider')); } + + /** + * getUrls() treats a missing end-session endpoint as optional: when the + * provider throws an OIDC error from getEndSessionUrl(), the inner catch + * swallows it and returns endSessionUrl => null with a 200 response. + * + * Regression guard for the openid-connect 5.0 exception rework — the inner + * catch must match on OpenIdConnectExceptionInterface (which + * MetadataException implements), not the deprecated abstract base. + */ + public function testGetUrlsReturnsNullEndSessionUrlWhenProviderHasNoEndSessionEndpoint(): void + { + $provider = $this->createMock(OpenIdConfigurationProvider::class); + $provider->method('generateNonce')->willReturn('test-nonce'); + $provider->method('generateState')->willReturn('test-state'); + $provider->method('getEndSessionUrl')->willThrowException(new MetadataException('no end_session_endpoint')); + $provider->method('getAuthorizationUrl')->willReturn('https://example.test/authorize'); + + $manager = $this->createMock(OpenIdConfigurationProviderManager::class); + $manager->method('getProvider')->with('internal')->willReturn($provider); + + $controller = new AuthOidcController( + $manager, + $this->createMock(AzureOidcAuthenticator::class), + $this->createMock(AuthenticationSuccessHandler::class), + $this->createMock(AuthenticationFailureHandler::class), + ); + $controller->setContainer(new Container()); + + $request = Request::create('/v2/authentication/oidc/urls?providerKey=internal'); + $request->setSession(new Session(new MockArraySessionStorage())); + + $response = $controller->getUrls($request); + + $this->assertSame(Response::HTTP_OK, $response->getStatusCode(), (string) $response->getContent()); + $data = json_decode((string) $response->getContent(), true, 512, \JSON_THROW_ON_ERROR); + $this->assertNull($data['endSessionUrl']); + $this->assertSame('https://example.test/authorize', $data['authorizationUrl']); + } + + /** + * getUrls() maps an unknown provider key to a 404. The provider manager + * throws InvalidProviderException, which the controller catches and + * rethrows as a NotFoundHttpException. + */ + public function testGetUrlsThrowsNotFoundForUnknownProvider(): void + { + $manager = $this->createMock(OpenIdConfigurationProviderManager::class); + $manager->method('getProvider')->with('bogus')->willThrowException(new InvalidProviderException('unknown provider')); + + $controller = new AuthOidcController( + $manager, + $this->createMock(AzureOidcAuthenticator::class), + $this->createMock(AuthenticationSuccessHandler::class), + $this->createMock(AuthenticationFailureHandler::class), + ); + $controller->setContainer(new Container()); + + $request = Request::create('/v2/authentication/oidc/urls?providerKey=bogus'); + $request->setSession(new Session(new MockArraySessionStorage())); + + try { + $controller->getUrls($request); + $this->fail('Expected NotFoundHttpException was not thrown.'); + } catch (NotFoundHttpException $e) { + $this->assertSame(Response::HTTP_NOT_FOUND, $e->getStatusCode()); + $this->assertSame('Unknown provider: bogus', $e->getMessage()); + } + } + + /** + * getUrls() maps a generic OIDC failure (e.g. malformed discovery document) + * to a 500. The outer catch must match on OpenIdConnectExceptionInterface; + * before the openid-connect 5.0 rework this caught the deprecated abstract + * base and would silently let the exception escape unmapped. + */ + public function testGetUrlsThrowsHttpExceptionWhenProviderFailsWithOidcError(): void + { + $provider = $this->createMock(OpenIdConfigurationProvider::class); + $provider->method('generateNonce')->willReturn('test-nonce'); + $provider->method('generateState')->willReturn('test-state'); + $provider->method('getEndSessionUrl')->willReturn('https://example.test/end-session'); + $provider->method('getAuthorizationUrl')->willThrowException(new MetadataException('malformed discovery document')); + + $manager = $this->createMock(OpenIdConfigurationProviderManager::class); + $manager->method('getProvider')->with('internal')->willReturn($provider); + + $controller = new AuthOidcController( + $manager, + $this->createMock(AzureOidcAuthenticator::class), + $this->createMock(AuthenticationSuccessHandler::class), + $this->createMock(AuthenticationFailureHandler::class), + ); + $controller->setContainer(new Container()); + + $request = Request::create('/v2/authentication/oidc/urls?providerKey=internal'); + $request->setSession(new Session(new MockArraySessionStorage())); + + try { + $controller->getUrls($request); + $this->fail('Expected HttpException was not thrown.'); + } catch (HttpException $e) { + $this->assertSame(Response::HTTP_INTERNAL_SERVER_ERROR, $e->getStatusCode()); + $this->assertSame('malformed discovery document', $e->getMessage()); + } + } } diff --git a/tests/Security/AzureOidcAuthenticatorTest.php b/tests/Security/AzureOidcAuthenticatorTest.php new file mode 100644 index 00000000..45170ea1 --- /dev/null +++ b/tests/Security/AzureOidcAuthenticatorTest.php @@ -0,0 +1,362 @@ +getMockBuilder(AzureOidcAuthenticator::class) + ->disableOriginalConstructor() + ->onlyMethods(['validateClaims']) + ->getMock(); + $authenticator->method('validateClaims') + ->willThrowException(new MetadataException('malformed discovery document')); + $authenticator->setLogger(new NullLogger()); + + $request = Request::create('/v2/authentication/oidc/token?state=test-state&code=test-code'); + + $this->expectException(CustomUserMessageAuthenticationException::class); + $this->expectExceptionMessage('malformed discovery document'); + + $authenticator->authenticate($request); + } + + /** + * #2 — internal provider, new user: the user is provisioned from the + * claims (full name, email, provider id == email, internal type, OIDC + * creator) and the configured authenticator is recorded as the provider. + */ + public function testAuthenticateProvisionsNewInternalUserFromClaims(): void + { + $persisted = []; + $em = $this->createEntityManager(null, $persisted); + + $authenticator = $this->createAuthenticator([ + 'open_id_connect_provider' => AzureOidcAuthenticator::OIDC_PROVIDER_INTERNAL, + self::CLAIM_NAME => 'Jane Doe', + self::CLAIM_EMAIL => 'jane@example.test', + self::CLAIM_GROUPS => [], + ], $em); + + $passport = $authenticator->authenticate(Request::create('/')); + + $this->assertInstanceOf(SelfValidatingPassport::class, $passport); + $user = $this->onlyPersistedUser($persisted); + $this->assertSame('Jane Doe', $user->getFullName()); + $this->assertSame('jane@example.test', $user->getEmail()); + $this->assertSame('jane@example.test', $user->getProviderId()); + $this->assertSame(UserTypeEnum::OIDC_INTERNAL, $user->getUserType()); + $this->assertSame(UserCreatorEnum::OIDC->value, $user->getCreatedBy()); + $this->assertSame(AzureOidcAuthenticator::class, $user->getProvider()); + } + + /** + * #1 — group claims map to tenant keys + roles: the "Admin" suffix grants + * ROLE_ADMIN, the "Redaktoer" suffix grants ROLE_EDITOR, and the tenant key + * is the group name with the suffix stripped. + */ + public function testAuthenticateMapsGroupClaimsToTenantRoles(): void + { + $persisted = []; + $em = $this->createEntityManager(null, $persisted); + + $authenticator = $this->createAuthenticator([ + 'open_id_connect_provider' => AzureOidcAuthenticator::OIDC_PROVIDER_INTERNAL, + self::CLAIM_NAME => 'Jane Doe', + self::CLAIM_EMAIL => 'jane@example.test', + self::CLAIM_GROUPS => ['Example1Admin', 'Example2Redaktoer'], + ], $em); + + $authenticator->authenticate(Request::create('/')); + + $this->assertSame([ + 'Example1' => [Roles::ROLE_ADMIN], + 'Example2' => [Roles::ROLE_EDITOR], + ], $this->tenantRoleMap($this->onlyPersistedUser($persisted))); + } + + /** + * #1 — multiple groups for the same tenant accumulate into a single + * tenant entry carrying both roles. + */ + public function testAuthenticateAccumulatesMultipleRolesForSameTenant(): void + { + $persisted = []; + $em = $this->createEntityManager(null, $persisted); + + $authenticator = $this->createAuthenticator([ + 'open_id_connect_provider' => AzureOidcAuthenticator::OIDC_PROVIDER_INTERNAL, + self::CLAIM_NAME => 'Jane Doe', + self::CLAIM_EMAIL => 'jane@example.test', + self::CLAIM_GROUPS => ['Example1Admin', 'Example1Redaktoer'], + ], $em); + + $authenticator->authenticate(Request::create('/')); + + $this->assertSame([ + 'Example1' => [Roles::ROLE_ADMIN, Roles::ROLE_EDITOR], + ], $this->tenantRoleMap($this->onlyPersistedUser($persisted))); + } + + /** + * #1 — de-provisioning: tenants no longer present in the claims are + * revoked from an existing user, while a tenant still present has its + * roles refreshed from the claims (here EDITOR -> ADMIN). + */ + public function testAuthenticateRevokesTenantsAbsentFromClaims(): void + { + $keep = (new Tenant())->setTenantKey('Example1'); + $stale = (new Tenant())->setTenantKey('OldTenant'); + + $existing = new User(); + $existing->setEmail('jane@example.test'); + $existing->setProviderId('jane@example.test'); + $existing->setRoleTenant([Roles::ROLE_EDITOR], $keep); + $existing->setRoleTenant([Roles::ROLE_ADMIN], $stale); + + $persisted = []; + $em = $this->createEntityManager($existing, $persisted); + + // The factory must return the same managed Tenant instances the user + // already references, mirroring TenantRepository::findByKeys() so the + // existing role-tenant link is updated rather than duplicated. + $authenticator = $this->createAuthenticator([ + 'open_id_connect_provider' => AzureOidcAuthenticator::OIDC_PROVIDER_INTERNAL, + self::CLAIM_NAME => 'Jane Doe', + self::CLAIM_EMAIL => 'jane@example.test', + self::CLAIM_GROUPS => ['Example1Admin'], + ], $em, ['Example1' => $keep, 'OldTenant' => $stale]); + + $authenticator->authenticate(Request::create('/')); + + $this->assertSame([ + 'Example1' => [Roles::ROLE_ADMIN], + ], $this->tenantRoleMap($existing)); + } + + /** + * #1 — a group whose name carries neither the admin nor the editor suffix + * is a configuration/claim error and aborts authentication with an + * \InvalidArgumentException (it is not one of the OIDC exception types the + * authenticator wraps). + */ + public function testAuthenticateThrowsOnGroupWithUnknownRoleSuffix(): void + { + $persisted = []; + $em = $this->createEntityManager(null, $persisted); + + $authenticator = $this->createAuthenticator([ + 'open_id_connect_provider' => AzureOidcAuthenticator::OIDC_PROVIDER_INTERNAL, + self::CLAIM_NAME => 'Jane Doe', + self::CLAIM_EMAIL => 'jane@example.test', + self::CLAIM_GROUPS => ['Example1Viewer'], + ], $em); + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('Unknown role for group: Example1Viewer'); + + $authenticator->authenticate(Request::create('/')); + } + + /** + * #2 — external provider, new user: the user is keyed by a hash of the + * sign-in name, gets a placeholder email/name to be filled in on + * activation, the external type, and is assigned no tenant roles. + */ + public function testAuthenticateProvisionsNewExternalUser(): void + { + $persisted = []; + $em = $this->createEntityManager(null, $persisted); + + $userService = $this->createMock(UserService::class); + $userService->method('generatePersonalIdentifierHash')->with('ext-sign-in')->willReturn('hashed-id'); + + $authenticator = $this->createAuthenticator([ + 'open_id_connect_provider' => AzureOidcAuthenticator::OIDC_PROVIDER_EXTERNAL, + self::CLAIM_SIGN_IN_NAME => 'ext-sign-in', + ], $em, [], $userService); + + $authenticator->authenticate(Request::create('/')); + + $user = $this->onlyPersistedUser($persisted); + $this->assertSame('hashed-id', $user->getProviderId()); + $this->assertSame('hashed-id@ext_not_set', $user->getEmail()); + $this->assertSame(UserService::EXTERNAL_USER_DEFAULT_NAME, $user->getFullName()); + $this->assertSame(UserTypeEnum::OIDC_EXTERNAL, $user->getUserType()); + $this->assertCount(0, $user->getUserRoleTenants()); + } + + /** + * #2 — external provider with a missing/empty required id claim fails with + * a CustomUserMessageAuthenticationException naming the missing claim. + */ + public function testAuthenticateRejectsExternalUserWithMissingIdClaim(): void + { + $persisted = []; + $em = $this->createEntityManager(null, $persisted); + + $authenticator = $this->createAuthenticator([ + 'open_id_connect_provider' => AzureOidcAuthenticator::OIDC_PROVIDER_EXTERNAL, + self::CLAIM_SIGN_IN_NAME => '', + ], $em); + + $this->expectException(CustomUserMessageAuthenticationException::class); + $this->expectExceptionMessage('Missing required claim '.self::CLAIM_SIGN_IN_NAME); + + $authenticator->authenticate(Request::create('/')); + } + + /** + * #2 — an unrecognised open_id_connect_provider value is rejected with a + * CustomUserMessageAuthenticationException. + */ + public function testAuthenticateRejectsUnsupportedProvider(): void + { + $persisted = []; + $em = $this->createEntityManager(null, $persisted); + + $authenticator = $this->createAuthenticator([ + 'open_id_connect_provider' => 'unsupported', + ], $em); + + $this->expectException(CustomUserMessageAuthenticationException::class); + $this->expectExceptionMessage('Unsupported open_id_connect_provider.'); + + $authenticator->authenticate(Request::create('/')); + } + + /** + * Builds an AzureOidcAuthenticator with real collaborators but a stubbed + * validateClaims() so tests can drive the post-validation logic directly. + * + * @param array $claims claims validateClaims() returns + * @param array $tenantSeed tenant key => instance the + * TenantFactory should hand back + */ + private function createAuthenticator( + array $claims, + EntityManagerInterface $em, + array $tenantSeed = [], + ?UserService $userService = null, + ): AzureOidcAuthenticator { + $authenticator = $this->getMockBuilder(AzureOidcAuthenticator::class) + ->setConstructorArgs([ + $em, + $this->createMock(OpenIdConfigurationProviderManager::class), + $this->createTenantFactory($tenantSeed), + $userService ?? $this->createMock(UserService::class), + self::CLAIM_NAME, + self::CLAIM_EMAIL, + self::CLAIM_GROUPS, + self::CLAIM_SIGN_IN_NAME, + ]) + ->onlyMethods(['validateClaims']) + ->getMock(); + $authenticator->method('validateClaims')->willReturn($claims); + $authenticator->setLogger(new NullLogger()); + + return $authenticator; + } + + /** + * @param list $persisted captures everything persist()ed + */ + private function createEntityManager(?User $found, array &$persisted): EntityManagerInterface + { + $repository = $this->createMock(EntityRepository::class); + $repository->method('findOneBy')->willReturn($found); + + $em = $this->createMock(EntityManagerInterface::class); + $em->method('getRepository')->willReturn($repository); + $em->method('persist')->willReturnCallback(static function (object $entity) use (&$persisted): void { + $persisted[] = $entity; + }); + + return $em; + } + + /** + * Mirrors TenantFactory::setupTenants(): returns the seeded instance for a + * known key, otherwise a fresh Tenant — so role assignment reuses managed + * instances exactly as production does. + * + * @param array $seed + */ + private function createTenantFactory(array $seed): TenantFactory + { + $factory = $this->createMock(TenantFactory::class); + $factory->method('setupTenants')->willReturnCallback( + static function (array $tenantKeys) use ($seed): array { + $tenants = []; + foreach ($tenantKeys as $tenantKey) { + $tenants[$tenantKey] = $seed[$tenantKey] ?? (new Tenant())->setTenantKey($tenantKey); + } + + return $tenants; + } + ); + + return $factory; + } + + /** + * @param list $persisted + */ + private function onlyPersistedUser(array $persisted): User + { + $users = array_values(array_filter($persisted, static fn (object $e): bool => $e instanceof User)); + $this->assertCount(1, $users, 'Expected exactly one User to be persisted.'); + + return $users[0]; + } + + /** + * @return array> tenant key => roles + */ + private function tenantRoleMap(User $user): array + { + $map = []; + foreach ($user->getUserRoleTenants() as $userRoleTenant) { + $map[$userRoleTenant->getTenant()->getTenantKey()] = $userRoleTenant->getRoles(); + } + + return $map; + } +}