feat: read mdui:UIInfo from AuthnRequest and forward service name to GSSP by kayjoosten · Pull Request #624 · OpenConext/Stepup-Gateway

kayjoosten · 2026-06-29T09:32:20Z

Summary

Closes #591.

When EngineBlock sends a mdui:UIInfo extension in the AuthnRequest, Gateway now reads the mdui:DisplayName values and forwards the service name in a mdui:UIInfo extension on the proxy AuthnRequest sent to the GSSP.

Feature-flagged via enable_service_name_from_saml_authnrequest (disabled by default, configure in parameters.yaml)
Applies to both SSO (GatewayBundle) and SFO (SecondFactorOnlyBundle) flows
Priority: middleware service_name (from SP configuration) overrides mdui:DisplayName from the AuthnRequest

Priority table

Middleware `service_name`	AuthnRequest `mdui:DisplayName`	Result
No	Yes	AuthnRequest value forwarded
Yes	Yes	Middleware value used
No	No	No extension added
Yes	No	Middleware value forwarded

Changes

UiInfoExtensionHelper — parses and builds mdui:UIInfo SAML extensions
DisplayName — value object for type-safe lang/value pairs (serializes as plain arrays in session)
FeatureConfiguration — named accessor replacing raw bool constructor args
ProxyStateHandler — stores/retrieves DisplayName[] in session state
ResponseContext::resolveServiceDisplayNames() — applies priority logic with real SP data
SSO + SFO LoginService — parses UIInfo from incoming request via UiInfoExtensionHelper::parseAndStore
SecondFactorVerificationService — forwards resolved display names to GSSP proxy AuthnRequest
parameters.yaml.dist — documents the new flag

Test plan

composer test — 258 tests pass
composer phpstan — clean
composer phpcs — clean
composer phpmd — clean
composer phplint — clean
New tests: ProxyStateHandlerDisplayNameTest, ResponseContextResolveDisplayNamesTest, extended UiInfoExtensionHelperTest, SFO branch in SecondFactorVerificationServiceTest
Behat: requires Docker environment (devconf/stepup)
Enable flag in parameters.yaml and verify service name appears in GSSP AuthnRequest with EngineBlock sending feature_stepup_send_service_name=true

johanib

I lost the overview of what we discussed in this feature. So this review is a bit messy. I think the main question is:

Where can I find the main, definitive spec we agreed on for the service name feature?

…ding If applied, this commit will apply the mdui:UIInfo display-name sanitization and length rules (MAX_CHARACTERS = 40) to every display name forwarded to a GSSP, regardless of source, plus a couple of smaller review-driven cleanups. Why is this change needed? UiInfoExtensionHelper accepted display names up to 1024 characters with no whitespace/control-character sanitization, but these values are forwarded into SMS/Tiqr push notifications, which have much tighter length limits (160/70 characters). The mdui spec calls for 40-character values (39 chars + ellipsis when truncated), trimmed and whitespace-collapsed. Also, 'second_factor_only.response_context' was duplicated as a magic string in two places in SecondFactorVerificationService. How does it address the issue? Adds sanitizeValue() to UiInfoExtensionHelper, applied once in buildExtensionsWithUiInfo so it covers both AuthnRequest-parsed and middleware-configured display names: strips control/format characters, collapses whitespace, trims, and truncates to 40 characters with a trailing ellipsis. Introduces a SFO_RESPONSE_CONTEXT_SERVICE_ID constant to replace the duplicated string literal. Adds a short comment on ResponseContext's 'en' fallback clarifying there's no request-scoped locale available for middleware-configured service names. Links / references: #624

kayjoosten · 2026-07-27T12:42:44Z

#587. Now linked in code next to MAX_CHARACTERS. MAX_VALUE_LENGTH is SAML2's own bound, not this RFC. MAX_LANG_LENGTH and MAX_DISPLAY_NAMES aren't spec derived at all, just internal limits. Also commented now.

