Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A critical filtering bypass and two moderate audience-handling defects remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Excludes disabled clients from OIDC token audiences and client-role claims across login, refresh, and token-exchange flows.
Changes:
- Filters disabled clients during role and audience resolution.
- Adds regression tests for login, refresh, and token exchange.
- Documents the changed behavior and upgrade impact.
| File | Review |
|---|---|
tests/base/src/test/java/org/keycloak/tests/oauth/tokenexchange/StandardTokenExchangeRefreshBaseTokenV2Test.java |
Adds token-exchange refresh coverage. |
tests/base/src/test/java/org/keycloak/tests/oauth/AudienceDisabledClientTest.java |
Adds disabled-client audience tests. |
services/src/main/java/org/keycloak/utils/RoleResolveUtil.java |
Critical: Hardcoded-role mapping can recreate disabled-client entries, bypassing the intended filtering. |
services/src/main/java/org/keycloak/protocol/oidc/refresh/AbstractRefreshTokenProvider.java |
Moderate: Audience filtering occurs after access-token creation, so it does not affect the current refreshed access token. |
services/src/main/java/org/keycloak/protocol/oidc/mappers/AudienceProtocolMapper.java |
Moderate: An empty client audience suppresses the configured custom-audience fallback. |
docs/documentation/upgrading/topics/changes/changes-26_8_0.adoc |
Documents the upgrade behavior. |
docs/documentation/server_admin/topics/clients/oidc/con-audience.adoc |
Documents disabled-client audience handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.webauthn.passwordless.WebAuthnPwdLessPropertyTest#timeout |
c19572f to
bed1049
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Refresh filtering is applied too late, and the intended refresh behavior must be reconciled with issue #53074.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
Resolved since last review (1)
bed1049 to
fdaa986
Compare
fdaa986 to
850c88f
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.adapter.servlet.SAMLServletAdapterTest#employeeSigPostNoIdpKeyTestKeycloak CI - Adapter IT Strict Cookies org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesPostEncTestKeycloak CI - Adapter IT Strict Cookies |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Refresh behavior contradicts the linked issue’s required rejection semantics.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Resolved since last review (1)
850c88f to
dee5268
Compare
Closes keycloak#53074 Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
dee5268 to
4ca4eb5
Compare
| .userId(userId).getEvent(); | ||
| Tokens tokens = sendTokenRequest(loginEvent, userId, "openid profile email audience-scope", "test-app"); | ||
|
|
||
| assertAudiences(tokens.accessToken, "test-app", "service-client", "confidential-cli"); |
There was a problem hiding this comment.
The audience-confidential-cli mapper of the realm is ignored, as confidential-cli is not a client of this realm
Closes #53074
A disabled client is no longer added to the
audclaim of a token:RoleResolveUtilskips the client roles of disabled clients, so the "Audience Resolve" mapper no longer resolves them as an audience. Their roles also are removed fromresource_access."Included Custom Audience" is not affected, as it is a string.
The refresh keeps succeeding, only without that client, consistently with a regular login. Token exchange is still rejected with
invalid_client/Client disabled, as before.