Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions config/packages/engineblock_features.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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%"
1 change: 1 addition & 0 deletions config/packages/parameters.yml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add this to changelog.md

feature_enable_sram_interrupt: false
feature_hide_bookmarkable_url: false

Expand Down
12 changes: 11 additions & 1 deletion library/EngineBlock/Corto/ProxyServer.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -486,7 +487,8 @@ public function sendStepupAuthenticationRequest(
Loa $authnContextClassRef,
NameID $nameId,
bool $isForceAuthn,
Assertion $originalAssertion
Assertion $originalAssertion,
?ServiceProvider $sp = null,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of passing the ServiceProvider down the line, consider passing the string $serviceName.

This makes the code more flexible and testable, as you do not need a instantiated $sp here. This also removes the $container access for the locale.

): void {
$ebRequest = EngineBlock_Saml2_AuthnRequestFactory::createFromRequest(
$spRequest,
Expand Down Expand Up @@ -555,6 +557,13 @@ public function sendStepupAuthenticationRequest(
}
}

$isSendServiceNameConfigured = $features->hasFeature('eb.stepup.send_service_name');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh, why not only rely on isSendServiceNameEnabled?

$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();
Expand Down Expand Up @@ -635,6 +644,7 @@ public function handleStepupAuthenticationCallout(
$nameId,
$sp->getCoins()->isStepupForceAuthn(),
$originalAssertion,
$sp,
);
}

Expand Down
73 changes: 73 additions & 0 deletions src/OpenConext/EngineBlock/Stepup/StepupServiceNameExtension.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
<?php

/**
* Copyright 2026 SURFnet B.V.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

declare(strict_types=1);

namespace OpenConext\EngineBlock\Stepup;

use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider;
use SAML2\DOMDocumentFactory;
use SAML2\Message;
use SAML2\XML\Chunk;

final class StepupServiceNameExtension
{
private const MDUI_NS = 'urn:oasis:names:tc:SAML:metadata:ui';
private const SUPPORTED_LOCALES = ['en', 'nl', 'pt'];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of hardcoding here, read symfony values or values?


public static function add(Message $message, ServiceProvider $sp, string $locale): void
{
if (!in_array($locale, self::SUPPORTED_LOCALES, true)) {
$locale = 'en';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fallback to global/symfony default?

}
$result = self::resolveName($sp, $locale);
if ($result === null && $locale !== 'en') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes the default locale === 'en'. Would be better to do $locale !== $defaultLocale

$result = self::resolveName($sp, 'en');
}
if ($result === null) {
return;
}
[$resolvedLocale, $name] = $result;

$dom = DOMDocumentFactory::create();
$uiInfo = $dom->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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if $ext already contains mdui:UIInfo.
I would guess this is never the case, and this is correct, so no change warranted.

But good to double check.

$message->setExtensions($ext);
}

/**
* @return array{string, string}|null
*/
private static function resolveName(ServiceProvider $sp, string $locale): ?array

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is duplicated business logic?

{
$name = $sp->getMdui()->getDisplayNameOrNull($locale);
if (empty($name)) {
$name = $sp->{'name' . ucfirst($locale)};
}
if (empty($name)) {
return null;
}
return [$locale, $name];
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
<?php

/**
* Copyright 2026 SURFnet B.V.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

declare(strict_types=1);

namespace OpenConext\EngineBlock\Stepup;

use DOMDocument;
use DOMElement;
use DOMNodeList;
use DOMXPath;
use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider;
use OpenConext\EngineBlock\Metadata\Mdui;
use PHPUnit\Framework\TestCase;
use SAML2\AuthnRequest;
use SAML2\XML\Chunk;

class StepupServiceNameExtensionTest extends TestCase
{
public function testAddsDisplayNameForRequestedLocale(): void
{
$sp = $this->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);
}
}