From fb21c226a59d792d988ac327ce387e05051cb191 Mon Sep 17 00:00:00 2001 From: Marko Ivancic Date: Fri, 28 Mar 2025 16:11:35 +0100 Subject: [PATCH] Enable showing supported claims in OP discovery --- UPGRADE.md | 3 +- config/module_oidc.php.dist | 8 +++- src/ModuleConfig.php | 9 +++++ src/Services/Container.php | 12 +++--- src/Services/OpMetadataService.php | 7 ++++ src/Utils/ClaimTranslatorExtractor.php | 19 ++++++--- tests/unit/src/ModuleConfigTest.php | 11 ++++++ .../src/Services/OpMetadataServiceTest.php | 39 ++++++++++++++++--- .../Utils/ClaimTranslatorExtractorTest.php | 20 ++++++++++ 9 files changed, 110 insertions(+), 18 deletions(-) diff --git a/UPGRADE.md b/UPGRADE.md index 9cc95e34..0484b251 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -35,7 +35,8 @@ https://openid.net/specs/openid-connect-core-1_0.html#RequestObject https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication ## New configuration options - +- (from v6.1) Show `claims_supported` claim in OP Discovery endpoint - you can now choose to show supported claims, +as is recommended by OpenID Connect Discovery specification https://openid.net/specs/openid-connect-discovery-1_0.html. - (optional) Issuer - you can now override the issuer (OP identifier). If not set, it falls back to current scheme, host and optionally a port (as in all previous module versions). - (optional) Protocol caching adapter and its arguments diff --git a/config/module_oidc.php.dist b/config/module_oidc.php.dist index caea6492..b1ca0118 100644 --- a/config/module_oidc.php.dist +++ b/config/module_oidc.php.dist @@ -78,7 +78,8 @@ $config = [ // to use this attribute as the 'sub' claim since it designates unique identifier for the user). ModuleConfig::OPTION_AUTH_USER_IDENTIFIER_ATTRIBUTE => 'uid', - // The default translate table from SAML attributes to OIDC claims. + // The default translate table from SAML attributes to OIDC claims. If you don't want to support specific default + // claim, set it to an empty array. ModuleConfig::OPTION_AUTH_SAML_TO_OIDC_TRANSLATE_TABLE => [ /* * The basic format is @@ -243,6 +244,11 @@ $config = [ // ModuleConfig::OPTION_AUTH_FORCED_ACR_VALUE_FOR_COOKIE_AUTHENTICATION => '0', ModuleConfig::OPTION_AUTH_FORCED_ACR_VALUE_FOR_COOKIE_AUTHENTICATION => null, + // Choose if OP discovery document will include 'claims_supported' claim, which is recommended per OpenID Connect + // Discovery specification https://openid.net/specs/openid-connect-discovery-1_0.html. The list will include all + // claims for which "SAML attribute to OIDC claim translation" has been defined above. + ModuleConfig::OPTION_PROTOCOL_DISCOVERY_SHOW_CLAIMS_SUPPORTED => false, + // Settings regarding Authentication Processing Filters. // Note: OIDC authN state array will not contain all the keys which are available during SAML authN, // like Service Provider metadata, etc. diff --git a/src/ModuleConfig.php b/src/ModuleConfig.php index 1e50289c..8f89fcaa 100644 --- a/src/ModuleConfig.php +++ b/src/ModuleConfig.php @@ -81,6 +81,7 @@ class ModuleConfig final public const OPTION_PROTOCOL_CACHE_ADAPTER_ARGUMENTS = 'protocol_cache_adapter_arguments'; final public const OPTION_PROTOCOL_USER_ENTITY_CACHE_DURATION = 'protocol_user_entity_cache_duration'; final public const OPTION_PROTOCOL_CLIENT_ENTITY_CACHE_DURATION = 'protocol_client_entity_cache_duration'; + final public const OPTION_PROTOCOL_DISCOVERY_SHOW_CLAIMS_SUPPORTED = 'protocol_discover_show_claims_supported'; final public const OPTION_FEDERATION_PARTICIPATION_LIMIT_BY_TRUST_MARKS = 'federation_participation_limit_by_trust_marks'; @@ -504,6 +505,14 @@ public function getProtocolClientEntityCacheDuration(): DateInterval ); } + public function getProtocolDiscoveryShowClaimsSupported(): bool + { + return $this->config()->getOptionalBoolean( + self::OPTION_PROTOCOL_DISCOVERY_SHOW_CLAIMS_SUPPORTED, + false, + ); + } + /***************************************************************************************************************** * OpenID Federation related config. diff --git a/src/Services/Container.php b/src/Services/Container.php index c1d5bf4b..3570fbad 100644 --- a/src/Services/Container.php +++ b/src/Services/Container.php @@ -189,12 +189,6 @@ public function __construct() ); $this->services[TemplateFactory::class] = $templateFactory; - $opMetadataService = new OpMetadataService($moduleConfig); - $this->services[OpMetadataService::class] = $opMetadataService; - - $metadataStorageHandler = MetaDataStorageHandler::getMetadataHandler(); - $this->services[MetaDataStorageHandler::class] = $metadataStorageHandler; - $claimSetEntityFactory = new ClaimSetEntityFactory(); $this->services[ClaimSetEntityFactory::class] = $claimSetEntityFactory; @@ -204,6 +198,12 @@ public function __construct() ))->build(); $this->services[ClaimTranslatorExtractor::class] = $claimTranslatorExtractor; + $opMetadataService = new OpMetadataService($moduleConfig, $claimTranslatorExtractor); + $this->services[OpMetadataService::class] = $opMetadataService; + + $metadataStorageHandler = MetaDataStorageHandler::getMetadataHandler(); + $this->services[MetaDataStorageHandler::class] = $metadataStorageHandler; + $processingChainFactory = new ProcessingChainFactory($moduleConfig); $this->services[ProcessingChainFactory::class] = $processingChainFactory; diff --git a/src/Services/OpMetadataService.php b/src/Services/OpMetadataService.php index 73b3ece7..921724bb 100644 --- a/src/Services/OpMetadataService.php +++ b/src/Services/OpMetadataService.php @@ -6,6 +6,7 @@ use SimpleSAML\Module\oidc\Codebooks\RoutesEnum; use SimpleSAML\Module\oidc\ModuleConfig; +use SimpleSAML\Module\oidc\Utils\ClaimTranslatorExtractor; use SimpleSAML\OpenID\Codebooks\ClaimsEnum; use SimpleSAML\OpenID\Codebooks\TokenEndpointAuthMethodsEnum; @@ -24,6 +25,7 @@ class OpMetadataService */ public function __construct( private readonly ModuleConfig $moduleConfig, + private readonly ClaimTranslatorExtractor $claimTranslatorExtractor, ) { $this->initMetadata(); } @@ -75,6 +77,11 @@ private function initMetadata(): void } $this->metadata[ClaimsEnum::BackChannelLogoutSupported->value] = true; $this->metadata[ClaimsEnum::BackChannelLogoutSessionSupported->value] = true; + + if ($this->moduleConfig->getProtocolDiscoveryShowClaimsSupported()) { + $claimsSupported = $this->claimTranslatorExtractor->getSupportedClaims(); + $this->metadata[ClaimsEnum::ClaimsSupported->value] = $claimsSupported; + } } /** diff --git a/src/Utils/ClaimTranslatorExtractor.php b/src/Utils/ClaimTranslatorExtractor.php index 83e7ba57..599a7b04 100644 --- a/src/Utils/ClaimTranslatorExtractor.php +++ b/src/Utils/ClaimTranslatorExtractor.php @@ -35,7 +35,7 @@ class ClaimTranslatorExtractor protected array $claimSets = []; /** @var string[] */ - protected array $protectedClaims = ['openid', 'profile', 'email', 'address', 'phone']; + protected array $protectedScopes = ['openid', 'profile', 'email', 'address', 'phone']; protected array $translationTable = [ 'sub' => [ @@ -85,13 +85,13 @@ class ClaimTranslatorExtractor 'preferredLanguage', ], 'updated_at' => [ - 'type' => 'int', +// 'type' => 'int', ], 'email' => [ 'mail', ], 'email_verified' => [ - 'type' => 'bool', +// 'type' => 'bool', ], 'address' => [ 'type' => 'json', @@ -105,7 +105,7 @@ class ClaimTranslatorExtractor 'homePhone', ], 'phone_number_verified' => [ - 'type' => 'bool', +// 'type' => 'bool', // Empty ], ]; @@ -198,7 +198,7 @@ public function addClaimSet(ClaimSetEntityInterface $claimSet): self { $scope = $claimSet->getScope(); - if (in_array($scope, $this->protectedClaims, true) && isset($this->claimSets[$scope])) { + if (in_array($scope, $this->protectedScopes, true) && isset($this->claimSets[$scope])) { throw OidcServerException::serverError( sprintf("%s is a protected scope and is pre-defined by the OpenID Connect specification.", $scope), ); @@ -352,4 +352,13 @@ private function extractAdditionalClaims(array $requestedClaims, array $claims): ARRAY_FILTER_USE_KEY, ); } + + /** + * Get supported claims for this OP. This will return all the claims for which the "SAML attribute to OIDC claim + * translation" has been defined in module config, meaning it is expected for OP to release those claims. + */ + public function getSupportedClaims(): array + { + return array_keys(array_filter($this->translationTable)); + } } diff --git a/tests/unit/src/ModuleConfigTest.php b/tests/unit/src/ModuleConfigTest.php index f13c2d41..22faea7d 100644 --- a/tests/unit/src/ModuleConfigTest.php +++ b/tests/unit/src/ModuleConfigTest.php @@ -372,4 +372,15 @@ public function testCanGetProtocolCacheConfiguration(): void $this->assertInstanceOf(DateInterval::class, $this->sut()->getProtocolUserEntityCacheDuration()); $this->assertInstanceOf(DateInterval::class, $this->sut()->getProtocolClientEntityCacheDuration()); } + + public function testCanGetProtocolDiscoveryShowClaimsSupported(): void + { + $this->assertFalse($this->sut()->getProtocolDiscoveryShowClaimsSupported()); + $this->assertTrue( + $this->sut( + null, + [ModuleConfig::OPTION_PROTOCOL_DISCOVERY_SHOW_CLAIMS_SUPPORTED => true], + )->getProtocolDiscoveryShowClaimsSupported(), + ); + } } diff --git a/tests/unit/src/Services/OpMetadataServiceTest.php b/tests/unit/src/Services/OpMetadataServiceTest.php index c56aa882..43ce4fa5 100644 --- a/tests/unit/src/Services/OpMetadataServiceTest.php +++ b/tests/unit/src/Services/OpMetadataServiceTest.php @@ -10,6 +10,8 @@ use SimpleSAML\Module\oidc\Codebooks\RoutesEnum; use SimpleSAML\Module\oidc\ModuleConfig; use SimpleSAML\Module\oidc\Services\OpMetadataService; +use SimpleSAML\Module\oidc\Utils\ClaimTranslatorExtractor; +use SimpleSAML\OpenID\Codebooks\ClaimsEnum; /** * @covers \SimpleSAML\Module\oidc\Services\OpMetadataService @@ -17,6 +19,7 @@ class OpMetadataServiceTest extends TestCase { protected MockObject $moduleConfigMock; + protected MockObject $claimTranslatorExtractorMock; /** * @throws \Exception @@ -46,14 +49,24 @@ public function setUp(): void $signer = $this->createMock(Rsa::class); $signer->method('algorithmId')->willReturn('RS256'); $this->moduleConfigMock->method('getProtocolSigner')->willReturn($signer); + + $this->claimTranslatorExtractorMock = $this->createMock(ClaimTranslatorExtractor::class); } /** * @throws \Exception */ - protected function prepareMockedInstance(): OpMetadataService - { - return new OpMetadataService($this->moduleConfigMock); + protected function sut( + ?ModuleConfig $moduleConfig = null, + ?ClaimTranslatorExtractor $claimTranslatorExtractor = null, + ): OpMetadataService { + $moduleConfig = $moduleConfig ?? $this->moduleConfigMock; + $claimTranslatorExtractor = $claimTranslatorExtractor ?? $this->claimTranslatorExtractorMock; + + return new OpMetadataService( + $moduleConfig, + $claimTranslatorExtractor, + ); } /** @@ -63,7 +76,7 @@ public function testItIsInitializable(): void { $this->assertInstanceOf( OpMetadataService::class, - $this->prepareMockedInstance(), + $this->sut(), ); } @@ -102,7 +115,23 @@ public function testItReturnsExpectedMetadata(): void 'backchannel_logout_supported' => true, 'backchannel_logout_session_supported' => true, ], - $this->prepareMockedInstance()->getMetadata(), + $this->sut()->getMetadata(), + ); + } + + public function testCanShowClaimsSupportedClaim(): void + { + $this->moduleConfigMock->method('getProtocolDiscoveryShowClaimsSupported')->willReturn(true); + $this->claimTranslatorExtractorMock->method('getSupportedClaims')->willReturn(['sample']); + + $sut = $this->sut(); + $this->assertArrayHasKey( + ClaimsEnum::ClaimsSupported->value, + $sut->getMetadata(), + ); + + $this->assertTrue( + in_array('sample', $sut->getMetadata()[ClaimsEnum::ClaimsSupported->value], true), ); } } diff --git a/tests/unit/src/Utils/ClaimTranslatorExtractorTest.php b/tests/unit/src/Utils/ClaimTranslatorExtractorTest.php index 9204479b..f5d4cba5 100644 --- a/tests/unit/src/Utils/ClaimTranslatorExtractorTest.php +++ b/tests/unit/src/Utils/ClaimTranslatorExtractorTest.php @@ -272,4 +272,24 @@ public function testExtractRequestClaimsIdToken(): void $claims = $claimTranslator->extractAdditionalIdTokenClaims($requestClaims, ['displayName' => ['bob']]); $this->assertEquals(['name' => 'bob'], $claims); } + + public function testCanGetSupportedClaims(): void + { + $translate = [ + 'custom' => [ + 'type' => 'int', + 'custom_attr', + ], + ]; + + $this->assertTrue(in_array('custom', $this->mock([], $translate)->getSupportedClaims(), true)); + } + + public function testCanUnsetClaimWhichIsSupportedByDefault(): void + { + $this->assertTrue(in_array('nickname', $this->mock()->getSupportedClaims(), true)); + + $translate = ['nickname' => []]; + $this->assertFalse(in_array('nickname', $this->mock([], $translate)->getSupportedClaims(), true)); + } }