Skip to content

Migrate IdP-hint and nonce-parameter broker tests - #52379

Merged
pedroigor merged 2 commits into
keycloak:mainfrom
sguilhen:52244-broker-oidc-idp-hint-nonce
Sep 4, 2026
Merged

pedroigor merged 2 commits into
keycloak:mainfrom
sguilhen:52244-broker-oidc-idp-hint-nonce

Conversation

@sguilhen

@sguilhen sguilhen commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Migrates the IdP-hint and nonce-parameter broker tests to the new JUnit 5 framework, landing in tests/base/.../broker/oidc/.

Two commits, following the project's move-then-adapt pattern:

  1. Pure rename - the 3 legacy Arquillian tests move directly to tests/base/.../tests/broker/oidc/ with zero content changes.
  2. Content adaptation - rewrites all 3 files to the abstract-class baseline.

Unlike the config-only variants in earlier batches, these bring their own @Test methods: KcOidcBrokerIdpHintTest (4), KcOidcBrokerHiddenIdpHintTest (1), KcOidcBrokerNonceParameterTest (2, including a genuinely new testNonceSet - legacy only had testNonceNotSet).

A structural note worth flagging for review: on the phase-2 branch these classes deliberately implemented only KcOidcBrokerConfigSupport, not BrokerLoginTest, specifically to avoid inheriting the generic login-flow tests. The current abstract-class hierarchy bakes testLogInAsUserInIDP()/testLoginWithExistingUser() into AbstractBrokerLoginTest, which AbstractKcOidcBrokerTest always extends - there's no way to pull in just the OIDC config/realm layer without them. I didn't restructure the hierarchy for this (would ripple into every other migrated batch); instead:

  • KcOidcBrokerIdpHintTest and KcOidcBrokerNonceParameterTest use configs compatible with the inherited tests, so they just run as harmless extra coverage.
  • KcOidcBrokerHiddenIdpHintTest's IDP is hidden from the login page, which would break the inherited tests (they click a social button that isn't rendered) - overridden and @Disabled with a reason, the same pattern already used by KcOidcBrokerSubMatchIntrospectionTest.

If a cleaner way to opt out of the login-test layer becomes worth doing (e.g. once more tests hit this), it'd be a separate follow-up touching the shared hierarchy, not something to force into this batch.

Added AbstractBrokerTest.disableUpdateProfileOnFirstLogin() - both nonce tests need to skip the first-broker-login review-profile interruption to test the nonce flow directly. This existed as a phase-1 "ripple fix" on the interfaces-based design but predates the abstract-class rework, so it's missing from the merged base; re-added in the form the phase-2 branch converged on.

Also carries forward two fixes independently re-derived per batch (branch is off upstream/main): createOidcIdentityProvider()/configureConsumerRealm() widened to protected, and the configureBrokerEndpoints() provider-client lookup fix from #52376.

The 3 legacy classes have no other references (no subclasses, no suite mentions) - fully removed.

Testing

  • mvn test -Dtest=KcOidcBrokerIdpHintTest,KcOidcBrokerHiddenIdpHintTest,KcOidcBrokerNonceParameterTest - 13/13 green, 2 intentionally @Disabled
  • Re-ran KcOidcBrokerTest, KcOidcBrokerClientSecretBasicAuthTest, KcSamlBrokerTest (phase-1 baseline) - 6/6 green
  • mvn spotless:check clean

Closes #52244

Copilot AI balanced review requested due to automatic review settings September 3, 2026 14:04
@sguilhen
sguilhen requested review from a team as code owners September 3, 2026 14:04

Copilot AI left a comment

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.

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 broker OIDC IdP-hint and nonce-parameter tests from the legacy Arquillian suite into the new JUnit 5-based test framework under tests/base/.../broker/oidc/, including required base-test utilities and endpoint configuration adjustments.

Changes:

  • Removed 3 legacy Arquillian broker tests and added 3 JUnit 5 equivalents under the new tests/base layout.
  • Updated AbstractKcOidcBrokerTest broker endpoint configuration to locate the provider-side client by seeded redirect URI rather than clientId.
  • Added AbstractBrokerTest.disableUpdateProfileOnFirstLogin() to bypass review-profile interruptions for tests that need uninterrupted broker redirects.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerNonceParameterTest.java Removes legacy nonce broker test (superseded by JUnit 5 version).
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerIdpHintTest.java Removes legacy IdP hint broker test (superseded by JUnit 5 version).
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerHiddenIdpHintTest.java Removes legacy hidden-IdP hint broker test (superseded by JUnit 5 version).
tests/base/src/test/java/org/keycloak/tests/broker/oidc/KcOidcBrokerNonceParameterTest.java Adds JUnit 5 nonce tests and realm configuration for federated ID token claim.
tests/base/src/test/java/org/keycloak/tests/broker/oidc/KcOidcBrokerIdpHintTest.java Adds JUnit 5 IdP hint redirect/error-flow tests.
tests/base/src/test/java/org/keycloak/tests/broker/oidc/KcOidcBrokerHiddenIdpHintTest.java Adds JUnit 5 hidden-IdP hint test and disables inherited social-button login tests.
tests/base/src/test/java/org/keycloak/tests/broker/AbstractKcOidcBrokerTest.java Adjusts endpoint configuration logic and widens helper visibility for migrated tests.
tests/base/src/test/java/org/keycloak/tests/broker/AbstractBrokerTest.java Adds helper to disable review-profile interruptions during first broker login.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Pure git rename of the three legacy IdP-hint and nonce-parameter broker
tests into the new testsuite oidc package. No content changes; the
new-testsuite adaptation follows in the next commit.

Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
Copilot AI review requested due to automatic review settings September 4, 2026 11:54
@sguilhen
sguilhen force-pushed the 52244-broker-oidc-idp-hint-nonce branch from 86944c5 to c79426e Compare September 4, 2026 11:54