Fix xml:lang correctness, feature-flag scope, and defensive edge cases raised on the service-name-during-2FA review: - resolveServiceDisplayNames() tagged the middleware service_name override with the session locale verbatim (e.g. "en_GB", "nl_NL"), which is Symfony's locale format, not a valid xml:lang value. Reduce it to its primary language subtag ("en", "nl") instead, and fall back to 'en' on an empty string as well as a null locale. - The same override branch ignored isServiceNameFromSamlAuthnRequestEnabled entirely, so disabling the flag only suppressed the AuthnRequest-derived fallback while middleware-configured names kept being forwarded to the GSSP regardless. Gate both branches behind the flag and rename the parameter (includeAuthnRequestFallback -> serviceNameFeatureEnabled) to match what it now actually controls. - buildExtensionsWithUiInfo() wrote every entry's lang into the outgoing SAML extension unchecked, including a path that ultimately traces back to an unsigned client locale cookie. Apply the same lang guard already used on the incoming parse side, and avoid emitting an empty UIInfo chunk when every entry gets filtered out. - Document that MAX_LANG_LENGTH/MAX_DISPLAY_NAMES are Gateway-internal defensive bounds rather than spec-mandated values, and link MAX_CHARACTERS/MAX_VALUE_LENGTH to their actual sources (the RFC vs. SAML 2.0 itself), per confusion raised outside the PR thread.

…ch to a locale map If applied, this commit will make the GSSP registration/vetting proxy path (LoginService::singleSignOn) behave correctly when the service name feature flag is disabled, stop 500ing on SPs unknown to Middleware, and consume the SP-configured service name as a real locale-to-name map instead of one untranslated string. Why is this change needed? Prior to this change, four separate gaps existed, all found via a live devconf test and a follow-up code/PR-comment audit: - With the feature flag off, a pre-existing mdui:UIInfo already on the incoming AuthnRequest was still forwarded to the GSSP untouched — the flag only gated Gateway's own additions, not pass-through. - LoginService::resolveServiceDisplayNames() called SamlEntityService::getServiceProvider(), which throws for an entity id present in the local allowed_sps YAML but absent from Middleware's database, turning that combination into an uncaught HTTP 500 on the registration path. - Middleware's service_name field was a single nullable string, not the locale-to-name map the design RFC (#587) specifies. Gateway worked around the missing locale data by tagging the one string with whatever locale the current session happened to be in, so a Dutch user would see the English name marked xml:lang="nl" — the tag changed, the text never did. - None of this was ever checked against the RFC in review: the PR that introduced the string shape (Stepup-Middleware#606) has zero review comments, and the one reviewer across the whole feature said on the record he had lost track of where the spec even was. How does it address the issue? - UiInfoExtensionHelper::stripUiInfo() removes any UIInfo chunk from the original request's Extensions when the flag is off, leaving other chunks (e.g. gssp:UserAttributes) untouched. - LoginService::resolveServiceDisplayNames() now checks the existing non-throwing SamlEntityService::hasServiceProvider() before calling the throwing getServiceProvider(), falling back to the original request's own mdui data when the SP has no Middleware entry. - ServiceProvider::getServiceName(): ?string is replaced by getServiceNames(): array (locale => name), fed straight through from Middleware's now-map-shaped service_name field (see the paired Stepup-Middleware commit). ResponseContext and LoginService both build one mdui:DisplayName per configured locale, each normalized via the existing UiInfoExtensionHelper::normalizeToPrimarySubtag(), removing the session/request-locale workaround entirely. Links / references: #587 (RFC) #624 (PR)

