From c71e61d8d8682d322cc5df25ea89cb85a3dd7c9f Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Fri, 10 Jul 2026 13:10:00 +0200 Subject: [PATCH] Add Stepup service display name Stepup currently receives a callout AuthnRequest without the requesting service's display name. That leaves the Stepup UI showing a generic proxy label instead of the application the user is logging in to, which makes the step-up prompt less clear. Add a dedicated Stepup AuthnRequest extension that emits the service DisplayName in the SAML extensions payload when the feature flag is enabled. The value follows the current EngineBlock locale, falls back to English when needed, and now validates the locale before resolving dynamic name fields so unsupported locales cannot break lookup. Resolves: #2011 --- config/packages/engineblock_features.yaml | 1 + config/packages/parameters.yml.dist | 1 + library/EngineBlock/Corto/ProxyServer.php | 12 +- .../Stepup/StepupServiceNameExtension.php | 73 +++++++ .../TestFeatureConfiguration.php | 1 + .../Features/Context/EngineBlockContext.php | 1 + .../Features/Stepup.feature | 17 ++ .../Stepup/StepupServiceNameExtensionTest.php | 178 ++++++++++++++++++ 8 files changed, 283 insertions(+), 1 deletion(-) create mode 100644 src/OpenConext/EngineBlock/Stepup/StepupServiceNameExtension.php create mode 100644 tests/unit/OpenConext/EngineBlock/Stepup/StepupServiceNameExtensionTest.php diff --git a/config/packages/engineblock_features.yaml b/config/packages/engineblock_features.yaml index 2fbf8dbeda..64db4320b3 100644 --- a/config/packages/engineblock_features.yaml +++ b/config/packages/engineblock_features.yaml @@ -15,5 +15,6 @@ parameters: eb.feature_enable_idp_initiated_flow: "%feature_enable_idp_initiated_flow%" eb.stepup.sfo.override_engine_entityid: "%feature_stepup_sfo_override_engine_entityid%" eb.stepup.send_user_attributes: "%feature_stepup_send_user_attributes%" + eb.stepup.send_service_name: "%feature_stepup_send_service_name%" eb.feature_enable_sram_interrupt: "%feature_enable_sram_interrupt%" eb.hide_bookmarkable_url: "%feature_hide_bookmarkable_url%" diff --git a/config/packages/parameters.yml.dist b/config/packages/parameters.yml.dist index 18212e78e5..dd72f2c37a 100644 --- a/config/packages/parameters.yml.dist +++ b/config/packages/parameters.yml.dist @@ -235,6 +235,7 @@ parameters: feature_enable_idp_initiated_flow: true feature_stepup_sfo_override_engine_entityid: false feature_stepup_send_user_attributes: false + feature_stepup_send_service_name: false feature_enable_sram_interrupt: false feature_hide_bookmarkable_url: false diff --git a/library/EngineBlock/Corto/ProxyServer.php b/library/EngineBlock/Corto/ProxyServer.php index e53e62b962..13eaeac5b8 100644 --- a/library/EngineBlock/Corto/ProxyServer.php +++ b/library/EngineBlock/Corto/ProxyServer.php @@ -27,6 +27,7 @@ use OpenConext\EngineBlock\Metadata\Service; use OpenConext\EngineBlock\Metadata\TransparentMfaEntity; use OpenConext\EngineBlock\Stepup\StepupGsspUserAttributeExtension; +use OpenConext\EngineBlock\Stepup\StepupServiceNameExtension; use OpenConext\EngineBlock\Metadata\X509\KeyPairFactory; use OpenConext\EngineBlockBundle\Authentication\AuthenticationState; use OpenConext\EngineBlockBundle\Exception\UnknownKeyIdException; @@ -486,7 +487,8 @@ public function sendStepupAuthenticationRequest( Loa $authnContextClassRef, NameID $nameId, bool $isForceAuthn, - Assertion $originalAssertion + Assertion $originalAssertion, + ?ServiceProvider $sp = null, ): void { $ebRequest = EngineBlock_Saml2_AuthnRequestFactory::createFromRequest( $spRequest, @@ -555,6 +557,13 @@ public function sendStepupAuthenticationRequest( } } + $isSendServiceNameConfigured = $features->hasFeature('eb.stepup.send_service_name'); + $isSendServiceNameEnabled = $features->isEnabled('eb.stepup.send_service_name'); + + if ($isSendServiceNameConfigured && $isSendServiceNameEnabled && $sp !== null) { + $locale = $container->getLocaleProvider()->getLocale(); + StepupServiceNameExtension::add($sspMessage, $sp, $locale); + } // Link with the original Request $diContainerRuntime = EngineBlock_ApplicationSingleton::getInstance()->getDiContainerRuntime(); @@ -635,6 +644,7 @@ public function handleStepupAuthenticationCallout( $nameId, $sp->getCoins()->isStepupForceAuthn(), $originalAssertion, + $sp, ); } diff --git a/src/OpenConext/EngineBlock/Stepup/StepupServiceNameExtension.php b/src/OpenConext/EngineBlock/Stepup/StepupServiceNameExtension.php new file mode 100644 index 0000000000..c4612ca0fb --- /dev/null +++ b/src/OpenConext/EngineBlock/Stepup/StepupServiceNameExtension.php @@ -0,0 +1,73 @@ +createElementNS(self::MDUI_NS, 'mdui:UIInfo'); + $displayName = $dom->createElementNS(self::MDUI_NS, 'mdui:DisplayName'); + $displayName->setAttributeNS('http://www.w3.org/XML/1998/namespace', 'xml:lang', $resolvedLocale); + $displayName->textContent = $name; + $uiInfo->appendChild($displayName); + + $ext = $message->getExtensions(); + $ext['mdui:UIInfo'] = new Chunk($uiInfo); + $message->setExtensions($ext); + } + + /** + * @return array{string, string}|null + */ + private static function resolveName(ServiceProvider $sp, string $locale): ?array + { + $name = $sp->getMdui()->getDisplayNameOrNull($locale); + if (empty($name)) { + $name = $sp->{'name' . ucfirst($locale)}; + } + if (empty($name)) { + return null; + } + return [$locale, $name]; + } +} diff --git a/src/OpenConext/EngineBlockBundle/Configuration/TestFeatureConfiguration.php b/src/OpenConext/EngineBlockBundle/Configuration/TestFeatureConfiguration.php index 15512f7e62..feaab55b79 100644 --- a/src/OpenConext/EngineBlockBundle/Configuration/TestFeatureConfiguration.php +++ b/src/OpenConext/EngineBlockBundle/Configuration/TestFeatureConfiguration.php @@ -47,6 +47,7 @@ public function __construct() $this->setFeature(new Feature('eb.stepup.sfo.override_engine_entityid', false)); $this->setFeature(new Feature('eb.feature_enable_idp_initiated_flow', true)); $this->setFeature(new Feature('eb.stepup.send_user_attributes', true)); + $this->setFeature(new Feature('eb.stepup.send_service_name', false)); $this->setFeature(new Feature('eb.feature_enable_sram_interrupt', true)); $this->setFeature(new Feature('eb.hide_bookmarkable_url', false)); } diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/EngineBlockContext.php b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/EngineBlockContext.php index 7e94d564b0..f7dba64249 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/EngineBlockContext.php +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/EngineBlockContext.php @@ -688,6 +688,7 @@ public function theAuthnRequestToSubmitShouldMatchXpath($xpath) $xpathObject = new DOMXPath($authnRequest); $xpathObject->registerNamespace('gssp', 'urn:mace:surf.nl:stepup:gssp-extensions'); + $xpathObject->registerNamespace('mdui', 'urn:oasis:names:tc:SAML:metadata:ui'); $nodeList = $xpathObject->query($xpath); if (!$nodeList || $nodeList->length === 0) { diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Stepup.feature b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Stepup.feature index ccb8d42c1d..d13e589d89 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Stepup.feature +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Stepup.feature @@ -291,3 +291,20 @@ Feature: And I pass through EngineBlock And I pass through the IdP Then the received AuthnRequest should match xpath '/samlp:AuthnRequest/samlp:Extensions/gssp:UserAttributes/saml:Attribute[@Name="urn:mace:dir:attribute-def:mail"]/saml:AttributeValue[text()="j.doe@institution-a.example.org"]' + + Scenario: Stepup callout includes service name in mdui:UIInfo when feature is enabled + Given the SP "SSO-SP" requires Stepup LoA "http://dev.openconext.local/assurance/loa2" + And feature "eb.stepup.send_service_name" is enabled + When I log in at "SSO-SP" + And I select "SSO-IdP" on the WAYF + And I pass through EngineBlock + And I pass through the IdP + Then the received AuthnRequest should match xpath '/samlp:AuthnRequest/samlp:Extensions/mdui:UIInfo/mdui:DisplayName' + + Scenario: Stepup callout does not include service name in mdui:UIInfo when feature is disabled + Given the SP "SSO-SP" requires Stepup LoA "http://dev.openconext.local/assurance/loa2" + When I log in at "SSO-SP" + And I select "SSO-IdP" on the WAYF + And I pass through EngineBlock + And I pass through the IdP + Then the received AuthnRequest should not match xpath '/samlp:AuthnRequest/samlp:Extensions/mdui:UIInfo/mdui:DisplayName' diff --git a/tests/unit/OpenConext/EngineBlock/Stepup/StepupServiceNameExtensionTest.php b/tests/unit/OpenConext/EngineBlock/Stepup/StepupServiceNameExtensionTest.php new file mode 100644 index 0000000000..5f460200eb --- /dev/null +++ b/tests/unit/OpenConext/EngineBlock/Stepup/StepupServiceNameExtensionTest.php @@ -0,0 +1,178 @@ +spWithNames('My Service EN', 'Mijn Service NL'); + $request = new AuthnRequest(); + + StepupServiceNameExtension::add($request, $sp, 'nl'); + + $nodes = $this->queryXpath($request, './/mdui:DisplayName[@xml:lang="nl"]'); + $this->assertSame(1, $nodes->length); + $this->assertSame('Mijn Service NL', $nodes->item(0)->textContent); + } + + public function testFallsBackToEnglishWhenRequestedLocaleHasNoName(): void + { + $sp = $this->spWithNames('My Service EN'); + $request = new AuthnRequest(); + + StepupServiceNameExtension::add($request, $sp, 'nl'); + + $nodes = $this->queryXpath($request, './/mdui:DisplayName[@xml:lang="en"]'); + $this->assertSame(1, $nodes->length); + $this->assertSame('My Service EN', $nodes->item(0)->textContent); + } + + public function testAddsNoExtensionWhenNeitherLocaleNorEnglishHasName(): void + { + $sp = $this->spWithNames(); + $request = new AuthnRequest(); + + StepupServiceNameExtension::add($request, $sp, 'nl'); + + $ext = $request->getExtensions(); + $this->assertArrayNotHasKey('mdui:UIInfo', $ext); + } + + public function testMduiDisplayNameTakesPrecedenceOverFlatField(): void + { + $sp = $this->spWithMduiDisplayName('en', 'Mdui Service Name'); + $sp->nameEn = 'Flat Field Name'; + $request = new AuthnRequest(); + + StepupServiceNameExtension::add($request, $sp, 'en'); + + $nodes = $this->queryXpath($request, './/mdui:DisplayName[@xml:lang="en"]'); + $this->assertSame(1, $nodes->length); + $this->assertSame('Mdui Service Name', $nodes->item(0)->textContent); + } + + public function testLocaleTagMatchesActualContentLocale(): void + { + $sp = $this->spWithNames('Only English Name'); + $request = new AuthnRequest(); + + StepupServiceNameExtension::add($request, $sp, 'nl'); + + $nlNodes = $this->queryXpath($request, './/mdui:DisplayName[@xml:lang="nl"]'); + $enNodes = $this->queryXpath($request, './/mdui:DisplayName[@xml:lang="en"]'); + $this->assertSame(0, $nlNodes->length, 'Should not add nl tag when using en fallback'); + $this->assertSame(1, $enNodes->length, 'Should add en tag matching the actual content locale'); + } + + public function testFallsBackToFlatFieldWhenMduiDisplayNameIsEmpty(): void + { + $mduiJson = '{"DisplayName":{"name":"DisplayName","values":{"en":{"value":"","language":"en"}}},' + . '"Description":{"name":"Description"},"Keywords":{"name":"Keywords"},' + . '"Logo":{"name":"Logo"},"PrivacyStatementURL":{"name":"PrivacyStatementURL"}}'; + $sp = new ServiceProvider('https://sp.example.org', Mdui::fromJson($mduiJson)); + $sp->nameEn = 'Flat Field Name'; + $request = new AuthnRequest(); + + StepupServiceNameExtension::add($request, $sp, 'en'); + + $nodes = $this->queryXpath($request, './/mdui:DisplayName[@xml:lang="en"]'); + $this->assertSame(1, $nodes->length); + $this->assertSame('Flat Field Name', $nodes->item(0)->textContent); + } + + public function testPreservesExistingExtensions(): void + { + $sp = $this->spWithNames('My Service'); + $request = new AuthnRequest(); + $request->setExtensions(['existing:Extension' => new Chunk( + (new DOMDocument())->createElement('existing:Extension') + )]); + + StepupServiceNameExtension::add($request, $sp, 'en'); + + $ext = $request->getExtensions(); + $this->assertArrayHasKey('existing:Extension', $ext); + $this->assertArrayHasKey('mdui:UIInfo', $ext); + } + + public function testUnsupportedLocaleFallsBackToEnglish(): void + { + $sp = $this->spWithNames('My Service EN', 'Mijn Service NL'); + $request = new AuthnRequest(); + + StepupServiceNameExtension::add($request, $sp, 'en/../evil'); + + $nodes = $this->queryXpath($request, './/mdui:DisplayName[@xml:lang="en"]'); + $this->assertSame(1, $nodes->length); + $this->assertSame('My Service EN', $nodes->item(0)->textContent); + } + + private function spWithNames(string $nameEn = '', string $nameNl = '', string $namePt = ''): ServiceProvider + { + $sp = new ServiceProvider('https://sp.example.org'); + $sp->nameEn = $nameEn; + $sp->nameNl = $nameNl; + $sp->namePt = $namePt; + return $sp; + } + + private function spWithMduiDisplayName(string $locale, string $displayName): ServiceProvider + { + $mduiJson = sprintf( + '{"DisplayName":{"name":"DisplayName","values":{"%s":{"value":"%s","language":"%s"}}},' + . '"Description":{"name":"Description"},"Keywords":{"name":"Keywords"},' + . '"Logo":{"name":"Logo"},"PrivacyStatementURL":{"name":"PrivacyStatementURL"}}', + $locale, + $displayName, + $locale + ); + return new ServiceProvider('https://sp.example.org', Mdui::fromJson($mduiJson)); + } + + private function getExtensionElement(AuthnRequest $request): DOMElement + { + $ext = $request->getExtensions(); + $this->assertArrayHasKey('mdui:UIInfo', $ext); + $chunk = $ext['mdui:UIInfo']; + $this->assertInstanceOf(Chunk::class, $chunk); + return $chunk->getXML(); + } + + private function queryXpath(AuthnRequest $request, string $expression): DOMNodeList + { + $el = $this->getExtensionElement($request); + $xpath = new DOMXPath($el->ownerDocument); + $xpath->registerNamespace('mdui', 'urn:oasis:names:tc:SAML:metadata:ui'); + $xpath->registerNamespace('xml', 'http://www.w3.org/XML/1998/namespace'); + return $xpath->query($expression, $el); + } +}