Copilot AI left a comment

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.

🟢 Approval recommended

The migration preserves the scoped legacy coverage, adds the requested nonce case, and handles inherited tests appropriately.

Review details
  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Migrate KcOidcBrokerIdpHintTest (4 own tests), KcOidcBrokerHiddenIdpHintTest
(1 own test), and KcOidcBrokerNonceParameterTest (2 own tests, including a
new testNonceSet that verifies the nonce is present in the token response -
legacy only had testNonceNotSet).

Adapted to the merged abstract-class baseline: concrete classes extend
AbstractKcOidcBrokerTest instead of implementing KcOidcBrokerConfigSupport
directly (the phase-2 branch deliberately skipped BrokerLoginTest here since
these classes bring their own @test methods and didn't want the generic
login tests as extra, possibly-conflicting coverage). The current abstract
hierarchy bakes testLogInAsUserInIDP()/testLoginWithExistingUser() into
AbstractBrokerLoginTest, which AbstractKcOidcBrokerTest always extends, so
there's no way to pull in just the OIDC config layer without them:
- KcOidcBrokerIdpHintTest and KcOidcBrokerNonceParameterTest use configs
  compatible with the inherited login tests (default or purely additive), so
  they run as harmless extra coverage.
- KcOidcBrokerHiddenIdpHintTest's IDP is hidden from the login page, which
  would break the inherited tests' logInWithBroker() (clicks a social button
  that isn't rendered) - overridden and @disabled with a reason, mirroring
  the existing KcOidcBrokerSubMatchIntrospectionTest pattern, rather than
  restructuring the shared hierarchy for one test class.

Added AbstractBrokerTest.disableUpdateProfileOnFirstLogin(): both nonce
tests need to skip the first-broker-login review-profile interruption to
test the nonce flow directly. Not present in the merged base (a phase-1
ripple fix that predates the abstract-class rework); re-added here in the
form the phase-2 branch already converged on the phase-1 fix (disables both
the review-profile flow step and the consumer's VERIFY_PROFILE required
action, mirroring the legacy suite which never enabled VERIFY_PROFILE).

