fix(authz): use standard Keycloak token exchange#3754
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughKeycloak is upgraded to 26.4.0 with revised bootstrap variables. Fixture provisioning enables standard token exchange, OAuth tests separate standard and certificate-exchange environments, entity-resolution setup enables unmanaged attributes, and BDD encryption flows use password-grant tokens. ChangesKeycloak token exchange
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant OAuthSuite
participant KeycloakContainer
participant KeycloakTokenEndpoint
OAuthSuite->>KeycloakContainer: start standard or certificate-exchange image
KeycloakContainer-->>OAuthSuite: expose HTTP or HTTPS token endpoint
OAuthSuite->>KeycloakTokenEndpoint: request access token
KeycloakTokenEndpoint-->>OAuthSuite: return token or reject missing DPoP proof
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request modernizes the Keycloak token exchange implementation by moving away from complex, legacy management-permission setups in favor of the standard Keycloak token exchange mechanism. Additionally, it upgrades the development and testing Keycloak images to version 26.2, cleans up environment configuration, and updates documentation to reflect the new provisioning approach. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. The legacy path we leave behind, A standard way we now shall find. With Keycloak bumped to twenty-six, We've fixed the tokens in the mix. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request upgrades Keycloak from version 25.0 to 26.2 across docker-compose, integration tests, and CI actions. It also simplifies the token exchange configuration by enabling the standard token exchange attribute directly on the requester client instead of creating complex fine-grained admin permissions and policies. A critical feedback point was raised regarding the retrieval of the requester client: Keycloak's GetClients API can return multiple clients if the ID is a prefix of another, so relying on the first element of the returned slice could lead to non-deterministic behavior. A code suggestion is provided to iterate and find the exact match.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
jp-ayyappan
changed the title
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Co-authored-by: Dave Mihalcik <dmihalcik@virtru.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/fixtures/keycloak.go (1)
1133-1145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider removing the obsolete
targetClientIDparameter.The standard token exchange flow simplifies configuration by universally enabling exchange for the requester client via the
standard.token.exchange.enabledattribute, eliminating the need for target-specific policies.Retaining
targetClientIDas a parameter and logging it implies that the token exchange is restricted to that target, which is misleading. Consider removing this parameter fromcreateTokenExchangeandcreateTokenExchangeWithTokenManagerto prevent confusion and avoid redundant idempotent calls if multiple targets are assumed.♻️ Proposed refactor
Update the function signatures and internal logic:
-func createTokenExchange(ctx context.Context, connectParams *KeycloakConnectParams, startClientID string, targetClientID string) error { +func createTokenExchange(ctx context.Context, connectParams *KeycloakConnectParams, startClientID string) error { // Create TokenManager and delegate to TokenManager version tm, err := NewTokenManager(ctx, connectParams, nil) if err != nil { return fmt.Errorf("failed to create token manager: %w", err) } - return createTokenExchangeWithTokenManager(ctx, connectParams, tm, startClientID, targetClientID) + return createTokenExchangeWithTokenManager(ctx, connectParams, tm, startClientID) } -func createTokenExchangeWithTokenManager(ctx context.Context, connectParams *KeycloakConnectParams, tm *TokenManager, startClientID string, targetClientID string) error { +func createTokenExchangeWithTokenManager(ctx context.Context, connectParams *KeycloakConnectParams, tm *TokenManager, startClientID string) error { ... - if err := client.UpdateClient(ctx, token.AccessToken, connectParams.Realm, requesterClient); err != nil { - return fmt.Errorf("error enabling standard token exchange for requester client %q targeting %q: %w", startClientID, targetClientID, err) - } + if err := client.UpdateClient(ctx, token.AccessToken, connectParams.Realm, requesterClient); err != nil { + return fmt.Errorf("error enabling standard token exchange for requester client %q: %w", startClientID, err) + } - slog.Info("enabled standard token exchange for client", - slog.String("requester_client_id", startClientID), - slog.String("target_client_id", targetClientID)) + slog.Info("enabled standard token exchange for client", + slog.String("requester_client_id", startClientID)) return nil }And update the call site (around line 383):
- // Create token exchange opentdf->opentdf sdk - if err := createTokenExchange(ctx, &kcConnectParams, opentdfClientID, opentdfSdkClientID); err != nil { + // Enable standard token exchange for opentdf client + if err := createTokenExchange(ctx, &kcConnectParams, opentdfClientID); err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/fixtures/keycloak.go` around lines 1133 - 1145, Remove the obsolete targetClientID parameter from createTokenExchange and createTokenExchangeWithTokenManager, then update all call sites accordingly. Remove target-specific wording and fields from the standard token exchange update error and slog.Info messages, while preserving universal enabling via withStandardTokenExchangeEnabled for the requester client.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/integration/oauth/oauth_test.go`:
- Around line 661-677: In the Keycloak container setup, simplify the
TESTCONTAINERS_PODMAN-based providerType assignment without changing its
Docker/Podman behavior, and replace the manual err check with require.NoError
using the file’s existing testing conventions.
- Line 218: Update the request construction in the OAuth test to use
http.NewRequestWithContext instead of http.NewRequest, passing the appropriate
test context while preserving the existing POST method, endpoint, and form body.
In `@tests-bdd/cukes/steps_encryption.go`:
- Around line 23-25: Add the appropriate //nolint:gosec suppression to the
hardcoded test credential constants userTokenClientSecret and bddUserPassword,
while preserving the existing userTokenClientID declaration and its suppression.
---
Outside diff comments:
In `@lib/fixtures/keycloak.go`:
- Around line 1133-1145: Remove the obsolete targetClientID parameter from
createTokenExchange and createTokenExchangeWithTokenManager, then update all
call sites accordingly. Remove target-specific wording and fields from the
standard token exchange update error and slog.Info messages, while preserving
universal enabling via withStandardTokenExchangeEnabled for the requester
client.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9cdeb37-6637-418f-9c3c-f42f7850bb89
📒 Files selected for processing (7)
docker-compose.yamllib/fixtures/keycloak.golib/fixtures/keycloak_test.gotest/integration/oauth/oauth_test.gotest/start-up-with-containers/action.yamltests-bdd/cukes/resources/keycloak_base.templatetests-bdd/cukes/steps_encryption.go
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
| accessTokenType = "urn:ietf:params:oauth:token-type:access_token" //nolint:gosec // URN identifier, not a credential | ||
| userTokenClientID = "opentdf-sdk" //nolint:gosec // Test credential. | ||
| userTokenClientSecret = "secret" //nolint:gosec // Test credential. | ||
| bddUserPassword = "testuser123" |
There was a problem hiding this comment.
add //nolint:gosec // Test credential.
There was a problem hiding this comment.
sec lint seems to have passed even without this; the bdd tests were failing with this comment in place though
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
service/entityresolution/integration/keycloak_test.go (1)
181-182: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate admin bootstrap environment variables for Keycloak 26.
The PR objective mentions updating to Keycloak 26 bootstrap environment variables, but this container configuration still uses the legacy variables. Keycloak 26 replaces
KEYCLOAK_ADMINandKEYCLOAK_ADMIN_PASSWORDwithKC_BOOTSTRAP_ADMIN_USERNAMEandKC_BOOTSTRAP_ADMIN_PASSWORD. The container may fail to create the test admin user if the legacy variables are ignored.🐛 Proposed fix
- "KEYCLOAK_ADMIN": a.config.AdminUser, - "KEYCLOAK_ADMIN_PASSWORD": a.config.AdminPass, + "KC_BOOTSTRAP_ADMIN_USERNAME": a.config.AdminUser, + "KC_BOOTSTRAP_ADMIN_PASSWORD": a.config.AdminPass,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/entityresolution/integration/keycloak_test.go` around lines 181 - 182, Update the container environment map in the Keycloak test setup to replace KEYCLOAK_ADMIN and KEYCLOAK_ADMIN_PASSWORD with KC_BOOTSTRAP_ADMIN_USERNAME and KC_BOOTSTRAP_ADMIN_PASSWORD, preserving the existing a.config.AdminUser and a.config.AdminPass values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@service/entityresolution/integration/keycloak_test.go`:
- Line 302: Update the json.Unmarshal call in the realm user profile response
test to pass realmUserProfileResp.Body() directly instead of converting the
response body to a string and back to []byte; leave the surrounding upConfig
parsing and error handling unchanged.
---
Outside diff comments:
In `@service/entityresolution/integration/keycloak_test.go`:
- Around line 181-182: Update the container environment map in the Keycloak test
setup to replace KEYCLOAK_ADMIN and KEYCLOAK_ADMIN_PASSWORD with
KC_BOOTSTRAP_ADMIN_USERNAME and KC_BOOTSTRAP_ADMIN_PASSWORD, preserving the
existing a.config.AdminUser and a.config.AdminPass values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c185b9eb-6ce7-472f-a03d-945fea7c7710
📒 Files selected for processing (5)
docker-compose.yamllib/fixtures/keycloak.golib/fixtures/keycloak_test.goservice/entityresolution/integration/keycloak_test.gotest/integration/oauth/oauth_test.go
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
3 participants
Summary
ghcr.io/opentdf/keycloak-standard:26.4.0, includingdocker-compose.yaml, OAuth testcontainers, and ERS Keycloak integration tests.standard.token.exchange.enabledon requester clients and adding a target-client audience mapper for exchanged tokens.token_exchangesfixture behavior and bump the workspace toolchain togo1.25.11for current govulncheck coverage.Testing
go test ./lib/fixtures -run 'TestWith|TestDefaultProtocolMappers|TestExactClient' -count=1DOCKER_HOST=unix://$HOME/.colima/default/docker.sock TESTCONTAINERS_RYUK_DISABLED=true GOWORK=off go test ./oauth -count=1 -vfromtest/integrationDOCKER_HOST=unix://$HOME/.colima/default/docker.sock TESTCONTAINERS_RYUK_DISABLED=true go test ./service/entityresolution/integration -run "TestKeycloak(UserAttributeSubjectMapping|EntityResolutionV2)$" -count=1 -vgo test ./...fromtests-bddGOWORK=off go test -run '^$' ./...fromtest/integration/oauthRelated
Summary by CodeRabbit
New Features
Bug Fixes