OSAC-478: Add Organization-scoped authorization for Admins - #498
Conversation
|
@CrystalChun: This pull request references OSAC-478 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds organization/tenant support by updating the Keycloak osac-cli client to include the organization scope and extending the OPA rego policy to: extract tenant claims and realm roles from JWTs, define tenant_admin_roles and tenant_idp_manager_roles, compute tenant-role flags, exclude tenant-scoped admins from generic client eligibility, expand client allow rules to include tenant admins/IdP managers, add tenant-scoped Users method allow rules, and map x-subject.tenants from the computed subject_tenants. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
internal/auth/organization_authz.go (2)
90-107: ⚡ Quick winAvoid
panicin a per-request authorization helper.
GetUserOrganizationspanics ifOrganizationsis neither universal nor finite. While that state isn't expected today, this function is called from request handlers, so a regression in thecollections.Setimplementation, an unexpectedSubjectshape (e.g., zero-valueOrganizationsinterface — see the related comment onauth_subject.go), or a future infinite-but-non-universal set would crash the gRPC server goroutine instead of denying the request.Prefer returning an empty slice (which
Containssemantics make a safe deny-default) or changing the signature to([]string, error):🛡️ Suggested defensive handling
func GetUserOrganizations(ctx context.Context) []string { subject := SubjectFromContext(ctx) + if subject.Organizations == nil { + return []string{} + } if subject.Organizations.Universal() { // Platform admins have access to all organizations // Return nil to indicate universal access return nil } if subject.Organizations.Finite() { return subject.Organizations.Inclusions() } - // This should not happen in practice - panic("organization set is infinite but not universal") + // Infinite-but-not-universal sets are not expected; deny-by-default + // rather than crashing the request handler. + return []string{} }Also worth noting: the docstring says "Returns nil for platform admins (indicating universal access)" — callers must know to interpret
nilas "all orgs" rather than "no orgs". Please add a short caveat in the godoc and consider returning a tri-state (e.g., a(orgs []string, universal bool)pair) if a future caller might iterate the slice.🤖 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 `@internal/auth/organization_authz.go` around lines 90 - 107, GetUserOrganizations currently panics if Subject.Organizations is neither Universal() nor Finite(), which can crash request goroutines; change GetUserOrganizations (and its docstring) to avoid panic by returning a safe deny-default (an empty []string) when the Organizations shape is unexpected, keep returning nil only when Organizations.Universal() is true, and document that nil means "universal access"; update references to SubjectFromContext, Subject.Organizations.Universal(), Subject.Organizations.Finite(), and Subject.Organizations.Inclusions() accordingly (or alternatively change the signature to return ([]string, bool) / ([]string, error) if callers need an explicit universal/err signal).
60-77: ⚡ Quick winInconsistent universal-organizations handling vs.
CanManageOrganization.
CanManageOrganization(Lines 26-47) short-circuits onsubject.Organizations.Universal()before checking admin role, butCanManageUsersInOrganizationdoes not. In practice this likely still works, because a universal set'sContains(...)returns true, but only for subjects that also haveIsOrgAdmin/IsOrgIdpManager— a platform-style subject that hasOrganizations: AllOrganizationswithoutIsOrgAdmin(e.g., a future "platform reader" persona) would unexpectedly be denied here while being allowed byCanManageOrganization. Also, the two helpers should encode the same model so reviewers don't have to compare them line by line.Suggest mirroring the structure exactly, or — better — having
CanManageUsersInOrganizationdelegate:♻️ Suggested refactor for consistency
func CanManageUsersInOrganization(ctx context.Context, organizationName string) bool { - subject := SubjectFromContext(ctx) - - // Platform admins can manage all users - if subject.Tenants.Universal() { - return true - } - - // Organization admins and IdP managers can manage users in their organization - if subject.Organizations.Contains(organizationName) { - if subject.IsOrgAdmin || subject.IsOrgIdpManager { - return true - } - } - - return false + // User management currently requires the same scope as organization + // management (org admin or IdP manager within the target org, or platform + // admin). Delegating keeps the two predicates in sync. + return CanManageOrganization(ctx, organizationName) }If the two are intended to diverge in the future (e.g., different role gates), please add a comment explaining the divergence and add a test for the universal-organizations-without-
IsOrgAdmincase.🤖 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 `@internal/auth/organization_authz.go` around lines 60 - 77, CanManageUsersInOrganization currently uses subject.Organizations.Contains(organizationName) then checks IsOrgAdmin/IsOrgIdpManager, which mismatches the short-circuit-on-universal behavior in CanManageOrganization; update CanManageUsersInOrganization (or have it delegate) so it mirrors CanManageOrganization's logic: check subject.Organizations.Universal() before role checks (or call CanManageOrganization(ctx, organizationName) and then additionally require IsOrgAdmin || IsOrgIdpManager), using SubjectFromContext and the existing subject fields to ensure universal-organization subjects without IsOrgAdmin are treated consistently; if you intentionally want different behavior, add a clarifying comment and tests for the universal-without-IsOrgAdmin case.internal/auth/organization_authz_test.go (1)
148-194: ⚡ Quick winAdd a
RequireUserManagementInOrganizationdescribe block to mirror the other helpers.
RequireOrganizationManagementis covered (Lines 93-146), butRequireUserManagementInOrganizationfromorganization_authz.gohas no direct tests — only its boolean counterpartCanManageUsersInOrganization. SinceRequire*wraps the boolean and formats a user-facing error message ("user does not have permission to manage users in organization %q"), regressions in the message format or the wrap won't be caught.Also, given the divergence between
CanManageOrganizationandCanManageUsersInOrganizationonOrganizations.Universal()handling (see theorganization_authz.gocomment), a test like:It("allows subjects with universal organizations regardless of admin role", func() { subject := &Subject{ User: "platform-reader", Tenants: collections.NewSet("acme-corp"), Organizations: AllOrganizations, } ctx = ContextWithSubject(ctx, subject) Expect(CanManageUsersInOrganization(ctx, "acme-corp")).To(BeTrue()) })would lock in (or surface) the intended semantics.
🤖 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 `@internal/auth/organization_authz_test.go` around lines 148 - 194, Add a new Describe("RequireUserManagementInOrganization") block that mirrors the existing RequireOrganizationManagement tests: create Subjects via ContextWithSubject (use Subject, AllTenants, AllOrganizations, collections.NewSet, IsOrgAdmin, IsOrgIdpManager) and call RequireUserManagementInOrganization(ctx, "acme-corp") to assert success for platform admins, org admins, and IdP managers and failure for regular users; also add the universal-orgs case (Subject with Organizations = AllOrganizations) to assert it allows management regardless of admin role; when asserting failures, check the returned error message matches the user-facing format "user does not have permission to manage users in organization %q" so regressions in the wrapper message are detected.charts/service/templates/grpc-server/authconfig.yaml (1)
325-338: ⚡ Quick winPattern confirmed in Authorino documentation; integration test recommended for verification.
The CEL expressions referencing
auth.authorization.default.subject_organizations,auth.authorization.default.subject_realm_roles,auth.authorization.default.is_org_admin, andauth.authorization.default.is_org_idp_managerare valid—Authorino's Keycloak Authorization Services documentation demonstrates this pattern withallValues: trueexposing intermediate Rego rule values for use in response header injection. However, the exact CEL path mapping is not explicitly documented. Consider adding an integration test to verify that thex-subjectheader includes these intermediate values in production-like manifests, as a missing-attribute lookup at CEL evaluation could silently degrade to default values withoutis_org_admin/is_org_idp_managerbeing honored downstream.🤖 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 `@charts/service/templates/grpc-server/authconfig.yaml` around lines 325 - 338, The CEL expressions for organizations, roles, is_org_admin, and is_org_idp_manager (using auth.authorization.default.subject_organizations, auth.authorization.default.subject_realm_roles, auth.authorization.default.is_org_admin, auth.authorization.default.is_org_idp_manager) should be validated with an integration test: add a production-like test that deploys the chart/template and performs a request which exercises Authorino’s authorization path and asserts the x-subject (or relevant) header contains the exported intermediate values; if the header is missing or values are empty, update the mapping or ensure Authorino is configured with allValues:true and the CEL paths exactly match the Rego exports so the attributes are present at evaluation time.
🤖 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 123-133: The rule subject_organizations currently has two
unconditional bodies that both match when a JWT includes both
input.auth.identity.organization and input.auth.identity.organizations, causing
eval_conflict_error; change the rule to make the branches mutually exclusive
(e.g., prefer organizations when present, or check that the other claim is
absent) by adding a negated existence condition to one branch so only one body
can fire—update the branches that reference input.auth.identity.organization and
input.auth.identity.organizations accordingly while keeping the default
subject_organizations = [] fallback.
- Around line 178-197: The OPA rules belong_to_organization and
can_manage_organization(target_org) are never used and handlers skip
application-layer checks; fix by wiring org-scoping into both layers: update the
Authorino/OPA policy to extract the target organization from gRPC request
metadata or body and gate allow rules with can_manage_organization(target_org)
(use the existing belongs_to_organization and can_manage_organization symbols),
and/or add explicit calls to RequireOrganizationManagement and
RequireUserManagementInOrganization at the start of all public handler
Create/Update/Delete methods for Roles, Users, and IdentityProviders before
delegating to the private servers so resource operations validate the target
org; also update organization-authorization.md to state which enforcement path
(OPA extraction vs handler checks) you implemented.
In `@docs/organization-authorization.md`:
- Around line 59-94: The doc currently instructs adding a User Attribute mapper
for the organization claim which conflicts with the shipped realm configuration
that already includes an Organization Membership mapper
(oidc-organization-membership-mapper) on the organization client scope and would
cause duplicate or mismatched claims; update Steps 2–3 to instead document the
existing Organization Membership mapper and how to add users to Organizations
via the Keycloak Organizations admin UI (referencing the organization client
scope and oidc-organization-membership-mapper), and only present the
user-attribute mapper as an explicit fallback path for deployments that do not
enable Keycloak Organizations, clearly calling out the behavioral differences
(multivalued JSON membership claim vs single string user.attribute) so
OPA/Subject consumers get the correct claim shape.
In `@internal/auth/auth_subject.go`:
- Around line 127-149: MarshalJSON can panic when Subject.Organizations is nil
because collections.Set[string] is an interface; update the MarshalJSON method
on Subject to defensively initialize s.Organizations to an empty set when nil
(mirror UnmarshalJSON behavior) before calling
s.Organizations.Universal()/Finite()/Inclusions(); ensure the same fix covers
the Guest zero-value case by checking and setting s.Organizations to an empty
set at the start of MarshalJSON (referencing the MarshalJSON method and the
Subject type/Guest variable).
---
Nitpick comments:
In `@charts/service/templates/grpc-server/authconfig.yaml`:
- Around line 325-338: The CEL expressions for organizations, roles,
is_org_admin, and is_org_idp_manager (using
auth.authorization.default.subject_organizations,
auth.authorization.default.subject_realm_roles,
auth.authorization.default.is_org_admin,
auth.authorization.default.is_org_idp_manager) should be validated with an
integration test: add a production-like test that deploys the chart/template and
performs a request which exercises Authorino’s authorization path and asserts
the x-subject (or relevant) header contains the exported intermediate values; if
the header is missing or values are empty, update the mapping or ensure
Authorino is configured with allValues:true and the CEL paths exactly match the
Rego exports so the attributes are present at evaluation time.
In `@internal/auth/organization_authz_test.go`:
- Around line 148-194: Add a new Describe("RequireUserManagementInOrganization")
block that mirrors the existing RequireOrganizationManagement tests: create
Subjects via ContextWithSubject (use Subject, AllTenants, AllOrganizations,
collections.NewSet, IsOrgAdmin, IsOrgIdpManager) and call
RequireUserManagementInOrganization(ctx, "acme-corp") to assert success for
platform admins, org admins, and IdP managers and failure for regular users;
also add the universal-orgs case (Subject with Organizations = AllOrganizations)
to assert it allows management regardless of admin role; when asserting
failures, check the returned error message matches the user-facing format "user
does not have permission to manage users in organization %q" so regressions in
the wrapper message are detected.
In `@internal/auth/organization_authz.go`:
- Around line 90-107: GetUserOrganizations currently panics if
Subject.Organizations is neither Universal() nor Finite(), which can crash
request goroutines; change GetUserOrganizations (and its docstring) to avoid
panic by returning a safe deny-default (an empty []string) when the
Organizations shape is unexpected, keep returning nil only when
Organizations.Universal() is true, and document that nil means "universal
access"; update references to SubjectFromContext,
Subject.Organizations.Universal(), Subject.Organizations.Finite(), and
Subject.Organizations.Inclusions() accordingly (or alternatively change the
signature to return ([]string, bool) / ([]string, error) if callers need an
explicit universal/err signal).
- Around line 60-77: CanManageUsersInOrganization currently uses
subject.Organizations.Contains(organizationName) then checks
IsOrgAdmin/IsOrgIdpManager, which mismatches the short-circuit-on-universal
behavior in CanManageOrganization; update CanManageUsersInOrganization (or have
it delegate) so it mirrors CanManageOrganization's logic: check
subject.Organizations.Universal() before role checks (or call
CanManageOrganization(ctx, organizationName) and then additionally require
IsOrgAdmin || IsOrgIdpManager), using SubjectFromContext and the existing
subject fields to ensure universal-organization subjects without IsOrgAdmin are
treated consistently; if you intentionally want different behavior, add a
clarifying comment and tests for the universal-without-IsOrgAdmin 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e0e8624-0029-44d2-a215-c8a7557908c8
📒 Files selected for processing (8)
charts/keycloak/files/realm.jsoncharts/keycloak/templates/_start.tplcharts/service/templates/grpc-server/authconfig.yamldocs/organization-authorization.mdinternal/auth/auth_subject.gointernal/auth/organization_authz.gointernal/auth/organization_authz_test.gointernal/auth/tenancy_logic.go
| # Get the subject's organization(s) from JWT claims | ||
| # Keycloak can set this via a custom claim mapper | ||
| default subject_organizations = [] | ||
| subject_organizations = input.auth.identity.organization if { | ||
| input.auth.identity.authnMethod == "jwt" | ||
| input.auth.identity.organization | ||
| } | ||
| subject_organizations = input.auth.identity.organizations if { | ||
| input.auth.identity.authnMethod == "jwt" | ||
| input.auth.identity.organizations | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Rego v1, do two complete-rule assignments with different right-hand-side expressions produce eval_conflict_error if both bodies evaluate to true?
💡 Result:
Yes—if you write Rego v1 “complete” (single-value) rules so that two separate rule bodies can both evaluate to true and they assign different right-hand-side (result) values, evaluation fails with eval_conflict_error. OPA defines “complete” (single-value) rules as documents/rule results that can only have one value at a time; if evaluation produces multiple different values for the same document, OPA returns an error reported as eval_conflict_error: “complete rules must not produce multiple outputs”. [1][2] This is exactly the scenario described in the error documentation: “assign two different values to a rule” and then let both bodies be true; OPA will raise eval_conflict_error unless the conditions are made mutually exclusive (e.g., via negation). [1] So, in your specific question: two complete-rule assignments with different RHS expressions, where both bodies evaluate to true, will produce eval_conflict_error. [1][2] If the two bodies evaluate true but produce the same value, then there’s no “multiple different outputs,” so the error is not triggered by that particular conflict condition. (The general rule is about producing multiple distinct outputs for a complete definition.) [2]
Citations:
- 1: https://docs.styra.com/opa/errors/eval-conflict-error/complete-rules-must-not-produce-multiple-outputs
- 2: https://openpolicyagent.org/docs/errors
Fix eval_conflict_error when both organization and organizations JWT claims are present.
subject_organizations is a complete rule with two unconditional assignments—one from input.auth.identity.organization, the other from input.auth.identity.organizations. If a JWT contains both claims (e.g., if both mappers are configured in Keycloak), both rule bodies evaluate to true with different values and Rego will raise eval_conflict_error: complete rules must not produce multiple outputs, failing the request at runtime.
Make the conditions mutually exclusive:
Proposed fix
- # Get the subject's organization(s) from JWT claims
- # Keycloak can set this via a custom claim mapper
- default subject_organizations = []
- subject_organizations = input.auth.identity.organization if {
- input.auth.identity.authnMethod == "jwt"
- input.auth.identity.organization
- }
- subject_organizations = input.auth.identity.organizations if {
- input.auth.identity.authnMethod == "jwt"
- input.auth.identity.organizations
- }
+ # Get the subject's organization(s) from JWT claims.
+ # Keycloak can set this via a custom claim mapper; we prefer the
+ # multivalued `organization` claim and fall back to `organizations`.
+ default subject_organizations = []
+ subject_organizations = input.auth.identity.organization if {
+ input.auth.identity.authnMethod == "jwt"
+ input.auth.identity.organization
+ }
+ subject_organizations = input.auth.identity.organizations if {
+ input.auth.identity.authnMethod == "jwt"
+ not input.auth.identity.organization
+ input.auth.identity.organizations
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Get the subject's organization(s) from JWT claims | |
| # Keycloak can set this via a custom claim mapper | |
| default subject_organizations = [] | |
| subject_organizations = input.auth.identity.organization if { | |
| input.auth.identity.authnMethod == "jwt" | |
| input.auth.identity.organization | |
| } | |
| subject_organizations = input.auth.identity.organizations if { | |
| input.auth.identity.authnMethod == "jwt" | |
| input.auth.identity.organizations | |
| } | |
| # Get the subject's organization(s) from JWT claims. | |
| # Keycloak can set this via a custom claim mapper; we prefer the | |
| # multivalued `organization` claim and fall back to `organizations`. | |
| default subject_organizations = [] | |
| subject_organizations = input.auth.identity.organization if { | |
| input.auth.identity.authnMethod == "jwt" | |
| input.auth.identity.organization | |
| } | |
| subject_organizations = input.auth.identity.organizations if { | |
| input.auth.identity.authnMethod == "jwt" | |
| not input.auth.identity.organization | |
| input.auth.identity.organizations | |
| } |
🤖 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 `@charts/service/templates/grpc-server/authconfig.yaml` around lines 123 - 133,
The rule subject_organizations currently has two unconditional bodies that both
match when a JWT includes both input.auth.identity.organization and
input.auth.identity.organizations, causing eval_conflict_error; change the rule
to make the branches mutually exclusive (e.g., prefer organizations when
present, or check that the other claim is absent) by adding a negated existence
condition to one branch so only one body can fire—update the branches that
reference input.auth.identity.organization and input.auth.identity.organizations
accordingly while keeping the default subject_organizations = [] fallback.
|
|
||
| var organizations []string | ||
| if s.Organizations.Universal() { | ||
| organizations = []string{ | ||
| universalMarker, | ||
| } | ||
| } else if s.Organizations.Finite() { | ||
| organizations = s.Organizations.Inclusions() | ||
| } else { | ||
| err = fmt.Errorf("the organization set is infinite") | ||
| return | ||
| } | ||
|
|
||
| data, err = json.Marshal(subjectJson{ | ||
| User: s.User, | ||
| Tenants: tenants, | ||
| User: s.User, | ||
| Tenants: tenants, | ||
| Organizations: organizations, | ||
| Roles: s.Roles, | ||
| IsOrgAdmin: s.IsOrgAdmin, | ||
| IsOrgIdpManager: s.IsOrgIdpManager, | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all Subject literal constructions to confirm whether Organizations is always set.
rg -nP --type=go -C2 '\bSubject\s*\{' -g '!**/organization_authz_test.go'
ast-grep --pattern 'Subject{$$$}'Repository: osac-project/fulfillment-service
Length of output: 33356
🏁 Script executed:
# Read the full auth_subject.go file to see both Marshal and Unmarshal implementations
cat -n internal/auth/auth_subject.goRepository: osac-project/fulfillment-service
Length of output: 5696
🏁 Script executed:
# Check if json.Marshal is called on Subject anywhere in the codebase
rg -n "json\.Marshal.*[Ss]ubject" --type=goRepository: osac-project/fulfillment-service
Length of output: 399
🏁 Script executed:
# Check if Guest is ever marshaled or serialized
rg -n "Guest" --type=go -A 3 -B 1Repository: osac-project/fulfillment-service
Length of output: 20824
🏁 Script executed:
# Check the UnmarshalJSON implementation to see how it handles Organizations
rg -n "UnmarshalJSON" --type=go -A 20 internal/auth/Repository: osac-project/fulfillment-service
Length of output: 1672
MarshalJSON will panic if Organizations is left as zero-value (nil set).
collections.Set[string] is an interface; a Subject constructed without explicitly setting Organizations (zero value nil) will panic on s.Organizations.Universal() at line 129. The Guest variable in auth_guest.go has this issue and is actively used in production interceptors. Any code that constructs Subject{User: ...} without Organizations and then marshals it will panic.
🛡️ Suggested fix — Default `Organizations` to empty set when nil (defensive)
func (s *Subject) MarshalJSON() (data []byte, err error) {
var tenants []string
if s.Tenants.Universal() {
tenants = []string{
universalMarker,
}
} else if s.Tenants.Finite() {
tenants = s.Tenants.Inclusions()
} else {
err = fmt.Errorf("the tenant set is infinite")
return
}
+ orgs := s.Organizations
+ if orgs == nil {
+ orgs = collections.NewSet[string]()
+ }
var organizations []string
- if s.Organizations.Universal() {
+ if orgs.Universal() {
organizations = []string{
universalMarker,
}
- } else if s.Organizations.Finite() {
- organizations = s.Organizations.Inclusions()
+ } else if orgs.Finite() {
+ organizations = orgs.Inclusions()
} else {
err = fmt.Errorf("the organization set is infinite")
return
}This approach is consistent with how UnmarshalJSON handles both fields—always initializing them to safe defaults (lines 70, 91).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var organizations []string | |
| if s.Organizations.Universal() { | |
| organizations = []string{ | |
| universalMarker, | |
| } | |
| } else if s.Organizations.Finite() { | |
| organizations = s.Organizations.Inclusions() | |
| } else { | |
| err = fmt.Errorf("the organization set is infinite") | |
| return | |
| } | |
| data, err = json.Marshal(subjectJson{ | |
| User: s.User, | |
| Tenants: tenants, | |
| User: s.User, | |
| Tenants: tenants, | |
| Organizations: organizations, | |
| Roles: s.Roles, | |
| IsOrgAdmin: s.IsOrgAdmin, | |
| IsOrgIdpManager: s.IsOrgIdpManager, | |
| }) | |
| return | |
| } | |
| func (s *Subject) MarshalJSON() (data []byte, err error) { | |
| var tenants []string | |
| if s.Tenants.Universal() { | |
| tenants = []string{ | |
| universalMarker, | |
| } | |
| } else if s.Tenants.Finite() { | |
| tenants = s.Tenants.Inclusions() | |
| } else { | |
| err = fmt.Errorf("the tenant set is infinite") | |
| return | |
| } | |
| orgs := s.Organizations | |
| if orgs == nil { | |
| orgs = collections.NewSet[string]() | |
| } | |
| var organizations []string | |
| if orgs.Universal() { | |
| organizations = []string{ | |
| universalMarker, | |
| } | |
| } else if orgs.Finite() { | |
| organizations = orgs.Inclusions() | |
| } else { | |
| err = fmt.Errorf("the organization set is infinite") | |
| return | |
| } | |
| data, err = json.Marshal(subjectJson{ | |
| User: s.User, | |
| Tenants: tenants, | |
| Organizations: organizations, | |
| Roles: s.Roles, | |
| IsOrgAdmin: s.IsOrgAdmin, | |
| IsOrgIdpManager: s.IsOrgIdpManager, | |
| }) | |
| return | |
| } |
🤖 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 `@internal/auth/auth_subject.go` around lines 127 - 149, MarshalJSON can panic
when Subject.Organizations is nil because collections.Set[string] is an
interface; update the MarshalJSON method on Subject to defensively initialize
s.Organizations to an empty set when nil (mirror UnmarshalJSON behavior) before
calling s.Organizations.Universal()/Finite()/Inclusions(); ensure the same fix
covers the Guest zero-value case by checking and setting s.Organizations to an
empty set at the start of MarshalJSON (referencing the MarshalJSON method and
the Subject type/Guest variable).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
docs/organization-authorization.md (1)
59-94:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMapper guidance contradicts the shipped realm configuration — previously flagged, still unresolved.
Steps 2–3 instruct adding a User Attribute mapper for the
organizationclaim, butcharts/keycloak/files/realm.jsonalready ships an Organization Membership mapper (oidc-organization-membership-mapper) on theorganizationclient scope. Following these steps would either duplicate the claim or produce a different shape, breaking the OPA/Subject claim consumption.🤖 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 `@docs/organization-authorization.md` around lines 59 - 94, The docs currently instruct adding a User Attribute mapper for "organization" which conflicts with the shipped realm configuration that already includes an oidc-organization-membership-mapper in charts/keycloak/files/realm.json; update the documentation to instruct readers to use the existing Organization Membership mapper (oidc-organization-membership-mapper) on the osac-cli-dedicated/client scope (or explain how to modify that existing mapper) instead of adding a duplicate User Attribute mapper, and clarify the expected token claim name and JSON shape (single "organization" string vs "organizations" JSON array) so it matches OPA/subject claim consumption.internal/auth/auth_subject.go (1)
127-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
MarshalJSONstill panics whenOrganizationsis nil — previously flagged, still unresolved.
s.Organizations.Universal()at line 129 dereferences a nil interface if aSubjectis constructed without settingOrganizations(e.g.,Subject{User: "x"}).UnmarshalJSONalways initializesOrganizationsto a non-nil set, but direct struct literals (including anyGuestvariable) do not. The fix from the prior review is still applicable.🤖 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 `@internal/auth/auth_subject.go` around lines 127 - 149, MarshalJSON panics when s.Organizations is nil because the methods s.Organizations.Universal()/Finite()/Inclusions() are called on a nil set; update MarshalJSON (in auth_subject.go) to treat a nil Organizations as an empty/finite set before calling those methods — e.g., if s.Organizations == nil initialize organizations to an empty slice (or create a zero-value set) and proceed, or explicitly check for nil and set organizations = []string{} so calls to Universal(), Finite(), and Inclusions() are avoided on a nil receiver; ensure this mirrors UnmarshalJSON's guarantee so Subject constructed as literals (e.g., Guest) won't panic.
🤖 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 `@docs/organization-authorization.md`:
- Around line 136-177: The example in UsersServer.CreateUser misuses
subject.Tenants.Universal() as the platform-admin sentinel and redundantly
re-checks organization membership after calling
auth.RequireUserManagementInOrganization; update the guard to use
subject.Organizations.Universal() where a platform/admin bypass is intended, and
remove the manual organization-membership if-block that calls
subject.Organizations.Contains(req.OrganizationName) so the code relies solely
on auth.RequireUserManagementInOrganization to enforce org-scoped access.
- Around line 11-24: The fenced ASCII architecture diagram uses a bare
triple-backtick fence; update the opening fence for the diagram (the backticks
that precede the ASCII art) to include a language identifier such as "text" or
"plaintext" so markdownlint rule MD040 is satisfied and the diagram remains
unchanged. Locate the ASCII diagram fence and change ``` to ```text (or
```plaintext) right at the start of the block.
In `@internal/servers/users_server.go`:
- Around line 266-285: The Update handler currently authorizes against the
caller-supplied organization (request.GetObject().GetSpec().GetOrganization())
which allows privilege escalation; change it to mirror Delete by first fetching
the stored user with private.Get (using the id from
request.GetObject().GetMeta().GetId()), extract the existing organization from
the returned user object, and call auth.RequireUserManagementInOrganization(ctx,
existingOrg) against that organization before applying the update; ensure you
still handle errors and log with s.logger.WarnContext like the Delete path.
---
Duplicate comments:
In `@docs/organization-authorization.md`:
- Around line 59-94: The docs currently instruct adding a User Attribute mapper
for "organization" which conflicts with the shipped realm configuration that
already includes an oidc-organization-membership-mapper in
charts/keycloak/files/realm.json; update the documentation to instruct readers
to use the existing Organization Membership mapper
(oidc-organization-membership-mapper) on the osac-cli-dedicated/client scope (or
explain how to modify that existing mapper) instead of adding a duplicate User
Attribute mapper, and clarify the expected token claim name and JSON shape
(single "organization" string vs "organizations" JSON array) so it matches
OPA/subject claim consumption.
In `@internal/auth/auth_subject.go`:
- Around line 127-149: MarshalJSON panics when s.Organizations is nil because
the methods s.Organizations.Universal()/Finite()/Inclusions() are called on a
nil set; update MarshalJSON (in auth_subject.go) to treat a nil Organizations as
an empty/finite set before calling those methods — e.g., if s.Organizations ==
nil initialize organizations to an empty slice (or create a zero-value set) and
proceed, or explicitly check for nil and set organizations = []string{} so calls
to Universal(), Finite(), and Inclusions() are avoided on a nil receiver; ensure
this mirrors UnmarshalJSON's guarantee so Subject constructed as literals (e.g.,
Guest) won't panic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a06d8954-03dd-4f7e-82d7-be72c025d595
📒 Files selected for processing (7)
charts/service/templates/grpc-server/authconfig.yamldocs/organization-authorization.mdinternal/auth/auth_subject.gointernal/auth/organization_authz.gointernal/auth/organization_authz_test.gointernal/auth/tenancy_logic.gointernal/servers/users_server.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/auth/tenancy_logic.go
- charts/service/templates/grpc-server/authconfig.yaml
- internal/auth/organization_authz.go
| ``` | ||
| ┌─────────────┐ JWT with ┌───────────┐ Authz Check ┌─────────┐ | ||
| │ Keycloak │ ───────────────> │ Authorino │ ───────────────> │ OPA │ | ||
| │ │ org claims │ │ + org context │ Rules │ | ||
| └─────────────┘ └───────────┘ └─────────┘ | ||
| │ │ | ||
| │ │ | ||
| v v | ||
| ┌──────────────────────────────────────┐ | ||
| │ gRPC Server (fulfillment-service) │ | ||
| │ - Subject with Organizations │ | ||
| │ - Application-layer authz checks │ | ||
| └──────────────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Fenced code block is missing a language identifier (MD040).
The architecture diagram at line 11 uses a bare fence. Adding text or plaintext silences the markdownlint warning.
📝 Proposed fix
-```
+```text
┌─────────────┐ JWT with ┌───────────┐ Authz Check ┌─────────┐🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/organization-authorization.md` around lines 11 - 24, The fenced ASCII
architecture diagram uses a bare triple-backtick fence; update the opening fence
for the diagram (the backticks that precede the ASCII art) to include a language
identifier such as "text" or "plaintext" so markdownlint rule MD040 is satisfied
and the diagram remains unchanged. Locate the ASCII diagram fence and change ```
to ```text (or ```plaintext) right at the start of the block.
| ```go | ||
| package servers | ||
|
|
||
| import ( | ||
| "context" | ||
| "github.com/osac-project/fulfillment-service/internal/auth" | ||
| "google.golang.org/grpc/codes" | ||
| "google.golang.org/grpc/status" | ||
| ) | ||
|
|
||
| type UsersServer struct { | ||
| // ... | ||
| } | ||
|
|
||
| func (s *UsersServer) CreateUser(ctx context.Context, req *CreateUserRequest) (*CreateUserResponse, error) { | ||
| // Verify the caller can manage users in the target organization | ||
| err := auth.RequireUserManagementInOrganization(ctx, req.OrganizationName) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.PermissionDenied, "%v", err) | ||
| } | ||
|
|
||
| // Get the subject to verify organization membership | ||
| subject, err := auth.SubjectFromContext(ctx) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "failed to get subject: %v", err) | ||
| } | ||
|
|
||
| // For org admins (non-platform admins), verify they can only create users in their org | ||
| if !subject.Tenants.Universal() { | ||
| if !subject.Organizations.Contains(req.OrganizationName) { | ||
| return nil, status.Errorf( | ||
| codes.PermissionDenied, | ||
| "cannot create user in organization %q: user does not belong to this organization", | ||
| req.OrganizationName, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| // Proceed with user creation | ||
| // ... | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Example code uses Tenants.Universal() as the platform-admin guard and duplicates checks already inside RequireUserManagementInOrganization.
Two issues with the "Example: User Management Server" snippet:
-
Line 164 checks
subject.Tenants.Universal()to decide whether to enforce the org-membership check. Platform admins carry bothAllTenantsandAllOrganizations, so this works incidentally, but within org-scoped code the canonical sentinel isOrganizations.Universal(). The inconsistency may mislead contributors into using tenant-universality as an org-access proxy. -
Lines 158–172 manually re-check
subject.Organizations.Contains(req.OrganizationName)after already callingauth.RequireUserManagementInOrganization(line 152–154). IfRequireUserManagementInOrganizationalready enforces this, the second check is dead code; if it doesn't, the call contract is incomplete. Either way, the example contradicts the simpler, authoritative pattern ininternal/servers/users_server.go(which only callsRequireUserManagementInOrganization).
Consider aligning the example with the actual production pattern.
🤖 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 `@docs/organization-authorization.md` around lines 136 - 177, The example in
UsersServer.CreateUser misuses subject.Tenants.Universal() as the platform-admin
sentinel and redundantly re-checks organization membership after calling
auth.RequireUserManagementInOrganization; update the guard to use
subject.Organizations.Universal() where a platform/admin bypass is intended, and
remove the manual organization-membership if-block that calls
subject.Organizations.Contains(req.OrganizationName) so the code relies solely
on auth.RequireUserManagementInOrganization to enforce org-scoped access.
| func (s *UsersServer) Update(ctx context.Context, | ||
| request *publicv1.UsersUpdateRequest) (response *publicv1.UsersUpdateResponse, err error) { | ||
| // Extract organization from the request object | ||
| organizationName := request.GetObject().GetSpec().GetOrganization() | ||
| if organizationName == "" { | ||
| return nil, errors.New("organization is required") | ||
| } | ||
|
|
||
| // Verify the user can manage users in this organization | ||
| err = auth.RequireUserManagementInOrganization(ctx, organizationName) | ||
| if err != nil { | ||
| s.logger.WarnContext( | ||
| ctx, | ||
| "User management permission denied", | ||
| slog.String("organization", organizationName), | ||
| slog.Any("error", err), | ||
| ) | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
Organization authorization in Update checks the requested org, not the existing user's org — privilege escalation.
The Delete handler correctly fetches the stored user first and derives the organization from it (lines 331–342). Update does not: it uses request.GetObject().GetSpec().GetOrganization(), which is fully caller-controlled.
An org-admin for "acme-corp" can supply any user-id (including one currently in "widgets-inc") and set organization: "acme-corp" in the update body. The RequireUserManagementInOrganization check passes for "acme-corp", so the user in "widgets-inc" gets mutated without the caller ever being authorized for "widgets-inc".
The fix is to mirror Delete: fetch the existing user via private.Get, extract their current org, and authorize against that org before applying the update.
🛡️ Proposed fix
func (s *UsersServer) Update(ctx context.Context,
request *publicv1.UsersUpdateRequest) (response *publicv1.UsersUpdateResponse, err error) {
- // Extract organization from the request object
- organizationName := request.GetObject().GetSpec().GetOrganization()
- if organizationName == "" {
- return nil, errors.New("organization is required")
- }
-
- // Verify the user can manage users in this organization
- err = auth.RequireUserManagementInOrganization(ctx, organizationName)
+ // First, fetch the existing user to determine their current organization
+ getRequest := &privatev1.UsersGetRequest{}
+ getRequest.SetId(request.GetObject().GetId())
+ getResponse, err := s.private.Get(ctx, getRequest)
if err != nil {
- s.logger.WarnContext(
- ctx,
- "User management permission denied",
- slog.String("organization", organizationName),
- slog.Any("error", err),
- )
- return nil, err
+ return nil, err
}
+
+ // Extract the *existing* organization (not the caller-supplied one)
+ organizationName := getResponse.GetObject().GetSpec().GetOrganization()
+ if organizationName == "" {
+ return nil, errors.New("user organization is required")
+ }
+
+ // Verify the user can manage users in this organization
+ err = auth.RequireUserManagementInOrganization(ctx, organizationName)
+ if err != nil {
+ s.logger.WarnContext(
+ ctx,
+ "User management permission denied",
+ slog.String("organization", organizationName),
+ slog.String("user_id", request.GetObject().GetId()),
+ slog.Any("error", err),
+ )
+ return nil, err
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *UsersServer) Update(ctx context.Context, | |
| request *publicv1.UsersUpdateRequest) (response *publicv1.UsersUpdateResponse, err error) { | |
| // Extract organization from the request object | |
| organizationName := request.GetObject().GetSpec().GetOrganization() | |
| if organizationName == "" { | |
| return nil, errors.New("organization is required") | |
| } | |
| // Verify the user can manage users in this organization | |
| err = auth.RequireUserManagementInOrganization(ctx, organizationName) | |
| if err != nil { | |
| s.logger.WarnContext( | |
| ctx, | |
| "User management permission denied", | |
| slog.String("organization", organizationName), | |
| slog.Any("error", err), | |
| ) | |
| return nil, err | |
| } | |
| func (s *UsersServer) Update(ctx context.Context, | |
| request *publicv1.UsersUpdateRequest) (response *publicv1.UsersUpdateResponse, err error) { | |
| // First, fetch the existing user to determine their current organization | |
| getRequest := &privatev1.UsersGetRequest{} | |
| getRequest.SetId(request.GetObject().GetId()) | |
| getResponse, err := s.private.Get(ctx, getRequest) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Extract the *existing* organization (not the caller-supplied one) | |
| organizationName := getResponse.GetObject().GetSpec().GetOrganization() | |
| if organizationName == "" { | |
| return nil, errors.New("user organization is required") | |
| } | |
| // Verify the user can manage users in this organization | |
| err = auth.RequireUserManagementInOrganization(ctx, organizationName) | |
| if err != nil { | |
| s.logger.WarnContext( | |
| ctx, | |
| "User management permission denied", | |
| slog.String("organization", organizationName), | |
| slog.String("user_id", request.GetObject().GetId()), | |
| slog.Any("error", err), | |
| ) | |
| return nil, 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 `@internal/servers/users_server.go` around lines 266 - 285, The Update handler
currently authorizes against the caller-supplied organization
(request.GetObject().GetSpec().GetOrganization()) which allows privilege
escalation; change it to mirror Delete by first fetching the stored user with
private.Get (using the id from request.GetObject().GetMeta().GetId()), extract
the existing organization from the returned user object, and call
auth.RequireUserManagementInOrganization(ctx, existingOrg) against that
organization before applying the update; ensure you still handle errors and log
with s.logger.WarnContext like the Delete path.
5c42f31 to
11289e9
Compare
c97db8d to
3d3dc60
Compare
| grpc_method := input.context.request.http.path | ||
|
|
||
| # Get the target organization from the request (extracted by the gRPC interceptor) | ||
| target_organization := input.context.context_extensions.organization |
There was a problem hiding this comment.
I think that Keycloak can be configured to add the organization identifier to the token as a claim by adding the "organization" scope to the client. For example, we can add it to the osac-cli client like this:
diff --git a/charts/keycloak/files/realm.json b/charts/keycloak/files/realm.json
index 285aa35..220d0fe 100644
--- a/charts/keycloak/files/realm.json
+++ b/charts/keycloak/files/realm.json
@@ -555,7 +555,7 @@
"authenticationFlowBindingOverrides" : { },
"fullScopeAllowed" : true,
"nodeReRegistrationTimeout" : -1,
- "defaultClientScopes" : [ "groups", "basic", "username" ],
+ "defaultClientScopes" : [ "groups", "basic", "username", "organization" ],
"optionalClientScopes" : [ ]
}, {
"id" : "e37d4620-4ae2-432e-90d8-2b7f6a662f43",Then, if you create a user and assign it to an organization, "org-a" for example, you will get the following token:
$ ./osac get token --payload
{
"auth_time": 1778168023,
"azp": "osac-cli",
"exp": 1778168326,
"iat": 1778168026,
"iss": "https://keycloak.keycloak.svc.cluster.local:8000/realms/osac",
"jti": "onrtdg:a330f03c-77a3-cdeb-af0d-791523bab9cc",
"organization": [
"org-a"
],
"scope": "openid organization",
"sid": "40a06f68-83e3-4223-8d86-217c5bb0b9f3",
"sub": "17a73e41-16e1-418a-b89f-a356b5346f01",
"typ": "Bearer",
"username": "bob@org-a.com"
}Note the new organization claim. Can we use that instead of requiring the server to extract it from the request?
There was a problem hiding this comment.
Thank you for the feedback! I wasn't sure about this so I asked Claude 😅
Scenario: Malicious Request
Alice is in org-a. She tries to create a user
in org-b:With extraction (current):
- JWT: organization = ["org-a"]
- Extract from request: target_organization =
"org-b"- OPA checks: Is "org-b" in ["org-a"]? ❌
DeniedWithout extraction:
- JWT: organization = ["org-a"]
- No extraction
- OPA: Alice is org admin? ✓ Method allowed
- Server creates user with organization =
"org-b", tenants = ["org-a"] (from our
override)- ❌ Mismatch: User says org-b but visible to
org-a!
Is this actually a valid concern or should we remove the server extraction?
I just added organization to the default client scopes, thank you for the suggestion!
There was a problem hiding this comment.
I think it isn't a valid concern. When an user creates an object (another user, or any other thing) the server decides what tenant to assign based on the "tenants" list passed by Authorino and on the "metadata.tenants" field of the object itself. If the user includes in "metadata.tenants" something that isn't in the tenants passed by Authorino the request is rejected. That is checked by the determineAssignedTenants method of the generic server:
fulfillment-service/internal/servers/generic_server.go
Lines 1152 to 1287 in 654c83a
Exactly here:
fulfillment-service/internal/servers/generic_server.go
Lines 1214 to 1242 in 654c83a
I think that is sufficient. I will check that we have a test verifying it, and add it if we don't.
There was a problem hiding this comment.
Gotcha, thank you for the explanation Juan! I've removed this section where we're extracting it.
There was a problem hiding this comment.
Great! Here is a new test to verify that behavior: #504
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/auth/grpc_external_auth_interceptor_test.go (1)
479-583: 💤 Low valueLGTM — the four new test cases cover the primary paths correctly.
The positive cases (Create/Update with
spec.organizationset) and negative cases (Get with noobjectfield; non-User Create where the spec has noorganizationfield) all align with theextractOrganizationguard logic. Assertions againstGetContextExtensions()are precise.One optional coverage gap: there is no test for
UsersCreateRequest{}(nilObject) orUsersCreateRequest{Object: &publicv1.User{}}(nilSpec), analogous to the existing"Should not include object identifier when update request has no object"test. TheHas-guards inextractOrganizationmake these safe, but an explicit test would document the contract.➕ Suggested additional test cases
It("Should not include organization in context extensions for user create requests with nil object", func() { mock.Func = func(ctx context.Context, request *envoyauthv3.CheckRequest) (response *envoyauthv3.CheckResponse, err error) { Expect(request).ToNot(BeNil()) extensions := request.GetAttributes().GetContextExtensions() Expect(extensions).ToNot(HaveKey("organization")) response = makeOkResponse(&Subject{ User: "my-user", }) return } handler := func(context.Context, any) (any, error) { return nil, nil } info := &grpc.UnaryServerInfo{ FullMethod: "/osac.public.v1.Users/Create", } request := &publicv1.UsersCreateRequest{} // nil Object _, err := interceptor.UnaryServer(ctx, request, info, handler) Expect(err).ToNot(HaveOccurred()) }) It("Should not include organization in context extensions for user create requests with nil spec", func() { mock.Func = func(ctx context.Context, request *envoyauthv3.CheckRequest) (response *envoyauthv3.CheckResponse, err error) { Expect(request).ToNot(BeNil()) extensions := request.GetAttributes().GetContextExtensions() Expect(extensions).ToNot(HaveKey("organization")) response = makeOkResponse(&Subject{ User: "my-user", }) return } handler := func(context.Context, any) (any, error) { return nil, nil } info := &grpc.UnaryServerInfo{ FullMethod: "/osac.public.v1.Users/Create", } request := &publicv1.UsersCreateRequest{ Object: &publicv1.User{}, // nil Spec } _, err := interceptor.UnaryServer(ctx, request, info, handler) Expect(err).ToNot(HaveOccurred()) })🤖 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 `@internal/auth/grpc_external_auth_interceptor_test.go` around lines 479 - 583, Add two tests to cover UsersCreateRequest with nil Object and with Object having nil Spec: create test cases named like "Should not include organization in context extensions for user create requests with nil object" and "...with nil spec" that set info.FullMethod = "/osac.public.v1.Users/Create", call interceptor.UnaryServer(ctx, request, info, handler) with request := &publicv1.UsersCreateRequest{} and request := &publicv1.UsersCreateRequest{Object: &publicv1.User{} } respectively, and assert the envoyauthv3.CheckRequest context extensions do NOT have the "organization" key; this exercises the extractOrganization guard logic and mirrors the existing update-nil-object test pattern.
🤖 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.
Nitpick comments:
In `@internal/auth/grpc_external_auth_interceptor_test.go`:
- Around line 479-583: Add two tests to cover UsersCreateRequest with nil Object
and with Object having nil Spec: create test cases named like "Should not
include organization in context extensions for user create requests with nil
object" and "...with nil spec" that set info.FullMethod =
"/osac.public.v1.Users/Create", call interceptor.UnaryServer(ctx, request, info,
handler) with request := &publicv1.UsersCreateRequest{} and request :=
&publicv1.UsersCreateRequest{Object: &publicv1.User{} } respectively, and assert
the envoyauthv3.CheckRequest context extensions do NOT have the "organization"
key; this exercises the extractOrganization guard logic and mirrors the existing
update-nil-object test pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c0cf482-919f-4e0f-a3be-081eb414983a
📒 Files selected for processing (4)
charts/keycloak/files/realm.jsoncharts/service/templates/grpc-server/authconfig.yamlinternal/auth/grpc_external_auth_interceptor.gointernal/auth/grpc_external_auth_interceptor_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/service/templates/grpc-server/authconfig.yaml
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 123-126: The policy currently allows org-admin user-management
solely by role+method without checking the organization claim; update the
relevant allow rule(s) that grant org-admin user-management access to also
require a non-empty subject_organizations (e.g., subject_organizations != [] or
length(subject_organizations) > 0) so requests without an organization claim
fail closed; apply the same additional guard to the other similar rule block
around the later section mentioned (the block at ~250-264) so both places
reference the existing default subject_organizations variable when enforcing
org-admin access.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 803c91e3-5e81-4c2e-8b03-99f6340c9acf
📒 Files selected for processing (2)
charts/keycloak/files/realm.jsoncharts/service/templates/grpc-server/authconfig.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/keycloak/files/realm.json
| # Get the subject's organization(s) from JWT claims or service account namespace | ||
| # For JWT users, this comes from the "organization" scope which is in defaultClientScopes. | ||
| # The organization claim is required for org admins and IdP managers. | ||
| default subject_organizations = [] |
There was a problem hiding this comment.
Enforce org-claim presence before granting org-admin user-management access.
Line 125 says the organization claim is required, but the allow rule only checks role + method. Add a non-empty organization guard to fail closed at policy level.
Suggested minimal fix
allow if {
is_org_admin
+ count(subject_organizations) > 0
grpc_method in {
"/osac.public.v1.Users/Create",
"/osac.public.v1.Users/Get",
"/osac.public.v1.Users/List",
"/osac.public.v1.Users/Update",
"/osac.public.v1.Users/Delete",
}
}Also applies to: 250-264
🤖 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 `@charts/service/templates/grpc-server/authconfig.yaml` around lines 123 - 126,
The policy currently allows org-admin user-management solely by role+method
without checking the organization claim; update the relevant allow rule(s) that
grant org-admin user-management access to also require a non-empty
subject_organizations (e.g., subject_organizations != [] or
length(subject_organizations) > 0) so requests without an organization claim
fail closed; apply the same additional guard to the other similar rule block
around the later section mentioned (the block at ~250-264) so both places
reference the existing default subject_organizations variable when enforcing
org-admin access.
|
|
||
| # Organization admin roles - users with these roles can manage users in their organization | ||
| org_admin_roles := { | ||
| "organization-admin", |
There was a problem hiding this comment.
Given that organizations and tenants are the same, and that we already have the "tenant" name in many parts of the code, would it make sense to name this tenant-admin and in general tenant-... to be consistent with the theme?
There was a problem hiding this comment.
Yes, updated! Thank you
|
|
||
| # Get the subject's organization(s) from JWT claims or service account namespace | ||
| # For JWT users, this comes from the "organization" scope which is in defaultClientScopes. | ||
| # The organization claim is required for org admins and IdP managers. |
There was a problem hiding this comment.
While the above comment is technically correct, I'd say that is too much detail. I think it is enough to say that we take the organizations from the organizations claim. How that is technically achieved in the Keycloak realm configuration is not that interesting here, and may change in the future leaving this comment out of date.
There was a problem hiding this comment.
Gotcha, that makes sense. I've removed these comments except for the first line, is that ok?
| # Get the subject's organization(s) from JWT claims or service account namespace | ||
| # For JWT users, this comes from the "organization" scope which is in defaultClientScopes. | ||
| # The organization claim is required for org admins and IdP managers. | ||
| default subject_organizations = [] |
There was a problem hiding this comment.
Can this be subject_tenants? In general, can we use "tenants" instead of "organizations" here as much as possible?
There was a problem hiding this comment.
Yes, updated! Thank you
| is_client if { | ||
| not is_admin | ||
| not is_org_admin | ||
| not is_org_idp_manager |
There was a problem hiding this comment.
Does this mean that if we make a user an tenant admin it will stop being able to create clusters, VMs, etc?
There was a problem hiding this comment.
Good point, I guess they should inherit the ability to manage all other resources too. I've updated it so they can now. How does it look?
d7e3871 to
037fd52
Compare
Extracts the Organization of a request in the interceptor to pass along to OPA. Assisted-by: Claude
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, jhernand 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 |
|
/jira refresh |
The AuthConfig Rego policy in manifests/base/ diverged from the Helm chart in charts/service/. PR osac-project#498 added organization-scoped authorization (tenant admin roles, subject tenants, realm roles, has_client_permissions) to the Helm chart but not to the kustomize manifests. Environments deployed via osac-installer used the stale manifest, causing OSAC-807. This syncs the manifest with the Helm chart by adding: - tenant_admin_roles and tenant_idp_manager_roles - subject_tenants resolution from JWT organization claims - subject_realm_roles from realm_access.roles - is_tenant_admin and is_tenant_idp_manager checks - has_client_permissions (union of client + tenant roles) - Users API allow rules for tenant admins - Roles/RoleBindings Get/List in client allowlist - Response section: use subject_tenants instead of groups Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
Description
Currently, Keycloak Organizations don't have fine-grained access control, so organizational access will need to be managed through OPA in order to ensure admins only have management over their Organization.
Key changes
Testing
go build ./...passesginkgo run -r ./internal/cc @jhernand
Summary by CodeRabbit