Feature: Add org‑unit scoped custom role management with full CRUD an…#103
Feature: Add org‑unit scoped custom role management with full CRUD an…#103Suryanshu185 wants to merge 1 commit into
Conversation
Suryanshu185
force-pushed
the
main
branch
6 times, most recently
from
f49f7de to
26fbbbf
Compare
WalkthroughAdds org-unit scoped custom-role management: protobuf RPCs and HTTP routes; OpenAPI schemas; gateway route metadata, response buffering/filtering and PBAC checks; server-side CRUD handlers, validation, and user-role cleanup; a Mongo-backed OrgUnitCustomRole table and constant; and startup wiring for the new table. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Gateway
participant OrgUnitRoleService
participant OrgUnitCustomRoleTable
participant Mongo
rect rgba(230,240,255,0.6)
note right of Client: Custom-role CRUD request
Client->>Gateway: HTTP /api/auth/v1/ou/{ou}/role[/{name}]
Gateway->>OrgUnitRoleService: gRPC Create/Update/Get/Delete
alt Create
OrgUnitRoleService->>OrgUnitCustomRoleTable: Insert OrgUnitCustomRole
OrgUnitCustomRoleTable->>Mongo: write
Mongo-->>OrgUnitCustomRoleTable: ack
OrgUnitCustomRoleTable-->>OrgUnitRoleService: created
OrgUnitRoleService-->>Gateway: CreateCustomRoleResp
else Read/Update/Delete
OrgUnitRoleService->>OrgUnitCustomRoleTable: Find/Update/Delete (binding checks)
OrgUnitCustomRoleTable->>Mongo: query/write
Mongo-->>OrgUnitCustomRoleTable: result
OrgUnitCustomRoleTable-->>OrgUnitRoleService: outcome
OrgUnitRoleService-->>Gateway: Resp
end
Gateway-->>Client: HTTP response
end
sequenceDiagram
autonumber
actor User
participant Gateway
participant OrgUnitCustomRoleTable
rect rgba(240,255,240,0.6)
note right of User: Gateway enforces resource-level PBAC for list/detail endpoints
User->>Gateway: HTTP request to OU-protected endpoint
Gateway->>Gateway: extract RouteInfo {resource, verb, resourceName}
alt resource references a custom-role name
Gateway->>OrgUnitCustomRoleTable: FindByNameAndOrgUnit(tenant, ou, name)
OrgUnitCustomRoleTable-->>Gateway: customRole (permissions)
Gateway->>Gateway: evaluate deny-before-allow rules
alt allowed
Gateway->>User: forward request / 200
else denied
Gateway-->>User: 403 Forbidden
end
else listing endpoint
Gateway->>Gateway: wrap response in filteringResponseWriter (buffer)
Gateway->>OrgUnitCustomRoleTable: fetch rules as needed for filtering pages
Gateway-->>User: filtered list response
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
main.go (1)
401-410: Variable shadowing ‘client’ breaks types; rename Keycloak client.client is used for the DB earlier; here it’s re-declared for Keycloak, likely causing type confusion for subsequent calls.
Apply:
- // create a new keycloak client - client, err := keycloak.New(conf.GetKeycloakEndpoint()) + // create a new keycloak client + kcClient, err := keycloak.New(conf.GetKeycloakEndpoint()) if err != nil { // failed to create keycloak client, nothing more can be done log.Panicf("failed to create keycloak client: %s", err) } defer func() { - _ = client.Logout(context.Background()) + _ = kcClient.Logout(context.Background()) }()- _, err = tenant.NewSetupController(client) + _, err = tenant.NewSetupController(kcClient) ... - _, err = tenant.NewRoleController(client) + _, err = tenant.NewRoleController(kcClient) ... - _, err = tenant.NewAdminController(client) + _, err = tenant.NewAdminController(kcClient)Optionally consider starting an event logger for the Org Unit Custom Role table (if supported) to match OU/OU-User.
Also applies to: 419-435
pkg/server/org-unit-user.go (1)
67-71: Inconsistent role validation and no custom roles on Add; include ‘default’ and reuse validateRole.
- Add path only allows built-in roles; should accept valid custom roles too.
- validateRole treats default as invalid; Update rejects default.
Apply:
func (s *OrgUnitUserServer) AddOrgUnitUser(ctx context.Context, req *api.OrgUnitUserAddReq) (*api.OrgUnitUserAddResp, error) { authInfo, _ := auth.GetAuthInfoFromContext(ctx) if authInfo == nil { return nil, status.Errorf(codes.Unauthenticated, "User not authenticated") } - // validate role, currently only admin, default and auditor roles are allowed - if req.Role != "admin" && req.Role != "default" && req.Role != "auditor" { - return nil, status.Errorf(codes.InvalidArgument, "Invalid role: %s", req.Role) - } + // validate role (built-in or custom) + if err := s.validateRole(ctx, req.Role, req.Ou, authInfo.Realm); err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Invalid role: %s", req.Role) + }func (s *OrgUnitUserServer) validateRole(ctx context.Context, roleName, orgUnitId, tenant string) error { // Check if it's a built-in system role - if roleName == "admin" || roleName == "auditor" { + if roleName == "admin" || roleName == "auditor" || roleName == "default" { return nil // Built-in roles are always valid }Also applies to: 104-107, 245-261
🧹 Nitpick comments (18)
api/org-unit-role.proto (5)
92-102: Use enums for role type and Timestamps for dates; align field naming.
- type should be an enum (e.g., ROLE_TYPE_BUILTIN, ROLE_TYPE_CUSTOM), not a free-form string.
- created should use google.protobuf.Timestamp, not int64.
- desc is inconsistent with description elsewhere; prefer description for consistency.
Apply these diffs:
- // role type: "built-in" for system roles (admin, auditor) or "custom" for user-defined roles - string type = 3; + // role type: built-in or custom + enum RoleType { + ROLE_TYPE_UNSPECIFIED = 0; + ROLE_TYPE_BUILTIN = 1; + ROLE_TYPE_CUSTOM = 2; + } + RoleType type = 3; - // role description, provding details about the role - string desc = 2; + // role description, providing details about the role + string description = 2; - // creation timestamp for custom roles (only applicable for custom roles) - int64 created = 5; + // creation timestamp for custom roles (only applicable for custom roles) + google.protobuf.Timestamp created = 5;Also add the import:
import "google/api/annotations.proto"; import "coreapis/api/role.proto"; +import "google/protobuf/timestamp.proto";
156-164: Support partial updates with a FieldMask (or mark fields optional).Without presence (proto3) you can’t distinguish “unset” vs “set empty,” making PUT updates risky. Prefer PATCH + FieldMask or keep PUT but require full object; if partial updates are intended, add FieldMask.
Example:
+import "google/protobuf/field_mask.proto";message UpdateCustomRoleReq { // Organization unit ID string ou = 1; // Name of the custom role to update string name = 2; // Updated display name for the custom role - string displayName = 3; + string displayName = 3; // Updated description explaining the purpose of this custom role - string description = 4; + string description = 4; // Updated list of permissions granted by this custom role repeated RolePermission permissions = 5; + + // Fields to update (if supporting partial updates) + google.protobuf.FieldMask update_mask = 6; }If adopting PATCH, change the HTTP annotation accordingly.
195-206: Use google.protobuf.Timestamp for created/updated in GetCustomRoleResp.Timestamps as int64 are ambiguous (seconds vs millis) and less interoperable.
- // Timestamp when the role was created - int64 created = 5; + // Timestamp when the role was created + google.protobuf.Timestamp created = 5; - // Timestamp when the role was last updated - int64 updated = 7; + // Timestamp when the role was last updated + google.protobuf.Timestamp updated = 7;Ensure you have:
import "google/protobuf/timestamp.proto";
65-67: Remove duplicated comment.The Delete comment is repeated twice.
- // Delete a custom role from the organization unit - // Delete (soft delete) a custom role from the organization unit + // Delete (soft delete) a custom role from the organization unit
115-122: Consider constraining verbs to a known set.repeated string verbs is flexible but error-prone. If you have a canonical verb set, use an enum or validate strictly server-side.
pkg/server/org-unit-user.go (1)
141-195: Streamline pre-delete fetch and guard nils.
- Prefer a typed Get/Find by key over a raw bson filter + FindMany(…, 1) if available, to reduce coupling.
- When handling existingRole from FindAnyByNameAndOrgUnit, guard against unexpected nils before dereferencing Active.
Example:
// Prefer: user, err := s.tbl.Find(ctx, key) // and check err == errors.NotFound, else use user.Role.pkg/gateway/server.go (1)
263-275: Support wildcards and case-insensitive verbs in custom-role checks.Small enhancement to make permissions more ergonomic and less brittle to verb casing.
Apply this diff:
func (s *gateway) validatePermissions(customRole *table.OrgUnitCustomRole, routeInfo *RouteInfo) bool { // Check each permission in the custom role for _, permission := range customRole.Permissions { - if permission.Resource == routeInfo.Resource { + if permission.Resource == routeInfo.Resource || permission.Resource == "*" { // Check if the required verb is allowed for _, allowedVerb := range permission.Verbs { - if allowedVerb == routeInfo.Verb { + if strings.EqualFold(allowedVerb, routeInfo.Verb) || allowedVerb == "*" { return true // Permission granted } } } } // If no matching permission found, deny access return false }api/swagger/apidocs.swagger.json (5)
302-317: Tighten DELETE summary and move behavior detail to description.Duplicate summary text; move soft-delete note to description.
- "summary": "Delete a custom role from the organization unit\nDelete (soft delete) a custom role from the organization unit", + "summary": "Delete a custom role from the organization unit", + "description": "Soft deletes when bindings exist; permanently deletes otherwise.",
3538-3553: Constrain verbs array and ensure uniqueness.Add minItems and uniqueItems to reduce invalid inputs; mirrors server’s “at least one verb” requirement.
"apiRolePermission": { "type": "object", "properties": { "resource": { "type": "string", "title": "Resource name this permission applies to" }, "verbs": { "type": "array", + "minItems": 1, + "uniqueItems": true, "items": { "type": "string" }, "title": "List of allowed verbs/actions for this resource" } }, "title": "Permission definition for custom roles" },
3347-3367: Fix typo in role description.“provding” → “providing”.
- "desc": { - "type": "string", - "title": "role description, provding details about the role" - }, + "desc": { + "type": "string", + "title": "role description, providing details about the role" + },
223-262: Consider 201 for create endpoint.POST create responses typically use 201 Created. Optional, but improves API semantics.
2433-2456: OpenAPI refs: avoid setting both $ref and type in items.When using $ref, omit the sibling "type": "object" under items. Several new arrays include both; many validators will ignore type, but cleaning this up reduces lint noise.
Also applies to: 2460-2477, 2786-2807
pkg/server/org-unit-role.go (4)
37-55: List roles: OK, but consider pagination for custom roles.Fetching up to 1000 items is a guard, but adding offset/limit to the request and returning count would scale better.
Also applies to: 56-74
139-152: Use a named bool for the Active pointer (readability).Minor readability improvement.
- customRole := &table.OrgUnitCustomRole{ + active := true + customRole := &table.OrgUnitCustomRole{ Key: &table.OrgUnitCustomRoleKey{ Tenant: authInfo.Realm, OrgUnitId: req.Ou, Name: req.Name, }, DisplayName: req.DisplayName, Description: req.Description, Permissions: permissions, Created: time.Now().Unix(), CreatedBy: authInfo.UserName, - Active: &[]bool{true}[0], // Mark role as active + Active: &active, // Mark role as active }
336-390: Create request validation matches API; consider character constraints.Add a simple pattern (e.g., ^[a-zA-Z][a-zA-Z0-9_-]{1,49}$) for Name to avoid spaces and odd chars; this can be mirrored in Swagger.
410-434: Update validation mirrors Create; consider deduping and canonicalization.
- Trim spaces and dedupe verbs per permission.
- Optionally validate resource + verb against route definitions to catch typos early.
pkg/table/org-unit-custom-role.go (2)
139-160: Bindings check relies on OrgUnitUser indexes; ensure supporting index.Counting by tenant+orgUnitId+role will need an index for scale.
Would you confirm there’s an index on {"key.tenant", "key.orgUnitId", "role"} in OrgUnitUser collection?
16-16: Optional: guard singleton init with sync.Once.Minor race window if Locate is called concurrently.
Also applies to: 250-269
📜 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 064ef50 and 4065bbc7478ad4a50ce5c27980871a5b493c2c72.
⛔ 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(3 hunks)pkg/gateway/server.go(5 hunks)pkg/server/org-unit-role.go(2 hunks)pkg/server/org-unit-user.go(7 hunks)pkg/table/const.go(1 hunks)pkg/table/org-unit-custom-role.go(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
api/swagger/apidocs.swagger.json
[medium] 2448-2455: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (15)
api/org-unit-role.pb.route.go (1)
20-46: Routes for custom roles look consistent with RPCs and scopes.Resource and verbs align with proto annotations; paths are coherent.
Please re-run route generation to ensure this file remains in sync with proto changes and swagger (if not already automated).
pkg/table/const.go (1)
30-32: LGTM: added collection name.Constant name and value align with naming convention.
pkg/gateway/routes.go (1)
26-28: Good addition: carry resource and verb per route.This enables gateway-level permission checks without extra lookups.
Confirm downstream permission checks consume these fields (e.g., in gateway/server) and handle empty values defensively for legacy routes.
Also applies to: 54-56, 67-69
pkg/server/org-unit-user.go (1)
206-215: LGTM: wiring custom role table.Table retrieval and panic-on-error are consistent with existing patterns.
pkg/gateway/server.go (4)
47-56: LGTM: wiring of custom-role table into gateway.Initialization and field addition look correct.
224-234: LGTM: built-in OU roles mapped to read/write semantics.Admin wildcard and auditor read-only checks are straightforward and correct.
300-310: LGTM: defer-based access logging hook.This ensures access logs even on early returns with proper context captured.
535-538: LGTM: custom-role table retrieval and gateway construction.Table retrieval with panic-on-error and dependency injection into gateway is appropriate.
Also applies to: 555-562
api/swagger/apidocs.swagger.json (1)
2448-2455: Address Checkov CKV_OPENAPI_21: add a maximum for permissions array.Handled by adding maxItems: 20 above, which matches server constraints.
Please re-run your OpenAPI linter/Checkov to confirm the warning is resolved.
pkg/server/org-unit-role.go (4)
92-96: Reserved names check: good coverage.Blocking admin/default/auditor prevents clashes with built-ins.
98-134: Name reuse with soft-deletes: solid, binding-aware flow.The “find any” + bindings check + permanent delete unblocks ghost entries safely.
175-191: Confirm Update semantics (partial vs. replace).updateRole omits Key and Active; ensure table.Update performs a partial $set and doesn’t overwrite the entire doc. If it’s a full replace, this will drop Key and Active.
Can you confirm the behavior of table.Update in github.com/go-core-stack/core/table? If it’s a partial update, we’re good; if not, we should switch to a patch/merge update call.
Also applies to: 199-207
299-309: LGTM: table acquisition at server init.Panicking on missing table ensures startup fails fast.
pkg/table/org-unit-custom-role.go (2)
64-79: Queries correctly filter active roles (nil or true).Consistent use of active != false across getters is good.
Also applies to: 91-105, 106-127
167-182: Deletion strategy is sound.Soft-delete on bindings and hard-delete otherwise is the right trade-off.
| "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.
🛠️ Refactor suggestion
Align schema with server validation: require fields and cap permissions array.
Server enforces non-empty name/displayName/description/permissions and max 20 permissions. Reflect this in OpenAPI to prevent client/server divergence.
"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",
+ "minItems": 1,
+ "maxItems": 20,
"items": {
- "type": "object",
"$ref": "#/definitions/apiRolePermission"
},
"title": "List of permissions granted by this custom role"
}
},
+ "required": ["name","displayName","description","permissions"],
"title": "Create custom role request"
},📝 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.
| "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" | |
| }, | |
| "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", | |
| "minItems": 1, | |
| "maxItems": 20, | |
| "items": { | |
| "$ref": "#/definitions/apiRolePermission" | |
| }, | |
| "title": "List of permissions granted by this custom role" | |
| } | |
| }, | |
| "required": ["name", "displayName", "description", "permissions"], | |
| "title": "Create custom role request" | |
| }, |
🧰 Tools
🪛 Checkov (3.2.334)
[medium] 2448-2455: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🤖 Prompt for AI Agents
In api/swagger/apidocs.swagger.json around lines 2433 to 2458, the
OrgUnitRoleCreateCustomRoleBody schema lacks required constraints and a cap on
the permissions array; update the schema to mark
"name","displayName","description","permissions" as required, add minLength: 1
(or equivalent non-empty constraint) to the three string properties, and set
permissions to have minItems: 1 and maxItems: 20 (with items unchanged) so the
OpenAPI contract matches server validation.
| "type": "object", | ||
| "properties": { | ||
| "displayName": { | ||
| "type": "string", | ||
| "title": "Updated display name for the custom role" | ||
| }, | ||
| "description": { | ||
| "type": "string", | ||
| "title": "Updated description explaining the purpose of this custom role" | ||
| }, | ||
| "permissions": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "object", | ||
| "$ref": "#/definitions/apiRolePermission" | ||
| }, | ||
| "title": "Updated list of permissions granted by this custom role" | ||
| } | ||
| }, | ||
| "title": "Update custom role request" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make update fields required and cap permissions array.
Server requires displayName/description/permissions and limits permissions to 20.
"OrgUnitRoleUpdateCustomRoleBody": {
"type": "object",
"properties": {
"displayName": {
"type": "string",
"title": "Updated display name for the custom role"
},
"description": {
"type": "string",
"title": "Updated description explaining the purpose of this custom role"
},
"permissions": {
"type": "array",
+ "minItems": 1,
+ "maxItems": 20,
"items": {
- "type": "object",
"$ref": "#/definitions/apiRolePermission"
},
"title": "Updated list of permissions granted by this custom role"
}
},
+ "required": ["displayName","description","permissions"],
"title": "Update custom role request"
},📝 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": { | |
| "displayName": { | |
| "type": "string", | |
| "title": "Updated display name for the custom role" | |
| }, | |
| "description": { | |
| "type": "string", | |
| "title": "Updated description explaining the purpose of this custom role" | |
| }, | |
| "permissions": { | |
| "type": "array", | |
| "items": { | |
| "type": "object", | |
| "$ref": "#/definitions/apiRolePermission" | |
| }, | |
| "title": "Updated list of permissions granted by this custom role" | |
| } | |
| }, | |
| "title": "Update custom role request" | |
| }, | |
| "type": "object", | |
| "properties": { | |
| "displayName": { | |
| "type": "string", | |
| "title": "Updated display name for the custom role" | |
| }, | |
| "description": { | |
| "type": "string", | |
| "title": "Updated description explaining the purpose of this custom role" | |
| }, | |
| "permissions": { | |
| "type": "array", | |
| "minItems": 1, | |
| "maxItems": 20, | |
| "items": { | |
| "$ref": "#/definitions/apiRolePermission" | |
| }, | |
| "title": "Updated list of permissions granted by this custom role" | |
| } | |
| }, | |
| "required": ["displayName","description","permissions"], | |
| "title": "Update custom role request" | |
| }, |
🤖 Prompt for AI Agents
In api/swagger/apidocs.swagger.json around lines 2460 to 2480, the Update custom
role request schema must mark displayName, description, and permissions as
required and enforce the server-side limit of 20 permissions; add a "required":
["displayName","description","permissions"] entry to the object and add
"maxItems": 20 to the permissions array definition (optionally include
"minItems": 1 if the API requires at least one permission).
| // GetOrgUnitCustomRoleTable returns the global custom role table instance | ||
| func GetOrgUnitCustomRoleTable() (*OrgUnitCustomRoleTable, error) { | ||
| if orgUnitCustomRoleTable != nil { | ||
| return orgUnitCustomRoleTable, nil | ||
| } | ||
|
|
||
| return nil, errors.Wrapf(errors.NotFound, "org unit custom role table not found") | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Ensure unique index on (tenant, orgUnitId, name).
To prevent duplicates under concurrent create, enforce a unique index at init if not already present in table.Initialize.
Please confirm Initialize sets a unique compound index on key.tenant+key.orgUnitId+key.name; if not, add it in LocateOrgUnitCustomRoleTable.
Also applies to: 250-269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "--- find file ---"
rg -n --hidden "org-unit-custom-role.go" || true
echo "--- show file (pkg/table/org-unit-custom-role.go) if present ---"
if [ -f "pkg/table/org-unit-custom-role.go" ]; then
sed -n '1,220p' pkg/table/org-unit-custom-role.go
echo "----- lines 221-440 -----"
sed -n '221,440p' pkg/table/org-unit-custom-role.go
else
echo "pkg/table/org-unit-custom-role.go not found"
fi
echo "--- search for related symbols ---"
rg -n --hidden "OrgUnitCustomRoleTable" || true
rg -n --hidden "LocateOrgUnitCustomRoleTable" || true
rg -n --hidden "GetOrgUnitCustomRoleTable" || true
echo "--- search for Initialize implementations in repo ---"
rg -n --hidden "func .*Initialize" --type-add 'go:*.go' -g '!**/vendor/**' || true
echo "--- search for index/index creation patterns mentioning tenant/orgUnit/name ---"
rg -n --hidden -S "CreateIndex|EnsureIndex|CreateIndexes|EnsureIndexes|Index\(" || true
rg -n --hidden -S "tenant.*orgUnitId.*name|tenant.*name.*orgUnitId|orgUnitId.*tenant.*name|orgUnitId.*name.*tenant|name.*tenant.*orgUnitId" || true
echo "--- search for 'tenant' usage in pkg/table ---"
rg -n --hidden "tenant" pkg/table || trueLength of output: 18095
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "----- show tenant.go -----"
sed -n '1,260p' pkg/table/tenant.go || true
echo "----- show org-unit.go -----"
sed -n '1,260p' pkg/table/org-unit.go || true
echo "----- show org-unit-user.go -----"
sed -n '1,320p' pkg/table/org-unit-user.go || true
echo "----- show user.go -----"
sed -n '1,320p' pkg/table/user.go || true
echo "----- show api-keys.go -----"
sed -n '1,320p' pkg/table/api-keys.go || true
echo "----- search for .Initialize( across repo -----"
rg -n "\.Initialize\(" || true
echo "----- search for EnsureIndex|CreateIndex|CreateIndexes|Index across repo -----"
rg -n -S "EnsureIndex|CreateIndex|CreateIndexes|CreateIndexes|Index\(" || true
echo "----- list Locate*Table functions -----"
rg -n "Locate.*Table" || trueLength of output: 19705
🌐 Web query:
github.com/go-core-stack/core table Initialize Table Initialize create index
💡 Result:
Short answer:
-
To initialize a Table from go-core-stack/core you must call its Initialize(col db.StoreCollection) method with a db.StoreCollection implementation before using the table. Initialize performs sanity checks and registers the key type with the collection. [1]
-
The table package does not create database indexes for you; index creation belongs to your backing store (migrations or the db.StoreCollection implementation). Create indexes in your DB (SQL CREATE INDEX / Mongo createIndex, or via your store's migration code) or implement index-creation in the StoreCollection you pass to Initialize. [1][2]
References:
- table.Initialize documentation — github.com/go-core-stack/core/table. [pkg.go.dev entry]. [1]
- Example guidance for creating DB tables/indexes (do it in DB/migrations or connection setup) — MySQL DB ops docs (example of creating tables / initialize DB connections). [2]
Add unique compound index on (key.tenant, key.orgUnitId, key.name)
go-core-stack's Table.Initialize doesn't create DB indexes and this repo contains no index-creation for this collection — add a unique compound index (tenant + orgUnitId + name) to prevent duplicate rows under concurrent creates. Add the index in LocateOrgUnitCustomRoleTable (pkg/table/org-unit-custom-role.go) during initialization or manage it via DB migrations/connection setup.
🤖 Prompt for AI Agents
In pkg/table/org-unit-custom-role.go around lines 241 to 249, the repo lacks a
unique compound index to prevent concurrent duplicate creates; modify
LocateOrgUnitCustomRoleTable (the table initialization path) to ensure a unique
compound index on the underlying collection for fields key.tenant,
key.orgUnitId, and key.name (or create it as part of DB migrations/connection
setup if preferred). Specifically, when creating/assigning
orgUnitCustomRoleTable, call the DB driver's index creation API to create a
unique compound index on (tenant, orgUnitId, name) with appropriate options
(unique=true, background if supported) and handle/index-exists errors gracefully
so initialization is idempotent. Ensure the index creation is executed before
returning the table instance.
740efaa to
37ac535
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/server/org-unit-user.go (1)
61-96: Update AddOrgUnitUser to use the validateRole helper.The
AddOrgUnitUsermethod still uses hardcoded role validation (lines 68-70), whileUpdateOrgUnitUseruses the newvalidateRolehelper. This creates inconsistency in role validation logic across the two methods.Apply this diff to use the consistent validation approach:
func (s *OrgUnitUserServer) AddOrgUnitUser(ctx context.Context, req *api.OrgUnitUserAddReq) (*api.OrgUnitUserAddResp, error) { authInfo, _ := auth.GetAuthInfoFromContext(ctx) if authInfo == nil { return nil, status.Errorf(codes.Unauthenticated, "User not authenticated") } - // validate role, currently only admin, default and auditor roles are allowed - if req.Role != "admin" && req.Role != "default" && req.Role != "auditor" { - return nil, status.Errorf(codes.InvalidArgument, "Invalid role: %s", req.Role) + // validate role - check if it's a built-in role or custom role + if err := s.validateRole(ctx, req.Role, req.Ou, authInfo.Realm); err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Invalid role: %s - %v", req.Role, err) }
♻️ Duplicate comments (1)
api/swagger/apidocs.swagger.json (1)
2481-2528: Address the previously flagged schema validation issues.The OpenAPI schema for custom role creation and update bodies still lacks the constraints enforced by server-side validation. This was previously flagged in past review comments but hasn't been addressed yet.
Server validation (in
pkg/server/org-unit-role.golines 337-434) enforces:
- Required fields: name, displayName, description, permissions (for create); displayName, description, permissions (for update)
- Non-empty string constraints for name, displayName, description
- Permissions array: minItems: 1, maxItems: 20
Please apply the fixes suggested in the past review comments to align the OpenAPI schema with server validation.
🧹 Nitpick comments (2)
pkg/server/org-unit-role.go (2)
29-79: Consider adding pagination support for custom roles.The hardcoded limit of 1000 custom roles (line 57) could be problematic for organizations with many custom roles. The method doesn't support pagination parameters from the request.
Consider either:
- Adding offset/limit parameters to the
OrgUnitRolesListReqmessage and using them here- Documenting the 1000-role limit in the proto file comments
- Deferring pagination to a future enhancement if 1000 roles is sufficient for the initial release
For now, the graceful degradation (returning built-in roles even if custom role fetch fails) is good defensive programming.
81-166: Use a more idiomatic approach for the boolean pointer.Line 150 uses
Active: &[]bool{true}[0]to create a pointer totrue, which is clever but non-idiomatic and less readable.Consider using a helper variable instead:
+ active := true customRole := &table.OrgUnitCustomRole{ Key: &table.OrgUnitCustomRoleKey{ Tenant: authInfo.Realm, OrgUnitId: req.Ou, Name: req.Name, }, DisplayName: req.DisplayName, Description: req.Description, Permissions: permissions, Created: time.Now().Unix(), CreatedBy: authInfo.UserName, - Active: &[]bool{true}[0], // Mark role as active + Active: &active, }The rest of the method correctly handles:
- Reserved name validation
- Existing role checks (including soft-deleted roles)
- Cleanup of orphaned soft-deleted roles before recreation
📜 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 4065bbc7478ad4a50ce5c27980871a5b493c2c72 and 37ac535b022ac7c1b520987bf2b15ba37bc149d4.
⛔ 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(3 hunks)pkg/gateway/server.go(5 hunks)pkg/server/org-unit-role.go(2 hunks)pkg/server/org-unit-user.go(7 hunks)pkg/table/const.go(1 hunks)pkg/table/org-unit-custom-role.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/gateway/routes.go
- pkg/table/const.go
- api/org-unit-role.pb.route.go
- pkg/gateway/server.go
- pkg/table/org-unit-custom-role.go
🧰 Additional context used
🪛 Checkov (3.2.334)
api/swagger/apidocs.swagger.json
[medium] 2496-2503: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (10)
main.go (1)
392-396: LGTM!The initialization of the Org Unit Custom Role table follows the established pattern used for other tables in this file, with appropriate error handling.
pkg/server/org-unit-user.go (2)
129-198: LGTM!The enhanced deletion flow correctly:
- Pre-fetches user data to preserve the role for potential cleanup
- Deletes the user first
- Then performs cleanup of orphaned soft-deleted custom roles with proper error handling
The non-fatal error handling for cleanup is appropriate since the primary operation (user deletion) has already succeeded.
200-242: LGTM!The server initialization correctly wires in the custom role table with proper error handling.
api/org-unit-role.proto (1)
1-221: LGTM!The protobuf definitions for custom role management are well-structured and comprehensive:
- Complete CRUD surface with clear documentation
- Consistent naming conventions
- RolePermission message properly models per-role permissions
- Extended OrgUnitRolesListEntry includes necessary metadata for both built-in and custom roles
pkg/server/org-unit-role.go (6)
168-212: LGTM!The update operation is straightforward and correctly handles authentication, validation, permission conversion, and error cases.
214-244: LGTM!The retrieval operation correctly fetches the role, converts permissions back to the protobuf format, and handles error cases appropriately.
246-273: LGTM!The delete operation correctly delegates the binding-aware deletion logic to the table layer, which handles the complexity of deciding between soft delete and permanent delete based on whether bindings exist.
275-297: LGTM!The permission conversion helpers are straightforward and correctly translate between protobuf and table representations.
299-335: LGTM!The server initialization correctly:
- Retrieves the custom role table with panic on failure (appropriate for initialization)
- Initializes the server struct with the table reference
- Registers gRPC services and HTTP routes
337-434: LGTM!The validation helpers are comprehensive and correctly enforce:
- Name, displayName, and description length constraints
- Permissions array size (1-20)
- Non-empty resource and verb values within permissions
These constraints match the requirements that should be reflected in the OpenAPI schema (as noted in the swagger file review).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
api/swagger/apidocs.swagger.json (2)
2481-2506: Schema constraints should match server validation.The schema lacks required field markers and array size constraints that the server enforces. The server requires all fields to be non-empty and limits permissions to 20 items (see
pkg/server/org-unit-role.golines 337-390).Apply this diff to align the schema with server validation:
"OrgUnitRoleCreateCustomRoleBody": { "type": "object", "properties": { "name": { "type": "string", + "minLength": 2, + "maxLength": 50, "title": "Name of the custom role (must be unique within the org unit)" }, "displayName": { "type": "string", + "minLength": 1, + "maxLength": 100, "title": "Display name for the custom role" }, "description": { "type": "string", + "minLength": 1, + "maxLength": 500, "title": "Description explaining the purpose of this custom role" }, "permissions": { "type": "array", + "minItems": 1, + "maxItems": 20, "items": { "$ref": "#/definitions/apiRolePermission" }, "title": "List of permissions granted by this custom role" } }, + "required": ["name", "displayName", "description", "permissions"], "title": "Create custom role request" },
2507-2528: Update schema constraints should match server validation.Similar to the create schema, the update schema lacks required field markers and array size constraints that the server enforces (see
pkg/server/org-unit-role.golines 392-434).Apply this diff:
"OrgUnitRoleUpdateCustomRoleBody": { "type": "object", "properties": { "displayName": { "type": "string", + "minLength": 1, + "maxLength": 100, "title": "Updated display name for the custom role" }, "description": { "type": "string", + "minLength": 1, + "maxLength": 500, "title": "Updated description explaining the purpose of this custom role" }, "permissions": { "type": "array", + "minItems": 1, + "maxItems": 20, "items": { "$ref": "#/definitions/apiRolePermission" }, "title": "Updated list of permissions granted by this custom role" } }, + "required": ["displayName", "description", "permissions"], "title": "Update custom role request" },
🧹 Nitpick comments (3)
pkg/gateway/server.go (1)
262-278: Consider short-circuiting on resource match.The nested loop iterates all permissions and verbs. While correct, you could slightly optimize by breaking the inner loop once the resource matches but before checking all verbs:
for _, permission := range customRole.Permissions { if permission.Resource == routeInfo.Resource { for _, allowedVerb := range permission.Verbs { if allowedVerb == routeInfo.Verb { return true } } // Resource matched but verb didn't - no need to check other permissions break } }This optimization is minor and can be deferred.
pkg/server/org-unit-user.go (1)
141-161: Consider using Find instead of FindMany.The code uses
FindManywithlimit=1to fetch a single user. Since you're looking up by a unique key, consider using a directFindmethod if available, which would be more semantically clear:// Get the user data before deletion to check their role userData, err := s.tbl.Find(ctx, key) if err != nil { if errors.IsNotFound(err) { return nil, status.Errorf(codes.NotFound, "Org Unit User %s, not found", req.User) } log.Printf("failed to get org unit user before deletion, got error: %s", err) return nil, status.Errorf(codes.Internal, "Something went wrong, please try again later") }If
Findis not available or doesn't fit the table API, the current approach is acceptable.pkg/server/org-unit-role.go (1)
56-73: Hardcoded limit of 1000 custom roles is reasonable for now.The query fetches up to 1000 custom roles. While this is a reasonable limit for most use cases, consider making it configurable or documenting this limitation if you expect organizations with more custom roles in the future.
Do you want me to help document this limitation or make the limit configurable?
📜 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 37ac535b022ac7c1b520987bf2b15ba37bc149d4 and 2180a27b45c00cfed51206017269178273707919.
⛔ 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(3 hunks)pkg/gateway/server.go(5 hunks)pkg/server/org-unit-role.go(2 hunks)pkg/server/org-unit-user.go(7 hunks)pkg/table/const.go(1 hunks)pkg/table/org-unit-custom-role.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- api/org-unit-role.pb.route.go
- main.go
- pkg/table/const.go
- pkg/table/org-unit-custom-role.go
- pkg/gateway/routes.go
🧰 Additional context used
🪛 Checkov (3.2.334)
api/swagger/apidocs.swagger.json
[medium] 2496-2503: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (4)
pkg/gateway/server.go (2)
236-248: LGTM: Custom role integration is correctly implemented.The custom role lookup is properly positioned after built-in role checks, with appropriate error handling and denial on not found. The integration with
checkCustomRolePermissionsensures permission validation.
286-307: LGTM: Path selection logic correctly mirrors ServeHTTP.The implementation now properly handles encoded paths by selecting
RawPathwhen available, falling back toPathotherwise. This ensures consistent route matching with ServeHTTP and addresses the previous review feedback.pkg/server/org-unit-user.go (1)
244-261: LGTM: "default" role correctly included in built-in check.The validation now properly recognizes "admin", "default", and "auditor" as built-in roles, addressing the previous review feedback. The custom role validation path is correctly implemented with appropriate error handling.
pkg/server/org-unit-role.go (1)
81-166: LGTM: Create logic correctly handles existing and soft-deleted roles.The implementation properly:
- Validates input and rejects reserved names
- Checks for existing active roles
- Handles soft-deleted roles: permits reuse only if no bindings exist
- Permanently deletes orphaned soft-deleted roles before recreation
The active pointer initialization at line 150 (
&[]bool{true}[0]) is idiomatic for creating a pointer to a boolean literal.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/org-unit-role.proto (1)
339-371: Validate role name charset to be path-safe.Name appears in a path segment; disallow slashes/unsafe chars to prevent routing issues.
import ( "context" "fmt" "log" "time" + "regexp" ... ) func (s *OrgUnitRoleServer) validateCreateCustomRoleRequest(req *api.CreateCustomRoleReq) error { // Validate role name if req.Name == "" { return errors.New("role name cannot be empty") } if len(req.Name) < 2 { return errors.New("role name must be at least 2 characters long") } if len(req.Name) > 50 { return errors.New("role name cannot exceed 50 characters") } + // allow letters, digits, hyphen, underscore, dot only + if ok, _ := regexp.MatchString(`^[A-Za-z0-9._-]+$`, req.Name); !ok { + return errors.New("role name may contain only letters, digits, hyphen (-), underscore (_), and dot (.)") + } ... }Also applies to: 374-390
pkg/server/org-unit-user.go (1)
67-70: AddOrgUnitUser blocks custom roles; use validateRole like Update.This prevents assigning newly created custom roles. Replace hard-coded check.
- // validate role, currently only admin, default and auditor roles are allowed - if req.Role != "admin" && req.Role != "default" && req.Role != "auditor" { - return nil, status.Errorf(codes.InvalidArgument, "Invalid role: %s", req.Role) - } + // validate role (built-in or custom) + if err := s.validateRole(ctx, req.Role, req.Ou, authInfo.Realm); err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Invalid role: %s - %v", req.Role, err) + }
♻️ Duplicate comments (3)
api/swagger/apidocs.swagger.json (3)
2481-2506: Align Create body schema with server validation (required + array caps).Mark fields required and bound permissions length. Also, drop redundant "type": "object" inside items.
"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", + "minItems": 1, + "maxItems": 20, "items": { - "type": "object", "$ref": "#/definitions/apiRolePermission" }, "title": "List of permissions granted by this custom role" } }, + "required": ["name","displayName","description","permissions"], "title": "Create custom role request" },Based on server validations.
2507-2528: Align Update body schema with server validation (required + array caps).Require displayName/description/permissions and cap permissions length; drop redundant type in items.
"OrgUnitRoleUpdateCustomRoleBody": { "type": "object", "properties": { "displayName": { "type": "string", "title": "Updated display name for the custom role" }, "description": { "type": "string", "title": "Updated description explaining the purpose of this custom role" }, "permissions": { "type": "array", + "minItems": 1, + "maxItems": 20, "items": { - "type": "object", "$ref": "#/definitions/apiRolePermission" }, "title": "Updated list of permissions granted by this custom role" } }, + "required": ["displayName","description","permissions"], "title": "Update custom role request" },Based on server validations.
3607-3623: Constrain RolePermission: require fields and non-empty verbs.Reflect server-side checks; ensure verbs are non-empty strings.
"apiRolePermission": { "type": "object", "properties": { "resource": { "type": "string", + "minLength": 1, "title": "Resource name this permission applies to" }, "verbs": { "type": "array", + "minItems": 1, "items": { - "type": "string" + "type": "string", + "minLength": 1 }, "title": "List of allowed verbs/actions for this resource" } }, + "required": ["resource","verbs"], "title": "Permission definition for custom roles" },Based on server validations.
🧹 Nitpick comments (13)
api/org-unit-role.proto (4)
65-68: Duplicate delete comment; keep a single clear summary.Remove the redundant line.
- // Delete a custom role from the organization unit - // Delete (soft delete) a custom role from the organization unit + // Delete (soft delete) a custom role from the organization unit
85-103: Prefer an enum for role type instead of free-form string.Avoid typos and ease client validation by defining RoleType enum (e.g., BUILT_IN, CUSTOM) and use it in OrgUnitRolesListEntry.
148-151: Avoid pointer-from-slice trick for bool.Use a named variable for clarity.
- Active: &[]bool{true}[0], // Mark role as active + Active: func() *bool { b := true; return &b }(), // Mark role as active
56-63: Hard cap of 1000 roles; consider pagination.List API currently truncates at 1000. Add pagination parameters to proto and implementation when feasible.
Also applies to: 75-78
api/swagger/apidocs.swagger.json (1)
301-303: Duplicate DELETE summary text; keep one line.Remove the repeated sentence.
- "summary": "Delete a custom role from the organization unit\nDelete (soft delete) a custom role from the organization unit", + "summary": "Delete (soft delete) a custom role from the organization unit",pkg/server/org-unit-role.go (4)
148-151: Set Active bool pointer clearly.Avoid slice literal hack for readability.
- Active: &[]bool{true}[0], // Mark role as active + Active: func() *bool { b := true; return &b }(), // Mark role as active
56-63: List truncation: add pagination when feasible.Hard-coded limit of 1000; expose offset/limit in proto and propagate to table.
299-314: Use contextual cancellation for handler registration.Prefer ctx from GrpcServerContext (if available) instead of context.Background() to inherit server shutdown signals.
81-96: Server-side authorization posture.Gateway enforces (api.role) permissions; direct gRPC access bypassing gateway would lack RBAC. Ensure gRPC is not publicly reachable or add server interceptors to enforce (api.role).
Can you confirm the gRPC port is internal-only (e.g., cluster-private, firewall restricted)? If not, I can propose a simple unary interceptor enforcing RBAC based on (api.role).
Also applies to: 168-178, 214-229, 246-268
pkg/server/org-unit-user.go (1)
141-158: Simplify pre-delete fetch if a keyed Find exists.Prefer tbl.Find(ctx, key) for a single document lookup instead of building a bson filter + FindMany.
pkg/gateway/server.go (2)
236-248: Avoid double route matching for custom-role checks.You already call matchRoute in ServeHTTP; pass RouteInfo into performOrgUnitRoleCheck/checkCustomRolePermissions to skip a second match.
Also applies to: 250-260, 262-279
224-234: Consider allowing CORS preflight (OPTIONS).Currently only admin/auditor/custom checks run; preflight OPTIONS may be denied. Recommend bypassing RBAC for OPTIONS or treat as read-only.
pkg/table/org-unit-custom-role.go (1)
128-137: Consider a helper for bool pointers (optional).The
&[]bool{false}[0]idiom is valid but less readable. For improved clarity, consider a helper function like:func boolPtr(b bool) *bool { return &b }Then use:
Active: boolPtr(false).
📜 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 2180a27b45c00cfed51206017269178273707919 and 4bedf69ee178983e732e8e65acdbac08b3a110ac.
⛔ 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(3 hunks)pkg/gateway/server.go(5 hunks)pkg/server/org-unit-role.go(2 hunks)pkg/server/org-unit-user.go(7 hunks)pkg/table/const.go(1 hunks)pkg/table/org-unit-custom-role.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/table/const.go
- main.go
- pkg/gateway/routes.go
- api/org-unit-role.pb.route.go
🧰 Additional context used
🪛 Checkov (3.2.334)
api/swagger/apidocs.swagger.json
[medium] 2496-2503: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (10)
pkg/server/org-unit-user.go (1)
172-195: Nice: binding-aware cleanup of soft-deleted custom roles.Good resilience; logs on failure without blocking deletion.
pkg/gateway/server.go (2)
287-308: Good fix: extractRouteInfo now mirrors ServeHTTP RawPath handling.Prevents encoded-path mismatches.
545-572: Wiring LGTM.Custom role table added to gateway; error handling on init is consistent.
pkg/table/org-unit-custom-role.go (7)
18-56: LGTM! Well-structured data model with proper audit trails.The data structures are well-designed with:
- Composite key structure for proper scoping
- Flexible permission model (resource + verbs)
- Complete audit trail (Created, CreatedBy, Updated, UpdatedBy)
- Three-state active field using
*boolfor soft-delete pattern (true/false/nil)
64-89: LGTM! Correct soft-delete filtering logic.The filter
"active": bson.M{"$ne": false}correctly implements the three-state soft-delete pattern, including roles whereactiveistrueornil(not set), while excluding soft-deleted roles.
139-160: LGTM! Binding check logic is correct.The method correctly checks for role assignments by querying the OrgUnitUserTable. Note that this creates coupling between tables through direct
colfield access, which is acceptable within the same package.
167-182: LGTM! Intelligent deletion strategy prevents data loss.The method correctly implements a two-phase deletion strategy:
- Soft delete when bindings exist (preserves referential integrity)
- Permanent delete when no bindings (reclaims space)
This prevents orphaning user assignments while allowing efficient cleanup.
184-205: LGTM! Correct implementation for name conflict checks.The method correctly finds roles regardless of active status, which is essential for preventing name conflicts with soft-deleted roles that still have bindings.
241-248: LGTM! Standard singleton pattern.The getter correctly returns an error if the table hasn't been initialized, preventing nil pointer dereferences.
216-216: Confirm FindMany(limit=0) semantics Ensure thatt.FindMany(ctx, filter, 0, 0)treats a0limit as “no limit” rather than returning zero results; otherwise this cleanup method never processes any roles.
| // Validate role name | ||
| if req.Name == "" { | ||
| return errors.New("role name cannot be empty") | ||
| } | ||
| if len(req.Name) < 2 { | ||
| return errors.New("role name must be at least 2 characters long") | ||
| } | ||
| if len(req.Name) > 50 { | ||
| return errors.New("role name cannot exceed 50 characters") | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Enforce path-safe role names.
Add a charset check (letters/digits/hyphen/underscore/dot) to prevent routing issues with {name} path param.
if len(req.Name) > 50 {
return errors.New("role name cannot exceed 50 characters")
}
+ if ok, _ := regexp.MatchString(`^[A-Za-z0-9._-]+$`, req.Name); !ok {
+ return errors.New("role name may contain only letters, digits, hyphen (-), underscore (_), and dot (.)")
+ }📝 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.
| // Validate role name | |
| if req.Name == "" { | |
| return errors.New("role name cannot be empty") | |
| } | |
| if len(req.Name) < 2 { | |
| return errors.New("role name must be at least 2 characters long") | |
| } | |
| if len(req.Name) > 50 { | |
| return errors.New("role name cannot exceed 50 characters") | |
| } | |
| // Validate role name | |
| if req.Name == "" { | |
| return errors.New("role name cannot be empty") | |
| } | |
| if len(req.Name) < 2 { | |
| return errors.New("role name must be at least 2 characters long") | |
| } | |
| if len(req.Name) > 50 { | |
| return errors.New("role name cannot exceed 50 characters") | |
| } | |
| if ok, _ := regexp.MatchString(`^[A-Za-z0-9._-]+$`, req.Name); !ok { | |
| return errors.New("role name may contain only letters, digits, hyphen (-), underscore (_), and dot (.)") | |
| } |
🤖 Prompt for AI Agents
In pkg/server/org-unit-role.go around lines 339 to 349, the role name validation
currently checks length only; add a path-safe charset check to ensure req.Name
contains only letters (A-Z, a-z), digits (0-9), hyphen (-), underscore (_), or
dot (.) — use a regex like `^[A-Za-z0-9._-]+$` to validate this and return an
explicit error (e.g., "role name contains invalid characters; only letters,
digits, hyphen, underscore and dot are allowed") when it fails; keep the
existing empty/length checks and perform this charset check after them.
| 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error logging and tracking for cleanup failures.
The cleanup loop silently swallows errors using continue, providing no visibility into failures. This could hide issues with binding checks or deletion operations.
Consider at minimum adding logging:
// Check each soft-deleted role for bindings
+ var failedRoles []string
for _, role := range softDeletedRoles {
hasBindings, err := t.HasBindings(ctx, tenant, orgUnitId, role.Key.Name)
if err != nil {
+ failedRoles = append(failedRoles, role.Key.Name)
+ // Log error: "Failed to check bindings for role %s: %v", role.Key.Name, err
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 {
+ failedRoles = append(failedRoles, role.Key.Name)
+ // Log error: "Failed to delete role %s: %v", role.Key.Name, err
// Log error but continue with other roles
continue
}
}
}
+
+ if len(failedRoles) > 0 {
+ // Log warning: "Cleanup completed with %d failures for roles: %v", len(failedRoles), failedRoles
+ }🤖 Prompt for AI Agents
In pkg/table/org-unit-custom-role.go around lines 222 to 235 the cleanup loop
currently swallows errors with continue; add error logging and basic failure
tracking so failures are visible and can be surfaced: when HasBindings returns
an error, log it with context (tenant, orgUnitId, role.Key.Name) and increment a
failure counter; likewise when PermanentDelete fails, log the deletion error
with the same context and increment the counter; after the loop, if the failure
counter is > 0, return or propagate an aggregated error indicating N cleanup
failures (or return the last error) so callers can react, otherwise continue to
succeed.
| // LocateOrgUnitCustomRoleTable initializes and returns the custom role table | ||
| func LocateOrgUnitCustomRoleTable(client db.StoreClient) (*OrgUnitCustomRoleTable, error) { | ||
| if orgUnitCustomRoleTable != nil { | ||
| return orgUnitCustomRoleTable, nil | ||
| } | ||
|
|
||
| col := client.GetCollection(AuthDatabaseName, OrgUnitCustomRoleCollectionName) | ||
| tbl := &OrgUnitCustomRoleTable{ | ||
| col: col, | ||
| } | ||
|
|
||
| err := tbl.Initialize(col) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| orgUnitCustomRoleTable = tbl | ||
|
|
||
| return orgUnitCustomRoleTable, nil | ||
| } |
There was a problem hiding this comment.
Add unique compound index on (key.tenant, key.orgUnitId, key.name)
As noted in the previous review, go-core-stack's Table.Initialize doesn't create DB indexes, and this repo contains no index-creation for this collection. Without a unique compound index on (tenant + orgUnitId + name), concurrent creates can insert duplicate roles, violating data integrity.
Based on learnings
Add the index in LocateOrgUnitCustomRoleTable during initialization or manage it via DB migrations/connection setup. For MongoDB, use the driver's CreateIndexes API with a unique compound index specification:
// After line 264, before setting orgUnitCustomRoleTable:
import "go.mongodb.org/mongo-driver/mongo"
import "go.mongodb.org/mongo-driver/mongo/options"
// Create unique compound index on (tenant, orgUnitId, name)
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),
}
// Ensure index exists (idempotent)
if mongoCol, ok := col.(*mongo.Collection); ok {
_, err = mongoCol.Indexes().CreateIndexes(ctx, []mongo.IndexModel{indexModel})
if err != nil && !isIndexExistsError(err) {
return nil, errors.Wrap(err, "failed to create unique index")
}
}🤖 Prompt for AI Agents
In pkg/table/org-unit-custom-role.go around lines 250 to 269, the
LocateOrgUnitCustomRoleTable initializer currently does not create a unique
compound index, allowing duplicate roles on concurrent inserts; add code during
initialization to create an idempotent unique compound index on key.tenant,
key.orgUnitId and key.name using the MongoDB driver's CreateIndexes API (build a
mongo.IndexModel with Keys for "key.tenant", "key.orgUnitId", "key.name" and
Options().SetUnique(true)), ensure you have a context, call
col.(*mongo.Collection).Indexes().CreateIndexes(ctx, []mongo.IndexModel{...})
only when the collection is a mongo.Collection, handle and return any non-
benign errors, and add the required mongo driver imports; keep the operation
idempotent and tolerate index-already-exists errors.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
pkg/server/org-unit-role.go (1)
6-22: Enforce path-safe role names (letters/digits/._-).Add a charset check for role names to prevent routing/storage issues. This was requested earlier.
Apply:
import ( "context" - "fmt" + "fmt" "log" - "time" + "time" + "regexp" @@ func (s *OrgUnitRoleServer) validateCreateCustomRoleRequest(req *api.CreateCustomRoleReq) error { // Validate role name if req.Name == "" { return errors.New("role name cannot be empty") } if len(req.Name) < 2 { return errors.New("role name must be at least 2 characters long") } if len(req.Name) > 50 { return errors.New("role name cannot exceed 50 characters") } + if ok, _ := regexp.MatchString(`^[A-Za-z0-9._-]+$`, req.Name); !ok { + return errors.New("role name may contain only letters, digits, hyphen (-), underscore (_), and dot (.)") + }Also applies to: 339-351
api/swagger/apidocs.swagger.json (3)
2507-2532: Align CreateCustomRole request with server validation: required fields + cap permissions.
Add required fields, non-empty strings, and min/max items for permissions. Also avoid mixing "type" with "$ref" in items. This prevents client/server divergence and satisfies CKV_OPENAPI_21."OrgUnitRoleCreateCustomRoleBody": { "type": "object", "properties": { "name": { "type": "string", + "minLength": 1, "title": "Name of the custom role (must be unique within the org unit)" }, "displayName": { "type": "string", + "minLength": 1, "title": "Display name for the custom role" }, "description": { "type": "string", + "minLength": 1, "title": "Description explaining the purpose of this custom role" }, "permissions": { "type": "array", + "minItems": 1, + "maxItems": 20, "items": { - "type": "object", "$ref": "#/definitions/apiRolePermission" }, "title": "List of permissions granted by this custom role" } }, + "required": ["name","displayName","description","permissions"], "title": "Create custom role request" },
2533-2554: Align UpdateCustomRole request with server validation: required fields + cap permissions.
Mark fields as required and enforce permissions limit; also remove redundant "type" with $ref."OrgUnitRoleUpdateCustomRoleBody": { "type": "object", "properties": { "displayName": { "type": "string", + "minLength": 1, "title": "Updated display name for the custom role" }, "description": { "type": "string", + "minLength": 1, "title": "Updated description explaining the purpose of this custom role" }, "permissions": { "type": "array", + "minItems": 1, + "maxItems": 20, "items": { - "type": "object", "$ref": "#/definitions/apiRolePermission" }, "title": "Updated list of permissions granted by this custom role" } }, + "required": ["displayName","description","permissions"], "title": "Update custom role request" },
3607-3627: Harden RolePermission schema: require fields, non-empty verbs, and enum action.
Match server validation by requiring resource and verbs, enforce non-empty, and constrain action to allowed values."apiRolePermission": { "type": "object", "properties": { "resource": { "type": "string", + "minLength": 1, "title": "Resource name this permission applies to (supports \"*\" for all resources)" }, "verbs": { "type": "array", + "minItems": 1, "items": { - "type": "string" + "type": "string", + "minLength": 1 }, "title": "List of allowed verbs/actions for this resource (supports \"*\" for all verbs)" }, "action": { - "type": "string", - "title": "Action type: \"Allow\" or \"Deny\" (Deny takes precedence over Allow)" + "type": "string", + "enum": ["Allow","Deny"], + "default": "Allow", + "title": "Action type (Deny takes precedence over Allow)" } }, + "required": ["resource","verbs"], "title": "Permission definition for custom roles" },
🧹 Nitpick comments (10)
pkg/server/org-unit-user.go (2)
172-195: Harden cleanup: log unexpected lookup errors for custom roles.Errors other than not-found during FindAnyByNameAndOrgUnit are silently ignored. Log them for observability.
Apply:
- existingRole, err := s.ouCustomRoleTbl.FindAnyByNameAndOrgUnit(ctx, authInfo.Realm, req.Ou, roleName) - if err == nil && existingRole.Active != nil && !*existingRole.Active { + existingRole, err := s.ouCustomRoleTbl.FindAnyByNameAndOrgUnit(ctx, authInfo.Realm, req.Ou, roleName) + if err == nil && existingRole.Active != nil && !*existingRole.Active { ... - } + } else if err != nil && !errors.IsNotFound(err) { + log.Printf("failed to lookup custom role %s for cleanup: %s", roleName, err) + }
244-261: LGTM: validateRole distinguishes built-in vs custom.Optional: centralize built-in roles in a shared set to avoid duplication across gateway and servers.
pkg/gateway/server.go (1)
262-328: LGTM: Deny precedence with wildcard support implemented.Optional: pass already-computed route info from ServeHTTP to avoid a second route match per request.
pkg/server/org-unit-role.go (4)
169-178: Validate role name in Update as well.Add the same charset check for req.Name to guard path param misuse.
Apply:
func (s *OrgUnitRoleServer) UpdateCustomRole(ctx context.Context, req *api.UpdateCustomRoleReq) (*api.UpdateCustomRoleResp, error) { authInfo, _ := auth.GetAuthInfoFromContext(ctx) if authInfo == nil { return nil, status.Errorf(codes.Unauthenticated, "User not authenticated") } // Validate request fields if err := s.validateUpdateCustomRoleRequest(req); err != nil { return nil, status.Errorf(codes.InvalidArgument, "Invalid request: %s", err) } + if ok, _ := regexp.MatchString(`^[A-Za-z0-9._-]+$`, req.Name); !ok { + return nil, status.Errorf(codes.InvalidArgument, "role name may contain only letters, digits, hyphen (-), underscore (_), and dot (.)") + }
215-219: Validate role name in Get.Apply:
func (s *OrgUnitRoleServer) GetCustomRole(ctx context.Context, req *api.GetCustomRoleReq) (*api.GetCustomRoleResp, error) { authInfo, _ := auth.GetAuthInfoFromContext(ctx) if authInfo == nil { return nil, status.Errorf(codes.Unauthenticated, "User not authenticated") } + if ok, _ := regexp.MatchString(`^[A-Za-z0-9._-]+$`, req.Name); !ok { + return nil, status.Errorf(codes.InvalidArgument, "role name may contain only letters, digits, hyphen (-), underscore (_), and dot (.)") + }
247-252: Validate role name in Delete.Apply:
func (s *OrgUnitRoleServer) DeleteCustomRole(ctx context.Context, req *api.DeleteCustomRoleReq) (*api.DeleteCustomRoleResp, error) { authInfo, _ := auth.GetAuthInfoFromContext(ctx) if authInfo == nil { return nil, status.Errorf(codes.Unauthenticated, "User not authenticated") } + if ok, _ := regexp.MatchString(`^[A-Za-z0-9._-]+$`, req.Name); !ok { + return nil, status.Errorf(codes.InvalidArgument, "role name may contain only letters, digits, hyphen (-), underscore (_), and dot (.)") + }
339-396: LGTM: request validation (lengths, permissions).Consider adding a per-permission verbs limit and duplicate elimination (optional).
api/org-unit-role.proto (1)
115-125: Prefer enum for action over string.Using an enum prevents typos and eases validation (“Allow”/“Deny”).
Example:
enum Action { ACTION_UNSPECIFIED = 0; ALLOW = 1; DENY = 2; } message RolePermission { string resource = 1; repeated string verbs = 2; Action action = 3; }api/swagger/apidocs.swagger.json (2)
302-303: Deduplicate and clarify DELETE summary.
Summary repeats itself and mentions “soft delete”; keep concise in summary, move details to description.- "summary": "Delete a custom role from the organization unit\nDelete (soft delete) a custom role from the organization unit", + "summary": "Delete (soft delete) a custom role from the organization unit", + "description": "Soft-deletes a custom role within the specified organization unit.",
2833-2876: Fix permissions.items to avoid mixing $ref with type.
OpenAPI 2.0 recommends using $ref without an extra “type”. Remove the redundant "type"."permissions": { "type": "array", "items": { - "type": "object", "$ref": "#/definitions/apiRolePermission" }, "title": "List of permissions granted by this custom role" },
📜 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 4bedf69ee178983e732e8e65acdbac08b3a110ac and 043d902e2e320c709e696c9eef815a52b0daa3ef.
⛔ 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(3 hunks)pkg/gateway/server.go(5 hunks)pkg/server/org-unit-role.go(2 hunks)pkg/server/org-unit-user.go(7 hunks)pkg/table/const.go(1 hunks)pkg/table/org-unit-custom-role.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- main.go
- pkg/table/org-unit-custom-role.go
- pkg/gateway/routes.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 (22)
pkg/table/const.go (1)
30-32: LGTM: collection name added consistently.pkg/server/org-unit-user.go (2)
26-28: LGTM: custom role table wired into server.
206-215: LGTM: custom role table retrieval added to constructor.pkg/gateway/server.go (6)
47-56: LGTM: gateway wired with custom role table and proxies.
250-260: LGTM: route-derived permission check for custom roles.
330-357: LGTM: extractRouteInfo mirrors ServeHTTP path selection.
359-377: LGTM: unified RawPath/Path handling in ServeHTTP.
594-598: LGTM: custom role table retrieval.
613-621: LGTM: gateway fields initialized with custom role table.pkg/server/org-unit-role.go (4)
26-27: LGTM: server holds custom role table.
32-55: LGTM: list returns built-in plus custom roles; non-fatal on fetch error.Also applies to: 56-78
301-311: LGTM: server wiring and handler/route registration.
398-444: LGTM: update validation mirrors create (sans name).api/org-unit-role.proto (3)
27-38: LGTM: CRUD RPCs with HTTP mappings and api.role metadata.Also applies to: 40-51, 53-63
92-103: LGTM: list entry enriched for custom roles.
27-38: Route resource and verb definitions align with gateway registrations. The resource string "org-unit-custom-role" and verbs ("create", "update", "get", "delete") in api/org-unit-role.proto match the entries in api.RoutesOrgUnitRole.api/swagger/apidocs.swagger.json (6)
223-262: New create endpoint looks correct and consistent.
POST path, params, tags, and response wiring align with the rest of the spec.
386-386: Updated list summary reads well.
Clear that built‑in and custom roles are included.
2706-2715: Create response schema is fine.
Simple confirmation message matches spec style elsewhere.
2823-2832: Delete response schema is fine.
Consistent with simple “message” response pattern.
3418-3437: List entry extensions look good.
The added fields (type/displayName/created/createdBy) improve list context without breaking existing clients.
4065-4074: Update response schema is fine.
Matches the simple “message” pattern used elsewhere.
| // Delete a custom role from the organization unit | ||
| // Delete (soft delete) a custom role from the organization unit | ||
| rpc DeleteCustomRole(DeleteCustomRoleReq) returns (DeleteCustomRoleResp) { | ||
| option (google.api.http) = { | ||
| delete: "/api/auth/v1/ou/{ou}/role/{name}" | ||
| }; | ||
| option (api.role) = { | ||
| resource: "org-unit-custom-role" | ||
| scope: "ou" | ||
| verb: "delete" | ||
| }; | ||
| } |
There was a problem hiding this comment.
Remove duplicate comment line.
There are two comment lines for Delete; keep the soft-delete note once.
Apply:
- // Delete a custom role from the organization unit
// Delete (soft delete) a custom role from the organization unit📝 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.
| // Delete a custom role from the organization unit | |
| // Delete (soft delete) a custom role from the organization unit | |
| rpc DeleteCustomRole(DeleteCustomRoleReq) returns (DeleteCustomRoleResp) { | |
| option (google.api.http) = { | |
| delete: "/api/auth/v1/ou/{ou}/role/{name}" | |
| }; | |
| option (api.role) = { | |
| resource: "org-unit-custom-role" | |
| scope: "ou" | |
| verb: "delete" | |
| }; | |
| } | |
| // Delete (soft delete) a custom role from the organization unit | |
| rpc DeleteCustomRole(DeleteCustomRoleReq) returns (DeleteCustomRoleResp) { | |
| option (google.api.http) = { | |
| delete: "/api/auth/v1/ou/{ou}/role/{name}" | |
| }; | |
| option (api.role) = { | |
| resource: "org-unit-custom-role" | |
| scope: "ou" | |
| verb: "delete" | |
| }; | |
| } |
🤖 Prompt for AI Agents
In api/org-unit-role.proto around lines 65 to 76, the RPC DeleteCustomRole has
duplicate leading comments; remove the redundant "Delete a custom role from the
organization unit" line and keep the single soft-delete description "Delete
(soft delete) a custom role from the organization unit" so only the clear
soft-delete note remains above the rpc definition.
| // Check if it's a custom role | ||
| customRole, err := s.ouCustomRoleTbl.FindByNameAndOrgUnit(r.Context(), authInfo.Realm, ou, ouUser.Role) | ||
| if err != nil { | ||
| if !errors.IsNotFound(err) { | ||
| log.Printf("failed to find custom role %s for org unit %s: %s", ouUser.Role, ou, err) | ||
| } | ||
| // If custom role not found, deny access | ||
| return false | ||
| } | ||
|
|
||
| // Check if the custom role allows the requested action | ||
| return s.checkCustomRolePermissions(customRole, r) | ||
| } |
There was a problem hiding this comment.
Handle built-in "default" role explicitly; avoid accidental deny.
With no "default" case, flow falls into custom-role lookup for role "default" and denies access. Add an explicit case with intended semantics (commonly “no special OU privileges”).
Apply:
switch ouUser.Role {
case "admin":
return true
case "auditor":
if r.Method == http.MethodGet || r.Method == http.MethodHead {
return true
}
return false
+case "default":
+ // No special OU privileges; handled as least-privileged
+ return false
}Optional: also allow OPTIONS for read-only roles to avoid CORS/preflight failures:
- if r.Method == http.MethodGet || r.Method == http.MethodHead {
+ if r.Method == http.MethodGet || r.Method == http.MethodHead || r.Method == http.MethodOptions {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/gateway/server.go around lines 236 to 248, the code currently falls
through to a custom-role lookup for the built-in "default" role and ends up
denying access when no custom role exists; add an explicit branch that checks if
ouUser.Role == "default" and return the intended semantics (commonly: treat as
"no special OU privileges" by returning true so it does not deny access here),
placing this check before the FindByNameAndOrgUnit call; optionally also allow
HTTP OPTIONS through for read-only roles to avoid CORS/preflight failures by
checking r.Method == "OPTIONS" and returning true for those roles.
| // Get the user data before deletion to check their role | ||
| filter := bson.M{ | ||
| "key.tenant": authInfo.Realm, | ||
| "key.username": req.User, | ||
| "key.orgUnitId": req.Ou, | ||
| } | ||
| users, err := s.tbl.FindMany(ctx, filter, 0, 1) | ||
| if err != nil { | ||
| log.Printf("failed to get org unit user before deletion, got error: %s", err) | ||
| return nil, status.Errorf(codes.Internal, "Something went wrong, please try again later") | ||
| } | ||
|
|
||
| if len(users) == 0 { | ||
| return nil, status.Errorf(codes.NotFound, "Org Unit User %s, not found", req.User) | ||
| } | ||
|
|
||
| userData := users[0] | ||
|
|
There was a problem hiding this comment.
Map NotFound correctly when pre-fetching user for deletion.
If FindMany returns a not-found error for zero results, current code returns Internal. Handle NotFound explicitly.
Apply:
users, err := s.tbl.FindMany(ctx, filter, 0, 1)
- if err != nil {
- log.Printf("failed to get org unit user before deletion, got error: %s", err)
- return nil, status.Errorf(codes.Internal, "Something went wrong, please try again later")
- }
+ if err != nil {
+ if errors.IsNotFound(err) {
+ return nil, status.Errorf(codes.NotFound, "Org Unit User %s, not found", req.User)
+ }
+ log.Printf("failed to get org unit user before deletion, got error: %s", err)
+ return nil, status.Errorf(codes.Internal, "Something went wrong, please try again later")
+ }
if len(users) == 0 {
return nil, status.Errorf(codes.NotFound, "Org Unit User %s, not found", req.User)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/server/org-unit-user.go around lines 141 to 158, the call to
s.tbl.FindMany treats any error as Internal; modify the error handling to detect
the repository's "not found" sentinel (e.g. errors.Is(err, store.ErrNotFound) or
mongo.ErrNoDocuments depending on the table implementation) and return
status.Errorf(codes.NotFound, ...) when that is the case, otherwise keep
returning the Internal error; keep the existing zero-length users check for
safety.
3d3c936 to
1be6b36
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/server/org-unit-user.go (1)
61-108:AddOrgUnitUserstill blocks custom roles.
We now havevalidateRole, yetAddOrgUnitUserkeeps the hard-coded “admin/default/auditor” check. That means newly created custom roles still can’t be assigned on creation — the core feature stalls at step zero. Please reusevalidateRolehere just likeUpdatedoes:@@ - // validate role, currently only admin, default and auditor roles are allowed - if req.Role != "admin" && req.Role != "default" && req.Role != "auditor" { - return nil, status.Errorf(codes.InvalidArgument, "Invalid role: %s", req.Role) - } + if err := s.validateRole(ctx, req.Role, req.Ou, authInfo.Realm); err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Invalid role: %s - %v", req.Role, err) + }
♻️ Duplicate comments (4)
pkg/server/org-unit-role.go (1)
357-368: Validate role names are path-safe.
Role names feed directly into{name}URL segments. Without constraining them, names containing/,%2F, spaces, etc. break the CRUD endpoints. Please add the simple charset guard (letters / digits /._-) as previously discussed:if len(req.Name) < 2 { return errors.New("role name must be at least 2 characters long") } if len(req.Name) > 50 { return errors.New("role name cannot exceed 50 characters") } + if ok, _ := regexp.MatchString(`^[A-Za-z0-9._-]+$`, req.Name); !ok { + return errors.New("role name may contain only letters, digits, hyphen (-), underscore (_), and dot (.)") + }(Remember to import
regexp.)api/org-unit-role.proto (1)
65-67: Remove the duplicated Delete comment line.
The two consecutive comments aboveDeleteCustomRoleare redundant; keep the soft-delete explanation once to avoid OpenAPI/doc bloat.pkg/table/org-unit-custom-role.go (1)
268-280: Still missing the unique compound index guard.
We again have no protection against concurrent duplicates:LocateOrgUnitCustomRoleTablenever creates a unique index on(key.tenant, key.orgUnitId, key.name), so two fast creates can persist conflicting roles. Please ensure this index is created during initialization (or via your migrations) before exposing the table; otherwise data integrity is at risk. A minimal fix is shown below—adjust to match your index helpers:@@ -import ( - "context" - "time" - - "github.com/go-core-stack/core/db" - "github.com/go-core-stack/core/errors" - "github.com/go-core-stack/core/table" - "go.mongodb.org/mongo-driver/bson" -) +import ( + "context" + "strings" + "time" + + "github.com/go-core-stack/core/db" + "github.com/go-core-stack/core/errors" + "github.com/go-core-stack/core/table" + "go.mongodb.org/mongo-driver/bson" + "go.mongodb.org/mongo-driver/mongo" + "go.mongodb.org/mongo-driver/mongo/options" +) @@ func LocateOrgUnitCustomRoleTable(client db.StoreClient) (*OrgUnitCustomRoleTable, error) { @@ - col := client.GetCollection(AuthDatabaseName, OrgUnitCustomRoleCollectionName) + col := client.GetCollection(AuthDatabaseName, OrgUnitCustomRoleCollectionName) @@ - err := tbl.Initialize(col) + err := tbl.Initialize(col) if err != nil { return nil, err } + + if mongoCol, ok := col.(*mongo.Collection); ok { + 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), + } + if _, idxErr := mongoCol.Indexes().CreateOne(context.Background(), indexModel); idxErr != nil && !strings.Contains(idxErr.Error(), "already exists") { + return nil, errors.Wrap(idxErr, "ensure org-unit custom role unique index") + } + }pkg/server/org-unit-user.go (1)
147-155: Surface not-found instead of masking it as internal.
IfFindManyreturnsNotFound(common when the user never existed), we now log it and bubble upcodes.Internal, which is the same gap noted earlier. Please restore the original behaviour by mapping that sentinel tocodes.NotFoundbefore returning:users, err := s.tbl.FindMany(ctx, filter, 0, 1) if err != nil { + if errors.IsNotFound(err) { + return nil, status.Errorf(codes.NotFound, "Org Unit User %s, not found", req.User) + } log.Printf("failed to get org unit user before deletion, got error: %s", err) return nil, status.Errorf(codes.Internal, "Something went wrong, please try again later") }
🧹 Nitpick comments (3)
pkg/server/org-unit-user.go (1)
244-261: ExposevalidateRolefor both Add & Update paths.
Now that we introducedvalidateRole, make sure every entry point (Add as well as Update/Delete flows) calls it. See comment above for wiring it intoAddOrgUnitUserso custom roles can actually be assigned during creation.api/swagger/apidocs.swagger.json (2)
2508-2554: Swagger schema is looser than server validation.
The OpenAPI still treats all custom-role fields as optional with unboundedpermissions, but the server rejects empty name/displayName/description/permissions and caps permissions at 20. Bring the contract in sync:"OrgUnitRoleCreateCustomRoleBody": { "type": "object", "properties": { "name": { - "type": "string", + "type": "string", + "minLength": 1, "title": "Name of the custom role (must be unique within the org unit)" }, "displayName": { - "type": "string", + "type": "string", + "minLength": 1, "title": "Display name for the custom role" }, "description": { - "type": "string", + "type": "string", + "minLength": 1, "title": "Description explaining the purpose of this custom role" }, "permissions": { "type": "array", + "minItems": 1, + "maxItems": 20, "items": { - "type": "object", "$ref": "#/definitions/apiRolePermission" }, "title": "List of permissions granted by this custom role" } }, + "required": ["name", "displayName", "description", "permissions"], "title": "Create custom role request" }, @@ "OrgUnitRoleUpdateCustomRoleBody": { "type": "object", "properties": { "displayName": { - "type": "string", + "type": "string", + "minLength": 1, "title": "Updated display name for the custom role" }, "description": { - "type": "string", + "type": "string", + "minLength": 1, "title": "Updated description explaining the purpose of this custom role" }, "permissions": { "type": "array", + "minItems": 1, + "maxItems": 20, "items": { - "type": "object", "$ref": "#/definitions/apiRolePermission" }, "title": "Updated list of permissions granted by this custom role" } }, + "required": ["displayName", "description", "permissions"], "title": "Update custom role request" },
3608-3645: Mark required fields onapiRolePermission.
Server-side validation insists on non-emptyresourceand at least one verb, but the schema leaves them optional. Please tighten the definition:"apiRolePermission": { "type": "object", "properties": { "resource": { - "type": "string", + "type": "string", + "minLength": 1, "title": "Resource name this permission applies to" }, "verbs": { "type": "array", + "minItems": 1, "items": { - "type": "string" + "type": "string", + "minLength": 1 }, "title": "List of allowed verbs/actions for this resource (supports \"*\" for all verbs)" } }, + "required": ["resource", "verbs"], "title": "Permission definition for custom roles" },
📜 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 043d902e2e320c709e696c9eef815a52b0daa3ef and 1be6b361a88f54cf0641ab9034d2f577dc9de832.
⛔ 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(3 hunks)pkg/gateway/server.go(8 hunks)pkg/server/org-unit-role.go(2 hunks)pkg/server/org-unit-user.go(7 hunks)pkg/table/const.go(1 hunks)pkg/table/org-unit-custom-role.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/org-unit-role.pb.route.go
- pkg/table/const.go
- main.go
🧰 Additional context used
🪛 ast-grep (0.39.6)
pkg/gateway/server.go
[warning] 351-351: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 Checkov (3.2.334)
api/swagger/apidocs.swagger.json
[medium] 2522-2529: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🪛 GitHub Actions: Validate build for pull request
pkg/gateway/server.go
[error] 141-141: golangci-lint: Error return value of io.Copy is not checked. (errcheck)
🪛 GitHub Check: build-pull-request
pkg/gateway/server.go
[failure] 361-361:
Error return value of resp.Body.Close is not checked (errcheck)
[failure] 170-170:
Error return value of w.ResponseWriter.Write is not checked (errcheck)
[failure] 163-163:
Error return value of io.Copy is not checked (errcheck)
[failure] 151-151:
Error return value of io.Copy is not checked (errcheck)
[failure] 141-141:
Error return value of io.Copy is not checked (errcheck)
[failure] 681-681:
QF1003: could use tagged switch on action (staticcheck)
[failure] 1065-1065:
ST1023: should omit type http.ResponseWriter from declaration; it will be inferred from the right-hand side (staticcheck)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
pkg/server/org-unit-user.go (2)
67-70: Let AddOrgUnitUser accept custom roles.
AddOrgUnitUserstill rejects every non built-in role, so callers cannot assign the custom roles this feature introduces. ReusevalidateRolehere just like the update path.- // validate role, currently only admin, default and auditor roles are allowed - if req.Role != "admin" && req.Role != "default" && req.Role != "auditor" { - return nil, status.Errorf(codes.InvalidArgument, "Invalid role: %s", req.Role) - } + // validate role - allow built-in or existing custom role + if err := s.validateRole(ctx, req.Role, req.Ou, authInfo.Realm); err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Invalid role: %s - %v", req.Role, err) + }
148-156: Return NotFound when the org-unit user is missing.
FindManycan surface the repo’s not-found sentinel. Right now we convert that into an Internal error, hiding the 404 from clients. Handleerrors.IsNotFoundbefore logging so the delete RPC returns the correct NotFound status.users, err := s.tbl.FindMany(ctx, filter, 0, 1) if err != nil { + if errors.IsNotFound(err) { + return nil, status.Errorf(codes.NotFound, "Org Unit User %s, not found", req.User) + } log.Printf("failed to get org unit user before deletion, got error: %s", err) return nil, status.Errorf(codes.Internal, "Something went wrong, please try again later") }pkg/table/org-unit-custom-role.go (1)
6-14: Enforce a unique index for (tenant, orgUnitId, name).Concurrent creates can still write duplicate roles because we never ensure a unique compound index on the collection. Please create the
(key.tenant, key.orgUnitId, key.name)index during table location, tolerating “already exists” responses.-import ( - "context" - "time" - - "github.com/go-core-stack/core/db" - "github.com/go-core-stack/core/errors" - "github.com/go-core-stack/core/table" - "go.mongodb.org/mongo-driver/bson" -) +import ( + stdErrors "errors" + "context" + "time" + + "github.com/go-core-stack/core/db" + "github.com/go-core-stack/core/errors" + "github.com/go-core-stack/core/table" + "go.mongodb.org/mongo-driver/bson" + "go.mongodb.org/mongo-driver/mongo" + "go.mongodb.org/mongo-driver/mongo/options" +) @@ func LocateOrgUnitCustomRoleTable(client db.StoreClient) (*OrgUnitCustomRoleTable, error) { if orgUnitCustomRoleTable != nil { return orgUnitCustomRoleTable, nil } col := client.GetCollection(AuthDatabaseName, OrgUnitCustomRoleCollectionName) @@ - err := tbl.Initialize(col) + err := tbl.Initialize(col) if err != nil { return nil, err } + if mongoCol, ok := col.(*mongo.Collection); ok { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + 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), + } + + if _, err := mongoCol.Indexes().CreateOne(ctx, indexModel); err != nil { + var cmdErr mongo.CommandError + if !stdErrors.As(err, &cmdErr) || cmdErr.Code != 85 { + return nil, errors.Wrapf(err, "failed to ensure unique index on %s", OrgUnitCustomRoleCollectionName) + } + } + } + orgUnitCustomRoleTable = tbl return orgUnitCustomRoleTable, nil }Also applies to: 262-280
pkg/gateway/server.go (1)
137-170: Handle response write errors in finalize.
golangci-lint(errcheck) is failing because we drop the results fromio.CopyandWrite. More importantly, we would never know if streaming the response fails. Please capture and log these errors.if !w.shouldFilter || w.statusCode != http.StatusOK || w.body.Len() == 0 { // If Nothing to filter if w.statusCode != http.StatusOK && w.body.Len() > 0 { w.ResponseWriter.WriteHeader(w.statusCode) - io.Copy(w.ResponseWriter, w.body) + if _, err := io.Copy(w.ResponseWriter, w.body); err != nil { + log.Printf("failed to stream original response body: %v", err) + } } return } @@ if err := json.Unmarshal(w.body.Bytes(), &data); err != nil { // If Not JSON or parse error ---> write original response w.ResponseWriter.WriteHeader(w.statusCode) - io.Copy(w.ResponseWriter, w.body) + if _, copyErr := io.Copy(w.ResponseWriter, w.body); copyErr != nil { + log.Printf("failed to stream original response body after unmarshal error: %v", copyErr) + } return } @@ if err != nil { // Marshal error ---> write original response w.ResponseWriter.WriteHeader(w.statusCode) - io.Copy(w.ResponseWriter, w.body) + if _, copyErr := io.Copy(w.ResponseWriter, w.body); copyErr != nil { + log.Printf("failed to stream original response body after marshal error: %v", copyErr) + } return } @@ w.ResponseWriter.Header().Set("Content-Length", fmt.Sprintf("%d", len(filteredBytes))) w.ResponseWriter.WriteHeader(w.statusCode) - w.ResponseWriter.Write(filteredBytes) + if _, writeErr := w.ResponseWriter.Write(filteredBytes); writeErr != nil { + log.Printf("failed to write filtered response body: %v", writeErr) + }
📜 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 1be6b361a88f54cf0641ab9034d2f577dc9de832 and 99d34374a3d9851ac34ae59aafeded923d1b4ca3.
⛔ 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(3 hunks)pkg/gateway/server.go(8 hunks)pkg/server/org-unit-role.go(2 hunks)pkg/server/org-unit-user.go(7 hunks)pkg/table/const.go(1 hunks)pkg/table/org-unit-custom-role.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- main.go
- pkg/gateway/routes.go
- api/org-unit-role.pb.route.go
🧰 Additional context used
🪛 ast-grep (0.39.6)
pkg/gateway/server.go
[warning] 360-360: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 Checkov (3.2.334)
api/swagger/apidocs.swagger.json
[medium] 2522-2529: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🪛 GitHub Actions: Validate build for pull request
pkg/gateway/server.go
[error] 141-141: golangci-lint: Error return value of io.Copy is not checked (errcheck)
🪛 GitHub Check: build-pull-request
pkg/gateway/server.go
[failure] 370-370:
Error return value of resp.Body.Close is not checked (errcheck)
[failure] 170-170:
Error return value of w.ResponseWriter.Write is not checked (errcheck)
[failure] 163-163:
Error return value of io.Copy is not checked (errcheck)
[failure] 151-151:
Error return value of io.Copy is not checked (errcheck)
[failure] 141-141:
Error return value of io.Copy is not checked (errcheck)
[failure] 690-690:
QF1003: could use tagged switch on action (staticcheck)
[failure] 1074-1074:
ST1023: should omit type http.ResponseWriter from declaration; it will be inferred from the right-hand side (staticcheck)
Suryanshu185
force-pushed
the
main
branch
4 times, most recently
from
01ede3d to
5cb1ae2
Compare
Suryanshu185
force-pushed
the
main
branch
2 times, most recently
from
2a4e808 to
44084ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/server/org-unit-user.go (1)
67-70: CallvalidateRoleso Add honours custom rolesAdd still hard-codes the old built-in check, so any org-unit custom role is rejected at creation time. That breaks the CRUD story introduced in this PR. Route Add through
validateRoleexactly like Update so both built-ins and existing custom roles work.Apply this diff:
- // validate role, currently only admin, default and auditor roles are allowed - if req.Role != "admin" && req.Role != "default" && req.Role != "auditor" { - return nil, status.Errorf(codes.InvalidArgument, "Invalid role: %s", req.Role) - } + // validate role - allow built-in roles or existing custom roles + if err := s.validateRole(ctx, req.Role, req.Ou, authInfo.Realm); err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Invalid role: %s - %v", req.Role, err) + }
♻️ Duplicate comments (1)
api/swagger/apidocs.swagger.json (1)
2508-2554: Sync schema validation with server-side rulesThe new CRUD handlers reject blank names/display names/descriptions and enforce 1–20 permissions, each with non-empty verbs/resources. The OpenAPI schemas still mark these fields optional and unconstrained, so generated clients will happily send payloads the server refuses. Please mirror the server validation (see
validateCreateCustomRoleRequest/validateUpdateCustomRoleRequest) by addingminLength,minItems/maxItems, and the appropriaterequiredlists.Suggested adjustments (abridged):
"name": { "type": "string", + "minLength": 1, "title": "Name of the custom role (must be unique within the org unit)" }, "displayName": { "type": "string", + "minLength": 1, "title": "Display name for the custom role" }, "description": { "type": "string", + "minLength": 1, "title": "Description explaining the purpose of this custom role" }, "permissions": { "type": "array", + "minItems": 1, + "maxItems": 20, "items": { "$ref": "#/definitions/apiRolePermission" }, "title": "List of permissions granted by this custom role" } }, + "required": ["name","displayName","description","permissions"],Apply the same treatment to
OrgUnitRoleUpdateCustomRoleBody, and inapiRolePermissionadd:"resource": { "type": "string", "minLength": 1, ... }, "verbs": { "type": "array", "minItems": 1, "items": { "type": "string", "minLength": 1 } }, "required": ["resource","verbs"]This keeps the documented contract in lockstep with the enforced business rules.
🧹 Nitpick comments (1)
pkg/server/org-unit-role.go (1)
357-411: Tighten role-name validation to path-safe charactersRole names travel in the
{name}path segment for the REST gateway. Without constraining the charset, names containing/,%, or whitespace can break routing or produce surprising encodings. Add a simple regexp (letters, digits,_,-,.) after the length checks so we reject unsafe names up front.Example adjustment:
if len(req.Name) > 50 { return errors.New("role name cannot exceed 50 characters") } + if ok, _ := regexp.MatchString(`^[A-Za-z0-9._-]+$`, req.Name); !ok { + return errors.New("role name may contain only letters, digits, hyphen (-), underscore (_), and dot (.)") + }Be sure to add the same guard inside
validateUpdateCustomRoleRequestfor symmetry and importregexp.
📜 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 e7b8fa3d611d3c1af3caab11cf617ef0e62d8ca1 and 44084ee209f7b25f4671aa2dc4e7615037b74e87.
⛔ 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(3 hunks)pkg/gateway/server.go(8 hunks)pkg/server/org-unit-role.go(2 hunks)pkg/server/org-unit-user.go(7 hunks)pkg/table/const.go(1 hunks)pkg/table/org-unit-custom-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
🪛 ast-grep (0.39.6)
pkg/gateway/server.go
[warning] 362-362: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 Checkov (3.2.334)
api/swagger/apidocs.swagger.json
[medium] 2522-2529: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
| items := []*api.OrgUnitRolesListEntry{ | ||
| { | ||
| Name: "admin", | ||
| Desc: "Administrator role to provide access to everything in the Organization Unit including management of users and resources", | ||
| Type: "built-in", | ||
| DisplayName: "Administrator", | ||
| Created: 0, | ||
| CreatedBy: "", | ||
| }, | ||
| { | ||
| Name: "auditor", | ||
| Desc: "Auditor role to provide read-only access to all the resources available in the Organization Unit", | ||
| Type: "built-in", | ||
| DisplayName: "Auditor", | ||
| Created: 0, | ||
| CreatedBy: "", | ||
| }, | ||
| } |
There was a problem hiding this comment.
Return the built-in default role too
ListOrgUnitRoles only emits admin and auditor, but default is still treated as a built-in everywhere else (e.g. Add validates it, Delete skips cleanup for it). Omitting it here means clients pulling this list will never discover default, so they cannot assign or manage users with that baseline role. Please add the default definition alongside the other built-ins.
Suggested addition:
{
Name: "auditor",
Desc: "Auditor role to provide read-only access to all the resources available in the Organization Unit",
Type: "built-in",
DisplayName: "Auditor",
Created: 0,
CreatedBy: "",
},
+ {
+ Name: "default",
+ Desc: "Baseline org unit role with no special privileges.",
+ Type: "built-in",
+ DisplayName: "Default",
+ Created: 0,
+ CreatedBy: "",
+ },📝 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.
| items := []*api.OrgUnitRolesListEntry{ | |
| { | |
| Name: "admin", | |
| Desc: "Administrator role to provide access to everything in the Organization Unit including management of users and resources", | |
| Type: "built-in", | |
| DisplayName: "Administrator", | |
| Created: 0, | |
| CreatedBy: "", | |
| }, | |
| { | |
| Name: "auditor", | |
| Desc: "Auditor role to provide read-only access to all the resources available in the Organization Unit", | |
| Type: "built-in", | |
| DisplayName: "Auditor", | |
| Created: 0, | |
| CreatedBy: "", | |
| }, | |
| } | |
| items := []*api.OrgUnitRolesListEntry{ | |
| { | |
| Name: "admin", | |
| Desc: "Administrator role to provide access to everything in the Organization Unit including management of users and resources", | |
| Type: "built-in", | |
| DisplayName: "Administrator", | |
| Created: 0, | |
| CreatedBy: "", | |
| }, | |
| { | |
| Name: "auditor", | |
| Desc: "Auditor role to provide read-only access to all the resources available in the Organization Unit", | |
| Type: "built-in", | |
| DisplayName: "Auditor", | |
| Created: 0, | |
| CreatedBy: "", | |
| }, | |
| { | |
| Name: "default", | |
| Desc: "Baseline org unit role with no special privileges.", | |
| Type: "built-in", | |
| DisplayName: "Default", | |
| Created: 0, | |
| CreatedBy: "", | |
| }, | |
| } |
🤖 Prompt for AI Agents
In pkg/server/org-unit-role.go around lines 37 to 54, the built-in "default"
role is missing from the items slice returned by ListOrgUnitRoles; add a third
entry for the "default" role with Name: "default", Desc: a short description
(e.g., baseline/default role for new users), Type: "built-in", DisplayName:
"Default", and Created/CreatedBy set consistently with the other built-ins (0
and empty string) so clients can discover and assign the baseline role.
…d permission checks Introduce complete support for custom roles scoped to an organization unit in the auth‑gateway. Includes the ability to create, update, retrieve, delete, and list roles within their org‑unit boundaries. Signed-off-by: suryanshu185 <suryanshu.gupta@kluisz.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/server/org-unit-user.go (1)
67-70: Let Add reusevalidateRoleso custom roles work.
AddOrgUnitUserstill hard-codes the three built-ins, so any custom role created by this feature is rejected at creation time.UpdateOrgUnitUseralready delegates tovalidateRole; do the same here so we honor built-ins and existing custom roles. Without this change, the new custom-role CRUD is unusable for new assignments.Apply this diff:
- // validate role, currently only admin, default and auditor roles are allowed - if req.Role != "admin" && req.Role != "default" && req.Role != "auditor" { - return nil, status.Errorf(codes.InvalidArgument, "Invalid role: %s", req.Role) - } + // validate role - allow built-in or existing custom role + if err := s.validateRole(ctx, req.Role, req.Ou, authInfo.Realm); err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Invalid role: %s - %v", req.Role, err) + }
♻️ Duplicate comments (5)
pkg/gateway/server.go (1)
634-654: Handle the built-indefaultrole explicitly.When an OU user has the built-in
defaultrole we immediately fall through to the custom-role lookup, which fails and denies access. We already treatdefaultas a first-class built-in elsewhere, so add an explicit branch so baseline users aren’t locked out.switch ouUser.Role { case "admin": // wildcard access to the org unit return true case "auditor": if r.Method == http.MethodGet || r.Method == http.MethodHead { // allow read-only access for auditor role return true } return false + case "default": + // Baseline role: no elevated privileges beyond general tenant checks + return false }pkg/server/org-unit-role.go (1)
37-54: Include the built-indefaultrole in the list response.The List RPC still only emits
adminandauditor. We rely on thedefaultrole elsewhere (validation, cleanup), so leaving it out means clients never discover or assign the baseline role. Re-add thedefaultentry alongside the other built-ins.items := []*api.OrgUnitRolesListEntry{ { Name: "admin", Desc: "Administrator role to provide access to everything in the Organization Unit including management of users and resources", Type: "built-in", DisplayName: "Administrator", Created: 0, CreatedBy: "", }, { Name: "auditor", Desc: "Auditor role to provide read-only access to all the resources available in the Organization Unit", Type: "built-in", DisplayName: "Auditor", Created: 0, CreatedBy: "", }, + { + Name: "default", + Desc: "Baseline org unit role with no special privileges.", + Type: "built-in", + DisplayName: "Default", + Created: 0, + CreatedBy: "", + }, }pkg/table/org-unit-custom-role.go (1)
262-280: Add the unique (tenant, orgUnitId, name) index during table init.We still never create a compound unique index on
(key.tenant, key.orgUnitId, key.name), so concurrent creates can insert duplicates. Please set up the index insideLocateOrgUnitCustomRoleTable(or equivalent migrations) before assigning the global table.Example (Mongo):
func LocateOrgUnitCustomRoleTable(client db.StoreClient) (*OrgUnitCustomRoleTable, error) { @@ - err := tbl.Initialize(col) + err := tbl.Initialize(col) if err != nil { return nil, err } + + if mongoCol, ok := col.(*mongo.Collection); ok { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + 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), + } + if _, err := mongoCol.Indexes().CreateOne(ctx, indexModel); err != nil && !isIndexExistsError(err) { + return nil, errors.Wrapf(err, "failed to ensure unique index on org unit custom roles") + } + }Reuse whatever helpers we already have for index-exists detection.
Without this, the store permits duplicate custom role documents. Based on learnings
api/swagger/apidocs.swagger.json (2)
2508-2554: Sync Create/Update body validation with the server.The server rejects blank
name,displayName,description, and enforces 1–20 permissions. The OpenAPI still marks everything optional, so generated clients will happily send payloads the backend rejects.Please:
- add
minLength: 1to those three strings,- set
permissionstominItems: 1,maxItems: 20,- declare
"required": ["name","displayName","description","permissions"]on the create schema and"required": ["displayName","description","permissions"]on the update schema.That keeps clients and server in agreement. Based on learnings
3608-3644: Add required + size constraints forapiRolePermission.
RolePermissionmust have a non-emptyresource, at least one verb, and each verb must be non-empty. The proto/server enforce this, but the schema leaves everything optional. Update the definition to include:"required": ["resource", "verbs"], "properties": { "resource": { "type": "string", "minLength": 1, ... }, "verbs": { "type": "array", "minItems": 1, "items": { "type": "string", "minLength": 1 } } }Otherwise clients can submit payloads that the backend refuses. Based on learnings
📜 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 44084ee209f7b25f4671aa2dc4e7615037b74e87 and 8838afb.
⛔ 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(3 hunks)pkg/gateway/server.go(8 hunks)pkg/server/org-unit-role.go(2 hunks)pkg/server/org-unit-user.go(7 hunks)pkg/table/const.go(1 hunks)pkg/table/org-unit-custom-role.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/table/const.go
- pkg/gateway/routes.go
- main.go
🧰 Additional context used
🪛 ast-grep (0.39.6)
pkg/gateway/server.go
[warning] 362-362: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 Checkov (3.2.334)
api/swagger/apidocs.swagger.json
[medium] 2522-2529: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
| if ouUser.Role == "admin" || ouUser.Role == "auditor" { | ||
| return nil, nil | ||
| } | ||
|
|
||
| // Get the custom role | ||
| customRole, err := s.ouCustomRoleTbl.FindByNameAndOrgUnit(r.Context(), authInfo.Realm, ou, ouUser.Role) | ||
| if err != nil { | ||
| return nil, nil | ||
| } | ||
|
|
||
| // Collect both Allow and Deny rules for the specified resource type | ||
| var allowRules []*table.ResourceMatch | ||
| var denyRules []*table.ResourceMatch | ||
|
|
||
| for _, perm := range customRole.Permissions { | ||
| // Skip non-matching resources early | ||
| if perm.Resource != resourceType && perm.Resource != "*" { | ||
| continue | ||
| } | ||
|
|
||
| // Skip permissions without match criteria | ||
| if perm.Match == nil { | ||
| continue | ||
| } | ||
|
|
||
| action := perm.Action | ||
| if action == "" { | ||
| action = "Allow" | ||
| } | ||
|
|
||
| switch action { | ||
| case "Allow": | ||
| allowRules = append(allowRules, perm.Match) | ||
| case "Deny": | ||
| denyRules = append(denyRules, perm.Match) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Filter rules must respect verbs.
getResourceFilterRules appends allow/deny matches without checking perm.Verbs, so a deny on ["delete"] will also hide resources from GET/list, and vice versa. That breaks PBAC semantics and can both over-expose and over-restrict results. Gate these matches on verb (e.g., require * or the current route verb) so we only use rules that actually apply to the request.
Example fix:
-func (s *gateway) getResourceFilterRules(... resourceType string, r *http.Request) ([]*table.ResourceMatch, []*table.ResourceMatch) {
+func (s *gateway) getResourceFilterRules(... resourceType, routeVerb string, r *http.Request) ([]*table.ResourceMatch, []*table.ResourceMatch) {
@@
- switch action {
- case "Allow":
- allowRules = append(allowRules, perm.Match)
- case "Deny":
- denyRules = append(denyRules, perm.Match)
+ if verbMatches(routeVerb, perm.Verbs) {
+ switch action {
+ case "Allow":
+ allowRules = append(allowRules, perm.Match)
+ case "Deny":
+ denyRules = append(denyRules, perm.Match)
+ }
}and pass routeInfo.Verb at the call site.
Add a small helper like:
func verbMatches(routeVerb string, verbs []string) bool {
for _, v := range verbs {
if v == "*" || strings.EqualFold(v, routeVerb) {
return true
}
}
return false
}Without this fix, verb-scoped denies/permits behave unpredictably.
🤖 Prompt for AI Agents
In pkg/gateway/server.go around lines 680 to 717, the loop collecting allow/deny
rules ignores perm.Verbs so verb-scoped permissions (e.g., only "delete") are
incorrectly applied to other verbs; update the call site to pass the current
route verb (e.g., routeInfo.Verb) into getResourceFilterRules, add a small
helper that returns true when the rule verbs include "*" or match the route verb
(case-insensitive), and then gate appending perm.Match to allowRules/denyRules
by calling that helper (skip the permission if it doesn't match the current
verb).
1 participant
…d permission checks
Introduce complete support for custom roles scoped to an organization unit in the auth‑gateway. Includes the ability to create, update, retrieve, delete, and list roles within their org‑unit boundaries.
Summary by CodeRabbit
New Features
Documentation
Chores