From ebb6914d8c1ebaecd6d23a00182d0f80fb17d7ea Mon Sep 17 00:00:00 2001 From: Marko Ivancic Date: Tue, 1 Apr 2025 17:07:40 +0200 Subject: [PATCH 1/3] WIP --- routing/routes/routes.php | 5 ++ src/Codebooks/RoutesEnum.php | 1 + .../Federation/EntityStatementController.php | 3 +- .../SubordinateListingsController.php | 66 +++++++++++++++++++ src/Utils/Routes.php | 5 ++ 5 files changed, 78 insertions(+), 2 deletions(-) create mode 100644 src/Controllers/Federation/SubordinateListingsController.php diff --git a/routing/routes/routes.php b/routing/routes/routes.php index f05ac6ac..caad53c7 100644 --- a/routing/routes/routes.php +++ b/routing/routes/routes.php @@ -15,6 +15,7 @@ use SimpleSAML\Module\oidc\Controllers\ConfigurationDiscoveryController; use SimpleSAML\Module\oidc\Controllers\EndSessionController; use SimpleSAML\Module\oidc\Controllers\Federation\EntityStatementController; +use SimpleSAML\Module\oidc\Controllers\Federation\SubordinateListingsController; use SimpleSAML\Module\oidc\Controllers\JwksController; use SimpleSAML\Module\oidc\Controllers\UserInfoController; use SimpleSAML\OpenID\Codebooks\HttpMethodsEnum; @@ -96,4 +97,8 @@ $routes->add(RoutesEnum::FederationFetch->name, RoutesEnum::FederationFetch->value) ->controller([EntityStatementController::class, 'fetch']) ->methods([HttpMethodsEnum::GET->value]); + + $routes->add(RoutesEnum::FederationList->name, RoutesEnum::FederationList->value) + ->controller([SubordinateListingsController::class, 'list']) + ->methods([HttpMethodsEnum::GET->value]); }; diff --git a/src/Codebooks/RoutesEnum.php b/src/Codebooks/RoutesEnum.php index f81a98b8..6c17691a 100644 --- a/src/Codebooks/RoutesEnum.php +++ b/src/Codebooks/RoutesEnum.php @@ -46,4 +46,5 @@ enum RoutesEnum: string case FederationConfiguration = '.well-known/openid-federation'; case FederationFetch = 'federation/fetch'; + case FederationList = 'federation/list'; } diff --git a/src/Controllers/Federation/EntityStatementController.php b/src/Controllers/Federation/EntityStatementController.php index 105d8100..6d2bbc04 100644 --- a/src/Controllers/Federation/EntityStatementController.php +++ b/src/Controllers/Federation/EntityStatementController.php @@ -211,7 +211,7 @@ public function configuration(): Response public function fetch(Request $request): Response { - $subject = $request->query->get(ClaimsEnum::Sub->value); + $subject = $request->query->getString(ClaimsEnum::Sub->value); if (empty($subject)) { return $this->routes->newJsonErrorResponse( @@ -222,7 +222,6 @@ public function fetch(Request $request): Response } /** @var non-empty-string $subject */ - $subject = (string)$subject; $cachedSubordinateStatement = $this->federationCache?->get( null, diff --git a/src/Controllers/Federation/SubordinateListingsController.php b/src/Controllers/Federation/SubordinateListingsController.php new file mode 100644 index 00000000..cebd59c6 --- /dev/null +++ b/src/Controllers/Federation/SubordinateListingsController.php @@ -0,0 +1,66 @@ +moduleConfig->getFederationEnabled()) { + throw OidcServerException::forbidden('federation capabilities not enabled'); + } + } + + public function list(Request $request): Response + { + // If unsupported query parameter is provided, we have to respond with an error: "If the responder does not + // support this feature, it MUST use the HTTP status code 400 and the content type application/json, with + // the error code unsupported_parameter." + + // Currently, we don't support any of the mentioned params in the spec, so let's return error for any of them. + $unsupportedParams = [ + ParamsEnum::EntityType->value, + ParamsEnum::TrustMarked->value, + ParamsEnum::TrustMarkId->value, + ParamsEnum::Intermediate->value, + ]; + + $requestedParams = array_keys($request->query->all()); + + if (!empty($intersectedParams = array_intersect($unsupportedParams, $requestedParams))) { + return $this->routes->newJsonErrorResponse( + ErrorsEnum::UnsupportedParameter->value, + 'Unsupported parameter: ' . implode(', ', $intersectedParams), + ); + } + + dd($request->query->all()); + + + if ($entityTypes = $request->query->all(ParamsEnum::EntityType->value)) { + } + + return new Response(); + } +} diff --git a/src/Utils/Routes.php b/src/Utils/Routes.php index a9fea448..d9134231 100644 --- a/src/Utils/Routes.php +++ b/src/Utils/Routes.php @@ -193,4 +193,9 @@ public function urlFederationFetch(array $parameters = []): string { return $this->getModuleUrl(RoutesEnum::FederationFetch->value, $parameters); } + + public function urlFederationList(array $parameters = []): string + { + return $this->getModuleUrl(RoutesEnum::FederationList->value, $parameters); + } } From be12203dbeebf0d7caac70a3682ca09484862f14 Mon Sep 17 00:00:00 2001 From: Marko Ivancic Date: Wed, 2 Apr 2025 14:27:07 +0200 Subject: [PATCH 2/3] Intitial implementation --- .../Federation/EntityStatementController.php | 8 +- .../SubordinateListingsController.php | 27 ++--- src/Repositories/ClientRepository.php | 70 ++++++++++- .../SubordinateListingsControllerTest.php | 110 ++++++++++++++++++ .../src/Repositories/ClientRepositoryTest.php | 44 ++++++- 5 files changed, 235 insertions(+), 24 deletions(-) create mode 100644 tests/unit/src/Controllers/Federation/SubordinateListingsControllerTest.php diff --git a/src/Controllers/Federation/EntityStatementController.php b/src/Controllers/Federation/EntityStatementController.php index 6d2bbc04..5f4494a7 100644 --- a/src/Controllers/Federation/EntityStatementController.php +++ b/src/Controllers/Federation/EntityStatementController.php @@ -4,7 +4,6 @@ namespace SimpleSAML\Module\oidc\Controllers\Federation; -use SimpleSAML\Module\oidc\Codebooks\RoutesEnum; use SimpleSAML\Module\oidc\Helpers; use SimpleSAML\Module\oidc\ModuleConfig; use SimpleSAML\Module\oidc\Repositories\ClientRepository; @@ -95,11 +94,10 @@ public function configuration(): Response ClaimsEnum::HomepageUri->value => $this->moduleConfig->getHomepageUri(), ], )), - ClaimsEnum::FederationFetchEndpoint->value => - $this->moduleConfig->getModuleUrl(RoutesEnum::FederationFetch->value), + ClaimsEnum::FederationFetchEndpoint->value => $this->routes->urlFederationFetch(), + ClaimsEnum::FederationListEndpoint->value => $this->routes->urlFederationList(), // TODO v7 mivanci Add when ready. Use ClaimsEnum for keys. // https://openid.net/specs/openid-federation-1_0.html#name-federation-entity - //'federation_list_endpoint', //'federation_resolve_endpoint', //'federation_trust_mark_status_endpoint', //'federation_trust_mark_list_endpoint', @@ -233,7 +231,7 @@ public function fetch(Request $request): Response return $this->prepareEntityStatementResponse((string)$cachedSubordinateStatement); } - $client = $this->clientRepository->findByEntityIdentifier($subject); + $client = $this->clientRepository->findFederatedByEntityIdentifier($subject); if (empty($client)) { return $this->routes->newJsonErrorResponse( ErrorsEnum::NotFound->value, diff --git a/src/Controllers/Federation/SubordinateListingsController.php b/src/Controllers/Federation/SubordinateListingsController.php index cebd59c6..f0eb5edc 100644 --- a/src/Controllers/Federation/SubordinateListingsController.php +++ b/src/Controllers/Federation/SubordinateListingsController.php @@ -4,11 +4,10 @@ namespace SimpleSAML\Module\oidc\Controllers\Federation; -use SimpleSAML\Module\oidc\Helpers; +use SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface; use SimpleSAML\Module\oidc\ModuleConfig; use SimpleSAML\Module\oidc\Repositories\ClientRepository; use SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException; -use SimpleSAML\Module\oidc\Services\LoggerService; use SimpleSAML\Module\oidc\Utils\Routes; use SimpleSAML\OpenID\Codebooks\ErrorsEnum; use SimpleSAML\OpenID\Codebooks\ParamsEnum; @@ -21,11 +20,9 @@ class SubordinateListingsController * @throws \SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException */ public function __construct( - private readonly ModuleConfig $moduleConfig, - private readonly ClientRepository $clientRepository, - private readonly Helpers $helpers, - private readonly Routes $routes, - private readonly LoggerService $loggerService, + protected readonly ModuleConfig $moduleConfig, + protected readonly ClientRepository $clientRepository, + protected readonly Routes $routes, ) { if (!$this->moduleConfig->getFederationEnabled()) { throw OidcServerException::forbidden('federation capabilities not enabled'); @@ -52,15 +49,19 @@ public function list(Request $request): Response return $this->routes->newJsonErrorResponse( ErrorsEnum::UnsupportedParameter->value, 'Unsupported parameter: ' . implode(', ', $intersectedParams), + 400, ); } - dd($request->query->all()); + $subordinateEntityIdList = array_filter(array_map( + function (ClientEntityInterface $clientEntity): ?string { + return $clientEntity->getEntityIdentifier(); + }, + $this->clientRepository->findAllFederated(), + )); - - if ($entityTypes = $request->query->all(ParamsEnum::EntityType->value)) { - } - - return new Response(); + return $this->routes->newJsonResponse( + $subordinateEntityIdList, + ); } } diff --git a/src/Repositories/ClientRepository.php b/src/Repositories/ClientRepository.php index 49654731..27bc952a 100644 --- a/src/Repositories/ClientRepository.php +++ b/src/Repositories/ClientRepository.php @@ -153,14 +153,10 @@ public function findByEntityIdentifier(string $entityIdentifier, ?string $owner <<getTableName()} WHERE - entity_identifier = :entity_identifier AND - is_enabled = :is_enabled AND - is_federated = :is_federated + entity_identifier = :entity_identifier EOS, [ 'entity_identifier' => $entityIdentifier, - 'is_enabled' => [true, PDO::PARAM_BOOL], - 'is_federated' => [true, PDO::PARAM_BOOL], ], $owner, ); @@ -190,6 +186,29 @@ public function findByEntityIdentifier(string $entityIdentifier, ?string $owner return $clientEntity; } + public function findFederatedByEntityIdentifier( + string $entityIdentifier, + ?string $owner = null, + ): ?ClientEntityInterface { + $clientEntity = $this->findByEntityIdentifier($entityIdentifier, $owner); + + if (is_null($clientEntity)) { + return null; + } + + if ( + is_null($clientEntity->getEntityIdentifier()) || + (! $clientEntity->isEnabled()) || + (! $clientEntity->isFederated()) || + (!is_array($clientEntity->getFederationJwks())) || + $clientEntity->isExpired() + ) { + return null; + } + + return $clientEntity; + } + private function addOwnerWhereClause(string $query, array $params, ?string $owner = null): array { if (isset($owner)) { @@ -234,6 +253,47 @@ public function findAll(?string $owner = null): array return $clients; } + /** + * @return \SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface[] + * @throws \JsonException + * @throws \SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException + */ + public function findAllFederated(?string $owner = null): array + { + /** + * @var string $query + * @var array $params + */ + [$query, $params] = $this->addOwnerWhereClause( + <<getTableName()} + WHERE + entity_identifier IS NOT NULL AND + federation_jwks IS NOT NULL AND + is_enabled = :is_enabled AND + is_federated = :is_federated + EOS, + [ + 'is_enabled' => [true, PDO::PARAM_BOOL], + 'is_federated' => [true, PDO::PARAM_BOOL], + ], + $owner, + ); + $stmt = $this->database->read( + "$query ORDER BY name ASC", + $params, + ); + + $clients = []; + + /** @var array $state */ + foreach ($stmt->fetchAll() as $state) { + $clients[] = $this->clientEntityFactory->fromState($state); + } + + return $clients; + } + /** * @return array{ * numPages: int, diff --git a/tests/unit/src/Controllers/Federation/SubordinateListingsControllerTest.php b/tests/unit/src/Controllers/Federation/SubordinateListingsControllerTest.php new file mode 100644 index 00000000..d1396f1c --- /dev/null +++ b/tests/unit/src/Controllers/Federation/SubordinateListingsControllerTest.php @@ -0,0 +1,110 @@ +moduleConfigMock = $this->createMock(ModuleConfig::class); + $this->clientRepositoryMock = $this->createMock(ClientRepository::class); + $this->routesMock = $this->createMock(Routes::class); + + $this->isFederationEnabled = true; + + $this->requestMock = $this->createMock(Request::class); + $this->requestQueryMock = $this->createMock(ParameterBag::class); + $this->requestMock->query = $this->requestQueryMock; + } + + public function sut( + ?ModuleConfig $moduleConfig = null, + ?ClientRepository $clientRepository = null, + ?Routes $routes = null, + ?bool $federationEnabled = null, + ): SubordinateListingsController { + $federationEnabled = $federationEnabled ?? $this->isFederationEnabled; + $this->moduleConfigMock->method('getFederationEnabled')->willReturn($federationEnabled); + + $moduleConfig = $moduleConfig ?? $this->moduleConfigMock; + $clientRepository = $clientRepository ?? $this->clientRepositoryMock; + $routes = $routes ?? $this->routesMock; + + return new SubordinateListingsController( + $moduleConfig, + $clientRepository, + $routes, + ); + } + + public function testCanConstruct(): void + { + $this->assertInstanceOf(SubordinateListingsController::class, $this->sut()); + } + + public function testThrowsIfFederationNotEnabled(): void + { + $this->expectException(OidcServerException::class); + $this->expectExceptionMessage('refused'); + + $this->sut(federationEnabled: false); + } + + public function testCanListFederatedEntities(): void + { + $client = $this->createMock(ClientEntityInterface::class); + $client->method('getEntityIdentifier')->willReturn('entity-id'); + + $federatedEntities = [ + $client, + ]; + + $this->clientRepositoryMock->expects($this->once())->method('findAllFederated') + ->willReturn($federatedEntities); + + $this->routesMock->expects($this->once())->method('newJsonResponse') + ->with([ + $client->getEntityIdentifier(), + ]); + + $this->sut()->list($this->requestMock); + } + + public function testListReturnsErrorOnUnsuportedQueryParameter(): void + { + $this->requestQueryMock->method('all')->willReturn([ + ParamsEnum::EntityType->value => 'something', + ]); + + $this->routesMock->expects($this->once())->method('newJsonErrorResponse') + ->with(ErrorsEnum::UnsupportedParameter->value); + + $this->sut()->list($this->requestMock); + } +} diff --git a/tests/unit/src/Repositories/ClientRepositoryTest.php b/tests/unit/src/Repositories/ClientRepositoryTest.php index d8a493bb..5da45064 100644 --- a/tests/unit/src/Repositories/ClientRepositoryTest.php +++ b/tests/unit/src/Repositories/ClientRepositoryTest.php @@ -391,7 +391,7 @@ public function testCanFindByIdFromCache(): void public function testCanFindByEntityIdentifier(): void { - $client = self::getClient(id: 'clientId', entityId: 'entityId', isFederated: true); + $client = self::getClient(id: 'clientId', entityId: 'entityId'); $this->repository->add($client); $this->clientEntityFactoryMock->expects($this->once())->method('fromState')->willReturn($client); @@ -404,6 +404,46 @@ public function testCanFindByEntityIdentifier(): void $this->assertNull($this->repository->findByEntityIdentifier('nonExistingEntityId')); } + public function testCanFindFederatedByEntityIdentifier(): void + { + $client = self::getClient(id: 'clientId', entityId: 'entityId', isFederated: true, federationJwks: []); + $this->repository->add($client); + + $this->clientEntityFactoryMock->expects($this->once())->method('fromState')->willReturn($client); + + $this->assertSame( + $client, + $this->repository->findFederatedByEntityIdentifier('entityId'), + ); + + $this->assertNull($this->repository->findFederatedByEntityIdentifier('nonExistingEntityId')); + } + + public function testCanNotFindFederatedByEntityIdentifierIfMissingFederationAttributes(): void + { + $client = self::getClient(id: 'clientId', entityId: 'entityId'); + $this->repository->add($client); + + $this->clientEntityFactoryMock->expects($this->atLeastOnce())->method('fromState')->willReturn($client); + + $this->assertSame( + $client, + $this->repository->findByEntityIdentifier('entityId'), + ); + + $this->assertNull($this->repository->findFederatedByEntityIdentifier('entityId')); + } + + public function testCanFindAllFederated(): void + { + $client = self::getClient(id: 'clientId', entityId: 'entityId', isFederated: true, federationJwks: []); + $this->repository->add($client); + + $this->clientEntityFactoryMock->expects($this->atLeastOnce())->method('fromState')->willReturn($client); + + $this->assertCount(1, $this->repository->findAllFederated()); + } + public function testCanFindByEntityIdFromCache(): void { $protocolCacheMock = $this->createMock(ProtocolCache::class); @@ -430,6 +470,7 @@ public static function getClient( ?string $owner = null, ?string $entityId = null, bool $isFederated = false, + ?array $federationJwks = null, ): ClientEntityInterface { return new ClientEntity( identifier: $id, @@ -443,6 +484,7 @@ public static function getClient( authSource: 'admin', owner: $owner, entityIdentifier: $entityId, + federationJwks: $federationJwks, isFederated: $isFederated, ); } From cdc266c9d17af2279a9605728c0833b4d46bb039 Mon Sep 17 00:00:00 2001 From: Marko Ivancic Date: Wed, 2 Apr 2025 14:36:41 +0200 Subject: [PATCH 3/3] Intitial implementation --- README.md | 1 + UPGRADE.md | 1 + templates/clients/includes/form.twig | 4 +++- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 46550471..d4b573c5 100644 --- a/README.md +++ b/README.md @@ -28,6 +28,7 @@ Currently, the following OIDF features are supported: * federation participation limiting based on Trust Marks * endpoint for issuing configuration entity statement (statement about itself) * fetch endpoint for issuing statements about subordinates (registered clients) +* subordinate listing endpoint OIDF support is implemented using the underlying [SimpleSAMLphp OpenID library](https://github.com/simplesamlphp/openid). diff --git a/UPGRADE.md b/UPGRADE.md index e3443186..a4efab84 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -15,6 +15,7 @@ be fetched by RPs, and do the switch between "old" and "new" key pair when you f - Federation participation limiting based on Trust Marks - Endpoint for issuing configuration entity statement (statement about itself) - Fetch endpoint for issuing statements about subordinates (registered clients) + - (from v6.1) Subordinate listing endpoint - Clients can now be configured with new properties: - Entity Identifier - Supported OpenID Federation Registration Types diff --git a/templates/clients/includes/form.twig b/templates/clients/includes/form.twig index c5049b3d..cf085f33 100644 --- a/templates/clients/includes/form.twig +++ b/templates/clients/includes/form.twig @@ -143,7 +143,9 @@

{{ 'OpenID Federation Related Properties'|trans }}

- + + {% trans %}In order for an entity to participate in federation contexts (for example, to be listed as subordinate to this OP), it must have an Entity Identifier and Federation JWKS set. {% endtrans %} +