feat: implement Log action for custom role permissions.#145
feat: implement Log action for custom role permissions.#145Suryanshu185 wants to merge 3 commits into
Conversation
- 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>
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>
WalkthroughAdds support for org-unit scoped custom roles: protobuf RPCs and HTTP routes for CRUD, gateway RBAC evaluation for custom-role permissions, a persistent OrgUnitCustomRole table with soft-delete/binding checks and event logging, server handlers for role lifecycle, and initialization wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant RouteMatch
participant PermEval as PermissionEval
participant CustomRoleTbl as OrgUnitCustomRoleTable
Client->>Gateway: HTTP request (method, path)
Gateway->>RouteMatch: matchRoute(method, path)
RouteMatch-->>Gateway: routeData, orgUnit, keys, values
rect rgb(230, 245, 230)
Note over Gateway,PermEval: Permission evaluation
Gateway->>PermEval: check built-in roles
alt built-in allows
PermEval-->>Gateway: allow
else built-in denies or no-match
Gateway->>CustomRoleTbl: GetByOrgUnit(orgUnit)
CustomRoleTbl-->>PermEval: custom roles
PermEval->>PermEval: evaluate permissions (resource, instance, verb)
alt match with DENY
PermEval-->>Gateway: deny
else match with ALLOW
PermEval-->>Gateway: allow
else match with LOG
PermEval->>PermEval: log decision
end
end
end
alt allowed
Gateway-->>Client: 200 / forwarded
else denied
Gateway-->>Client: 403 Forbidden
end
sequenceDiagram
participant Client
participant Server as OrgUnitRoleServer
participant Validator
participant Tbl as OrgUnitCustomRoleTable
Client->>Server: CreateCustomRoleReq
Server->>Validator: validateCreateCustomRoleRequest
Validator-->>Server: ok / error
alt valid
Server->>Tbl: FindAnyByNameAndOrgUnit(name)
alt exists (active)
Server-->>Client: AlreadyExists error
else exists (soft-deleted, no bindings)
Server->>Tbl: PermanentDelete(old)
Tbl-->>Server: deleted
Server->>Tbl: Insert new role
Tbl-->>Server: created
Server-->>Client: CreateCustomRoleResp
end
else invalid
Server-->>Client: InvalidArgument error
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/gateway/routes.go (1)
78-146: Critical: Returning pointer to map value copy creates memory safety violation.The review comment is correct. The current code at line 144 returns
&data, wheredatais a local copy from reading the map. In Go, map reads return value copies, and taking their address creates an unsafe pointer to a stack variable.The proposed fix is sound:
- Change
type routeNodesfrommap[route.MethodType]routeDatatomap[route.MethodType]*routeData- Update assignments at lines 46–50 and 60 to wrap values with
&routeData{...}- Change line 144 from
return &data, ...toreturn data, ...(data becomes*routeDatafrom map read)All affected code paths are in this file and have been verified. No external code depends on the routeData type directly.
🧹 Nitpick comments (9)
pkg/server/org-unit-user.go (2)
29-47: Normalize role and avoid string literals for built-ins.Trim/EqualFold role names and use shared constants to prevent casing/whitespace issues and drift.
Apply:
@@ -import ( +import ( "context" "log" "time" + "strings" @@ -func (s *OrgUnitUserServer) isValidRole(ctx context.Context, tenant, orgUnitId, role string) error { +func (s *OrgUnitUserServer) isValidRole(ctx context.Context, tenant, orgUnitId, role string) error { + r := strings.TrimSpace(role) + if r == "" { + return status.Errorf(codes.InvalidArgument, "Role must be non-empty") + } // Check if it's a built-in role - if role == "admin" || role == "auditor" { + if strings.EqualFold(r, "admin") || strings.EqualFold(r, "auditor") { return nil } // Check if it's a valid custom role - _, err := s.customRoleTable.FindByNameAndOrgUnit(ctx, tenant, orgUnitId, role) + _, err := s.customRoleTable.FindByNameAndOrgUnit(ctx, tenant, orgUnitId, r) @@ - return status.Errorf(codes.InvalidArgument, "Invalid role: '%s'. Must be 'admin', 'auditor', or a valid custom role", role) + return status.Errorf(codes.InvalidArgument, "Invalid role: '%s'. Must be 'admin', 'auditor', or a valid custom role", r)Optionally centralize built-ins in a const slice to reuse across packages.
86-90: Role validation integration — OK. Add brief context on failure.Calls correctly propagate status errors. Consider wrapping NotFound with role and OU for easier client diagnosis.
Also applies to: 124-128
pkg/gateway/server.go (3)
353-365: Escape regex meta in wildcard patterns to avoid unintended matches.Replacing “” with “.” without escaping other meta chars makes patterns like bucket.prod match as regex. Quote the pattern first.
Apply:
- // Convert wildcard pattern to regex - pattern := strings.ReplaceAll(match.Key, "*", ".*") - pattern = "^" + pattern + "$" + // Convert wildcard pattern to safe regex + // 1) Quote meta, 2) turn escaped \* back into .* + pattern := regexp.QuoteMeta(match.Key) + pattern = strings.ReplaceAll(pattern, `\*`, ".*") + pattern = "^" + pattern + "$"
252-255: Use structured zap logger for audit events (consistent format/rotation).Switch from log.Printf to zap fields; same logger used in access logs.
Apply:
- log.Printf("[LOG] User: %s, Tenant: %s, OrgUnit: %s, Role: %s, Resource: %s, Verb: %s, Instance: %s, Method: %s, Path: %s, Allowed: %v", - authInfo.UserName, authInfo.Realm, ou, ouUser.Role, resource, verb, resourceInstance, r.Method, r.URL.Path, allowed) + logger.Info("custom_role_audit", + zap.String("username", authInfo.UserName), + zap.String("tenant", authInfo.Realm), + zap.String("org_unit", ou), + zap.String("role", ouUser.Role), + zap.String("resource", resource), + zap.String("verb", verb), + zap.String("instance", resourceInstance), + zap.String("method", r.Method), + zap.String("path", r.URL.Path), + zap.Bool("allowed", allowed), + )
392-398: Resource instance extraction is brittle.Hard-coding keys ("name", "id", "username", "bucket", "object") will miss routes with different param names. Consider reading a resourceInstanceKey from route metadata or falling back to the last path parameter.
api/swagger/apidocs.swagger.json (2)
2508-2532: Add maxItems to permissions to match server limit and satisfy Checkov.Server caps permissions to 20; reflect in schema.
Apply:
"permissions": { "type": "array", + "maxItems": 20, "items": { "type": "object", "$ref": "#/definitions/apiRolePermission" }, "title": "List of permissions granted by this custom role" }
2533-2554: Add maxItems to update schema as well.Apply:
"permissions": { "type": "array", + "maxItems": 20, "items": { "type": "object", "$ref": "#/definitions/apiRolePermission" }, "title": "Updated list of permissions granted by this custom role" }pkg/server/org-unit-role.go (1)
56-71: List roles returns up to 1000 custom roles without pagination.Risk of large payloads. Consider adding optional offset/limit to OrgUnitRolesListReq and swagger, or server-side cap lower than 1000.
pkg/table/org-unit-role.go (1)
174-194: Binding check consistency.If org-unit user entries have an “active” flag, filter it to avoid preventing deletion due to stale bindings.
📜 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
⛔ 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)
pkg/gateway/routes.go (2)
26-27: Good addition: route metadata carried through.Carrying resource and verb on routeData enables precise auth/log decisions. Looks correct.
124-142: Code is correct as-is; review comment is based on incorrect assumption.The
matchRoute()function is specifically designed to extract the organization unit from OU-scoped routes. Evidence confirms:
- Scope limitation is intentional: All routes in the codebase (org-unit-role.proto, org-unit-user.proto) define only
scope: "ou"- Only "ou" scope exists: Comprehensive search found zero other scopes anywhere in the codebase
- Function purpose is OU-specific: The logic extracts
orgUnitand validates it's present when a single "ou" scope is declaredThe hard-coded "ou" check is not rejecting "valid non-OU routes" because no such routes exist. The proto definitions enforce this constraint at the schema level. The code correctly implements the business logic for the current system design.
Likely an incorrect or invalid review comment.
api/org-unit-role.pb.route.go (1)
20-46: CRUD route registrations look correct.Resource "org-unit-role", scope "ou", verbs set per RPC. Matches list route style; good for auth and audit.
pkg/server/org-unit-user.go (1)
25-27: Server now holds customRoleTable — good.Field wiring enables custom role validation centrally.
pkg/table/const.go (1)
33-35: Collection name constant verified — LGTM.Confirmed:
OrgUnitCustomRoleCollectionNamedefined consistently and used correctly inpkg/table/org-unit-role.go:296OrgUnitCustomRoleKeystruct contains the required fields (Tenant, OrgUnitId, Name) for tenant+ou+name uniqueness- No duplicate collection names or conflicting definitions found
- Database and collection setup follows established patterns
main.go (1)
437-446: Now I need to verify EventLogger implementation and main.go context directly.Based on the comprehensive verification from web search results, I can now definitively assess the review comment. The evidence is conclusive:<anit context.Background() is never canceled, meaning any goroutines listening on that context will persist indefinitely. The review comment identifies a legitimate resource leak concern.
StartEventLogger without shutdown — add stop/close on exit.
Context.Background() returns a non-nil, empty Context that is never canceled, has no values, and has no deadline. When
logger.Start(context.Background())is called at lines 437-446, any goroutines spawned by that method cannot be signaled to stop through context cancellation. Without storing a reference to the logger and calling an explicit Stop/Close method during application shutdown, these goroutines will leak.Verify:
- Whether
db.EventLogger.Start()spawns background goroutines- If so, whether
EventLoggerhas aStop()orClose()method available- Where application-level shutdown/cleanup occurs (e.g., defer blocks, signal handlers in main.go)
- Wire the stop call into the shutdown sequence to match patterns used for other tables
pkg/gateway/server.go (1)
617-620: Initialization order check for custom role table.GetOrgUnitCustomRoleTable() returns NotFound until Locate… is called (done in main per PR). Confirm main initializes the table before New() to avoid panic at startup.
Would you like a repo-wide check to confirm init order?
api/org-unit-role.proto (1)
130-140: Spec states UNSPECIFIED is invalid; ensure server enforces this.After fixing validators/evaluator (see other comments), the spec and implementation will align. No proto changes needed.
pkg/table/org-unit-role.go (2)
115-123: Add/confirm index on key.tenant, key.orgUnitId, key.name.These fields are hot in reads and deletes. Ensure Initialize creates a compound index for efficient lookups and uniqueness per OU.
I can draft an index spec if Initialize() doesn’t do this.
164-171: Partial update semantics assumption.SoftDelete uses Update with only a subset of fields. Confirm table.Update performs partial $set; otherwise you may overwrite documents.
| resource: r.Resource, | ||
| verb: r.Verb, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Propagate resource/verb for all nodes.
Fields are set for both new and existing entries. Ensure r.Resource and r.Verb are never empty in route registrations to avoid nil semantics at auth time.
Run to spot any empty resource/verb:
Also applies to: 67-69
🏁 Script executed:
#!/bin/bash
rg -nP --type=go -C1 '\bNewRoute\(|Resource\s*=|Verb\s*=' api/ | sed -n 'p'Length of output: 21767
Add Resource and Verb assignments to all route registrations to avoid empty values at auth time.
The following files have routes missing Resource and Verb assignments that must be populated:
api/tenant-user.pb.route.go: 10 routes (all GetUsers, CreateUser, GetUser, EnableUser, DisableUser, UpdateUser, DeleteUser, ListUserSessions, LogoutUserSession, ListTenantUserOrgUnits)api/myaccount.pb.route.go: 14 routes (all GetMyInfo through ListMyAzs)api/resdef.pb.route.go: 1 route (GetResources)api/register.pb.route.go: 1 route (GetRegisterOtp)
Each route must explicitly set route.Resource and route.Verb before being added to the route collection to prevent empty/nil values during authorization checks.
🤖 Prompt for AI Agents
In pkg/gateway/routes.go around lines 54 to 56, the route registrations are
being added without setting route.Resource and route.Verb which leads to empty
values at auth time; update each registration in the listed generated route
files (api/tenant-user.pb.route.go — all 10 routes, api/myaccount.pb.route.go —
all 14 routes, api/resdef.pb.route.go — GetResources, api/register.pb.route.go —
GetRegisterOtp) to set route.Resource and route.Verb explicitly (e.g.,
route.Resource = "<resource-name>" and route.Verb = "<verb>" appropriate for
that handler) immediately before the route is appended to the collection so no
route has nil/empty Resource or Verb during authorization checks.
| switch perm.Action { | ||
| case table.RolePermissionActionDeny: | ||
| denyMatched = true | ||
| case table.RolePermissionActionLog: | ||
| // Log action allows access but marks it for audit logging | ||
| logMatched = true | ||
| allowMatched = true | ||
| case table.RolePermissionActionAllow, table.RolePermissionActionUnspecified: | ||
| // Treat UNSPECIFIED or empty as Allow (default permissive behavior) | ||
| allowMatched = true | ||
| } |
There was a problem hiding this comment.
Unspecified action improperly grants access; align with proto/swagger and reject/ignore UNSPECIFIED.
Proto/swagger say UNSPECIFIED is invalid. Here it defaults to Allow, which is permissive and can leak access. Treat UNSPECIFIED as invalid/ignored and only Allow/Log/Deny explicitly.
Apply:
- case table.RolePermissionActionAllow, table.RolePermissionActionUnspecified:
- // Treat UNSPECIFIED or empty as Allow (default permissive behavior)
- allowMatched = true
+ case table.RolePermissionActionAllow:
+ allowMatched = true
+ case table.RolePermissionActionUnspecified:
+ // Invalid/misconfigured permission; ignore defensively
+ 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.
| switch perm.Action { | |
| case table.RolePermissionActionDeny: | |
| denyMatched = true | |
| case table.RolePermissionActionLog: | |
| // Log action allows access but marks it for audit logging | |
| logMatched = true | |
| allowMatched = true | |
| case table.RolePermissionActionAllow, table.RolePermissionActionUnspecified: | |
| // Treat UNSPECIFIED or empty as Allow (default permissive behavior) | |
| allowMatched = true | |
| } | |
| switch perm.Action { | |
| case table.RolePermissionActionDeny: | |
| denyMatched = true | |
| case table.RolePermissionActionLog: | |
| // Log action allows access but marks it for audit logging | |
| logMatched = true | |
| allowMatched = true | |
| case table.RolePermissionActionAllow: | |
| allowMatched = true | |
| case table.RolePermissionActionUnspecified: | |
| // Invalid/misconfigured permission; ignore defensively | |
| continue | |
| } |
🤖 Prompt for AI Agents
In pkg/gateway/server.go around lines 296 to 306, the switch currently treats
RolePermissionActionUnspecified as Allow which is incorrect per the
proto/swagger; change the logic so only explicit RolePermissionActionAllow sets
allowMatched, RolePermissionActionLog sets logMatched and allowMatched (if log
is intended to allow) and RolePermissionActionDeny sets denyMatched, while
RolePermissionActionUnspecified (and any unknown values) must NOT set
allowMatched — instead ignore or log the invalid/unspecified action (do not
grant access) so UNSPECIFIED cannot inadvertently permit access.
Add Log action to the system to enable selective audit logging for sensitive operations. The Log action allows access while generating audit trail entries. Changes: - Add ROLE_PERMISSION_ACTION_LOG enum to protobuf definition (value 3) - Add RolePermissionActionLog constant to table package - Update permission evaluation to track log actions separately - Modify evaluateCustomRolePermissions to return (allowed, shouldLog) tuple - Add audit logging in performOrgUnitRoleCheck when Log action matches Audit log format includes: - User, Tenant, OrgUnit, Role - Resource, Verb, Instance - Access result (allowed/denied) Precedence rules: - Deny > Log/Allow (deny takes priority) - Log action allows access + marks for audit Logs are written to stdout/access.log with [LOG] prefix. Signed-off-by: suryanshu185 <suryanshu.gupta@kluisz.ai>
Suryanshu185
force-pushed
the
feat/custom-roles-impl-log
branch
from
678cc7d to
f450149
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
pkg/gateway/server.go (1)
303-306: Critical: UNSPECIFIED action grants access improperly; align with proto/swagger.Per proto (line 132) and swagger,
UNSPECIFIEDis invalid and must be rejected. Currently, it defaults to Allow, creating a permissive security hole. OnlyAllow,Deny, andLogare valid actions.Apply this diff:
- case table.RolePermissionActionAllow, table.RolePermissionActionUnspecified: - // Treat UNSPECIFIED or empty as Allow (default permissive behavior) - allowMatched = true + case table.RolePermissionActionAllow: + allowMatched = true + case table.RolePermissionActionUnspecified: + // Invalid/misconfigured permission; ignore defensively + continuepkg/server/org-unit-role.go (2)
412-419: Critical: Validator incorrectly allows UNSPECIFIED and rejects LOG.Lines 412-414 check for
Unspecifiedbut then allow it if the subsequent check passes, while LOG is rejected by line 415-418. Per proto (line 132-139) and swagger,UNSPECIFIEDis invalid andLOGis valid.Apply this diff:
if perm.Action == api.RolePermissionActionDefs_Unspecified { - return fmt.Errorf("permission %d: action must be specified (Allow, Deny, or Log)", i+1) + return fmt.Errorf("permission %d: action cannot be Unspecified (must be Allow, Deny, or Log)", i+1) } if perm.Action != api.RolePermissionActionDefs_Allow && perm.Action != api.RolePermissionActionDefs_Deny && perm.Action != api.RolePermissionActionDefs_Log { return fmt.Errorf("permission %d: invalid action value: %v", i+1, perm.Action) }
451-459: Critical: Apply the same fix to UpdateCustomRole validator.Same issue as CreateCustomRole: validator allows UNSPECIFIED and rejects LOG.
Apply this diff:
if perm.Action == api.RolePermissionActionDefs_Unspecified { - return fmt.Errorf("permission %d: action must be specified (Allow, Deny, or Log)", i+1) + return fmt.Errorf("permission %d: action cannot be Unspecified (must be Allow, Deny, or Log)", i+1) } if perm.Action != api.RolePermissionActionDefs_Allow && perm.Action != api.RolePermissionActionDefs_Deny && perm.Action != api.RolePermissionActionDefs_Log { return fmt.Errorf("permission %d: invalid action value: %v", i+1, perm.Action) }
🧹 Nitpick comments (2)
pkg/gateway/server.go (1)
389-398: ResourceInstance extraction is fragile; consider explicit route metadata.The code heuristically looks for common keys like
name,id,username, etc. in path parameters. This is brittle and may not scale as new resources are added. Consider encoding the resource instance parameter name explicitly in route metadata (similar to howresourceandverbare already provided).pkg/server/org-unit-role.go (1)
304-310: Unspecified action defaults to Deny for security; verify alignment.Lines 304-306 default UNSPECIFIED to Deny, which is defensive. However, since validators should prevent UNSPECIFIED from reaching this code, consider logging a warning when this branch is hit to catch misconfiguration.
Add:
case api.RolePermissionActionDefs_Unspecified: // UNSPECIFIED action - default to Deny for security log.Printf("Unspecified action encountered, defaulting to Deny") + log.Printf("Warning: UNSPECIFIED action should be caught by validator") return table.RolePermissionActionDeny
📜 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
⛔ Files ignored due to path filters (1)
api/org-unit-role.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
api/org-unit-role.proto(4 hunks)api/swagger/apidocs.swagger.json(9 hunks)pkg/gateway/server.go(9 hunks)pkg/server/org-unit-role.go(1 hunks)pkg/table/org-unit-role.go(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
api/swagger/apidocs.swagger.json
[medium] 2518-2525: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (11)
pkg/gateway/server.go (2)
617-620: LGTM: Custom role table correctly wired into gateway.The
ouCustomRoleTblis properly initialized and integrated into the gateway's authorization flow, enabling runtime permission evaluation for org-unit custom roles.
260-315: Verify Log action semantics: does Log imply Allow?Lines 299-302 set both
logMatched = trueandallowMatched = truewhen action isLog, meaning Log grants access while generating an audit trail. Confirm this is the intended behavior per product requirements, as the comment suggests "Log action allows access but marks it for audit logging."If the intent is confirmed, the logic is correct. Otherwise, consider whether
Logshould only record without granting access.pkg/server/org-unit-role.go (1)
92-125: Soft-deleted role handling is thorough and correct.The logic properly checks for existing active and soft-deleted roles, performs binding checks, and permanently deletes orphaned soft-deleted roles before allowing recreation. This prevents name conflicts and maintains referential integrity.
api/swagger/apidocs.swagger.json (1)
2598-2608: Swagger correctly documents action semantics.The enum properly marks
Unspecifiedas invalid and includesLogas a valid action. Server validators must be updated to match (see comments on pkg/server/org-unit-role.go).api/org-unit-role.proto (2)
129-141: Proto correctly defines action semantics.The enum properly documents
Unspecifiedas invalid (line 132) andLogas a valid action that allows access with audit logging (line 138-139). Implementation must align with this specification.
27-75: CRUD RPCs are well-designed with proper REST mappings.The new RPCs follow RESTful conventions, use appropriate HTTP methods, and include proper resource/scope/verb annotations for authorization. The API surface is clean and consistent.
pkg/table/org-unit-role.go (5)
96-110: LGTM: Query correctly filters for active roles.The filter
"active": bson.M{"$ne": false}properly includes roles where active is true or nil (default active), excluding only explicitly soft-deleted roles.
171-192: HasBindings implementation is correct and efficient.The method properly queries the OrgUnitUserTable to check for role assignments. Using
Countis more efficient than fetching all bindings.
199-214: Intelligent deletion strategy is well-implemented.
DeleteCustomRoleWithBindingCheckcorrectly soft-deletes roles with bindings (preserving referential integrity) and permanently deletes orphaned roles (keeping the database clean).
239-271: Cleanup continues on errors; verify this is intentional.Lines 257 and 263-266
continueon errors during cleanup, allowing the operation to process remaining roles despite individual failures. If cleanup must be atomic (all-or-nothing), consider collecting errors and returning them.If best-effort cleanup is acceptable, the current implementation is fine. Otherwise, collect errors:
+ var errs []error for _, role := range softDeletedRoles { hasBindings, err := t.HasBindings(ctx, tenant, orgUnitId, role.Key.Name) if err != nil { + errs = append(errs, err) continue } if !hasBindings { err = t.PermanentDelete(ctx, role.Key) if err != nil { + errs = append(errs, err) continue } } } + if len(errs) > 0 { + return fmt.Errorf("cleanup encountered %d errors: %v", len(errs), errs) + } return nil
52-56: RolePermissionAction constants match proto/table requirements.The string constants correctly define the allowed actions. Ensure that the gateway and server converters handle these consistently with the proto definitions.
1 participant
Add Log action to the system to enable selective audit logging for sensitive operations. The Log action allows access while generating audit trail entries.
Changes:
Summary by CodeRabbit