feat: Add org-unit scoped custom role management #144
Conversation
WalkthroughIntroduces a complete custom role management system for organization units, spanning proto definitions, gateway routing enhancements for resource/verb tracking, permission evaluation logic with flexible matching, server-side CRUD operations with validation, and a new MongoDB-backed table layer with lifecycle management including soft delete and binding checks. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Gateway
participant AuthZ
participant CustomRoleTable
participant OrgUnitUserTable
Client->>Gateway: HTTP Request (resource, verb, instance)
Gateway->>Gateway: Extract resource, verb, resourceInstance from route
Gateway->>AuthZ: performOrgUnitRoleCheck(authInfo, ou, resource, verb, instance)
alt Built-in Role Grants Access
AuthZ->>AuthZ: Check admin/auditor roles
AuthZ-->>Gateway: ✓ Allowed
else Need Custom Role Check
AuthZ->>CustomRoleTable: Fetch custom roles for org-unit
CustomRoleTable-->>AuthZ: [CustomRole, ...]
loop For each custom role assigned to user
AuthZ->>AuthZ: evaluateCustomRolePermissions(perms, resource, verb, instance)
Note over AuthZ: Match resource vs. Exact/Prefix/Suffix/Regex<br/>Check verb in permission<br/>Deny takes precedence
alt Permission Found
AuthZ-->>AuthZ: Decision (Deny/Allow/Log)
else No Match
AuthZ->>AuthZ: Continue to next role
end
end
AuthZ-->>Gateway: ✓ or ✗ Based on Custom Perms
end
Gateway-->>Client: Response or 403 Forbidden
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.go (1)
411-419: Critical:clientvariable shadowed by Keycloak client breaks subsequent DB injections.
client, err := keycloak.New(...)creates a new localclientthat hides the DBclient. Controllers later receive the Keycloak client instead ofdb.StoreClient— compile/runtime breakage.- client, err := keycloak.New(conf.GetKeycloakEndpoint()) + kcClient, err := keycloak.New(conf.GetKeycloakEndpoint()) if err != nil { log.Panicf("failed to create keycloak client: %s", err) } defer func() { - _ = client.Logout(context.Background()) + _ = kcClient.Logout(context.Background()) }() @@ - _, err = tenant.NewSetupController(client) + _, err = tenant.NewSetupController(client) // db.StoreClient (unchanged)Also update any other references in this block from
client(Keycloak) tokcClient.Also applies to: 448-470
🧹 Nitpick comments (24)
api/org-unit-role.proto (9)
91-101: Use enums for role type and fix naming/typo.
- type as string invites drift; suggest an enum (BUILT_IN, CUSTOM).
- desc vs. description elsewhere is inconsistent; also “providing” typo on Line 88.
- // role description, provding details about the role - string desc = 2; + // Role description, providing details about the role + string desc = 2; // consider renaming to `description` (same tag=2 keeps wire-compat)Optional enum:
+enum RoleType { ROLE_TYPE_UNSPECIFIED = 0; ROLE_TYPE_BUILT_IN = 1; ROLE_TYPE_CUSTOM = 2; } - string type = 3; + RoleType type = 3;
114-129: Default/unspecified criteria semantics are ambiguous.Comment says “unspecified treated as wildcard” while WILDCARD is also a value. Prefer making WILDCARD the zero value to avoid dual meanings, or clarify server behavior.
-enum ResourceMatchCriteria { - RESOURCE_MATCH_CRITERIA_UNSPECIFIED = 0; - RESOURCE_MATCH_CRITERIA_EXACT = 1; - ... - RESOURCE_MATCH_CRITERIA_WILDCARD = 5; -} +enum ResourceMatchCriteria { + RESOURCE_MATCH_CRITERIA_WILDCARD = 0; // default + RESOURCE_MATCH_CRITERIA_EXACT = 1; + RESOURCE_MATCH_CRITERIA_PREFIX = 2; + RESOURCE_MATCH_CRITERIA_SUFFIX = 3; + RESOURCE_MATCH_CRITERIA_REGEX = 4; +}
152-166: Clarify DENY precedence and verbs normalization.
- Document conflict resolution when multiple RolePermission entries target same resource.
- Normalize verbs (lowercase, dedupe, non-empty) and decide behavior for
"*"combined with explicit verbs.I can add a tiny server-side sanitizer to enforce these invariants.
167-184: Add basic field validation hints.
- name: constrain charset/length (e.g.,
^[a-z0-9-]{3,64}$).- displayName/description: length bounds.
- permissions: non-empty, reasonable upper bound.
I can wire a validation proto comment block + server checks.
185-190: Return the created role, not a generic message.Clients usually need echo of canonicalized fields, timestamps, and creator. Consider mirroring GetCustomRoleResp here.
-message CreateCustomRoleResp { string message = 1; } +message CreateCustomRoleResp { GetCustomRoleResp role = 1; }
191-208: Partial update vs full replace; consider field presence.If fields are optional, mark as
optional(proto3) or use wrappers; otherwise document that omitted fields are cleared.I can prepare a PATCH variant + presence-aware fields if desired.
209-214: Update response should return the updated resource.Returning only a message is limiting; return the updated role snapshot.
-message UpdateCustomRoleResp { string message = 1; } +message UpdateCustomRoleResp { GetCustomRoleResp role = 1; }
225-249: Consider soft-delete flags in Get response.Since deletes are soft, add fields like deleted(bool), deletedAt, deletedBy to let clients reason about state.
I can add these fields and corresponding table indices.
251-264: Specify idempotent delete behavior and bindings precheck.Document if deleting an already-deleted or non-existent role is a no-op; include reason in response (blocked by bindings).
I can propose a small error model extension for “cannot delete due to bindings”.
pkg/gateway/routes.go (2)
19-28: Adding resource/verb to routeData is good; consider value return instead of pointer.You return a pointer to a copied
routeData; returning by value avoids accidental shared state confusion.-func matchRoute(m string, url string) (*routeData, string, []string, []string, error) { +func matchRoute(m string, url string) (routeData, string, []string, []string, error) { @@ - return &data, orgUnit, keys, values, nil + return data, orgUnit, keys, values, nilThis change requires updating call sites; I can provide a follow-up diff if desired.
42-70: populateRoutes correctly propagates resource/verb; ensure zero-value safety.If r.Resource or r.Verb are empty for some routes, downstream auth checks should handle it. Consider defaulting to empty strings explicitly or validating when inserting.
I can add a guard to log/warn on empty resource/verb.
pkg/server/org-unit-user.go (3)
29-47: Role validation centralization is solid; harden built-in role checks.
- Consider a small set/map for built-in roles and case normalization.
- Ensure soft-deleted custom roles are not considered valid by FindByNameAndOrgUnit.
Do we filter out soft-deleted roles in
FindByNameAndOrgUnit? I can add a test and a predicate at the table level.
106-107: Insert behavior: confirm duplicate semantics.Insert returns AlreadyExists — that’s fine. If you expect idempotent adds, switch to Locate/upsert guarded by ETag/version.
I can propose an upsert variant if needed.
139-140: Update: consider auditing Updated/UpdatedBy.Currently only Role is set; capture updater and timestamp if your model supports it.
I can extend table model and server write path accordingly.
pkg/gateway/server.go (3)
265-271: Normalize verbs to lowercase to avoid case-sensitive mismatches.Apply:
- verbMatches := false - for _, allowedVerb := range perm.Verbs { - if allowedVerb == "*" || allowedVerb == verb { + v := strings.ToLower(verb) + verbMatches := false + for _, allowedVerb := range perm.Verbs { + av := strings.ToLower(allowedVerb) + if av == "*" || av == v { verbMatches = true break } }
375-384: Path-param heuristics are brittle; drive instance key from route metadata.Hard-coding keys ("name", "id", "username", …) will miss routes using different param names and can bypass instance checks. Add an instanceParam (or resource key) to route metadata and use that instead.
Would you like a small patch to extend route.Routes with an InstanceParam and wire it through matchRoute?
305-313: Unused parameter in matchesResource; simplify.The match argument isn’t used. Remove it from signature and callers to reduce noise.
pkg/server/org-unit-role.go (3)
330-347: Normalize and deduplicate verbs during conversion.Lowercase, trim, and dedup to make permission checks robust.
Apply:
func (s *OrgUnitRoleServer) convertProtoPermissionsToTable(protoPerms []*api.RolePermission) []*table.RolePermission { var permissions []*table.RolePermission for _, perm := range protoPerms { + // normalize verbs + seen := make(map[string]struct{}, len(perm.Verbs)) + normVerbs := make([]string, 0, len(perm.Verbs)) + for _, v := range perm.Verbs { + vv := strings.ToLower(strings.TrimSpace(v)) + if vv == "" { + continue + } + if _, ok := seen[vv]; !ok { + seen[vv] = struct{}{} + normVerbs = append(normVerbs, vv) + } + } var match *table.ResourceMatch if perm.Match != nil { match = &table.ResourceMatch{ Criteria: s.convertProtoResourceMatchCriteriaToTable(perm.Match.Criteria), Key: perm.Match.Key, } } permissions = append(permissions, &table.RolePermission{ Resource: perm.Resource, Match: match, - Verbs: perm.Verbs, + Verbs: normVerbs, Action: s.convertProtoActionToTable(perm.Action), }) } return permissions }
90-102: Reserved names: compare case-insensitively and after trimming.Avoid “Admin” vs “admin” bypass.
Apply:
- if req.Name == "admin" || req.Name == "default" || req.Name == "auditor" { + n := strings.TrimSpace(strings.ToLower(req.Name)) + if n == "admin" || n == "default" || n == "auditor" { return nil, status.Errorf(codes.InvalidArgument, "Role name '%s' is reserved and cannot be used for custom roles", req.Name) }
56-61: List roles uses a fixed limit (1000); add pagination to API.Expose offset/limit on ListOrgUnitRoles and thread through to the table to avoid large payloads.
I can draft the proto/Swagger changes and handler wiring if helpful.
api/swagger/apidocs.swagger.json (2)
2522-2529: Cap permissions array size to match server validation (max 20).Add minItems/maxItems to align with backend limits and satisfy static analysis (CKV_OPENAPI_21).
Apply:
"permissions": { "type": "array", + "minItems": 1, + "maxItems": 20, "items": { "type": "object", "$ref": "#/definitions/apiRolePermission" }, "title": "List of permissions granted by this custom role" }Repeat the same for OrgUnitRoleUpdateCustomRoleBody.permissions.
Also applies to: 2544-2551
3647-3652: Set verbs array minItems to 1 (matches server validation).Apply:
"verbs": { "type": "array", + "minItems": 1, "items": { "type": "string" }, "title": "List of allowed verbs/actions for this resource (supports \"*\" for all verbs)" }pkg/table/org-unit-role.go (2)
172-193: HasBindings: confirm you intend to block delete for disabled users too.Current check counts all assignments regardless of user/assignment status. If only active bindings should block, add the appropriate filter.
274-278: Event logger lifecycle.Start() without a context cancel or stop hook can leak goroutines on shutdown. Consider wiring a repo-wide shutdown/cancel.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📥 Commits
Reviewing files that changed from the base of the PR and between 9233c9a and 6089d63379aaac3957ae906d00937a54051d3e5b.
⛔ Files ignored due to path filters (3)
api/org-unit-role.pb.gois excluded by!**/*.pb.goapi/org-unit-role.pb.gw.gois excluded by!**/*.pb.gw.goapi/org-unit-role_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
api/org-unit-role.pb.route.go(1 hunks)api/org-unit-role.proto(4 hunks)api/swagger/apidocs.swagger.json(8 hunks)main.go(1 hunks)pkg/gateway/routes.go(7 hunks)pkg/gateway/server.go(9 hunks)pkg/server/org-unit-role.go(1 hunks)pkg/server/org-unit-user.go(6 hunks)pkg/table/const.go(1 hunks)pkg/table/org-unit-role.go(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
api/swagger/apidocs.swagger.json
[medium] 2522-2529: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (10)
api/org-unit-role.proto (1)
27-75: RPCs and HTTP bindings look coherent; confirm idempotency and partial‑update semantics.
- Endpoints/verbs align with conventions; scope/resource metadata matches.
- For Update, if you intend partial updates, prefer PATCH or mark fields optional and document merge behavior; PUT typically requires full replacement.
How should missing fields in UpdateCustomRoleReq be interpreted (clear vs. no‑change)? If partial merge is intended, consider
optionalproto fields or wrappers for presence, and switch to PATCH in routing. Do you want a patch diff for that?pkg/table/const.go (1)
30-32: LGTM: collection name aligns with existing naming (kebab plural).Confirm the table implementation uses this constant consistently (indexes, TTLs, event logger).
main.go (1)
396-406: LGTM: table locate + event logger wired consistently with other tables.Ensure StartEventLogger is non-blocking and resilient to transient store outages.
api/org-unit-role.pb.route.go (1)
20-47: Routes mirror proto bindings correctly.Resource, verb, and ou-scope look consistent with the proto annotations.
Confirm these are registered in the route table at startup (OrgUnitRole server init) and that swagger reflects them. I can script-check registration if you want.
pkg/server/org-unit-user.go (4)
25-27: LGTM: wiring customRoleTable into the server state.
86-90: Good: validate role on Add before write.We should error with InvalidArgument on unknown roles — matches your helper. Add a unit test?
124-128: Good: validate role on Update before write.
180-188: LGTM: table getters used; panic on setup failure is consistent with rest.pkg/gateway/routes.go (1)
78-145: Verify slice ownership semantics with patricia library and confirm defensive copy necessity.The review raises two concerns:
HTTP method in error messages: Valid suggestion—though the call site in server.go:371 already adds method context, matchRoute's own error messages (lines 86, 104) could include the method for better debugging at the source.
Slice copying from patricia: The concern depends on patricia's Match() internals. The usage pattern shows keys/values are consumed immediately and synchronously (server.go:378–384) within a for loop before any async or deferred operations that might invalidate them. This suggests the immediate usage is safe, though defensive copying would be more robust if patricia returns slices backed by internal structures.
Finding: The single call site (server.go:368) already matches the current 5-return signature—no outdated callers exist to update.
Recommended next step: Consult go-core-stack/patricia documentation or source to confirm slice backing behavior. If confirmed backed, adding
keys = append([]string{}, keys...)andvalues = append([]string{}, values...)is a small safety improvement. Otherwise, the current code is sufficient given the synchronous usage pattern.api/swagger/apidocs.swagger.json (1)
3661-3669: Action semantics: ensure docs match enforcement (UNSPECIFIED is invalid).Docs already state UNSPECIFIED is invalid; after server/gateway changes, behavior will align.
Would you like me to open a follow-up to confirm generated Swagger from proto matches this description post-change?
| // For non-list operations, check if the resource instance matches the criteria | ||
| // List operations skip instance matching (show all, filter on individual access) | ||
| if verb != "list" && resourceInstance != "" { | ||
| if !s.matchesResourceInstance(resourceInstance, perm.Match) { | ||
| continue | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Instance-guard: deny when permission has match but instance is missing.
Current logic skips instance checks if resourceInstance is empty, unintentionally broadening access when a permission defines a Match. Enforce instance presence when Match is set for non-list verbs.
Apply:
- // For non-list operations, check if the resource instance matches the criteria
- // List operations skip instance matching (show all, filter on individual access)
- if verb != "list" && resourceInstance != "" {
- if !s.matchesResourceInstance(resourceInstance, perm.Match) {
- continue
- }
- }
+ // For non-list operations, enforce instance matching when a match is defined.
+ // If the permission defines Match, an instance must be present and must match.
+ if verb != "list" && perm.Match != nil {
+ if resourceInstance == "" {
+ continue
+ }
+ if !s.matchesResourceInstance(resourceInstance, perm.Match) {
+ continue
+ }
+ }📝 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.
| // For non-list operations, check if the resource instance matches the criteria | |
| // List operations skip instance matching (show all, filter on individual access) | |
| if verb != "list" && resourceInstance != "" { | |
| if !s.matchesResourceInstance(resourceInstance, perm.Match) { | |
| continue | |
| } | |
| } | |
| // For non-list operations, enforce instance matching when a match is defined. | |
| // If the permission defines Match, an instance must be present and must match. | |
| if verb != "list" && perm.Match != nil { | |
| if resourceInstance == "" { | |
| continue | |
| } | |
| if !s.matchesResourceInstance(resourceInstance, perm.Match) { | |
| continue | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/gateway/server.go around lines 277 to 284, the current non-list instance
check skips validation when resourceInstance is empty which allows access if a
permission defines a Match; change the logic so that for non-list verbs you
first check if perm.Match is non-empty and if so require resourceInstance to be
present (if resourceInstance == "" then treat as no match/deny and continue),
otherwise if perm.Match is empty keep the existing behavior; ensure list verbs
still skip instance matching.
| col := client.GetCollection(AuthDatabaseName, OrgUnitCustomRoleCollectionName) | ||
| tbl := &OrgUnitCustomRoleTable{ | ||
| col: col, | ||
| } | ||
|
|
||
| err := tbl.Initialize(col) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| orgUnitCustomRoleTable = tbl | ||
|
|
||
| return orgUnitCustomRoleTable, nil |
There was a problem hiding this comment.
🧩 Analysis chain
Add a unique compound index on key.tenant+key.orgUnitId+key.name.
Guarantees uniqueness and accelerates lookups used across the table methods.
If no index exists, let me draft an Initialize-time index creation mirroring your db helper patterns.
🏁 Script executed:
#!/bin/bash
# Search for existing index creation on OrgUnitCustomRole collection
rg -nP -C2 'OrgUnitCustomRole(Collection|Table)|EnsureIndex|CreateIndex|Indexes|IndexModel'Length of output: 13080
🏁 Script executed:
#!/bin/bash
# Search for Initialize method implementation in org-unit-role.go
rg -nP -A10 'func \(t \*OrgUnitCustomRoleTable\) Initialize' pkg/table/org-unit-role.goLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for how other table types create indexes - look for patterns
rg -nP -B2 -A5 'CreateIndex|EnsureIndex|IndexModel' pkg/table/ -g '*.go' | head -100Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for Table interface definition to understand expected Initialize signature
rg -nP -B2 -A10 'type Table\[' pkg/table/ -g '*.go' | head -50Length of output: 0
🏁 Script executed:
#!/bin/bash
# List all methods in OrgUnitCustomRoleTable - grep for func
rg -n 'func' pkg/table/org-unit-role.go | grep -i 'initialize\|table'Length of output: 1548
🏁 Script executed:
#!/bin/bash
# Check similar table files for Initialize implementation patterns
for file in pkg/table/org-unit.go pkg/table/org-unit-user.go pkg/table/tenant.go; do
echo "=== File: $file ==="
rg -n -A8 'func.*Initialize' "$file" 2>/dev/null || echo "No Initialize found"
doneLength of output: 170
🏁 Script executed:
#!/bin/bash
# Search for Table embedded type and check imports
rg -n 'type OrgUnitCustomRoleTable struct' -A5 pkg/table/org-unit-role.goLength of output: 211
🏁 Script executed:
#!/bin/bash
# Look for db package and Table interface definition
fd -e go 'table\.go|types\.go' pkg/db/ 2>/dev/null | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get imports and full context around Initialize call in org-unit-role.go
head -20 pkg/table/org-unit-role.goLength of output: 513
🏁 Script executed:
#!/bin/bash
# Show full context of LocateOrgUnitCustomRoleTable function (lines 289-307 area)
rg -n -B5 -A15 'func LocateOrgUnitCustomRoleTable' pkg/table/org-unit-role.goLength of output: 643
🏁 Script executed:
#!/bin/bash
# Search for Table type definition - check in table package first
fd -e go . pkg/table/ | xargs rg -l 'type Table\[' | head -3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for Table generic type definition
rg -n 'type Table\[' pkg/table/ -g '*.go'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check db package structure
ls -la pkg/db/ 2>/dev/null | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any MongoDB index-related operations in codebase
rg -n 'Indexes|CreateIndexModel|IndexModel|mongo.*Index' . --type=go --type=proto 2>/dev/null | grep -v third_party | head -30Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for collection.Indexes() pattern calls
rg -n '\.Indexes\(' . --type=go 2>/dev/null | grep -v third_partyLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any db/store collection patterns with index creation
rg -n 'StoreCollection.*Index|EnsureIndex|index' . --type=go 2>/dev/null | grep -v third_party | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if go-core-stack is in go.mod to understand version/capabilities
rg 'go-core-stack' go.mod 2>/dev/nullLength of output: 368
Add unique compound index on (key.tenant, key.orgUnitId, key.name) to OrgUnitCustomRoleTable.
The queries FindByNameAndOrgUnit, GetByOrgUnit, and FindAnyByNameAndOrgUnit filter on these fields and would benefit from index acceleration. A unique compound index also enforces data integrity. Add index creation in LocateOrgUnitCustomRoleTable after initializing the collection (around line 300), following MongoDB driver patterns with collection.Indexes().CreateOne().
🤖 Prompt for AI Agents
In pkg/table/org-unit-role.go around lines 295 to 307, after tbl.Initialize(col)
(around line 300) add creation of a unique compound index on the fields
"key.tenant", "key.orgUnitId", and "key.name" using col.Indexes().CreateOne(...)
following MongoDB driver patterns; build an IndexModel with the compound keys in
ascending order and Options.SetUnique(true), call CreateOne and handle/return
any error so table creation fails if the index cannot be created.
Suryanshu185
force-pushed
the
feat/custom-roles-impl
branch
from
6089d63 to
880b48e
Compare
- Add OrgUnitCustomRole table with MongoDB schema - Define ResourceMatchCriteria: Any, Exact, Prefix, Suffix, Regex - Define RolePermissionAction: Deny, Allow, Log (secure by default) - Remove Unspecified enum values - Remove displayName field - Add soft delete support with binding checks - Update protobuf definitions to use enum wrappers (Defs pattern) Signed-off-by: suryanshu185 <suryanshu.gupta@kluisz.ai>
- Add org-unit-role.proto with 5 RPC methods - Generate gRPC/gateway code and routes - Add stub server implementation - Update Swagger documentation - ListOrgUnitRoles returns built-in roles (admin, auditor, default) - CRUD operations return Unimplemented status Signed-off-by: suryanshu185 <suryanshu.gupta@kluisz.ai>
Suryanshu185
force-pushed
the
feat/custom-roles-impl
branch
from
880b48e to
052f588
Compare
Implement comprehensive custom role management system for organization units with fine-grained permission-based access control (PBAC) and response filtering capabilities. Core Features: - Full CRUD operations for custom roles (Create, Read, Update, Delete) - Org-unit scoped roles with tenant isolation - Dynamic role validation for user assignments - Soft delete with binding checks (prevents deletion if users assigned) - Response filtering for list operations based on role permissions Signed-off-by: suryanshu185 <suryanshu.gupta@kluisz.ai>
Suryanshu185
force-pushed
the
feat/custom-roles-impl
branch
from
052f588 to
2e5eaf4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
main.go (2)
452-460: Critical: variable shadowing replaces DB client with Keycloak client, breaking subsequent calls.
client, err := keycloak.New(...)shadows the package-levelclient db.StoreClient. All later uses (e.g., Tenant/User controllers) will receive the Keycloak client, causing compile/runtime issues.Fix by using a distinct name (e.g.,
kcClient).- // create a new keycloak client - client, err := keycloak.New(conf.GetKeycloakEndpoint()) + // create a new keycloak client + kcClient, err := keycloak.New(conf.GetKeycloakEndpoint()) if err != nil { // failed to create keycloak client, nothing more can be done - log.Panicf("failed to create keycloak client: %s", err) + log.Panicf("failed to create keycloak client: %s", err) } defer func() { - _ = client.Logout(context.Background()) + _ = kcClient.Logout(context.Background()) }()Also applies to: 487-515
421-425: Wrong log message: table name mismatch.Starting event logger for Org Unit table, but the panic message mentions “Org Unit User table.”
- if err != nil { - log.Panicf("failed to start event logger for Org Unit User table: %s", err) - } + if err != nil { + log.Panicf("failed to start event logger for Org Unit table: %s", err) + }pkg/server/org-unit-user.go (1)
69-73: Minor typo in list response placeholders.“Dummy Fist Name” → “Dummy First Name”.
- Firstname: "Dummy Fist Name", + Firstname: "Dummy First Name", Lastname: "Dummy Last Name",
♻️ Duplicate comments (2)
pkg/gateway/server.go (1)
300-306: Instance-guard: deny when permission has Match but instance is missing.The current logic skips instance checks when
resourceInstanceis empty, which unintentionally grants access even when a permission definesMatchcriteria. For non-list verbs, ifperm.Matchis set, an instance must be present and must match.Apply the fix from the previous review:
- // For non-list operations, check if the resource instance matches the criteria - // List operations skip instance matching (show all, filter on individual access) - if verb != "list" && resourceInstance != "" { - if !s.matchesResourceInstance(resourceInstance, perm.Match) { - continue - } - } + // For non-list operations, enforce instance matching when a match is defined. + // If the permission defines Match, an instance must be present and must match. + if verb != "list" && perm.Match != nil { + if resourceInstance == "" { + continue + } + if !s.matchesResourceInstance(resourceInstance, perm.Match) { + continue + } + }pkg/table/org-unit-role.go (1)
286-305: Missing unique compound index on (tenant, orgUnitId, name).The table lacks a unique compound index on the key fields. This impacts both performance and data integrity, as the database won't enforce uniqueness at the storage layer.
Add index creation after line 300:
err := tbl.Initialize(col) if err != nil { return nil, err } + + // Create unique compound index on the key fields + indexModel := mongo.IndexModel{ + Keys: bson.D{ + {Key: "key.tenant", Value: 1}, + {Key: "key.orgUnitId", Value: 1}, + {Key: "key.name", Value: 1}, + }, + Options: options.Index().SetUnique(true), + } + _, err = col.Indexes().CreateOne(context.Background(), indexModel) + if err != nil { + return nil, errors.Wrapf(err, "failed to create unique index on custom role table") + } orgUnitCustomRoleTable = tbl return orgUnitCustomRoleTable, nilDon't forget to add the required imports:
import ( "go.mongodb.org/mongo-driver/mongo" "go.mongodb.org/mongo-driver/mongo/options" )
🧹 Nitpick comments (7)
pkg/gateway/routes.go (1)
78-145: Optional: return routeData by value to avoid pointer-to-copy confusion.You return
&datawheredatais a copy from the map. It’s safe (heap-escaped), but returning by value avoids confusion and accidental mutation expectations.-func matchRoute(m string, url string) (*routeData, string, []string, []string, error) { +func matchRoute(m string, url string) (routeData, string, []string, []string, error) { ... - return &data, orgUnit, keys, values, nil + return data, orgUnit, keys, values, nilNote: adjust call sites accordingly.
pkg/server/org-unit-user.go (1)
29-47: Normalize role input; avoid case/whitespace bugs.Lowercase/trim
rolebefore checks; preserves behavior and avoids surprises.@@ -import ( +import ( "context" "log" "time" + "strings" @@ func (s *OrgUnitUserServer) isValidRole(ctx context.Context, tenant, orgUnitId, role string) error { - // Check if it's a built-in role - if role == "admin" || role == "auditor" { + role = strings.ToLower(strings.TrimSpace(role)) + // Check if it's a built-in role + if role == "admin" || role == "auditor" { return nil } // Check if it's a valid custom role _, err := s.customRoleTable.FindByNameAndOrgUnit(ctx, tenant, orgUnitId, role)pkg/gateway/server.go (1)
400-409: Consider resource-specific instance extraction.The current generic approach iterates through common parameter names and breaks on the first match. This may miss resource-specific identifiers or select an incorrect parameter when multiple keys match (e.g., a route with both
{id}and{name}).Consider mapping resource types to their specific instance parameter names:
// Extract resource instance name from URL path parameters resourceInstance := "" if match.resource != "" { // Resource-specific parameter mapping paramName := "" switch match.resource { case "s3-object": paramName = "object" case "bucket": paramName = "bucket" case "user": paramName = "username" default: // Fall back to common names for unmapped resources for i, k := range keys { if k == "name" || k == "id" { paramName = k break } } } // Extract value for the determined parameter if paramName != "" { for i, k := range keys { if k == paramName { resourceInstance = values[i] break } } } }pkg/server/org-unit-role.go (1)
141-141: Consider more idiomatic bool pointer creation.The expression
&[]bool{true}[0]creates a bool pointer but is less readable than alternatives.Consider using a helper or inline variable:
+ active := true customRole := &table.OrgUnitCustomRole{ Key: &table.OrgUnitCustomRoleKey{ Tenant: authInfo.Realm, OrgUnitId: req.Ou, Name: req.Name, }, Description: req.Description, Permissions: permissions, Created: time.Now().Unix(), CreatedBy: authInfo.UserName, - Active: &[]bool{true}[0], + Active: &active, }pkg/table/org-unit-role.go (3)
169-190: Consider adding a public method to OrgUnitUserTable.The
HasBindingsmethod directly accessesorgUnitUserTable.colto count role assignments. For better encapsulation and maintainability, consider adding a publicCountByRolemethod toOrgUnitUserTable.In
pkg/table/org-unit-user.go, add:// CountByRole counts users assigned to a specific role in an org unit func (t *OrgUnitUserTable) CountByRole(ctx context.Context, tenant, orgUnitId, roleName string) (int64, error) { filter := bson.M{ "key.tenant": tenant, "key.orgUnitId": orgUnitId, "role": roleName, } return t.col.Count(ctx, filter) }Then update
HasBindingsin this file:func (t *OrgUnitCustomRoleTable) HasBindings(ctx context.Context, tenant, orgUnitId, roleName string) (bool, error) { orgUnitUserTable, err := GetOrgUnitUserTable() if err != nil { return false, err } - filter := bson.M{ - "key.tenant": tenant, - "key.orgUnitId": orgUnitId, - "role": roleName, - } - - count, err := orgUnitUserTable.col.Count(ctx, filter) + count, err := orgUnitUserTable.CountByRole(ctx, tenant, orgUnitId, roleName) if err != nil { return false, err } return count > 0, nil }
95-108: Clarify active field filter behavior.The filter
"active": bson.M{"$ne": false}correctly handles roles whereActiveistrueornil(unset). However, this implicit nil-as-active behavior could be surprising.Consider documenting this more explicitly:
func (t *OrgUnitCustomRoleTable) GetByOrgUnit(ctx context.Context, tenant, orgUnitId string, offset, limit int32) ([]*OrgUnitCustomRole, error) { filter := bson.M{ "key.tenant": tenant, "key.orgUnitId": orgUnitId, - "active": bson.M{"$ne": false}, // Include roles where active is true or nil + "active": bson.M{"$ne": false}, // Include roles where active is true or nil (nil for backward compatibility) }Alternatively, consider explicitly setting
Active: &trueduring initialization and migrations to eliminate nil ambiguity.
158-167: Consider more idiomatic bool pointer creation.Similar to the server code, the expression
&[]bool{false}[0]is less readable than alternatives.func (t *OrgUnitCustomRoleTable) SoftDelete(ctx context.Context, key *OrgUnitCustomRoleKey, deletedBy string) error { + active := false update := &OrgUnitCustomRole{ - Active: &[]bool{false}[0], + Active: &active, UpdatedBy: deletedBy, Updated: time.Now().Unix(), } return t.Update(ctx, key, update) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📥 Commits
Reviewing files that changed from the base of the PR and between 6089d63379aaac3957ae906d00937a54051d3e5b and 2e5eaf4.
📒 Files selected for processing (8)
api/org-unit-role.proto(1 hunks)main.go(1 hunks)pkg/gateway/routes.go(7 hunks)pkg/gateway/server.go(9 hunks)pkg/server/org-unit-role.go(1 hunks)pkg/server/org-unit-user.go(6 hunks)pkg/table/const.go(1 hunks)pkg/table/org-unit-role.go(1 hunks)
🔇 Additional comments (11)
pkg/table/const.go (1)
33-35: LGTM: collection name added and consistent with existing conventions.api/org-unit-role.proto (1)
249-251: No functional changes in this hunk; proto surface remains stable.main.go (1)
437-446: LGTM: OU custom-role table located and event logger started.Wiring mirrors OU/OU-User patterns and panics on failure, consistent with startup behavior.
pkg/server/org-unit-user.go (1)
180-188: LGTM: custom role table wired into server initialization.Centralized validation via
isValidRoleis a solid improvement.pkg/gateway/routes.go (1)
26-28: Guard against empty resource/verb in route entries — manual verification required.The route.Route struct comes from external package
github.com/go-core-stack/auth/route, so I cannot inspect whether Resource/Verb can be empty. However, the current code assigns them directly without validation (lines 54–55, 67–68), and downstream permission evaluation would silently accept empty values. The suggestion is reasonable defensive programming, but confirming whether this risk exists requires:
- Checking if the external route package can produce empty Resource/Verb
- Reviewing how routes are created in your actual deployment
- Determining if this guard is necessary for your specific usage
pkg/gateway/server.go (2)
366-381: LGTM: Wildcard pattern matching.The implementation correctly handles wildcard patterns by converting them to regex with proper anchoring. Error handling appropriately logs and denies access on invalid patterns.
633-636: LGTM: Custom role table initialization.The initialization follows the established pattern for table wiring and appropriately panics on startup failures.
Also applies to: 660-660
pkg/server/org-unit-role.go (2)
363-408: LGTM: Comprehensive validation.The validation correctly enforces length constraints, permission limits, and rejects invalid action values including UNSPECIFIED. Well done!
447-482: LGTM: Server initialization.The initialization follows established patterns and appropriately handles errors during startup.
pkg/table/org-unit-role.go (2)
28-66: LGTM: Clean type definitions.The type definitions use string constants for enums, which ensures stability across versions and aligns with MongoDB storage. The exclusion of "Unspecified" action is correct and enforces explicit intent.
237-269: LGTM: Cleanup logic with resilient error handling.The cleanup implementation correctly identifies orphaned soft-deleted roles and removes them. Error handling appropriately continues processing remaining roles if individual operations fail. The use of
FindMany(ctx, filter, 0, 0)with limit=0 returns all the documents, which is standard MongoDB behavior, confirming the "Get all" intent is correct.
| // Validate that role name is not one of the system reserved names | ||
| if req.Name == "admin" || req.Name == "default" || req.Name == "auditor" { | ||
| return nil, status.Errorf(codes.InvalidArgument, "Role name '%s' is reserved and cannot be used for custom roles", req.Name) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify "default" as reserved name.
The validation rejects "default" as a reserved name, but it doesn't appear in the built-in roles list in ListOrgUnitRoles (lines 37-52). Confirm whether "default" should be reserved or remove it from this check.
🏁 Script executed:
#!/bin/bash
# Search for "default" role usage in the codebase
rg -nP '\b(default|Default)\b.*role' --type=go -C3Length of output: 538
---
🏁 Script executed:
```shell
#!/bin/bash
# Look at ListOrgUnitRoles function to see what roles are defined
rg -nA20 'func.*ListOrgUnitRoles' --type=go
Length of output: 9814
🏁 Script executed:
#!/bin/bash
# Search for "default" as a reserved role more broadly
rg -nP '"default"\s*' pkg/server/org-unit-role.go -C5Length of output: 644
🏁 Script executed:
#!/bin/bash
# Check for any system reserved role definitions
rg -nP 'reserved|system.*role' --type=go -iLength of output: 6048
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full ListOrgUnitRoles function body to see all built-in roles
rg -nA30 'items := \[\]\*api.OrgUnitRolesListEntry' pkg/server/org-unit-role.go
Length of output: 1031
Remove "default" from reserved role validation—it's not defined as a built-in role.
The validation at lines 88–89 rejects "default" as a reserved name, but it doesn't exist in the built-in roles defined in ListOrgUnitRoles (lines 37–52), which only contains "admin" and "auditor". The proto file documentation further confirms only these two are system roles. Remove "default" from the reserved names check.
// Before:
if req.Name == "admin" || req.Name == "default" || req.Name == "auditor" {
// After:
if req.Name == "admin" || req.Name == "auditor" {🤖 Prompt for AI Agents
In pkg/server/org-unit-role.go around lines 87 to 90, the validation incorrectly
treats "default" as a reserved role name; remove "default" from the conditional
so only "admin" and "auditor" are rejected. Update the if check to only compare
req.Name against "admin" and "auditor" and keep the same error returned for
those cases.
| // Check if there's an existing role (including soft-deleted ones) with the same name | ||
| existingRole, err := s.customRoleTable.FindAnyByNameAndOrgUnit(ctx, authInfo.Realm, req.Ou, req.Name) | ||
| if err == nil { | ||
| // Role exists - check if it's active or soft-deleted with bindings | ||
| if existingRole.Active == nil || *existingRole.Active { | ||
| return nil, status.Errorf(codes.AlreadyExists, "Custom role '%s' already exists in organization unit", req.Name) | ||
| } | ||
|
|
||
| // Soft-deleted role exists - check if it has bindings | ||
| hasBindings, err := s.customRoleTable.HasBindings(ctx, authInfo.Realm, req.Ou, req.Name) | ||
| if err != nil { | ||
| log.Printf("failed to check bindings for role %s: %s", req.Name, err) | ||
| return nil, status.Errorf(codes.Internal, "Something went wrong, please try again later") | ||
| } | ||
|
|
||
| if hasBindings { | ||
| return nil, status.Errorf(codes.AlreadyExists, "Custom role '%s' already exists in organization unit", req.Name) | ||
| } | ||
|
|
||
| // Soft-deleted role without bindings - can be permanently removed and recreated | ||
| key := &table.OrgUnitCustomRoleKey{ | ||
| Tenant: authInfo.Realm, | ||
| OrgUnitId: req.Ou, | ||
| Name: req.Name, | ||
| } | ||
| err = s.customRoleTable.PermanentDelete(ctx, key) | ||
| if err != nil { | ||
| log.Printf("failed to permanently delete orphaned role %s: %s", req.Name, err) | ||
| return nil, status.Errorf(codes.Internal, "Something went wrong, please try again later") | ||
| } | ||
| } else if !errors.IsNotFound(err) { | ||
| log.Printf("failed to check existing role %s: %s", req.Name, err) | ||
| return nil, status.Errorf(codes.Internal, "Something went wrong, please try again later") | ||
| } |
There was a problem hiding this comment.
Race condition: binding check and permanent delete are not atomic.
Between checking HasBindings (line 101) and calling PermanentDelete (line 117), another concurrent request could assign the role to a user. This creates a window where a role with bindings could be permanently deleted, leaving orphaned user assignments.
Consider one of these approaches:
- Transactional approach (if MongoDB transactions are available):
// Wrap the binding check and delete in a transaction
session, err := client.StartSession()
if err != nil {
return nil, status.Errorf(codes.Internal, "Something went wrong, please try again later")
}
defer session.EndSession(ctx)
err = mongo.WithSession(ctx, session, func(sc mongo.SessionContext) error {
hasBindings, err := s.customRoleTable.HasBindings(sc, authInfo.Realm, req.Ou, req.Name)
if err != nil {
return err
}
if hasBindings {
return status.Errorf(codes.AlreadyExists, "Custom role '%s' already exists in organization unit", req.Name)
}
return s.customRoleTable.PermanentDelete(sc, key)
})- Simpler approach: Don't permanently delete during creation. Let the periodic cleanup job handle orphaned soft-deleted roles:
- // Soft-deleted role without bindings - can be permanently removed and recreated
- key := &table.OrgUnitCustomRoleKey{
- Tenant: authInfo.Realm,
- OrgUnitId: req.Ou,
- Name: req.Name,
- }
- err = s.customRoleTable.PermanentDelete(ctx, key)
- if err != nil {
- log.Printf("failed to permanently delete orphaned role %s: %s", req.Name, err)
- return nil, status.Errorf(codes.Internal, "Something went wrong, please try again later")
- }
+ // Soft-deleted role exists - prevent reuse even without bindings
+ // This avoids race conditions and maintains audit trail
+ return nil, status.Errorf(codes.AlreadyExists, "Custom role '%s' already exists in organization unit", req.Name)1 participant
Implement comprehensive custom role management system for organization units with fine-grained permission-based access control (PBAC) and response filtering capabilities.
Summary by CodeRabbit