Feat/custom roles db impl#139
Conversation
WalkthroughThis change introduces a new MongoDB-backed OrgUnitRole table infrastructure with composite key modeling, resource-based permission structures, lifecycle management, and route wiring. It includes adding a collection constant, implementing table operations with filtering, initializing the table on startup, registering route handlers, and adding a binding check method to support role deletion logic. Changes
Sequence Diagram(s)sequenceDiagram
participant main as Application<br/>Startup
participant table as table<br/>Package
participant db as MongoDB<br/>Client
participant cache as Singleton<br/>Cache
main->>table: LocateOrgUnitRoleTable(client)
alt Table cached
table->>cache: Check singleton
cache-->>table: Return cached instance
else First initialization
table->>db: Retrieve OrgUnitRoleTable from DB
db-->>table: Table object
table->>cache: Store in singleton
end
table-->>main: *OrgUnitRoleTable or error
rect rgb(200, 220, 240)
Note over main: Initialization complete
end
sequenceDiagram
participant client as API Client
participant server as OrgUnitRole<br/>Server
participant table as OrgUnitRole<br/>Table
participant db as MongoDB
client->>server: GetCustomRole(tenant, orgUnitId)
server->>table: GetByOrgUnit(ctx, tenant, orgUnitId, offset, limit)
table->>db: Query with filters: key.tenant, key.orgUnitId, isDeleted!=true
db-->>table: []*OrgUnitRole
table-->>server: Roles or error
server-->>client: Response
sequenceDiagram
participant admin as Admin
participant server as OrgUnitRole<br/>Server
participant roleTable as OrgUnitRole<br/>Table
participant userTable as OrgUnitUser<br/>Table
participant db as MongoDB
admin->>server: DeleteCustomRole(tenant, orgUnitId, roleName)
server->>roleTable: Retrieve role metadata
server->>userTable: HasRoleBindings(tenant, orgUnitId, roleName)
userTable->>db: Count with key filters + role
db-->>userTable: count
alt Bindings exist
rect rgb(255, 200, 200)
userTable-->>server: true
server->>roleTable: Mark isDeleted=true, set updatedBy
roleTable->>db: Soft-delete update
db-->>roleTable: Success
roleTable-->>server: Role preserved for audit
end
else No bindings
rect rgb(200, 255, 200)
userTable-->>server: false
server->>roleTable: DeleteKey(tenant, orgUnitId, roleName)
roleTable->>db: Hard-delete transaction
db-->>roleTable: Success
roleTable-->>server: Role removed from storage
end
end
server-->>admin: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 4
🧹 Nitpick comments (4)
pkg/server/org-unit-role.go (1)
46-92: Consider extracting common authentication logic.All four stub methods have identical authentication checks and error handling. Consider extracting this to a helper method to reduce code duplication.
Example refactor:
// Helper method for authentication func (s *OrgUnitRoleServer) authenticateRequest(ctx context.Context) error { authInfo, _ := auth.GetAuthInfoFromContext(ctx) if authInfo == nil { return status.Errorf(codes.Unauthenticated, "User not authenticated") } return nil } // Then use in each method: func (s *OrgUnitRoleServer) CreateCustomRole(ctx context.Context, req *api.CreateCustomRoleReq) (*api.CreateCustomRoleResp, error) { if err := s.authenticateRequest(ctx); err != nil { return nil, err } // Stub implementation return nil, status.Errorf(codes.Unimplemented, "Custom role creation not yet implemented") }This reduces duplication and makes the code more maintainable for when the actual implementation is added.
api/swagger/apidocs.swagger.json (1)
2522-2529: Consider adding maxItems constraint to permissions array.Static analysis flagged that the permissions array lacks a maximum items constraint. While not critical, adding a reasonable limit helps prevent potential abuse and documents expected usage patterns.
Consider adding a maxItems constraint:
"permissions": { "type": "array", "maxItems": 100, "items": { "type": "object", "$ref": "#/definitions/apiRolePermission" }, "title": "List of permissions granted by this custom role" }Adjust the maxItems value based on your expected use case. A limit of 50-100 permissions per role is typically reasonable for most RBAC systems.
pkg/table/org-unit-role.go (2)
48-68: Consider adding field validation and documentation.The OrgUnitCustomRole structure is comprehensive, but consider:
- Maximum length constraints for DisplayName and Description
- Validation for role Name (e.g., alphanumeric + hyphens only)
- Documentation of the Active field's tri-state behavior (true, false, nil)
Consider adding validation tags or documenting expected constraints:
// OrgUnitCustomRole defines a custom role within an organization unit type OrgUnitCustomRole struct { // Custom role key Key *OrgUnitCustomRoleKey `bson:"key,omitempty"` // Display name for the custom role (max 100 chars) DisplayName string `bson:"displayName,omitempty"` // Description explaining the purpose of this custom role (max 500 chars) Description string `bson:"description,omitempty"` // List of permissions granted by this custom role Permissions []*RolePermission `bson:"permissions,omitempty"` // Created timestamp Created int64 `bson:"created,omitempty"` // User who created this custom role CreatedBy string `bson:"createdBy,omitempty"` // Last updated timestamp Updated int64 `bson:"updated,omitempty"` // User who last updated this custom role UpdatedBy string `bson:"updatedBy,omitempty"` // Whether this custom role is currently active // nil = active (default), true = active, false = soft-deleted Active *bool `bson:"active,omitempty"` }
118-138: Consider consolidating duplicate find logic.FindByNameAndOrgUnit and FindAnyByNameAndOrgUnit have nearly identical implementations, differing only in the active filter. Consider refactoring to reduce duplication.
Example refactor:
// findByNameAndOrgUnitWithFilter is a private helper func (t *OrgUnitCustomRoleTable) findByNameAndOrgUnitWithFilter( ctx context.Context, tenant, orgUnitId, roleName string, includeInactive bool, ) (*OrgUnitCustomRole, error) { filter := bson.M{ "key.tenant": tenant, "key.orgUnitId": orgUnitId, "key.name": roleName, } if !includeInactive { filter["active"] = bson.M{"$ne": false} } results, err := t.FindMany(ctx, filter, 0, 1) if err != nil { return nil, err } if len(results) == 0 { return nil, errors.Wrapf(errors.NotFound, "custom role not found") } return results[0], nil } // FindByNameAndOrgUnit finds an active custom role func (t *OrgUnitCustomRoleTable) FindByNameAndOrgUnit(ctx context.Context, tenant, orgUnitId, roleName string) (*OrgUnitCustomRole, error) { return t.findByNameAndOrgUnitWithFilter(ctx, tenant, orgUnitId, roleName, false) } // FindAnyByNameAndOrgUnit finds any custom role (including soft-deleted) func (t *OrgUnitCustomRoleTable) FindAnyByNameAndOrgUnit(ctx context.Context, tenant, orgUnitId, roleName string) (*OrgUnitCustomRole, error) { return t.findByNameAndOrgUnitWithFilter(ctx, tenant, orgUnitId, roleName, true) }Also applies to: 196-217
📜 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 d08eb9d and af88a81a3e6bfb195cd78d8f36cf2494fa6aa665.
⛔ 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 (7)
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/server/org-unit-role.go(3 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 (8)
main.go (1)
396-400: LGTM! Table initialization follows established patterns.The OrgUnitCustomRole table initialization is correctly placed in the startup sequence, follows the same pattern as other tables, and includes appropriate error handling.
pkg/table/const.go (1)
32-34: LGTM! Collection name follows established conventions.The constant name and value are consistent with existing collection naming patterns in the codebase.
api/org-unit-role.pb.route.go (1)
19-46: LGTM! Route definitions are well-structured and consistent.The four new routes follow REST conventions and are properly scoped with resource "org-unit-custom-role" and scope "ou". Path patterns align with the existing API structure.
api/org-unit-role.proto (1)
114-144: Ensure comprehensive validation for the permission model.The ResourceMatch and RolePermission structures introduce a flexible but complex permission system. Verify that validation logic is in place to:
- Validate
criteriavalues are from the allowed set (exact, prefix, suffix, regex, wildcard)- Validate regex patterns in
keyfield when criteria is "regex"- Validate
actionvalues are "Allow" or "Deny"- Prevent malformed wildcard patterns
- Handle empty or null
matchfields appropriatelyConsider implementing server-side validation for these fields before they are persisted to the database to prevent invalid permission configurations.
pkg/table/org-unit-role.go (4)
18-26: LGTM! Composite key structure is well-designed.The OrgUnitCustomRoleKey properly represents the three-level hierarchy (Tenant → OrgUnit → Role) and follows BSON tagging conventions.
28-46: LGTM! Permission model provides flexibility for fine-grained access control.The ResourceMatch and RolePermission structures support multiple matching strategies and explicit Allow/Deny actions, enabling sophisticated permission configurations.
253-260: LGTM! Singleton pattern correctly implemented.The GetOrgUnitCustomRoleTable function properly checks for initialization and returns an appropriate error if the table hasn't been located yet.
70-74: Verify that database indexes are created for OrgUnitCustomRoleTable.No index creation code was found in the codebase. The table performs frequent queries on composite keys ({tenant, orgUnitId, active}, {tenant, orgUnitId, name}) and the active field, but indexes are not explicitly defined in LocateOrgUnitCustomRoleTable or elsewhere in the initialization flow.
Table.Initialize() only sets up the table structure—indexes must be created separately using MongoDB's index API. Verify that indexes are either:
- Created in the LocateOrgUnitCustomRoleTable initialization after calling Initialize(), or
- Created through a separate deployment/migration process, or
- Handled at the database/infrastructure level
If indexes are not currently created, add them to LocateOrgUnitCustomRoleTable for optimal query performance on the fields identified in the query methods.
| // GetByOrgUnit retrieves all custom roles for a specific organization unit | ||
| 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 | ||
| } | ||
|
|
||
| list, err := t.FindMany(ctx, filter, offset, limit) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return list, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clarify the active field filtering logic.
The filter "active": bson.M{"$ne": false} matches roles where active is true OR nil. This tri-state behavior should be explicitly documented in the method and struct comments to avoid confusion.
Add a comment explaining the tri-state logic:
func (t *OrgUnitCustomRoleTable) GetByOrgUnit(ctx context.Context, tenant, orgUnitId string, offset, limit int32) ([]*OrgUnitCustomRole, error) {
+ // Filter matches roles where active is true or nil (default).
+ // Roles with active=false are soft-deleted and excluded.
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},
}Apply this pattern to all methods using the same filter (lines 81, 97, 107, 124).
📝 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.
| // GetByOrgUnit retrieves all custom roles for a specific organization unit | |
| 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 | |
| } | |
| list, err := t.FindMany(ctx, filter, offset, limit) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return list, nil | |
| } | |
| // GetByOrgUnit retrieves all custom roles for a specific organization unit | |
| func (t *OrgUnitCustomRoleTable) GetByOrgUnit(ctx context.Context, tenant, orgUnitId string, offset, limit int32) ([]*OrgUnitCustomRole, error) { | |
| // Filter matches roles where active is true or nil (default). | |
| // Roles with active=false are soft-deleted and excluded. | |
| filter := bson.M{ | |
| "key.tenant": tenant, | |
| "key.orgUnitId": orgUnitId, | |
| "active": bson.M{"$ne": false}, | |
| } | |
| list, err := t.FindMany(ctx, filter, offset, limit) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return list, nil | |
| } |
🤖 Prompt for AI Agents
In pkg/table/org-unit-role.go around lines 76 to 90, the filter "active":
bson.M{"$ne": false} intentionally matches active == true OR active == nil
(tri-state: true/false/absent); update the function and relevant struct comments
to explicitly document this tri-state behavior and its intent (that absent/null
means treated as active) and then apply the same explanatory comment to every
method using the same filter at lines 81, 97, 107, and 124 so callers and
maintainers understand the semantics.
| // SoftDelete marks a custom role as inactive instead of physically deleting it | ||
| func (t *OrgUnitCustomRoleTable) SoftDelete(ctx context.Context, key *OrgUnitCustomRoleKey, deletedBy string) error { | ||
| update := &OrgUnitCustomRole{ | ||
| Active: &[]bool{false}[0], // Mark as inactive | ||
| UpdatedBy: deletedBy, // Track who performed the deletion | ||
| Updated: time.Now().Unix(), // Update timestamp | ||
| } | ||
|
|
||
| return t.Update(ctx, key, update) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor the pointer initialization for better readability.
The expression &[]bool{false}[0] is unnecessarily complex. Use a clearer approach to create a pointer to a boolean.
Apply this diff:
func (t *OrgUnitCustomRoleTable) SoftDelete(ctx context.Context, key *OrgUnitCustomRoleKey, deletedBy string) error {
+ active := false
update := &OrgUnitCustomRole{
- Active: &[]bool{false}[0], // Mark as inactive
+ Active: &active, // Mark as inactive
UpdatedBy: deletedBy, // Track who performed the deletion
Updated: time.Now().Unix(), // Update timestamp
}📝 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.
| // SoftDelete marks a custom role as inactive instead of physically deleting it | |
| func (t *OrgUnitCustomRoleTable) SoftDelete(ctx context.Context, key *OrgUnitCustomRoleKey, deletedBy string) error { | |
| update := &OrgUnitCustomRole{ | |
| Active: &[]bool{false}[0], // Mark as inactive | |
| UpdatedBy: deletedBy, // Track who performed the deletion | |
| Updated: time.Now().Unix(), // Update timestamp | |
| } | |
| return t.Update(ctx, key, update) | |
| } | |
| // SoftDelete marks a custom role as inactive instead of physically deleting it | |
| func (t *OrgUnitCustomRoleTable) SoftDelete(ctx context.Context, key *OrgUnitCustomRoleKey, deletedBy string) error { | |
| active := false | |
| update := &OrgUnitCustomRole{ | |
| Active: &active, // Mark as inactive | |
| UpdatedBy: deletedBy, // Track who performed the deletion | |
| Updated: time.Now().Unix(), // Update timestamp | |
| } | |
| return t.Update(ctx, key, update) | |
| } |
🤖 Prompt for AI Agents
In pkg/table/org-unit-role.go around lines 140 to 149, the pointer
initialization Active: &[]bool{false}[0] is overly clever and hard to read;
replace it with a clear pointer-to-bool such as declaring a local variable (b :=
false) and using Active: &b, or call a small helper (func boolPtr(v bool) *bool
{ return &v }) and use Active: boolPtr(false); keep UpdatedBy and Updated
assignments unchanged.
| // HasBindings checks if a custom role has any users assigned to it | ||
| func (t *OrgUnitCustomRoleTable) HasBindings(ctx context.Context, tenant, orgUnitId, roleName string) (bool, error) { | ||
| // Get the org unit user table to check for role assignments | ||
| orgUnitUserTable, err := GetOrgUnitUserTable() | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| // Check if any users are assigned this custom role | ||
| filter := bson.M{ | ||
| "key.tenant": tenant, | ||
| "key.orgUnitId": orgUnitId, | ||
| "role": roleName, | ||
| } | ||
|
|
||
| count, err := orgUnitUserTable.col.Count(ctx, filter) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| return count > 0, nil | ||
| } |
There was a problem hiding this comment.
Encapsulation violation: avoid direct access to collection.
The HasBindings method directly accesses orgUnitUserTable.col, breaking encapsulation. The OrgUnitUserTable should provide a proper method for counting role assignments.
Apply this refactor:
- Add a method to OrgUnitUserTable (in pkg/table/org-unit-user.go):
// 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)
}- Update HasBindings in 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
}🤖 Prompt for AI Agents
In pkg/table/org-unit-role.go around lines 151 to 172, HasBindings is directly
accessing orgUnitUserTable.col which breaks encapsulation; add a CountByRole
method to pkg/table/org-unit-user.go on OrgUnitUserTable that accepts (ctx
context.Context, tenant, orgUnitId, roleName string) and performs the bson.M
filter and t.col.Count(ctx, filter), returning (int64, error); then update
HasBindings to call orgUnitUserTable.CountByRole(ctx, tenant, orgUnitId,
roleName), check the returned count and error, and return count > 0, nil (or
false, err) instead of directly using orgUnitUserTable.col.
| // CleanupOrphanedSoftDeletedRoles removes soft-deleted roles that no longer have bindings | ||
| func (t *OrgUnitCustomRoleTable) CleanupOrphanedSoftDeletedRoles(ctx context.Context, tenant, orgUnitId string) error { | ||
| // Find all soft-deleted roles | ||
| filter := bson.M{ | ||
| "key.tenant": tenant, | ||
| "key.orgUnitId": orgUnitId, | ||
| "active": false, // Only soft-deleted roles | ||
| } | ||
|
|
||
| softDeletedRoles, err := t.FindMany(ctx, filter, 0, 0) // Get all | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Check each soft-deleted role for bindings | ||
| for _, role := range softDeletedRoles { | ||
| hasBindings, err := t.HasBindings(ctx, tenant, orgUnitId, role.Key.Name) | ||
| if err != nil { | ||
| continue // Skip on error, don't fail the entire cleanup | ||
| } | ||
|
|
||
| // If no bindings, permanently delete | ||
| if !hasBindings { | ||
| err = t.PermanentDelete(ctx, role.Key) | ||
| if err != nil { | ||
| // Log error but continue with other roles | ||
| continue | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Improve error handling in cleanup maintenance.
The CleanupOrphanedSoftDeletedRoles method silently ignores errors with continue, which could mask issues during maintenance operations. At minimum, errors should be logged.
Consider this improvement:
+import (
+ "log"
+)
+
func (t *OrgUnitCustomRoleTable) CleanupOrphanedSoftDeletedRoles(ctx context.Context, tenant, orgUnitId string) error {
// Find all soft-deleted roles
filter := bson.M{
"key.tenant": tenant,
"key.orgUnitId": orgUnitId,
"active": false,
}
softDeletedRoles, err := t.FindMany(ctx, filter, 0, 0)
if err != nil {
return err
}
// Check each soft-deleted role for bindings
for _, role := range softDeletedRoles {
hasBindings, err := t.HasBindings(ctx, tenant, orgUnitId, role.Key.Name)
if err != nil {
+ log.Printf("Error checking bindings for role %s: %v", role.Key.Name, err)
continue
}
if !hasBindings {
err = t.PermanentDelete(ctx, role.Key)
if err != nil {
+ log.Printf("Error permanently deleting role %s: %v", role.Key.Name, err)
continue
}
}
}
return nil
}Alternatively, consider collecting errors and returning them as a multi-error to provide visibility into partial failures.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/table/org-unit-role.go around lines 219-251, the
CleanupOrphanedSoftDeletedRoles function currently swallows errors with continue
which hides failures; update it to log errors whenever t.HasBindings or
t.PermanentDelete return an error and collect failures into a slice (or a
multi-error) to return at the end if any occurred. Specifically, on HasBindings
error call the table's logger or standard logger with contextual fields (tenant,
orgUnitId, role.Key.Name, err) instead of continue, and append the error to an
errors slice; on PermanentDelete error do the same (log with context and
append); after the loop, if the errors slice is non-empty return a combined
error (e.g., fmt.Errorf or github.com/hashicorp/go-multierror) so callers see
partial-failure details, otherwise return nil.
Suryanshu185
force-pushed
the
feat/custom-roles-DB-impl
branch
from
af88a81 to
d714509
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
pkg/table/org-unit-role.go (3)
161-170: Avoid clever pointer initReplace &[]bool{false}[0] with a clear local variable.
func (t *OrgUnitCustomRoleTable) SoftDelete(ctx context.Context, key *OrgUnitCustomRoleKey, deletedBy string) error { - update := &OrgUnitCustomRole{ - Active: &[]bool{false}[0], // Mark as inactive + inactive := false + update := &OrgUnitCustomRole{ + Active: &inactive, // Mark as inactive UpdatedBy: deletedBy, // Track who performed the deletion Updated: time.Now().Unix(), // Update timestamp } return t.Update(ctx, key, update) }
172-193: Encapsulation: don’t access orgUnitUserTable.col directlyCall a method on OrgUnitUserTable (e.g., CountByRole) instead of using its collection.
- count, err := orgUnitUserTable.col.Count(ctx, filter) + count, err := orgUnitUserTable.CountByRole(ctx, tenant, orgUnitId, roleName)Outside this file, add to pkg/table/org-unit-user.go:
// 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) }
240-272: Cleanup swallows errors; log and aggregateErrors in HasBindings/PermanentDelete are ignored; at least log, ideally return a multi‑error.
+import "log" @@ - hasBindings, err := t.HasBindings(ctx, tenant, orgUnitId, role.Key.Name) - if err != nil { - continue // Skip on error, don't fail the entire cleanup - } + hasBindings, err := t.HasBindings(ctx, tenant, orgUnitId, role.Key.Name) + if err != nil { + log.Printf("cleanup: HasBindings failed tenant=%s ou=%s role=%s err=%v", tenant, orgUnitId, role.Key.Name, err) + continue + } @@ - err = t.PermanentDelete(ctx, role.Key) - if err != nil { - // Log error but continue with other roles - continue - } + if err := t.PermanentDelete(ctx, role.Key); err != nil { + log.Printf("cleanup: PermanentDelete failed tenant=%s ou=%s role=%s err=%v", tenant, orgUnitId, role.Key.Name, err) + continue + }
🧹 Nitpick comments (8)
api/swagger/apidocs.swagger.json (4)
223-262: 201/204 status codes for create/deletePOST should return 201 (Created) and DELETE can return 204 (No Content) for clearer REST semantics. Keep 200 as fallback if needed.
2533-2554: Update body: same validation as CreateMirror constraints (required fields as applicable, permissions min/maxItems) to avoid divergent validation paths.
3411-3437: Typo and naming consistency
- Typo: "provding" → "providing" (Line 3419).
- Consider aligning field names across models: desc vs description; displayName vs dispName elsewhere. Not blocking, but confusing for clients.
2522-2529: Array limits flagged by toolingStatic analysis flagged missing maxItems on arrays (permissions, verbs). Add reasonable caps (e.g., permissions ≤ 100, verbs ≤ 20) to prevent abuse and reduce payloads.
As per static analysis hints
Also applies to: 3647-3652
api/org-unit-role.proto (2)
167-189: Response messages carry only message stringCreate/Update/Delete responses return just a message. Consider returning the resource (or at least etags/audit fields) to support idempotent clients and immediate UI refresh. Not blocking if your API style prefers empty/ack.
Also applies to: 191-213, 215-249, 251-264
225-249: Validation: reject unspecified action and empty verbsProto3 can’t enforce required; add server‑side validation (or PGV) to ensure:
- permissions not empty
- verbs not empty
- action != ROLE_PERMISSION_ACTION_UNSPECIFIED
- when criteria == REGEX, key is a valid regex
pkg/table/org-unit-role.go (2)
69-89: Document tri‑state Active semantics on the modelAdd a struct comment stating: Active == nil or true ⇒ active; false ⇒ soft‑deleted. This guards future maintainers from changing filters incorrectly.
type OrgUnitCustomRole struct { // Custom role key Key *OrgUnitCustomRoleKey `bson:"key,omitempty"` @@ - // Whether this custom role is currently active - Active *bool `bson:"active,omitempty"` + // Whether this custom role is currently active. + // Tri-state: nil (treated as active for backward compatibility), true (active), false (soft-deleted). + Active *bool `bson:"active,omitempty"` }
113-122: Count return typeCasting to int32 can overflow in large tenants. Consider returning int64 to mirror underlying Count.
📜 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 af88a81a3e6bfb195cd78d8f36cf2494fa6aa665 and d7145093875debc57ac7fe4175e117af266b0307.
⛔ Files ignored due to path filters (1)
api/org-unit-role.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
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/table/const.go(1 hunks)pkg/table/org-unit-role.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- main.go
- pkg/table/const.go
- api/org-unit-role.pb.route.go
🧰 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 (4)
api/swagger/apidocs.swagger.json (1)
386-386: List summary wording LGTMClear that both built‑in and custom roles are included.
api/org-unit-role.proto (2)
27-39: Auth/HTTP bindings look correctRoutes and api.role options align with the documented paths and verbs.
Also applies to: 40-52, 53-76
114-138: Add converter helpers before implementing CreateCustomRole/UpdateCustomRole handlersEnum/value alignment issue verified: API proto layer uses int32 enums with full names (RESOURCE_MATCH_CRITERIA_EXACT, ROLE_PERMISSION_ACTION_ALLOW), while storage layer uses string constants with different casing ("exact", "Allow", "Deny"). No converters currently exist. When CreateCustomRole and UpdateCustomRole handlers are implemented (currently stubs), they must include bidirectional mapping logic to serialize/deserialize proto enums to/from storage strings. Otherwise, data round-trip will fail and queries will be inconsistent.
Recommended approach:
- Implement converter helpers (e.g., protoResourceMatchCriteriaToTable, tableResourceMatchCriteriaToProto) before completing handler implementations, or
- Choose a canonical representation and persist both layers identically.
pkg/table/org-unit-role.go (1)
283-302: The review comment is based on non-existent infrastructure and contradicts the actual codebase patterns.After exhaustive verification across the entire codebase:
EnsureIndex does not exist — Searching for "EnsureIndex", "CreateIndex", or "Indexes" across all
.gofiles returned zero results. This method is not available anywhere.Context parameter mismatch — The function signature
LocateOrgUnitCustomRoleTable(client db.StoreClient)has no context parameter, yet the suggestion attempts to passctxtoEnsureIndex.Pattern is consistent — All 8+ table initialization functions in the codebase (User, Tenant, OrgUnit, OrgUnitUser, etc.) follow the identical pattern:
Initialize(col)with no subsequent index creation. None of them create indices post-initialization.External dependency responsibility — Both
table.Tableand initialization logic are from external packages (github.com/go-core-stack/core/). Index management is either handled byInitialize()or belongs in a separate migration/setup process, not in the Locate function.The code is correct as-is. If indices are needed for the org-unit-roles collection, that concern should be addressed through the external package's initialization mechanism or database migrations, not by adding non-existent method calls to this function.
Likely an incorrect or invalid review comment.
| "OrgUnitRoleCreateCustomRoleBody": { | ||
| "type": "object", | ||
| "properties": { | ||
| "name": { | ||
| "type": "string", | ||
| "title": "Name of the custom role (must be unique within the org unit)" | ||
| }, | ||
| "displayName": { | ||
| "type": "string", | ||
| "title": "Display name for the custom role" | ||
| }, | ||
| "description": { | ||
| "type": "string", | ||
| "title": "Description explaining the purpose of this custom role" | ||
| }, | ||
| "permissions": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "object", | ||
| "$ref": "#/definitions/apiRolePermission" | ||
| }, | ||
| "title": "List of permissions granted by this custom role" | ||
| } | ||
| }, | ||
| "title": "Create custom role request" | ||
| }, |
There was a problem hiding this comment.
Add required fields and constraints to Create body
Enforce shape at the API boundary:
- required: ["name","displayName","permissions"]
- name: pattern and maxLength (e.g., ^[a-z][a-z0-9-]{2,62}$)
- permissions: minItems: 1, maxItems: N (see Checkov hint)
Apply:
"OrgUnitRoleCreateCustomRoleBody": {
"type": "object",
+ "required": ["name","displayName","permissions"],
"properties": {
"name": {
- "type": "string",
+ "type": "string",
+ "maxLength": 63,
+ "pattern": "^[a-z][a-z0-9-]{2,62}$",
"title": "Name of the custom role (must be unique within the org unit)"
},
"displayName": { "type": "string", "title": "Display name for the custom role" },
"description": { "type": "string", "title": "Description explaining the purpose of this custom role" },
"permissions": {
"type": "array",
"items": { "type": "object", "$ref": "#/definitions/apiRolePermission" },
- "title": "List of permissions granted by this custom role"
+ "minItems": 1,
+ "maxItems": 100,
+ "title": "List of permissions granted by this custom role"
}
},
"title": "Create custom role request"
}🧰 Tools
🪛 Checkov (3.2.334)
[medium] 2522-2529: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🤖 Prompt for AI Agents
In api/swagger/apidocs.swagger.json around lines 2507 to 2532, the
OrgUnitRoleCreateCustomRoleBody schema lacks required fields and validation
constraints; update the schema to add "required":
["name","displayName","permissions"], set "name" to enforce a regex pattern
(e.g. ^[a-z][a-z0-9-]{2,62}$) and a maxLength (63), and add array bounds for
"permissions" such as "minItems": 1 and a sensible "maxItems" per Checkov
guidance; ensure these constraints are added alongside existing properties and
the title remains unchanged.
| "type": "object", | ||
| "properties": { | ||
| "criteria": { | ||
| "$ref": "#/definitions/apiResourceMatchCriteria", | ||
| "title": "Matching criteria type" | ||
| }, | ||
| "key": { | ||
| "type": "string", | ||
| "title": "The matching key/pattern based on criteria\nFor wildcard: supports * (e.g., \"bucket-*\", \"*-prod\", \"*\")\nFor regex: valid regex pattern\nFor exact/prefix/suffix: literal string" | ||
| } | ||
| }, | ||
| "title": "Resource matching criteria for fine-grained permission control" | ||
| }, | ||
| "apiResourceMatchCriteria": { | ||
| "type": "string", | ||
| "enum": [ | ||
| "RESOURCE_MATCH_CRITERIA_UNSPECIFIED", | ||
| "RESOURCE_MATCH_CRITERIA_EXACT", | ||
| "RESOURCE_MATCH_CRITERIA_PREFIX", | ||
| "RESOURCE_MATCH_CRITERIA_SUFFIX", | ||
| "RESOURCE_MATCH_CRITERIA_REGEX", | ||
| "RESOURCE_MATCH_CRITERIA_WILDCARD" | ||
| ], | ||
| "default": "RESOURCE_MATCH_CRITERIA_UNSPECIFIED", | ||
| "description": "- RESOURCE_MATCH_CRITERIA_UNSPECIFIED: Unspecified/default (treated as wildcard)\n - RESOURCE_MATCH_CRITERIA_EXACT: Exact resource name match\n - RESOURCE_MATCH_CRITERIA_PREFIX: Resource name starts with the key\n - RESOURCE_MATCH_CRITERIA_SUFFIX: Resource name ends with the key\n - RESOURCE_MATCH_CRITERIA_REGEX: Resource name matches the regex pattern in key\n - RESOURCE_MATCH_CRITERIA_WILDCARD: Key uses wildcard matching with * (e.g., \"bucket-*\", \"*-prod\", \"*\")", | ||
| "title": "Matching criteria types for resource-based permissions" | ||
| }, | ||
| "apiRolePermission": { | ||
| "type": "object", | ||
| "properties": { | ||
| "resource": { | ||
| "type": "string", | ||
| "title": "Resource name this permission applies to" | ||
| }, | ||
| "match": { | ||
| "$ref": "#/definitions/apiResourceMatch", | ||
| "title": "Resource matching criteria (optional, defaults to wildcard with key=\"*\")" | ||
| }, | ||
| "verbs": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string" | ||
| }, | ||
| "title": "List of allowed verbs/actions for this resource (supports \"*\" for all verbs)" | ||
| }, | ||
| "action": { | ||
| "$ref": "#/definitions/apiRolePermissionAction", | ||
| "title": "Action type: Allow or Deny (Deny takes precedence over Allow)" | ||
| } | ||
| }, | ||
| "title": "Permission definition for custom roles" | ||
| }, | ||
| "apiRolePermissionAction": { | ||
| "type": "string", | ||
| "enum": [ | ||
| "ROLE_PERMISSION_ACTION_UNSPECIFIED", | ||
| "ROLE_PERMISSION_ACTION_ALLOW", | ||
| "ROLE_PERMISSION_ACTION_DENY" | ||
| ], | ||
| "default": "ROLE_PERMISSION_ACTION_UNSPECIFIED", | ||
| "description": "- ROLE_PERMISSION_ACTION_UNSPECIFIED: Unspecified action (invalid)\n - ROLE_PERMISSION_ACTION_ALLOW: Allow the specified verbs on the resource\n - ROLE_PERMISSION_ACTION_DENY: Deny the specified verbs on the resource (takes precedence over Allow)", | ||
| "title": "Action types for role permissions" | ||
| }, |
There was a problem hiding this comment.
Tighten apiRolePermission schema
- verbs: add minItems: 1; consider enum of allowed verbs per resource if feasible; add uniqueItems: true.
- action: make it required; reject ROLE_PERMISSION_ACTION_UNSPECIFIED at request validation.
- match.key: add maxLength; optional pattern when criteria=REGEX.
Apply:
"apiRolePermission": {
"type": "object",
+ "required": ["resource","verbs","action"],
"properties": {
"resource": { "type": "string", "title": "Resource name this permission applies to" },
"match": { "$ref": "#/definitions/apiResourceMatch", "title": "Resource matching criteria (optional, defaults to wildcard with key=\"*\")" },
"verbs": {
"type": "array",
"items": { "type": "string" },
- "title": "List of allowed verbs/actions for this resource (supports \"*\" for all verbs)"
+ "minItems": 1,
+ "uniqueItems": true,
+ "title": "List of allowed verbs/actions for this resource (supports \"*\" for all verbs)"
},
"action": { "$ref": "#/definitions/apiRolePermissionAction", "title": "Action type: Allow or Deny (Deny takes precedence over Allow)" }
},
"title": "Permission definition for custom roles"
}Also:
"apiResourceMatch": {
"type": "object",
+ "required": ["criteria","key"],
"properties": {
"criteria": { "$ref": "#/definitions/apiResourceMatchCriteria", "title": "Matching criteria type" },
- "key": { "type": "string", "title": "The matching key/pattern based on criteria\nFor wildcard: supports * (e.g., \"bucket-*\", \"*-prod\", \"*\")\nFor regex: valid regex pattern\nFor exact/prefix/suffix: literal string" }
+ "key": { "type": "string", "maxLength": 512, "title": "The matching key/pattern..." }
},
"title": "Resource matching criteria for fine-grained permission control"
}📝 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.
| "type": "object", | |
| "properties": { | |
| "criteria": { | |
| "$ref": "#/definitions/apiResourceMatchCriteria", | |
| "title": "Matching criteria type" | |
| }, | |
| "key": { | |
| "type": "string", | |
| "title": "The matching key/pattern based on criteria\nFor wildcard: supports * (e.g., \"bucket-*\", \"*-prod\", \"*\")\nFor regex: valid regex pattern\nFor exact/prefix/suffix: literal string" | |
| } | |
| }, | |
| "title": "Resource matching criteria for fine-grained permission control" | |
| }, | |
| "apiResourceMatchCriteria": { | |
| "type": "string", | |
| "enum": [ | |
| "RESOURCE_MATCH_CRITERIA_UNSPECIFIED", | |
| "RESOURCE_MATCH_CRITERIA_EXACT", | |
| "RESOURCE_MATCH_CRITERIA_PREFIX", | |
| "RESOURCE_MATCH_CRITERIA_SUFFIX", | |
| "RESOURCE_MATCH_CRITERIA_REGEX", | |
| "RESOURCE_MATCH_CRITERIA_WILDCARD" | |
| ], | |
| "default": "RESOURCE_MATCH_CRITERIA_UNSPECIFIED", | |
| "description": "- RESOURCE_MATCH_CRITERIA_UNSPECIFIED: Unspecified/default (treated as wildcard)\n - RESOURCE_MATCH_CRITERIA_EXACT: Exact resource name match\n - RESOURCE_MATCH_CRITERIA_PREFIX: Resource name starts with the key\n - RESOURCE_MATCH_CRITERIA_SUFFIX: Resource name ends with the key\n - RESOURCE_MATCH_CRITERIA_REGEX: Resource name matches the regex pattern in key\n - RESOURCE_MATCH_CRITERIA_WILDCARD: Key uses wildcard matching with * (e.g., \"bucket-*\", \"*-prod\", \"*\")", | |
| "title": "Matching criteria types for resource-based permissions" | |
| }, | |
| "apiRolePermission": { | |
| "type": "object", | |
| "properties": { | |
| "resource": { | |
| "type": "string", | |
| "title": "Resource name this permission applies to" | |
| }, | |
| "match": { | |
| "$ref": "#/definitions/apiResourceMatch", | |
| "title": "Resource matching criteria (optional, defaults to wildcard with key=\"*\")" | |
| }, | |
| "verbs": { | |
| "type": "array", | |
| "items": { | |
| "type": "string" | |
| }, | |
| "title": "List of allowed verbs/actions for this resource (supports \"*\" for all verbs)" | |
| }, | |
| "action": { | |
| "$ref": "#/definitions/apiRolePermissionAction", | |
| "title": "Action type: Allow or Deny (Deny takes precedence over Allow)" | |
| } | |
| }, | |
| "title": "Permission definition for custom roles" | |
| }, | |
| "apiRolePermissionAction": { | |
| "type": "string", | |
| "enum": [ | |
| "ROLE_PERMISSION_ACTION_UNSPECIFIED", | |
| "ROLE_PERMISSION_ACTION_ALLOW", | |
| "ROLE_PERMISSION_ACTION_DENY" | |
| ], | |
| "default": "ROLE_PERMISSION_ACTION_UNSPECIFIED", | |
| "description": "- ROLE_PERMISSION_ACTION_UNSPECIFIED: Unspecified action (invalid)\n - ROLE_PERMISSION_ACTION_ALLOW: Allow the specified verbs on the resource\n - ROLE_PERMISSION_ACTION_DENY: Deny the specified verbs on the resource (takes precedence over Allow)", | |
| "title": "Action types for role permissions" | |
| }, | |
| "type": "object", | |
| "required": ["criteria","key"], | |
| "properties": { | |
| "criteria": { | |
| "$ref": "#/definitions/apiResourceMatchCriteria", | |
| "title": "Matching criteria type" | |
| }, | |
| "key": { | |
| "type": "string", | |
| "maxLength": 512, | |
| "title": "The matching key/pattern based on criteria\nFor wildcard: supports * (e.g., \"bucket-*\", \"*-prod\", \"*\")\nFor regex: valid regex pattern\nFor exact/prefix/suffix: literal string" | |
| } | |
| }, | |
| "title": "Resource matching criteria for fine-grained permission control" | |
| }, |
| "type": "object", | |
| "properties": { | |
| "criteria": { | |
| "$ref": "#/definitions/apiResourceMatchCriteria", | |
| "title": "Matching criteria type" | |
| }, | |
| "key": { | |
| "type": "string", | |
| "title": "The matching key/pattern based on criteria\nFor wildcard: supports * (e.g., \"bucket-*\", \"*-prod\", \"*\")\nFor regex: valid regex pattern\nFor exact/prefix/suffix: literal string" | |
| } | |
| }, | |
| "title": "Resource matching criteria for fine-grained permission control" | |
| }, | |
| "apiResourceMatchCriteria": { | |
| "type": "string", | |
| "enum": [ | |
| "RESOURCE_MATCH_CRITERIA_UNSPECIFIED", | |
| "RESOURCE_MATCH_CRITERIA_EXACT", | |
| "RESOURCE_MATCH_CRITERIA_PREFIX", | |
| "RESOURCE_MATCH_CRITERIA_SUFFIX", | |
| "RESOURCE_MATCH_CRITERIA_REGEX", | |
| "RESOURCE_MATCH_CRITERIA_WILDCARD" | |
| ], | |
| "default": "RESOURCE_MATCH_CRITERIA_UNSPECIFIED", | |
| "description": "- RESOURCE_MATCH_CRITERIA_UNSPECIFIED: Unspecified/default (treated as wildcard)\n - RESOURCE_MATCH_CRITERIA_EXACT: Exact resource name match\n - RESOURCE_MATCH_CRITERIA_PREFIX: Resource name starts with the key\n - RESOURCE_MATCH_CRITERIA_SUFFIX: Resource name ends with the key\n - RESOURCE_MATCH_CRITERIA_REGEX: Resource name matches the regex pattern in key\n - RESOURCE_MATCH_CRITERIA_WILDCARD: Key uses wildcard matching with * (e.g., \"bucket-*\", \"*-prod\", \"*\")", | |
| "title": "Matching criteria types for resource-based permissions" | |
| }, | |
| "apiRolePermission": { | |
| "type": "object", | |
| "properties": { | |
| "resource": { | |
| "type": "string", | |
| "title": "Resource name this permission applies to" | |
| }, | |
| "match": { | |
| "$ref": "#/definitions/apiResourceMatch", | |
| "title": "Resource matching criteria (optional, defaults to wildcard with key=\"*\")" | |
| }, | |
| "verbs": { | |
| "type": "array", | |
| "items": { | |
| "type": "string" | |
| }, | |
| "title": "List of allowed verbs/actions for this resource (supports \"*\" for all verbs)" | |
| }, | |
| "action": { | |
| "$ref": "#/definitions/apiRolePermissionAction", | |
| "title": "Action type: Allow or Deny (Deny takes precedence over Allow)" | |
| } | |
| }, | |
| "title": "Permission definition for custom roles" | |
| }, | |
| "apiRolePermissionAction": { | |
| "type": "string", | |
| "enum": [ | |
| "ROLE_PERMISSION_ACTION_UNSPECIFIED", | |
| "ROLE_PERMISSION_ACTION_ALLOW", | |
| "ROLE_PERMISSION_ACTION_DENY" | |
| ], | |
| "default": "ROLE_PERMISSION_ACTION_UNSPECIFIED", | |
| "description": "- ROLE_PERMISSION_ACTION_UNSPECIFIED: Unspecified action (invalid)\n - ROLE_PERMISSION_ACTION_ALLOW: Allow the specified verbs on the resource\n - ROLE_PERMISSION_ACTION_DENY: Deny the specified verbs on the resource (takes precedence over Allow)", | |
| "title": "Action types for role permissions" | |
| }, | |
| "apiRolePermission": { | |
| "type": "object", | |
| "required": ["resource","verbs","action"], | |
| "properties": { | |
| "resource": { | |
| "type": "string", | |
| "title": "Resource name this permission applies to" | |
| }, | |
| "match": { | |
| "$ref": "#/definitions/apiResourceMatch", | |
| "title": "Resource matching criteria (optional, defaults to wildcard with key=\"*\")" | |
| }, | |
| "verbs": { | |
| "type": "array", | |
| "items": { | |
| "type": "string" | |
| }, | |
| "minItems": 1, | |
| "uniqueItems": true, | |
| "title": "List of allowed verbs/actions for this resource (supports \"*\" for all verbs)" | |
| }, | |
| "action": { | |
| "$ref": "#/definitions/apiRolePermissionAction", | |
| "title": "Action type: Allow or Deny (Deny takes precedence over Allow)" | |
| } | |
| }, | |
| "title": "Permission definition for custom roles" | |
| }, |
🤖 Prompt for AI Agents
In api/swagger/apidocs.swagger.json around lines 3608-3670, the
apiRolePermission schema is too permissive: update the "verbs" array to include
"minItems": 1 and "uniqueItems": true (and replace or augment free-form items
with an enum of allowed verbs per resource where feasible), mark "action" as
required and ensure request validation disallows
"ROLE_PERMISSION_ACTION_UNSPECIFIED", add a "maxLength" to "match.key" (choose a
sensible limit e.g. 256) and enforce an additional "pattern" constraint when
"match.criteria" == "RESOURCE_MATCH_CRITERIA_REGEX" (or document that regex
criteria must be a valid pattern) so requests are properly validated and default
match behavior remains wildcard when omitted.
66171ec to
76573bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 66171ec459b95d4fa6332c424b436703d50f362e and 76573bd607777c6d92188d51ea5374079134d332.
📒 Files selected for processing (5)
api/org-unit-role.proto(2 hunks)api/swagger/apidocs.swagger.json(1 hunks)main.go(1 hunks)pkg/table/const.go(1 hunks)pkg/table/org-unit-role.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/swagger/apidocs.swagger.json
- pkg/table/org-unit-role.go
🔇 Additional comments (2)
api/org-unit-role.proto (1)
202-204: LGTM! Documentation improvement.The inline comment clarifies the response behavior and maintains consistency with
CreateCustomRoleResp.pkg/table/const.go (1)
35-37: LGTM! Follows established conventions.The constant naming and value are consistent with existing collection name patterns.
| // locate Org Unit Custom Role table for managing custom roles | ||
| _, err = table.LocateOrgUnitCustomRoleTable(client) | ||
| if err != nil { | ||
| log.Panicf("failed to locate Org Unit Custom Role table: %s", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify event logging and cleanup maintenance requirements.
The initialization follows the established pattern, but two questions need verification:
-
Event logging: Both
OrgUnitTable(line 421-424) andOrgUnitUserTable(line 432-435) start event loggers after initialization. ShouldOrgUnitCustomRoleTablealso have event logging enabled? -
Cleanup maintenance: The PR description mentions "Add orphaned role cleanup maintenance," but no cleanup controller is started here (unlike
EmailVerificationCleanupControlleron lines 507-510). Should a cleanup controller be initialized for orphaned custom roles?
🏁 Script executed:
#!/bin/bash
# Check if OrgUnitCustomRoleTable has StartEventLogger method and if cleanup controller exists
# Search for StartEventLogger method in org-unit-role table
echo "=== Checking for StartEventLogger in OrgUnitCustomRoleTable ==="
ast-grep --pattern $'func ($_ *OrgUnitCustomRoleTable) StartEventLogger() $_ {
$$$
}'
# Search for cleanup controller for custom roles
echo -e "\n=== Checking for custom role cleanup controller ==="
rg -nP -A5 'CleanupController.*role|role.*CleanupController' --type=goLength of output: 116
🏁 Script executed:
#!/bin/bash
# Broader verification of OrgUnitCustomRoleTable and cleanup patterns
echo "=== 1. Find OrgUnitCustomRoleTable definition and methods ==="
rg -n 'type OrgUnitCustomRoleTable' --type=go -A 20
echo -e "\n=== 2. Find all StartEventLogger calls in main.go ==="
rg -n 'StartEventLogger' main.go -B 2 -A 2
echo -e "\n=== 3. Find all CleanupController initializations ==="
rg -n 'CleanupController' --type=go -B 2 -A 2
echo -e "\n=== 4. Check table initialization patterns in main.go around custom role ==="
rg -n 'LocateOrgUnit.*Table|StartEventLogger|CleanupController' main.go -B 1 -A 3Length of output: 4986
🏁 Script executed:
# Also check if there's documentation about custom roles cleanup in PR
echo "=== 5. Search for 'orphaned' or 'cleanup' in relation to roles ==="
rg -in 'orphan.*role|cleanup.*role' --type=goLength of output: 339
🏁 Script executed:
#!/bin/bash
# Verify base class and check for custom role cleanup controller
echo "=== 1. Check table.Table base class for StartEventLogger ==="
rg -n 'type Table\[' pkg/table --type=go -A 10
echo -e "\n=== 2. Check if StartEventLogger is method on Table interface ==="
rg -n 'func.*StartEventLogger|interface.*Table' pkg/table --type=go -B 2 -A 5
echo -e "\n=== 3. Search for any custom role cleanup controller creation ==="
rg -n 'OrgUnitCustomRole.*Controller|OrgUnitCustomRoleCleanup' --type=go
echo -e "\n=== 4. Check full CleanupOrphanedSoftDeletedRoles method ==="
rg -n 'CleanupOrphanedSoftDeletedRoles' pkg/table/org-unit-role.go -A 20Length of output: 1992
Add event logging and cleanup controller for custom role table.
Two missing initializations:
-
Event logging:
OrgUnitTable(line 421) andOrgUnitUserTable(line 432) both callStartEventLogger(), butOrgUnitCustomRoleTabledoes not. Add the call after line 438 to enable event logging for custom roles. -
Cleanup maintenance: The PR description mentions "orphaned role cleanup maintenance," but no controller is initialized. A
CleanupOrphanedSoftDeletedRoles()method exists inOrgUnitCustomRoleTable(pkg/table/org-unit-role.go:237-258) but is never invoked. Create and initialize a cleanup controller following theEmailVerificationCleanupControllerpattern (lines 507-510).
🤖 Prompt for AI Agents
In main.go around lines 437 to 441, add initialization calls for
OrgUnitCustomRoleTable: after locating the OrgUnitCustomRoleTable (line ~438)
call its StartEventLogger() to enable event logging for custom roles, and then
create and register a cleanup controller mirroring the
EmailVerificationCleanupController pattern (lines ~507-510) that invokes
OrgUnitCustomRoleTable.CleanupOrphanedSoftDeletedRoles() on its ticker; ensure
the controller is started/registered the same way as
EmailVerificationCleanupController so orphaned soft-deleted roles are
periodically cleaned up.
44e58c4 to
bc522b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/table/org-unit-role.go (1)
94-108: Tri-state logic is properly documented.The inline comment at line 99 correctly explains that the filter matches roles where
activeistrueornil. This pattern is consistently documented across all similar filter usages in the file.
🧹 Nitpick comments (3)
pkg/table/org-unit-role.go (3)
158-167: Consider simplifying the pointer initialization.The expression
&[]bool{false}[0]at line 161 works but is unnecessarily complex. A clearer approach would improve readability.Consider this alternative:
func (t *OrgUnitCustomRoleTable) SoftDelete(ctx context.Context, key *OrgUnitCustomRoleKey, deletedBy string) error { + active := false update := &OrgUnitCustomRole{ - Active: &[]bool{false}[0], // Mark as inactive + Active: &active, // Mark as inactive UpdatedBy: deletedBy, // Track who performed the deletion Updated: time.Now().Unix(), // Update timestamp }
237-269: Consider improving error visibility in cleanup maintenance.The method silently continues on errors at lines 255 and 263, which could mask operational issues. For better observability, consider logging errors or collecting them.
Consider adding error logging:
func (t *OrgUnitCustomRoleTable) CleanupOrphanedSoftDeletedRoles(ctx context.Context, tenant, orgUnitId string) error { // Find all soft-deleted roles filter := bson.M{ "key.tenant": tenant, "key.orgUnitId": orgUnitId, "active": false, } softDeletedRoles, err := t.FindMany(ctx, filter, 0, 0) if err != nil { return err } // Check each soft-deleted role for bindings for _, role := range softDeletedRoles { hasBindings, err := t.HasBindings(ctx, tenant, orgUnitId, role.Key.Name) if err != nil { + // Log error for operational visibility + // TODO: Add proper logging with context fields continue } if !hasBindings { err = t.PermanentDelete(ctx, role.Key) if err != nil { + // Log error for operational visibility + // TODO: Add proper logging with context fields continue } } } return nil }Alternatively, collect errors and return a combined error to provide visibility into partial failures.
169-190: Encapsulation violation: direct access to collection field breaks abstraction.The
OrgUnitUserTablestruct exposes itscolfield as public, and line 184 directly accesses it viaorgUnitUserTable.col.Count(ctx, filter). WhileOrgUnitUserTablehas similar count methods (likeCountByOrgUnitId), aCountByRolemethod does not exist.To resolve this, add a
CountByRolemethod toOrgUnitUserTableinpkg/table/org-unit-user.go:// 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 line 184 in
HasBindingsto call this method instead.
📜 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 44e58c4c8735fe58a5e5ab8b4f529cd2653b81b6 and bc522b1.
📒 Files selected for processing (4)
api/org-unit-role.proto(1 hunks)main.go(1 hunks)pkg/table/const.go(1 hunks)pkg/table/org-unit-role.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/org-unit-role.proto
- main.go
🔇 Additional comments (7)
pkg/table/const.go (1)
35-37: LGTM! Clean constant addition.The new collection name constant follows the established pattern and naming convention.
pkg/table/org-unit-role.go (6)
1-92: LGTM! Well-structured type definitions.The custom role data model is comprehensive and properly structured. The use of pointer types for optional fields and the tri-state
Activefield design enables soft-delete semantics.
110-134: LGTM! Query methods follow consistent patterns.The
CountByOrgUnitandGetByTenantmethods apply the same active-filter semantics asGetByOrgUnit, with proper documentation.
136-156: LGTM! Role lookup method is correctly implemented.The method properly filters by composite key and returns an appropriate
NotFounderror when the role doesn't exist.
192-212: LGTM! Deletion logic properly handles binding awareness.The
DeleteCustomRoleWithBindingCheckmethod correctly implements the soft-delete-when-bound pattern, preventing data integrity issues from deleting roles that are still in use.
214-235: LGTM! Name conflict check properly includes soft-deleted roles.The method correctly omits the active filter to find both active and soft-deleted roles, preventing name collisions with roles that may still have bindings.
271-299: LGTM! Singleton pattern correctly implemented.The table accessor and initialization methods follow the established pattern used elsewhere in the codebase and properly handle singleton lifecycle.
| } | ||
|
|
||
| // FindByNameAndOrgUnit finds a specific custom role by name within an org unit | ||
| func (t *OrgUnitCustomRoleTable) FindByNameAndOrgUnit(ctx context.Context, tenant, orgUnitId, roleName string) (*OrgUnitCustomRole, error) { |
There was a problem hiding this comment.
you will already have a find function by key from the generic table itself, so do we still have any use of this function
| } | ||
|
|
||
| // SoftDelete marks a custom role as inactive instead of physically deleting it | ||
| func (t *OrgUnitCustomRoleTable) SoftDelete(ctx context.Context, key *OrgUnitCustomRoleKey, deletedBy string) error { |
There was a problem hiding this comment.
we need to follow symentics for the function definitions appropriately, this function ideally should not be required, instead an update with just setting the Active field should be triggerable
| } | ||
|
|
||
| // HasBindings checks if a custom role has any users assigned to it | ||
| func (t *OrgUnitCustomRoleTable) HasBindings(ctx context.Context, tenant, orgUnitId, roleName string) (bool, error) { |
There was a problem hiding this comment.
This should be impletemented as part of the separate table org unit user and role associations
| } | ||
|
|
||
| // PermanentDelete permanently removes a custom role from the database | ||
| func (t *OrgUnitCustomRoleTable) PermanentDelete(ctx context.Context, key *OrgUnitCustomRoleKey) error { |
There was a problem hiding this comment.
function not needed
| } | ||
|
|
||
| // DeleteCustomRoleWithBindingCheck performs intelligent deletion based on binding status | ||
| func (t *OrgUnitCustomRoleTable) DeleteCustomRoleWithBindingCheck(ctx context.Context, key *OrgUnitCustomRoleKey, deletedBy string) error { |
There was a problem hiding this comment.
need to plan this logic somewhere else, while ensuring atomicity of the operations being performed, either by holding locks or some other synchronization constructs
|
|
||
| // FindAnyByNameAndOrgUnit finds a custom role by name (including soft-deleted ones) | ||
| // This is used to check for name conflicts including soft-deleted roles with bindings | ||
| func (t *OrgUnitCustomRoleTable) FindAnyByNameAndOrgUnit(ctx context.Context, tenant, orgUnitId, roleName string) (*OrgUnitCustomRole, error) { |
There was a problem hiding this comment.
how this function is different from FindByNameAndOrgUnit, probably not needed again as the Find from generic table should be enough
| return nil, err | ||
| } | ||
|
|
||
| orgUnitCustomRoleTable = tbl |
There was a problem hiding this comment.
Why Custom Roles, it is just OrgUnitRoles
Suryanshu185
force-pushed
the
feat/custom-roles-DB-impl
branch
from
bc522b1 to
ed123a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
main.go (2)
446-455: Critical: variable shadowing swaps DB client with Keycloak client.
client, err := keycloak.New(...)masks the DBclientand breaks subsequent calls expecting a DB handle. Rename to avoid shadowing.Apply:
- // create a new keycloak client - client, err := keycloak.New(conf.GetKeycloakEndpoint()) + // create a new keycloak client (avoid shadowing DB client) + kc, 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()) + _ = kc.Logout(context.Background()) }()
421-425: Log message refers to wrong table.Message says “Org Unit User table” but this block handles Org Unit table.
Apply:
- log.Panicf("failed to start event logger for Org Unit User table: %s", err) + log.Panicf("failed to start event logger for Org Unit table: %s", err)
♻️ Duplicate comments (1)
main.go (1)
437-441: Also start event logging and wire cleanup for roles.After locating the OrgUnitRole table, start its event logger and register the orphaned-role cleanup controller, mirroring other tables/cleanups. This was already raised earlier.
🧹 Nitpick comments (5)
pkg/table/org-unit-user.go (1)
88-99: HasRoleBindings looks correct; add index + doc.
- Add a compound index on key.tenant,key.orgUnitId,role to keep Count fast at scale.
- Add a brief comment documenting soft-delete/visibility semantics if any (e.g., whether deleted users are excluded).
pkg/server/org-unit-role.go (1)
105-110: Wire a background cleanup for soft-deleted, unbound roles.Implement a periodic controller to call the table’s cleanup method (scan isDeleted=true, drop if no bindings). Register alongside EmailVerificationCleanupController.
pkg/table/org-unit-role.go (3)
67-85: Document tri‑state delete semantics.
IsDeleted *boolwith filterisDeleted: {$ne: true}treats nil as active. Add a short comment on the field explaining this, and mention it in getters to avoid confusion.
93-118: Queries are fine; ensure indexes.Make sure there’s a unique index on key.tenant,key.orgUnitId,key.name and a sparse/non-unique on isDeleted for filter efficiency. If Table.Initialize doesn’t create these, add EnsureIndexes here.
135-163: Add StartEventLogger for parity with other tables.Match OrgUnit/OrgUnitUser patterns.
Apply:
+// StartEventLogger starts the change-event logger for this table +func (t *OrgUnitRoleTable) StartEventLogger() error { + logger := db.NewEventLogger[OrgUnitRoleKey, OrgUnitRole](t.col, nil) + return logger.Start(context.Background()) +}
📜 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 bc522b1 and ed123a42683958e93abab44545b1d817edc301b8.
📒 Files selected for processing (6)
api/org-unit-role.proto(1 hunks)main.go(1 hunks)pkg/server/org-unit-role.go(1 hunks)pkg/table/const.go(1 hunks)pkg/table/org-unit-role.go(1 hunks)pkg/table/org-unit-user.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/org-unit-role.proto
🔇 Additional comments (2)
pkg/table/const.go (1)
35-37: LGTM: clear collection name.Consistent with existing naming. Ensure any migrations/seeding use the same identifier.
pkg/server/org-unit-role.go (1)
90-100: <!-- [scratchpad_start]
[task review_file_1/1 verification_2/5]
Verify DeleteRoleWithBindingCheck TODO at pkg/server/org-unit-role.go:90-100
[observations]
- File examined: pkg/server/org-unit-role.go lines 80-110
- Lines 90-100 show the exact TODO comment from the review
- Function DeleteCustomRole() is a STUB - returns Unimplemented at line 102
- No actual implementation exists yet; this is future work
- DeleteRoleWithBindingCheck is mentioned only in the TODO, no implementation found
- No transaction/lock patterns found with standard search terms
- Table interface methods not found in initial search
[pending]
- Search for DB wrapper/table abstraction to understand transaction capability
- Look for similar delete patterns in codebase
- Check if MongoDB or SQL transaction support exists
- Verify what deletion patterns are already used
[actions]
- Search for "table." usage patterns and DB wrapper setup
- Find existing delete/update patterns to understand current atomicity handling
- Check context about database wrapper/ORM being used
- 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>
Suryanshu185
force-pushed
the
feat/custom-roles-DB-impl
branch
from
ed123a4 to
421bbd5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
main.go (1)
437-441: Add event logging and cleanup controller for OrgUnitRoleTable.The initialization is missing two components:
Event logging:
OrgUnitTable(line 421) andOrgUnitUserTable(line 432) both callStartEventLogger(), butOrgUnitRoleTabledoes not. This inconsistency should be addressed.Cleanup maintenance: The PR objectives mention "Add orphaned role cleanup maintenance," but no cleanup controller is initialized here (unlike
EmailVerificationCleanupControllerat lines 507-510).Based on past review feedback, add the following after line 441:
_, err = table.LocateOrgUnitRoleTable(client) if err != nil { log.Panicf("failed to locate Org Unit Role table: %s", err) } + + // Start event logger for org unit role table if supported + // Note: Verify if StartEventLogger method exists on OrgUnitRoleTable + + // TODO: Initialize cleanup controller for orphaned soft-deleted roles + // following the pattern of EmailVerificationCleanupController
🧹 Nitpick comments (2)
pkg/table/org-unit-role.go (2)
27-53: Consider consistent capitalization for enum-like string constants.
ResourceMatchCriteriauses lowercase values ("any", "exact", "prefix"), whileRolePermissionActionuses capitalized values ("Deny", "Allow", "Log"). For consistency and maintainability, consider using a uniform capitalization style across all enum-like types.
83-84: Document the tri-state semantics of IsDeleted.The
IsDeletedfield uses a pointer to bool, creating tri-state semantics (true/false/nil). The filtering logic treatsnilandfalseas active roles. Update the comment to explain this behavior explicitly.Apply this diff:
- // Whether this role has been soft-deleted + // Soft-delete flag. Uses pointer for tri-state semantics: + // nil or false = active role, true = soft-deleted role IsDeleted *bool `bson:"isDeleted,omitempty"`
📜 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 ed123a42683958e93abab44545b1d817edc301b8 and 421bbd5.
📒 Files selected for processing (5)
main.go(1 hunks)pkg/server/org-unit-role.go(1 hunks)pkg/table/const.go(1 hunks)pkg/table/org-unit-role.go(1 hunks)pkg/table/org-unit-user.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/table/org-unit-user.go
🔇 Additional comments (5)
pkg/table/const.go (1)
35-37: LGTM!The constant addition follows the established naming convention and is properly documented.
pkg/server/org-unit-role.go (2)
111-139: LGTM!The server initialization follows the established pattern used by other servers in the codebase, with proper gRPC registration, HTTP handler registration, and route wiring.
90-100: HasRoleBindings method is implemented and available.The
HasRoleBindings()method exists inpkg/table/org-unit-user.go(line 88) with the signature expected by the TODO. The method correctly filters by tenant, orgUnitId, and roleName, and returns true if bindings exist. The implementation plan referenced in the TODO is feasible with existing infrastructure.pkg/table/org-unit-role.go (2)
93-107: LGTM!The method correctly filters out soft-deleted roles and uses proper BSON nested-field syntax for MongoDB queries.
109-133: LGTM!Both methods apply consistent soft-delete filtering and follow the expected query patterns.
| // LocateOrgUnitRoleTable initializes and returns the role table | ||
| func LocateOrgUnitRoleTable(client db.StoreClient) (*OrgUnitRoleTable, error) { | ||
| if orgUnitRoleTable != nil { | ||
| return orgUnitRoleTable, nil | ||
| } | ||
|
|
||
| col := client.GetCollection(AuthDatabaseName, OrgUnitRoleCollectionName) | ||
| tbl := &OrgUnitRoleTable{ | ||
| col: col, | ||
| } | ||
|
|
||
| err := tbl.Initialize(col) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| orgUnitRoleTable = tbl | ||
|
|
||
| return orgUnitRoleTable, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add unique index on composite key fields.
The table initialization should create a unique compound index on key.tenant, key.orgUnitId, and key.name to enforce uniqueness and prevent duplicate role entries. Without this index, data integrity cannot be guaranteed.
Add index creation after line 158:
err := tbl.Initialize(col)
if err != nil {
return nil, err
}
+
+ // Create unique compound index on the composite key
+ 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, err
+ }
orgUnitRoleTable = tblYou'll need to import:
"go.mongodb.org/mongo-driver/mongo"
"go.mongodb.org/mongo-driver/mongo/options"🤖 Prompt for AI Agents
In pkg/table/org-unit-role.go around lines 144 to 163, after tbl.Initialize
(around line 158) add creation of a unique compound index on key.tenant,
key.orgUnitId and key.name to enforce uniqueness: import "context",
"go.mongodb.org/mongo-driver/mongo" and
"go.mongodb.org/mongo-driver/mongo/options", then build an IndexModel with Keys
bson.D{{"key.tenant", 1}, {"key.orgUnitId", 1}, {"key.name", 1}} and Options
options.Index().SetUnique(true), and call
col.Indexes().CreateOne(context.Background(), model) (handle and return any
error) so duplicate role entries are prevented.
2 participants
Summary by CodeRabbit