Also carries forward two fixes independently re-derived per batch since this
branch is off upstream/main: createOidcIdentityProvider()/
configureConsumerRealm() widened to protected (oidc/ subpackage cross-package
access, same as keycloak#52372/keycloak#52376), and the configureBrokerEndpoints() provider-
client lookup fixed to match by placeholder redirect URI instead of the
fixed CLIENT_ID (same fix as keycloak#52376).

Closes keycloak#52244

Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
@sguilhen
sguilhen force-pushed the 52244-broker-oidc-idp-hint-nonce branch from c79426e to d17c781 Compare September 4, 2026 12:20
@keycloak-github-bot

Copy link
Copy Markdown

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.forms.VerifyProfileTest#testUsernameOnlyIfEditAllowed

Keycloak CI - Forms IT (chrome)

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
...

Report flaky test

@keycloak-github-bot keycloak-github-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@pedroigor
pedroigor merged commit 05784e5 into keycloak:main Sep 4, 2026
94 checks passed
sguilhen added a commit to sguilhen/keycloak that referenced this pull request Sep 4, 2026
Migrate KcOidcBrokerJWETest (1 own test), KcOidcBrokerJWEUserInfoJustEncryptedTest,
and all 8 ECDH-ES variant tests, all landing in oidc/.

Architecture change from the phase-2 branch: the phase-2/interim design used
interface traits (JweBrokerConfigSupport, EcdhEsJweBrokerConfigSupport,
JweUserInfoBrokerTest) because that's how the whole shared support layer was
built at the time. Converted to a 2-layer abstract-class extension instead -
AbstractKcOidcJweBrokerTest extends AbstractKcOidcBrokerTest (JWE key/algorithm
config + the shared testIdentityClaimsFromUserInfoEndpoint()), and
AbstractKcOidcEcdhEsJweBrokerTest extends AbstractKcOidcJweBrokerTest
(ECDH-specific key overrides + abstract getCurve()) - rather than keeping them
as interfaces mixed in alongside BrokerLoginTest/EcdhEsJweBrokerConfigSupport.

Re-examined whether this is genuinely a "multi-axis" case (the documented
reason interfaces are reserved over abstract classes elsewhere in this
migration): it isn't. Every one of the 10 variants wants the full stack -
protocol config, basic login tests, and JWE support - in the same order, with
no case needing to skip a layer (unlike e.g. KcOidcBrokerHiddenIdpHintTest in
chain expresses this without forcing AbstractBrokerTest's protected
fields/methods (oauth, webDriver, logInAsUserInIDP(), etc.) to be widened to
public just so interface default methods could call them - interface methods
are implicitly public, and a protected inherited method does not satisfy a
public abstract interface method, so keeping these as interfaces against the
current abstract-class baseline would have forced that wider visibility
change. Concrete classes drop straight to their per-variant algorithm/curve
overrides (4 one-line methods, or none at all for the RSA base case).

Also removes the legacy AbstractKcOidcBrokerJWEEcdhEsTest shared base (all 8
subclasses migrated) and drops all 10 now-migrated class names from the
legacy fips-suite list (they are not yet added to the new-testsuite FIPS
suites - already tracked separately as deferred pending a test-framework
HTTPS-trust fix, per the migration plan's Infrastructure section).

Found and fixed another real gap in the merged AbstractBrokerTest (same
category as keycloak#52244's disableUpdateProfileOnFirstLogin): enableUnmanagedAttributes()
was dropped during the abstract-class rework. Without it, the declarative user
profile silently drops attributes outside its managed schema - exactly what
this batch's IdP mapper does (maps a custom OIDC claim onto a "user-info" user
attribute), so testIdentityClaimsFromUserInfoEndpoint failed identically
across all 10 variants until this was restored. Caught by running the tests,
not by inspection.

Carries forward the visibility widening + configureBrokerEndpoints() and
assertUserCreatedInConsumerRealm() fixes from keycloak#52372/keycloak#52376/keycloak#52379
(independently re-derived per batch, this branch is off upstream/main).

Closes keycloak#52245

Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
pedroigor pushed a commit that referenced this pull request Sep 4, 2026
Migrate KcOidcBrokerJWETest (1 own test), KcOidcBrokerJWEUserInfoJustEncryptedTest,
and all 8 ECDH-ES variant tests, all landing in oidc/.

Architecture change from the phase-2 branch: the phase-2/interim design used
interface traits (JweBrokerConfigSupport, EcdhEsJweBrokerConfigSupport,
JweUserInfoBrokerTest) because that's how the whole shared support layer was
built at the time. Converted to a 2-layer abstract-class extension instead -
AbstractKcOidcJweBrokerTest extends AbstractKcOidcBrokerTest (JWE key/algorithm
config + the shared testIdentityClaimsFromUserInfoEndpoint()), and
AbstractKcOidcEcdhEsJweBrokerTest extends AbstractKcOidcJweBrokerTest
(ECDH-specific key overrides + abstract getCurve()) - rather than keeping them
as interfaces mixed in alongside BrokerLoginTest/EcdhEsJweBrokerConfigSupport.

Re-examined whether this is genuinely a "multi-axis" case (the documented
reason interfaces are reserved over abstract classes elsewhere in this
migration): it isn't. Every one of the 10 variants wants the full stack -
protocol config, basic login tests, and JWE support - in the same order, with
no case needing to skip a layer (unlike e.g. KcOidcBrokerHiddenIdpHintTest in
chain expresses this without forcing AbstractBrokerTest's protected
fields/methods (oauth, webDriver, logInAsUserInIDP(), etc.) to be widened to
public just so interface default methods could call them - interface methods
are implicitly public, and a protected inherited method does not satisfy a
public abstract interface method, so keeping these as interfaces against the
current abstract-class baseline would have forced that wider visibility
change. Concrete classes drop straight to their per-variant algorithm/curve
overrides (4 one-line methods, or none at all for the RSA base case).

Also removes the legacy AbstractKcOidcBrokerJWEEcdhEsTest shared base (all 8
subclasses migrated) and drops all 10 now-migrated class names from the
legacy fips-suite list (they are not yet added to the new-testsuite FIPS
suites - already tracked separately as deferred pending a test-framework
HTTPS-trust fix, per the migration plan's Infrastructure section).

Found and fixed another real gap in the merged AbstractBrokerTest (same
category as #52244's disableUpdateProfileOnFirstLogin): enableUnmanagedAttributes()
was dropped during the abstract-class rework. Without it, the declarative user
profile silently drops attributes outside its managed schema - exactly what
this batch's IdP mapper does (maps a custom OIDC claim onto a "user-info" user
attribute), so testIdentityClaimsFromUserInfoEndpoint failed identically
across all 10 variants until this was restored. Caught by running the tests,
not by inspection.

Carries forward the visibility widening + configureBrokerEndpoints() and
assertUserCreatedInConsumerRealm() fixes from #52372/#52376/#52379
(independently re-derived per batch, this branch is off upstream/main).

Closes #52245

Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
@sguilhen
sguilhen deleted the 52244-broker-oidc-idp-hint-nonce branch September 21, 2026 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate IdP-hint and nonce-parameter broker tests

3 participants