Preserve and expose mdui:UIInfo during authentication and registratio… - #49
Open
kayjoosten wants to merge 16 commits into
Open
Preserve and expose mdui:UIInfo during authentication and registratio…#49kayjoosten wants to merge 16 commits into
kayjoosten wants to merge 16 commits into
Conversation
Prior to this change, the GSSP bundle discarded the mdui:UIInfo extension forwarded by the Gateway on proxy AuthnRequests. Consumers had no way to retrieve per-language service display names from session state, making it impossible to show the correct service name during step-up authentication and registration flows. This change saves mdui:UIInfo to session state in both saveAuthenticationRequest() and saveRegistrationRequest(), and exposes it through getMdui(): ?MduiChunk and hasMdui(): bool on StateHandler and StateHandlerInterface. Behat scenarios cover the present and absent cases for both flows. SSOController is corrected to read the relay state from the query string only, matching the HTTP-Redirect binding specification. Links: - #48 - OpenConext/Stepup-saml-bundle#137
…sting Prior to this change, composer.json required surfnet/stepup-saml-bundle ^7.0 from packagist, which does not yet contain MduiChunk. This caused phpstan to fail with class-not-found errors for all mdui-related symbols, blocking CI. This change adds a VCS repository pointing to the GitHub source and pins the requirement to the feature branch that implements MduiChunk. This is temporary and must be reverted to a packagist version constraint once the saml-bundle pull request is merged and a new release is tagged. Links: - OpenConext/Stepup-saml-bundle#137
kayjoosten
force-pushed
the
feature/issue-48-preserve-mdui-uiinfo
branch
from
July 2, 2026 07:34
e990646 to
ab8ae0d
Compare
Add public getMdui(): ?MduiChunk method to the AuthenticationService interface and implement it in StateBasedAuthenticationService to expose the mdui:UIInfo extension data that is already preserved in the underlying StateHandler.
Mirror the prior AuthenticationService change to the registration side: - Add getMdui(): ?MduiChunk interface method to RegistrationService - Implement in StateBasedRegistrationService, delegating to stateHandler - Add two test cases for presence and absence of MduiChunk
4 tasks
Regression coverage for the saveRequestExtensions() refactor, not part of the mdui feature itself — noted per review to avoid future readers assuming otherwise.
kayjoosten
force-pushed
the
feature/issue-48-preserve-mdui-uiinfo
branch
from
July 3, 2026 07:02
af428f8 to
a6d0c38
Compare
…visories Mirrors the same audit-ignore config added in Stepup-saml-bundle PR #137, needed here too since composer's advisory-blocking applies per root package -- each repo that resolves simplesamlphp/saml2 as part of its own composer install needs its own ignore entries, even though the underlying package and its advisories are identical.
PKSA-rxdv-j1j4-96fj and PKSA-1983-c8jn-trgm targeted simplesamlphp/saml2 versions below 4.16.16 and 4.6.14 respectively. The resolved version here is 4.19.x, well past both fixes, so composer audit no longer flags them and the ignore entries were dead weight - noise that wrongly implies the vulnerabilities are still present and mitigated by us. Kept the two entries that still apply (PKSA-1fc7, PKSA-yk3g), which have no upstream fix and are documented as mitigated/unreachable.
Both remaining ignored advisories (PKSA-1fc7 XPath DoS, PKSA-yk3g HTTP-Artifact TLS confusion) claimed no patched simplesamlphp/saml2 release existed. That's no longer true: comparing the v4.19.2..v4.19.3 tags shows both fixes were backported and released, and our resolved version is 4.19.3. The audit.ignore config key only affects a dev manually running composer audit locally - CI's daily-security-check.yml uses symfonycorp/security-checker-action, a separate tool that doesn't read this config at all, so removing it doesn't change CI behaviour. Composer's own advisory feed still reports a coarse affected-version range that hasn't caught up with the precise per-branch patch, so `composer audit` may show these two as findings again until that feed updates - that's noise in the advisory database, not a real gap.
…tched" This reverts commit ab0e713.
GSSP apps could not read the mdui:UIInfo data that arrives with proxied AuthnRequests, so they had to fall back to less accurate service naming or duplicate request parsing outside the bundle. Persist the UIInfo chunk in session state for both authentication and registration, then expose it through the public service interfaces that consuming applications already depend on. - add coverage for present and absent UIInfo data in both flows - centralize service-name resolution and formatting for consumers - pin the unreleased stepup-saml-bundle branch needed for integration until the upstream change is released Resolves: #48
kayjoosten
force-pushed
the
feature/issue-48-preserve-mdui-uiinfo
branch
from
July 10, 2026 11:09
f7d9790 to
c6f1b2a
Compare
4 tasks
…ng them Control-character stripping ran before whitespace collapsing, so a tab or newline between words (itself a control character) was deleted outright instead of being normalized to a single space, joining adjacent words together. Swap the order so whitespace is collapsed first, then remaining control/format characters are stripped.
kayjoosten
force-pushed
the
feature/issue-48-preserve-mdui-uiinfo
branch
from
July 14, 2026 11:34
6e6ed84 to
049107f
Compare
…iinfo' into feature/issue-48-preserve-mdui-uiinfo
… truncation length
Add an Unreleased CHANGELOG section covering the new getMdui()/hasMdui() accessors and the ServiceNameFormatter/ServiceNameResolver 40-character sanitization contract, so downstream consumers know what they're depending on before this gets tagged.
johanib
reviewed
Jul 30, 2026
| "ext-openssl": "*", | ||
| "beberlei/assert": "^3", | ||
| "surfnet/stepup-saml-bundle": "^7.0", | ||
| "surfnet/stepup-saml-bundle": "dev-feature/issue-48-mdui-chunk as 7.0.99", |
Contributor
There was a problem hiding this comment.
Mandatory: update before merging this pr
| parameters: | ||
| ignoreErrors: | ||
| - | ||
| # phpstan cannot parse the dot in PHPMD annotation names (PHPMD.ExcessiveClassComplexity). |
Contributor
There was a problem hiding this comment.
/** ignore "PHPMD.ExcessiveClassComplexity" */ instead of /** ignore PHPMD.ExcessiveClassComplexity */.
Then this ignore can go
…icate ServiceNameFormatter now lives in Stepup-saml-bundle (which this bundle already depends on) as the single canonical implementation, alongside Stepup-Gateway's now-identical code path. This bundle's own copy had already diverged once (41-char truncation bug vs the RFC's 40).
kayjoosten
added a commit
to OpenConext/Stepup-Gateway
that referenced
this pull request
Jul 30, 2026
…icate Per Johan's review comment: this sanitization logic (truncate to 40 chars + ellipsis, whitespace collapse, control-char strip) existed as three independently-drifting copies (Gateway, Stepup-gssp-bundle, Stepup-Webauthn), one of which truncated to 41 chars instead of 40. Moved the canonical implementation to Stepup-saml-bundle, which both Gateway and gssp-bundle already depend on. gssp-bundle's own copy is removed in a companion PR (OpenConext/Stepup-gssp-bundle#49) so its ServiceNameResolver delegates to the same class - WebAuthn/DemoGSSP/Tiqr need no changes, they already only call gssp-bundle's ServiceNameResolver, never the formatter directly. Temporarily pinned to Stepup-saml-bundle's feature/issue-48-mdui-chunk branch (OpenConext/Stepup-saml-bundle#137, still open) via a VCS repository entry, matching the same pattern already used in gssp-bundle#49. Needs swapping to a tagged release once #137 merges.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…n (#48)
Reads the mdui:UIInfo extension from incoming AuthnRequests and saves it to session state during both saveAuthenticationRequest and saveRegistrationRequest. Downstream GSSP apps can call getMdui() on StateHandler to retrieve per-language display names.
AuthenticationService::getMdui()/RegistrationService::getMdui(), delegating toStateHandler::getMdui(), so downstream GSSP apps (e.g. gssp-example) can read the mdui data through the public service interfaces they actually consume, not justStateHandlerdirectlyDependency / merge order
composer.jsonpinssurfnet/stepup-saml-bundletodev-feature/issue-48-mdui-chunk as 7.0.99via a VCS repository, withminimum-stability: dev. That branch is Stepup-saml-bundle#137, still open. This must not reach a tagged release as-is — theas 7.0.99alias would hide the dev dependency from downstream constraint solvers.Merge order: saml-bundle#137 → tag a release → swap this PR's composer.json to the tagged version → tag this repo → swap the same pin in Stepup-gssp-example#141.
Test plan
surfnet/stepup-saml-bundledev-branch pin for a tagged release before merge