Repository navigation
Migrate KcSamlSignedDocumentOnlyBrokerTest and SAML package reorg - #52391
Merged
pedroigor merged 2 commits intoSep 4, 2026
Merged
Conversation
Pure git rename of the legacy signed-document-only SAML broker test into the new testsuite saml package. The phase-1 KcSamlBrokerTest already in the new suite also moves from the flat broker package into saml/, establishing the same protocol-subpackage layout the oidc/ tests already use. No content changes; the new-testsuite adaptation follows in the next commit. Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
Configures SAML provider to sign documents only (not assertions) and the consumer IDP to validate signatures but not require signed assertions. The legacy test had a bug where KcSamlSignedBrokerConfiguration inherited its INSTANCE from KcSamlBrokerConfiguration, so the signing config overrides were never applied. The new test correctly applies the configuration via @beforeeach using the provider realm's auto-generated RSA signing certificate. Also restores the legacy signed-AuthnRequest coverage that the original migration dropped: client signature validation on the provider SAML client (saml.client.signature plus the consumer's signing certificate) and wantAuthnRequestsSigned on the consumer IdP. Relocates KcSamlBrokerTest from the flat broker/ package into saml/, establishing the same protocol-subpackage layout the oidc/ tests already use (only a package declaration + import fix - no behavior change). Adapted to the merged abstract-class baseline: the concrete class extends AbstractKcSamlBrokerTest with no realm field overrides (both provider and consumer use the default SAML config), just the @beforeeach that configures document-only signing. Carries forward the fixes from prior batches (independently re-derived per batch since this branch is off upstream/main): enableUnmanagedAttributes(), disableUpdateProfileOnFirstLogin(), and the assertUserCreatedInConsumerRealm() extraction. No SAML-side equivalent of the OIDC configureBrokerEndpoints() clientId-lookup fix is needed here - that bug was specific to the OIDC base's hardcoded CLIENT_ID constant, which this batch doesn't touch. Closes keycloak#52250 Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Migrates the legacy SAML “signed document only” broker test to the new JUnit 5 test framework and reorganizes SAML broker tests into a saml/ subpackage layout consistent with existing OIDC tests.
Changes:
- Removed the legacy
KcSamlSignedDocumentOnlyBrokerTestand added a new JUnit 5 version undertests/base/.../broker/saml/with runtime realm-based signing configuration. - Updated
KcSamlBrokerTestto live in theorg.keycloak.tests.broker.samlpackage. - Extended the new testsuite broker baselines with shared setup helpers (unmanaged attributes; optional first-broker-login profile update disabling; reusable post-login assertion hook).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlSignedDocumentOnlyBrokerTest.java | Removes legacy Arquillian-based JUnit 4 test and its (previously ineffective) configuration inheritance. |
| tests/base/src/test/java/org/keycloak/tests/broker/saml/KcSamlSignedDocumentOnlyBrokerTest.java | Adds JUnit 5 integration test configuring document-only signing and AuthnRequest signing validation using active realm keys. |
| tests/base/src/test/java/org/keycloak/tests/broker/KcSamlBrokerTest.java | Moves the test into the broker.saml package (protocol-subpackage structure). |
| tests/base/src/test/java/org/keycloak/tests/broker/AbstractBrokerTest.java | Adds shared broker-test setup and utilities (unmanaged attributes; disable UPFLM + VERIFY_PROFILE). |
| tests/base/src/test/java/org/keycloak/tests/broker/AbstractBrokerLoginTest.java | Adds a reusable post-login assertion hook and calls it from the default login flow. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+25
to
+31
| ClientRepresentation client = providerRealm.admin().clients().findByClientId(samlClientId).get(0); | ||
| client.getAttributes().put(SamlConfigAttributes.SAML_ASSERTION_SIGNATURE, "false"); | ||
| client.getAttributes().put(SamlConfigAttributes.SAML_SERVER_SIGNATURE, "true"); | ||
| client.getAttributes().put(SamlConfigAttributes.SAML_SIGNATURE_ALGORITHM, "RSA_SHA256"); | ||
| // Require and validate the consumer's signature on incoming AuthnRequests. | ||
| client.getAttributes().put(SamlConfigAttributes.SAML_CLIENT_SIGNATURE_ATTRIBUTE, "true"); | ||
| client.getAttributes().put(SamlConfigAttributes.SAML_SIGNING_CERTIFICATE_ATTRIBUTE, consumerSigningCert); |
pedroigor
approved these changes
Sep 4, 2026
Merged
1 task
Member
|
Looks like this broke compilation of the main branch. |
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
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.
Migrates
KcSamlSignedDocumentOnlyBrokerTestto the new JUnit 5 framework, landing intests/base/.../broker/saml/, and relocatesKcSamlBrokerTestfrom the flatbroker/package intosaml/- establishing the same protocol-subpackage layout theoidc/tests already use (matches what #52372 did for the two phase-1 OIDC classes).Two commits, following the project's move-then-adapt pattern.
Configures the SAML provider to sign documents only (not assertions) and the consumer IdP to validate signatures but not require signed assertions. The legacy test had a bug where
KcSamlSignedBrokerConfigurationinherited itsINSTANCEfromKcSamlBrokerConfiguration, so the signing config overrides were never applied - the new version fixes this by applying the configuration correctly via@BeforeEachusing the provider realm's auto-generated RSA signing certificate. Also restores legacy signed-AuthnRequest coverage (client signature validation on the provider SAML client,wantAuthnRequestsSignedon the consumer IdP).Adapted to the merged abstract-class baseline: the concrete class extends
AbstractKcSamlBrokerTestwith no realm field overrides (both provider and consumer use the default SAML config) - just the@BeforeEachthat configures document-only signing.KcSamlBrokerTest's relocation is a pure package-declaration + import fix, no behavior change.Carries forward
enableUnmanagedAttributes(),disableUpdateProfileOnFirstLogin(), andassertUserCreatedInConsumerRealm()from prior batches (independently re-derived, this branch is offupstream/main). No SAML-side equivalent of the OIDCconfigureBrokerEndpoints()clientId-lookup fix (#52376) is needed - that bug was specific to the OIDC base's hardcodedCLIENT_IDconstant, untouched by this batch.The legacy class has no other references (0
@Testbeyond what's migrated, no subclasses, no suite mentions) - fully removed.Testing
mvn test -Dtest=KcSamlSignedDocumentOnlyBrokerTest,KcSamlBrokerTest- 4/4 greenKcOidcBrokerTest,KcOidcBrokerClientSecretBasicAuthTest(phase-1 baseline) - 4/4 greenmvn spotless:checkcleanCloses #52250