Fix feature flag incorrectly suppressing middleware service name override - #636
Closed
kayjoosten wants to merge 4 commits into
Closed
Conversation
If applied, this commit will forward the mdui:DisplayName service name from incoming AuthnRequests to the GSSP proxy AuthnRequest. Why is this change needed? Prior to this change, Gateway discarded mdui:UIInfo extensions from incoming AuthnRequests, leaving GSSPs without a service name to display during second-factor authentication. EngineBlock sends this data when feature_stepup_send_service_name is enabled (Middleware #589). How does it address the issue? This change parses mdui:DisplayName entries from AuthnRequest extensions in both SSO and SFO flows, stores them in session state via ProxyStateHandler, and forwards them as mdui:UIInfo on the proxy AuthnRequest to the GSSP. Priority: middleware service_name from SP configuration overrides the AuthnRequest value. Gated by enable_service_name_from_saml_authnrequest (false by default). Key design decisions: - DisplayName value object for type safety; serialised as plain arrays in session for safe cross-deploy deserialisation - FeatureConfiguration replaces raw bool constructor args - UiInfoExtensionHelper::parseAndStore deduplicates the SSO/SFO block - ResponseContext::resolveServiceDisplayNames owns the priority logic Links / references: #591
- Clear stored display names when a request carries no mdui:UIInfo, so a service name from a previous request in the same session can no longer leak into the next authentication - Cap parsed display names at 10 entries, langs at 35 bytes and values at 1024 bytes to bound session growth from attacker-sized input - Tolerate missing keys when restoring DisplayName from session data
…ride The isServiceNameFromSamlAuthnRequestEnabled flag wrapped the entire resolveServiceDisplayNames() call in SecondFactorVerificationService, so disabling it also suppressed the middleware-configured service_name override, not just the AuthnRequest-derived fallback it was meant to gate. resolveServiceDisplayNames() now takes an includeAuthnRequestFallback parameter: the middleware override is always considered, while only the AuthnRequest mdui:DisplayName fallback is gated behind the flag.
Contributor
Author
|
Closing - fix has been pushed directly to feature/issue-591-service-name-saml-extension instead (see PR #624). |
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
1 participant
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.
Problem
With the AuthnRequest-service-name feature flag disabled, the middleware-configured
service_nameoverride for a service provider was not shown during second factor verification either, even though the flag is only meant to control the AuthnRequest mdui:DisplayName fallback.Root cause
SecondFactorVerificationService::sendSecondFactorVerificationAuthnRequest()wrapped the entireresolveServiceDisplayNames()call (both the middleware-override branch and the AuthnRequest-fallback branch) in theisServiceNameFromSamlAuthnRequestEnabled()flag check, instead of only gating the AuthnRequest-derived fallback.Fix
ResponseContext::resolveServiceDisplayNames()now accepts anincludeAuthnRequestFallbackparameter. The middleware override is always considered; only the AuthnRequest fallback is skipped when the parameter is false.SecondFactorVerificationServicenow always callsresolveServiceDisplayNames(), passing the feature flag value as this parameter instead of wrapping the whole call.Testing
Added tests confirming the middleware override survives with the flag disabled, and that no fallback occurs without it. Verified against the old code that these tests fail (confirming they reproduce the bug). Full suite (265/265), phpcs and phpmd pass.