Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The helper bypasses framework-managed certificates, causing HTTPS runs to fail before testing SAML validation.
1 open issue
What changed in this PR
Adds broker-level SAML regression coverage for #54066 in the new test suite.
Changes:
- Introduces an HTTP helper that intercepts and modifies IdP assertions.
- Adds five tests checking rejection of missing subject-confirmation elements or attributes.
| File | Description |
|---|---|
| tests/base/src/test/java/org/keycloak/tests/broker/saml/SamlBrokerResponseTampering.java | Drives login and mutates SAML responses. |
| tests/base/src/test/java/org/keycloak/tests/broker/saml/KcSamlBrokerTest.java | Verifies malformed responses return HTTP 400 and “Invalid requester”. |
Review effort: Balanced
| import javax.swing.text.MutableAttributeSet; | ||
| import javax.swing.text.html.HTML; | ||
| import javax.swing.text.html.HTMLEditorKit; | ||
| import javax.swing.text.html.parser.ParserDelegator; |
There was a problem hiding this comment.
nit: consider jsoup instead of javax.swing.text.html for form parsing
parseForms/attribute here use javax.swing.text.html.HTMLEditorKit.ParserCallback + ParserDelegator to scrape the auto-submit login/SAML forms. That's Swing's GUI HTML renderer, not a general-purpose HTML parser and using it here pulls java.desktop into a server test module for something it wasn't designed to do, and it's quirky/under-documented for edge cases.
The legacy suite already solves this exact problem SamlClient/ModifySamlResponseStepBuilder extracting an auto-submit SAMLResponse form) using jsoup, which is purpose-built for this. It's not currently a dependency of tests/base/tests/pom.xml, but adding it as a test-scope dependency and swapping the ParserDelegator callback for Jsoup.parse(html).select("form") would be a small, mechanical change that doesn't touch the HTTP-client or SAML-mutation logic, and keeps the new testsuite's SAML tooling aligned with the legacy convention it's replacing.
|
Hi @sguilhen , Thanks for the suggestion. I have replaced the Swing parser with jsoup and added it as a test dependency. All five SAML regression tests have completed successfully. |
There was a problem hiding this comment.
Thanks one more time for the contribution, @atiqur-rahman-pro . LGTM!
| <dependency> | ||
| <groupId>org.jsoup</groupId> | ||
| <artifactId>jsoup</artifactId> | ||
| <version>1.23.1</version> |
There was a problem hiding this comment.
What is the reasoning behind the new dependency? Also, it does not follow the same definition as other dependencies; managed dependency + version property.
There was a problem hiding this comment.
Same dependency exists in the legacy testsuite for SAML tests that need to manipulate the response
There was a problem hiding this comment.
Do you mean docs/documentation/tests/pom.xml. This is not the legacy test suite. AFAIK, versions are not explicitly set for individual dependencies either.
If we really need to add this dependency, we should probably talk first with @lhanusov.
There was a problem hiding this comment.
Thanks for the clarification, @pedroigor. I meant tests/base, where SamlBrokerResponseTampering uses jsoup to parse the IdP auto-submit form. I mentioned the legacy suite only as a precedent for parsing and modifying SAML responses, not as the target module.
I understand that adding jsoup to the new test suite needs discussion. @lhanusov, could you advise whether this dependency is acceptable here? If not, I can keep the existing parser approach.
There was a problem hiding this comment.
@pedroigor No, I meant the Arquillian testsuite. jsoup is used there in SamlClient and ModifySamlResponseStepBuilder, which is the exact "intercept the auto-submit form and rewrite SAMLResponse" pattern this PR reproduces, plus LoginBuilder, OtpLoginBuilder, UpdateProfileBuilder, RequiredConsentBuilder and a few tests. It doesn't show up in a pom grep because legacy never declares it, it comes in transitively via arquillian-browser-screenshooter
That said, your concern is fair: tests/base has no jsoup today (I checked with dependency:tree), so this is a genuinely new declared dependency rather than an inherited one, and that is worth @lhanusov's input.
One alternative if we would rather not add it: tests/base already pulls org.htmlunit:htmlunit + neko-htmlunit transitively through keycloak-test-framework-ui, so the form parsing could be done with that instead. It is the same trade legacy made though, depending on something undeclared.
There was a problem hiding this comment.
@sguilhen @atiqur-rahman-pro Sorry, I missed it. I'll check with Lukas.
7a3e03f to
60b8c58
Compare
|
Thanks, @sguilhen, for clarifying the use case. The helper uses jsoup to parse the IdP auto-submit form and modify the SAML response, consistent with the legacy test suite. @pedroigor, I’ve updated the dependency to follow Keycloak’s conventions: its version is defined as a root POM property and managed through dependencyManagement, while tests/base/pom.xml keeps it test-scoped. |
There was a problem hiding this comment.
@sguilhen @atiqur-rahman-pro @pedroigor I suggest to extend the new framework’s SamlClient (class) with the response-processing capabilities needed here, following the legacy processSamlResponse(...).transformDocument(...) pattern. This helper introduces a separate login/form/redirect driver for five assertion mutations, which is not optimal. Jsoup is reasonable for HTML parsing, but the reusable flow handling should live in the SAML test utilities.
Missing pieces for SamlClientBuilder, SamlClient are in Arquillian -> https://github.com/keycloak/keycloak/blob/main/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/SamlClient.java
|
@pskopek may I ask you for your suggestion as well? thanks :+1 |
Thanks for the input, @lhanusov |
@lhanusov this is a good suggestion. |
5c6185e to
3f69a5c
Compare
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.
Copilot review overview
6 open issues
deflateBase64URLEncode(xml)suggests the value is already URL-encoded, but… · NewfindBearerConfirmation(...)can returnnullwhen no Bearer confirmation is present. Callers… · NewtargetAttributeoverrides the original input name, this loop only filters out… · NewtargetAttributeoverrides the original input name, this loop only filters out… · NewcurrentResponseis only closed afterresultTransformer.extract(...)completes successfully. If… · New\"Invalid requester\"makes the test sensitive to message text… · New
Review effort: Lite
e0967ee to
ce189fb
Compare
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.
Copilot review overview
11 open issues
deflateBase64URLEncode(xml)suggests the value is already URL-encoded, but…null(orstepsis empty),currentResponseremainsnulland… · NewHttpClientContext#getRedirectLocations()can retain redirect locations from previous requests… · Newnull(orstepsis empty),currentResponseremainsnulland… · NewcurrentResponseis only closed afterresultTransformer.extract(...)completes successfully. If…targetAttributeoverrides the original input name, this loop only filters out…targetAttributeoverrides the original input name, this loop only filters out…findBearerConfirmation(...)can returnnullwhen no Bearer confirmation is present. Callers…IOException,StandardCharsets,EntityUtils, and… · New\"Invalid requester\"makes the test sensitive to message text…
Review effort: Lite
| private void assertInvalidSamlResponse(Consumer<Document> mutator) throws Exception { | ||
| new SamlClientBuilder(certificates) | ||
| .navigateTo(oauth.loginForm().param("kc_idp_hint", IDP_SAML_ALIAS).build()) | ||
| .processSamlResponse(SamlClient.Binding.POST) | ||
| .targetAttributeSamlRequest() | ||
| .build() | ||
| .login().user(getUserLogin(), getUserPassword()).build() | ||
| .processSamlResponse(SamlClient.Binding.POST) | ||
| .transformDocument(mutator) | ||
| .build() | ||
| .execute(response -> { | ||
| String body = ""; | ||
| try { |
ce189fb to
f038807
Compare
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.
Copilot review overview
15 open issues
IOException,StandardCharsets, andEntityUtilsin… · NewSaml2DocumentTransformer.transformcan returnnull(the chaining code in… · NewdeflateBase64URLEncode(xml)suggests the value is already URL-encoded, but…id(e.g., id=\"username\") but has an empty/missingnameattribute,… · Newnull(orstepsis empty),currentResponseremainsnulland…HttpClientContext#getRedirectLocations()can retain redirect locations from previous requests…null(orstepsis empty),currentResponseremainsnulland…currentResponseis only closed afterresultTransformer.extract(...)completes successfully. If…targetAttributeoverrides the original input name, this loop only filters out…targetAttributeoverrides the original input name, this loop only filters out…findBearerConfirmation(...)can returnnullwhen no Bearer confirmation is present. Callers…IOException,StandardCharsets,EntityUtils, and…\"Invalid requester\"makes the test sensitive to message text…
Review effort: Lite
| private void assertInvalidSamlResponse(Consumer<Document> mutator) throws Exception { | ||
| new SamlClientBuilder(certificates) | ||
| .navigateTo(oauth.loginForm().param("kc_idp_hint", IDP_SAML_ALIAS).build()) | ||
| .processSamlResponse(SamlClient.Binding.POST) | ||
| .targetAttributeSamlRequest() | ||
| .build() | ||
| .login().user(getUserLogin(), getUserPassword()).build() | ||
| .processSamlResponse(SamlClient.Binding.POST) | ||
| .transformDocument(mutator) | ||
| .build() | ||
| .execute(response -> assertEquals(400, response.getStatusLine().getStatusCode(), | ||
| "Malformed broker SAML response should be rejected")); | ||
| } |
Add a test framework SAML client pipeline for mutating broker responses and cover missing or incomplete bearer subject confirmations. Configure managed certificates for HTTPS tests and parse auto-submit forms with jsoup. Closes keycloak#54066 Signed-off-by: Atiqur Rahman <rahman.atiqur.pro@gmail.com>
f038807 to
c086558
Compare
There was a problem hiding this comment.
🟢 Approval recommended
No blocking issues were identified in the focused test infrastructure and regression coverage; final runtime validation remains for CI.
3 open issues
12 resolved since last review
Saml2DocumentTransformer.transformcan returnnull(the chaining code in…IOException,StandardCharsets, andEntityUtilsin…deflateBase64URLEncode(xml)suggests the value is already URL-encoded, but…null(orstepsis empty),currentResponseremainsnulland…HttpClientContext#getRedirectLocations()can retain redirect locations from previous requests…null(orstepsis empty),currentResponseremainsnulland…currentResponseis only closed afterresultTransformer.extract(...)completes successfully. If…targetAttributeoverrides the original input name, this loop only filters out…targetAttributeoverrides the original input name, this loop only filters out…findBearerConfirmation(...)can returnnullwhen no Bearer confirmation is present. Callers…IOException,StandardCharsets,EntityUtils, and…\"Invalid requester\"makes the test sensitive to message text…
Review effort: Balanced
|
Hi @pedroigor, @lhanusov I’ve refactored the tests to use SamlClientBuilder and aligned the flow with the new test framework. Could you please review the latest changes when you have a chance? |
There was a problem hiding this comment.
Thanks, @atiqur-rahman-pro.
Comments addressed as now we have the SamlClient updated and based on the existing capabilities from the old test suite.
Closes #54066
Adds broker-level regression tests for SAML assertions missing a Subject, Bearer SubjectConfirmation, SubjectConfirmationData, NotOnOrAfter, or Recipient.
A focused test helper intercepts the IdP response, modifies the assertion, and submits it to the broker endpoint. Each case verifies rejection with HTTP 400.
Validation
.m2repository; CI will validate it.git diff --checkpassed.