Repository navigation
Migrate KcOidcBrokerPassMaxAgeTest and port TestKeycloakOidcIdentityProviderFactory - #52390
Merged
Merged
Conversation
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 KcOidcBrokerPassMaxAgeTest to the new JUnit 5 test framework and ports its custom OIDC IdP factory into tests/custom-providers so the migrated test can validate max_age propagation (and the negative “ignore max_age” path).
Changes:
- Move/port
KcOidcBrokerPassMaxAgeTestintotests/base/.../broker/oidcand adapt it to the new test framework (with two inherited tests intentionally disabled). - Add
TestKeycloakOidcIdentityProviderFactorytotests/custom-providersand register it viaMETA-INF/services. - Update shared broker test infrastructure (broker endpoints matching, user-profile handling, and shared post-login assertions).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerPassMaxAgeTest.java | Removes the legacy JUnit 4 test after migration. |
| tests/custom-providers/src/main/resources/META-INF/services/org.keycloak.broker.provider.IdentityProviderFactory | Registers the ported custom OIDC IdP factory for ServiceLoader discovery. |
| tests/custom-providers/src/main/java/org/keycloak/tests/providers/broker/oidc/TestKeycloakOidcIdentityProviderFactory.java | Adds the custom OIDC IdP factory implementation used by the migrated test. |
| tests/base/src/test/java/org/keycloak/tests/broker/oidc/KcOidcBrokerPassMaxAgeTest.java | New JUnit 5 integration test covering max_age propagation and the enforced re-auth error path. |
| tests/base/src/test/java/org/keycloak/tests/broker/AbstractKcOidcBrokerTest.java | Adjusts broker endpoint configuration matching and widens helper visibility for reuse. |
| tests/base/src/test/java/org/keycloak/tests/broker/AbstractBrokerTest.java | Adds user-profile/first-broker-login flow helpers used by multiple broker tests. |
| tests/base/src/test/java/org/keycloak/tests/broker/AbstractBrokerLoginTest.java | Extracts and reuses the “user created in consumer realm” assertion after broker login. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
sguilhen
force-pushed
the
52249-broker-oidc-pass-max-age
branch
from
September 3, 2026 15:24
ebe3c3c to
6f41402
Compare
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
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
| // Legacy KcOidcBrokerConfigurationWithPassMaxAge removed the "prompt" attribute so the | ||
| // broker does not force an interactive login; without this the provider always shows its | ||
| // login page and the ignore-max-age error path is never exercised. | ||
| .attribute("prompt", null)); |
Comment on lines
+40
to
+42
| return new KeycloakOIDCIdentityProvider(session, new OIDCIdentityProviderConfig(model)) { | ||
|
|
||
| private static final Set<String> usernames = new HashSet<>(); |
| } | ||
| return super.createAuthorizationUrl(request); | ||
| } finally { | ||
| authSession.setClientNote(OIDCLoginProtocol.MAX_AGE_PARAM, maxAge); |
sguilhen
force-pushed
the
52249-broker-oidc-pass-max-age
branch
from
September 4, 2026 11:54
6f41402 to
0dd61f0
Compare
sguilhen
force-pushed
the
52249-broker-oidc-pass-max-age
branch
from
September 4, 2026 12:20
0dd61f0 to
7798db2
Compare
Pure git rename of the legacy pass-max-age broker test 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>
sguilhen
force-pushed
the
52249-broker-oidc-pass-max-age
branch
from
September 4, 2026 15:58
7798db2 to
cfc88df
Compare
…roviderFactory Tests propagation of the max_age parameter for brokered logins using the TestKeycloakOidcIdentityProviderFactory custom provider. The IDP is created with the test-keycloak-oidc providerId at realm setup time since providerId is immutable after creation via the admin API. Ports TestKeycloakOidcIdentityProviderFactory to tests/custom-providers. The legacy original is kept in place (not removed) - it is still used by other, not-yet-migrated legacy tests (KcCustomOidcBrokerTest, KcOidcFirstBrokerLoginTest, PasskeysKcOidcFirstBrokerLoginTest), confirmed via grep. No dependency on the other custom-provider migrations (keycloak#52383, coordinate. Adapted to the merged abstract-class baseline: extends AbstractKcOidcBrokerTest for the shared infra (provider realm uses the default config, only consumer is customized), but overrides and @Disables the two inherited login tests (testLogInAsUserInIDP, testLoginWithExistingUser) - legacy explicitly @ignore'd the same inherited test for this exact configuration (no client-forwarded prompt, custom provider factory), so this mirrors that rather than risk introducing untested, possibly-failing coverage. The two real tests (loginWithExistingUser without the "test" prefix, and testEnforceReAuthenticationWhenMaxAgeIsSet) are independent methods that exercise the max_age-specific flows directly, matching the phase-2 branch this content was sourced from. The consumer IDP clears the prompt attribute set by the shared config so the broker does not force an interactive login, mirroring the legacy KcOidcBrokerConfigurationWithPassMaxAge; without it the ignore-max-age error path is never exercised. Carries forward the fixes from prior batches (independently re-derived per batch since this branch is off upstream/main): visibility widening, configureBrokerEndpoints() placeholder-matching fix, enableUnmanagedAttributes(), disableUpdateProfileOnFirstLogin(), and the assertUserCreatedInConsumerRealm() extraction. Closes keycloak#52249 Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
sguilhen
force-pushed
the
52249-broker-oidc-pass-max-age
branch
from
September 4, 2026 16:13
cfc88df to
876d20d
Compare
pedroigor
approved these changes
Sep 4, 2026
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
KcOidcBrokerPassMaxAgeTestto the new JUnit 5 framework, landing intests/base/.../broker/oidc/. Tests propagation of themax_ageparameter for brokered logins using a custom OIDC IdP factory.Two commits, following the project's move-then-adapt pattern.
Ports
TestKeycloakOidcIdentityProviderFactorytotests/custom-providers. The legacy original is kept in place (not removed) - it's still used by other, not-yet-migrated legacy tests (KcCustomOidcBrokerTest,KcOidcFirstBrokerLoginTest,PasskeysKcOidcFirstBrokerLoginTest), confirmed via grep. No dependency on #52383/#52387 (the other custom-provider migrations) - this provider is never removed from legacy, so there's nothing to coordinate.Adaptation note: the concrete class extends
AbstractKcOidcBrokerTestfor the shared infra.testLogInAsUserInIDPis overridden and@Disabled- legacy@Ignores the same inherited test for this exact configuration (no client-forwarded prompt, custom provider factory).testLoginWithExistingUseris overridden with real max_age-specific login logic instead of being disabled, mirroring legacy's own override of that method - so this class has exactly one disabled test, matching legacy 1-for-1.testEnforceReAuthenticationWhenMaxAgeIsSetis a new, independent test exercising the ignore-max-age error path.Carries forward the fixes from #52372/#52376/#52379/#52382/#52383/#52387/#52388 (independently re-derived per batch, this branch is off
upstream/main): visibility widening,configureBrokerEndpoints()placeholder-matching fix,enableUnmanagedAttributes(),disableUpdateProfileOnFirstLogin(), andassertUserCreatedInConsumerRealm().Testing
mvn test -Dtest=KcOidcBrokerPassMaxAgeTest- 2/2 green, 1 intentionally@Disabled(matches legacy's single@Ignore)KcOidcBrokerTest,KcOidcBrokerClientSecretBasicAuthTest,KcSamlBrokerTest(phase-1 baseline) - 6/6 greenmvn spotless:checkcleanCloses #52249