Skip to content

Fix: SAML metadata ACS URL ignores zone subdomain when entityBaseURL is set#3915

Open
fhanik wants to merge 2 commits into
developfrom
pr/fix-saml-url-regression-from-pr-3739
Open

Fix: SAML metadata ACS URL ignores zone subdomain when entityBaseURL is set#3915
fhanik wants to merge 2 commits into
developfrom
pr/fix-saml-url-regression-from-pr-3739

Conversation

@fhanik
Copy link
Copy Markdown
Contributor

@fhanik fhanik commented May 15, 2026

Fix: SAML metadata ACS URL ignores zone subdomain when entityBaseURL is set

Background — the regression introduced by #3739

PR #3739 ("Use entityBaseURL for URLs when generating SAML Metadata", merged 2026-02-19, commit 4123092) changed UaaRelyingPartyRegistrationResolver to use the operator-configured login.entityBaseURL as the base for all SAML SP metadata URLs.

The intent was correct — a static, canonical base URL produces stable metadata behind proxies and load balancers. However, the implementation applied entityBaseURL verbatim for all zones, including non-default identity zones accessed via subdomain (e.g. testzone-jnakrm.localhost). Because entityBaseURL is a single static value (http://localhost:8080/uaa), the generated metadata for any non-default zone contained the wrong host:

<!-- Regression: host is localhost, not the zone subdomain host -->
<md:AssertionConsumerService
    Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST"
    Location="http://localhost:8080/uaa/saml/SSO/alias/testzone-jnakrm.integration-saml-entity-id"
    index="0"
    isDefault="true"/>

The correct URL for a subdomain-based zone is:

<md:AssertionConsumerService
    Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST"
    Location="http://testzone-jnakrm.localhost:8080/uaa/saml/SSO/alias/testzone-jnakrm.integration-saml-entity-id"
    index="0"
    isDefault="true"/>

Why the existing test did not catch this

SamlMetadataEndpointMockMvcTests#nonDefaultZoneSamlMetadataXMLValidation — the test that directly exercises non-default zone metadata — only asserted with containsString on the path suffix:

// Before: only checks the alias path, not the full URL — passes even with the wrong host
xpath(".../AssertionConsumerService/@Location")
    .string(containsString("/saml/SSO/alias/testzone-jnakrm.integration-saml-entity-id"))

The path suffix was correct (/saml/SSO/alias/testzone-jnakrm.integration-saml-entity-id); only the host was wrong (localhost vs testzone-jnakrm.localhost). Because the assertion never checked the full URL, the test kept passing throughout the regression.

What this PR fixes

UaaRelyingPartyRegistrationResolver — adds resolveBaseUrl which selects the correct base URL for each access pattern:

Scenario Behaviour
entityBaseURL not set Derive base URL from the incoming HttpServletRequest
Default (UAA) zone, entityBaseURL set Use entityBaseURL as-is
Non-default zone, subdomain access, entityBaseURL set Prepend zone subdomain to entityBaseURL host via UaaUrlUtils.addSubdomainToUrl
Non-default zone, path access (/z/{subdomain}/), any entityBaseURL Always derive from request — entityBaseURL is a static value that cannot encode the dynamic zone path; ZonePathContextRewritingFilter has already embedded the zone in the context path

SamlMetadataEndpointMockMvcTests — three improvements:

  1. The default-zone ACS Location assertion is tightened from containsString to equalTo with the full expected URL, so no future host regression can go undetected.
  2. nonDefaultZoneSamlMetadataXMLValidation now asserts the full ACS Location URL (scheme + host + path), not just the path suffix.
  3. A new SamlMetadataZonePathMockMvcTests nested class covers identity zones accessed via path (/z/{subdomain}/). It is placed in a @Nested @TestPropertySource(properties = {"zones.paths.enabled=true"}) class — the same pattern the other config-variant nested classes use — so the test always runs unconditionally without relying on a Gradle system-property default.

UaaRelyingPartyRegistrationResolverTests — adds two unconditional unit tests (no @EnabledIfZonePathsEnabled gate) covering resolveBaseUrl for both zone access patterns:

  • resolveWhenEntityBaseUrlIsSetAndNonDefaultZonePathIgnoresEntityBaseUrlAndUsesRequestContextPath — uses ZONE_ORIGINAL_CONTEXT_PATH = "" and contextPath = "/z/myzone", which is the realistic value the filter sets in a no-servlet-context-path environment (MockMvc / embedded server). This test is the unconditional safety net: it fails immediately if isZonePathRequest is broken or removed, regardless of whether zones.paths.enabled is set as a system property.
  • resolveWhenEntityBaseUrlIsSetAndNonDefaultZonePathWithServletContextPathIgnoresEntityBaseUrl — covers the production deployment variant (contextPath = "/uaa/z/myzone", ZONE_ORIGINAL_CONTEXT_PATH = "/uaa").

SamlMetadataEndpointKeyRotationTests — fixes a bug introduced by #3739 in the test itself: the resolver was constructed with entityBaseURL = "http://zone-id.localhost:8080/uaa" — the zone-specific URL with the subdomain already baked in. Because resolveBaseUrl now correctly prepends the zone subdomain, this produced a doubled subdomain (http://zone-id.zone-id.localhost:8080/uaa). The test is corrected to use "http://localhost:8080/uaa" — the UAA instance base URL, consistent with what operators configure in login.entityBaseURL and what integration_test_properties.yml contains.

@fhanik fhanik requested review from duanemay and strehle May 15, 2026 16:55
@fhanik
Copy link
Copy Markdown
Contributor Author

fhanik commented May 15, 2026

@georgikpetrov00 - can you review?

@duanemay
Copy link
Copy Markdown
Member

I think we should add a section to UAA-Configuration-Reference.md that has a summary of the table from the initial comment for how entityBaseURL works in different scenarios.

Expanded `entityBaseURL` documentation to detail behavior across different zone access patterns, added examples, and explained implications for path-based zone access with SAML metadata generation.
Copy link
Copy Markdown
Contributor

@georgikpetrov00 georgikpetrov00 left a comment

Choose a reason for hiding this comment

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

My only thoughts are on the 2 comments mentioned. Everything else looks great.

Comment on lines +181 to +183
// Path-based zone: ZonePathContextRewritingFilter has embedded /z/{subdomain}
// into the context path. entityBaseURL is a static value that cannot reflect this
// dynamically, so always derive from the request for this access pattern.
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.

Suggestion: This is already explained in the javadoc above and can be deleted.

Comment on lines +192 to +193
// Subdomain-based zone with entityBaseURL configured: prepend the zone
// subdomain to the host so ACS/SLO endpoints resolve to the right virtual host.
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 one as well

@github-project-automation github-project-automation Bot moved this from Inbox to Pending Merge | Prioritized in Foundational Infrastructure Working Group May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Pending Merge | Prioritized

Development

Successfully merging this pull request may close these issues.

4 participants