If applied, this commit will make Gateway read mdui:UIInfo DisplayName data from an incoming AuthnRequest (or fall back to Middleware's per-locale service_name map), sanitize and forward it as mdui:UIInfo on the outgoing GSSP request for both the second-factor and the registration/vetting proxy paths, gated behind a feature flag. Why is this change needed? Prior to this change, GSSPs (Webauthn, tiqr) had no reliable way to show the end user which SP they were authenticating or registering for. The only identifying information available was the SP's technical entity id, which is meaningless to end users and forces GSSPs to maintain their own SP display-name lookups. How does it address the issue? - UiInfoExtensionHelper reads mdui:UIInfo off the original SAML Extensions, or builds it from Middleware's ServiceProvider service_name map (locale => name) when none is present, normalizing locale subtags and truncating/sanitizing names to the SAML mdui spec length before forwarding. - LoginService (SecondFactorOnly and SamlStepupProvider bundles) wires SamlEntityService and FeatureConfiguration in and calls the helper when building the outgoing GSSP AuthnRequest for both the normal SSO path and the registration/vetting path (LoginService::singleSignOn), using the non-throwing hasServiceProvider() check so SPs unknown to Middleware degrade to the original request's own mdui data instead of a 500. - With the feature flag off, stripUiInfo() removes any pre-existing UIInfo chunk from the forwarded Extensions rather than passing it through untouched, so the flag genuinely gates the feature end to end. - ProxyStateHandler and ResponseContext carry the resolved DisplayName(s) through the proxy round trip back to the original requester. Links / references: #587 (RFC) #624 (PR)

johanib

It would be very valuable to have these mdui scenarios, happy + non happy covered in behat features.

johanib · 2026-07-30T08:44:09Z

+        $activeResponseContext = $responseContextServiceId === self::SFO_RESPONSE_CONTEXT_SERVICE_ID
+            ? $this->sfoResponseContext
+            : $this->responseContext;
+        $locale = $this->requestStack->getCurrentRequest()?->getLocale() ?? 'en';


Does getLocale return en_GB or en?

johanib · 2026-07-30T11:34:53Z

+     * characters, and truncates to MAX_CHARACTERS (with an ellipsis) so the display name fits the
+     * SMS/Tiqr push notifications it's forwarded into.
+     */
+    private function sanitizeValue(string $value): string


Why re-implement when we've got src/Service/ServiceName/ServiceNameFormatter.php in https://github.com/OpenConext/Stepup-gssp-bundle/pull/49/changes?

the Stepup-gssp-bundle is not in Gateway atm. I dont think its worth adding for 1 sanitizer. But you are right its duplicate code.

What we could do is move this logic to the saml-bundle that is in the gateway. Or leave it as is. What do you think?

johanib

If the latest dots on the i are finished (borrow from stepup-saml bundle and cleanup comments, I think we are good.

Test coverage is very good. 👍

johanib · 2026-07-30T12:14:42Z

            $configuration['setSsoCookieOn2fa'] = $decodedConfiguration['set_sso_cookie_on_2fa'];
        }
+        // service_name is expected to be a locale => name map. Middleware's own validator
+        // (Stepup-Middleware#606) currently only enforces "nullable string", so a plain


Please cleanup comments

johanib · 2026-07-30T12:15:56Z

+    // Defensive parsing bounds, not derived from any spec — cap how much an incoming
+    // AuthnRequest's UIInfo extension can make this parser do.
+    private const MAX_DISPLAY_NAMES = 10;
+    private const MAX_LANG_LENGTH = 35;


Check if can be replaced by by stepup-saml-bundle shared logic

The Remote IdP forwards a service name for the SP in mdui:UIInfo, but that name never reached the GSSP during step-up: the GSSP only saw the SP's own metadata, not what the Remote IdP resolved for this specific AuthnRequest. Users switching locale or hitting an SP with no configured display name would see an inconsistent or missing service name during 2FA. ServiceDisplayNameResolver and UiInfoExtensionMapper now resolve and propagate that name end-to-end: SamlEntity/ServiceProvider carry the resolved display name, DisplayName is a value object enforcing locale fallback, and it's threaded through ProxyStateHandler into the GSSP's AuthnRequest as mdui:UIInfo. normalizeLocale() intentionally drops the region subtag (e.g. en_GB becomes en): our locale configuration only ships base-language translations, so keeping the region would just cause lookups to miss and fall through anyway. Replaced a locally duplicated formatter with stepup-saml-bundle's ServiceNameFormatter once the dependency added one, removing the duplication introduced earlier in this same change; the resulting phpstan finding (a pre-existing issue newly visible after the bump) is baselined rather than fixed here since it's unrelated to this feature. Adds Behat coverage for the mdui service-name feature and the FeatureToggle test fixture it relies on.

The init script skipped install when vendor/ was already present, which meant a stale vendor/ from caching could silently mask dependency drift instead of failing CI.

surfnet/stepup-saml-bundle 7.1.0-beta1 (PR #137's branch tip) is now tagged, so this no longer needs to float on a dev-branch requirement. Verified: composer test (287/287), phpstan, phpcs all pass.

…ion, and session cookie SameSite If applied, this commit will make middleware-configured service names work regardless of the AuthnRequest feature flag, resolve a name even when no exact/English locale match exists, sanitize display names as soon as they're parsed rather than only on write, and stop the Gateway session from silently dropping on the IdP round trip. Why is this change needed? Prior to this change, the feature flag guarded both the SP-controlled AuthnRequest source and the Middleware-configured source, so a disabled flag (the default) suppressed the trusted source too. selectByLocale() returned null when no exact or English match existed, even though sources now emit a single already-resolved name with no "wrong language" risk to protect against. UiInfoExtensionMapper::read() only sanitized on write, leaving unsanitized values reachable from any session read path added later. Most importantly, Gateway's prod session config never set cookie_samesite, so it fell back to the browser default (Lax) -- Lax cookies aren't sent on the cross-site POST the remote IdP makes back to consume-assertion, so the session written during singleSignOn() was invisible on return, crashing GatewayController::respond() with a null service name looked up from an empty session. How does it address the issue? Moves the flag check to guard only the AuthnRequest branch in ServiceDisplayNameResolver, leaving the middleware lookup unconditional. Adds a last-resort fallback to the first available name in selectByLocale(). Extracts per-node sanitize-and-build logic from UiInfoExtensionMapper::read() into its own method so sanitization runs at parse time without exceeding the cyclomatic complexity threshold. Sets cookie_samesite: none on the main session config block (it was previously only set under the smoketest override), fixing the actual crash. Updates the SamlStepupProviderBundle tests and the service-name.feature Behat scenarios that encoded the old, incorrect behavior. Links / references: Stepup-Gateway#624, RFC #587

FILTER_DEFAULT is deprecated as of PHP 8.5 in favour of FILTER_UNSAFE_RAW (identical behaviour: no filtering applied). Removes the stale phpstan baseline ignore that no longer matched on CI's PHP version.

kayjoosten force-pushed the feature/issue-591-service-name-saml-extension branch from 0e6e735 to c1b3e59 Compare June 29, 2026 11:28

kayjoosten requested a review from johanib June 29, 2026 11:56

kayjoosten force-pushed the feature/issue-591-service-name-saml-extension branch from 6ff6b05 to 787a33b Compare July 10, 2026 11:09

kayjoosten mentioned this pull request Jul 14, 2026

Fix feature flag incorrectly suppressing middleware service name override #636

Closed

kayjoosten force-pushed the feature/issue-591-service-name-saml-extension branch 2 times, most recently from c1546f9 to 9641bcb Compare July 14, 2026 11:34

johanib reviewed Jul 23, 2026

View reviewed changes

kayjoosten force-pushed the feature/issue-591-service-name-saml-extension branch from 1c0ab52 to 4877cdc Compare July 28, 2026 08:02

johanib reviewed Jul 28, 2026

View reviewed changes

Comment thread src/Surfnet/StepupGateway/SecondFactorOnlyBundle/Service/Gateway/LoginService.php

Comment thread src/Surfnet/StepupGateway/SamlStepupProviderBundle/Service/Gateway/LoginService.php Outdated

johanib reviewed Jul 29, 2026

View reviewed changes

Comment thread src/Surfnet/StepupGateway/GatewayBundle/Saml/UiInfoExtensionHelper.php Outdated

Comment thread src/Surfnet/StepupGateway/GatewayBundle/Saml/UiInfoExtensionHelper.php Outdated

Comment thread src/Surfnet/StepupGateway/GatewayBundle/Entity/SamlEntity.php Outdated