OSAC-953: Update AuthConfig to use organization groups - #666
Conversation
|
@CrystalChun: This pull request references OSAC-953 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughJWT organization claims now accept array or object shapes, tenant and org-group normalization follows that shape, Projects RPC authorization uses tenant-scoped group membership, and test setup provisions Keycloak organizations and memberships before integration tests run. ChangesOrganization-based authorization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ToolSetup as Tool.Setup
participant KeycloakAdminAPI as Keycloak admin API
participant IntegrationTests as integration tests
ToolSetup->>KeycloakAdminAPI: create tenants, org memberships, and /members groups
ToolSetup->>IntegrationTests: provide provisioned organizations and claims
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 6❌ Failed checks (6 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@charts/service/templates/grpc-server/authconfig.yaml`:
- Around line 282-295: The current authorization rule inside the allow if block
applies the same group check to Get/Update/Delete and therefore denies read-only
viewers; modify the policy by splitting the rule into two checks: one allow
branch that permits grpc_method == "/osac.public.v1.Projects/Get" when
input.auth.identity.authnMethod == "jwt" and some group in
subject_org_groups[tenant].groups where group == sprintf("/%s/viewers", [name])
or group == sprintf("/%s/managers", [name]); and a second allow branch that
permits grpc_method in
{"/osac.public.v1.Projects/Update","/osac.public.v1.Projects/Delete"} only when
the same jwt check and some group in subject_org_groups[tenant].groups where
group == sprintf("/%s/managers", [name]); retain use of tenant :=
input.context.context_extensions.tenant and name :=
input.context.context_extensions.name in both branches.
- Around line 104-108: The current rule assigns subject_tenants = subject_groups
when input.auth.identity.authnMethod == "jwt" and organization is missing, which
incorrectly treats project group paths as tenants; update the logic so that when
authnMethod is "jwt" and input.auth.identity.organization is absent,
subject_tenants remains empty (do not assign subject_groups). Concretely, remove
or guard the branch that sets subject_tenants from subject_groups for JWTs
without organization, relying solely on input.auth.identity.organization as the
authoritative tenant source and leaving subject_tenants unset in that case.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 99d70ea3-613b-4bd1-95d3-1a70fbb81c66
📒 Files selected for processing (1)
charts/service/templates/grpc-server/authconfig.yaml
5b0c673 to
41f59c9
Compare
jhernand
left a comment
jhernand
left a comment
There was a problem hiding this comment.
You need to update the authconfig.yaml file in the manifests directory as well.
61f8677 to
0b765cb
Compare
Thank you! Updated that one as well |
3b6cd8b to
930964d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@charts/service/templates/grpc-server/authconfig.yaml`:
- Around line 271-301: Add an allow rule for the Projects/List gRPC method:
create a new allow-if block checking grpc_method ==
"/osac.public.v1.Projects/List", extract tenant :=
input.context.context_extensions.tenant, require input.auth.identity.authnMethod
== "jwt", and verify some group in subject_org_groups[tenant].groups (or narrow
to viewers/managers if you prefer filtering at the app layer); this mirrors the
Get/Update rules and ensures JWT-authenticated tenant members can call
Projects/List while the server can perform any result-level filtering.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3f312678-7c68-4944-b608-71f503969b9d
📒 Files selected for processing (2)
charts/service/templates/grpc-server/authconfig.yamlmanifests/base/grpc-server/authconfig.yaml
ad5b834 to
14bff45
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@it/charts/keycloak/files/realm.json`:
- Around line 836-846: The realm config sets protocolMapper
"oidc-organization-group-membership-mapper" (id "org-group-membership-001") but
the chart defaults images.keycloak to 26.3 and realm.json declares
keycloakVersion 26.3.5, which is <26.6.0 where that mapper is guaranteed; update
the deployment to a compatible Keycloak version (bump values.images.keycloak
default and chart/appVersion to >=26.6.0 and align realm.json keycloakVersion)
or alternatively gate/remove the protocolMapper
"oidc-organization-group-membership-mapper" (org-group-membership-001) for older
versions and provide a fallback mapper implementation so org-scoped JWT
membership continues to be emitted.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3a58f4ce-ff1b-4fd9-a490-e39d1ef78a29
📒 Files selected for processing (3)
charts/service/templates/grpc-server/authconfig.yamlit/charts/keycloak/files/realm.jsonmanifests/base/grpc-server/authconfig.yaml
bd603a9 to
2312cef
Compare
e508a5a to
3a230f9
Compare
|
🔴 CI Triage: Root cause: PR #666 removes the Rego policy fallback that derived tenant membership from the JWT Causal chain:
Evidence:
[ [
Suggestion: The E2E snapshot's Keycloak realm needs to be updated to include the Prow job | Build For deeper investigation, use the |
9881ce4 to
48b9bfe
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@internal/auth/grpc_authz_interceptor.go`:
- Around line 481-498: The authz interceptor is logging sensitive JWT
organization data and usernames in the organization-claim handling path. Update
the logging in grpc_authz_interceptor.go around the organization claim parsing
logic to remove raw orgValue/orgArray/orgObj and username fields from
InfoContext/WarnContext calls, and replace them with safe metadata like claim
type, presence, and count only. Keep the behavior in the same org claim branch,
but ensure any logs in the claimAsAnySlice, orgValue map handling, and “No
organization claim in JWT” paths are fully redacted.
In `@internal/auth/policies/authz.rego`:
- Around line 325-332: The project authorization rules currently assign tenant
and name together before they are both needed, but Regal wants name deferred
until right before it is used in the group checks. Update the relevant project
rule blocks in authz.rego so tenant is bound first, then move the name
assignment closer to the sprintf-based group comparison while keeping the logic
in the same rule structure and preserving the existing authnMethod and
subject_org_groups behavior.
In `@it/it_tool.go`:
- Around line 1914-1915: The Keycloak organization log in the it_tool.go flow is
exposing potentially sensitive identifiers via the slog fields in the
InfoContext call. Update the logging in the code path around the "Added user to
Keycloak organization" message so the keys for username and orgName are prefixed
with !, ensuring the repository’s redaction handler masks those values. Keep the
same log location and context, and only adjust the field keys used in the
InfoContext call.
In `@manifests/base/grpc-server/authconfig.yaml`:
- Around line 309-316: The embedded Rego in the AuthConfig policy is still using
the old eager name binding and needs to match the Regal cleanup in
internal/auth/policies/authz.rego. Update the rule that uses tenant and name so
the name value is assigned later via the same deferred pattern, and keep the
group membership check unchanged while preserving the existing identifiers in
the policy block.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: fdbf861c-289f-4ff1-93ec-241ae05a2cdc
📒 Files selected for processing (6)
internal/auth/grpc_authz_interceptor.gointernal/auth/grpc_authz_interceptor_test.gointernal/auth/policies/authz.regoit/it_multitenancy_test.goit/it_tool.gomanifests/base/grpc-server/authconfig.yaml
💤 Files with no reviewable changes (1)
- internal/auth/grpc_authz_interceptor_test.go
5e53989 to
b70a51c
Compare
b70a51c to
566bf1a
Compare
566bf1a to
8070957
Compare
Determine if a user can get/update/delete a project based on the organization group they're part of. Assisted-by: Claude Code <noreply@anthropic.com>
|
/test e2e-vmaas |
|
Triggered: E2E VMaaS Full Install |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, jhernand, oourfali The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
5 participants
Description
Determine if a user can get/update/delete a project based on the organization group they're part of.
Testing
Assisted-by: Claude Code noreply@anthropic.com
/cc @jhernand
Summary by CodeRabbit
New Features
organizationclaims, supporting both array and object formats.Bug Fixes
Tests