Repository navigation
Migrate IdP-hint and nonce-parameter broker tests - #52379
Conversation
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 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/baselayout. - Updated
AbstractKcOidcBrokerTestbroker 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>
86944c5 to
c79426e
Compare
There was a problem hiding this comment.
🟢 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>
c79426e to
d17c781
Compare
Unreported flaky test detectedIf 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#testUsernameOnlyIfEditAllowedKeycloak CI - Forms IT (chrome) |
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>
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>
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:
tests/base/.../tests/broker/oidc/with zero content changes.Unlike the config-only variants in earlier batches, these bring their own
@Testmethods:KcOidcBrokerIdpHintTest(4),KcOidcBrokerHiddenIdpHintTest(1),KcOidcBrokerNonceParameterTest(2, including a genuinely newtestNonceSet- legacy only hadtestNonceNotSet).A structural note worth flagging for review: on the phase-2 branch these classes deliberately implemented only
KcOidcBrokerConfigSupport, notBrokerLoginTest, specifically to avoid inheriting the generic login-flow tests. The current abstract-class hierarchy bakestestLogInAsUserInIDP()/testLoginWithExistingUser()intoAbstractBrokerLoginTest, whichAbstractKcOidcBrokerTestalways 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:KcOidcBrokerIdpHintTestandKcOidcBrokerNonceParameterTestuse 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@Disabledwith a reason, the same pattern already used byKcOidcBrokerSubMatchIntrospectionTest.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 toprotected, and theconfigureBrokerEndpoints()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@DisabledKcOidcBrokerTest,KcOidcBrokerClientSecretBasicAuthTest,KcSamlBrokerTest(phase-1 baseline) - 6/6 greenmvn spotless:checkcleanCloses #52244