Fix route-level RBAC enforcement for privileged endpoints#19
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 59 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR implements fine-grained RBAC authorization enforcement across protected API endpoints. It adds permission-validating middleware, applies it to routes requiring specific permissions, updates API documentation to reflect authorization requirements, extends the RBAC service with org-aware logic, and introduces database infrastructure and tests for permission validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router & Middleware
participant RBACService
participant PostgreSQL
Client->>Router & Middleware: HTTP Request + JWT Token
Router & Middleware->>Router & Middleware: Extract userID, orgID from context
alt Missing Identity
Router & Middleware-->>Client: 401 Unauthorized
else Identity Present
Router & Middleware->>RBACService: Authorize(ctx, orgID, userID, permission)
RBACService->>PostgreSQL: GetUserByID(orgID, userID)
PostgreSQL-->>RBACService: User entity
alt User is Superuser
RBACService-->>Router & Middleware: nil (authorized)
else Not Superuser
RBACService->>PostgreSQL: UserHasPermission(orgID, userID, permission)
PostgreSQL-->>RBACService: bool result
alt Permission Granted
RBACService-->>Router & Middleware: nil (authorized)
else Permission Denied
RBACService-->>Router & Middleware: ErrForbidden
Router & Middleware-->>Client: 403 Forbidden
end
end
alt Authorization Succeeded
Router & Middleware->>Router & Middleware: next.ServeHTTP(w, r)
Router & Middleware-->>Client: 200/201 OK (handler response)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/router.go (1)
98-142:⚠️ Potential issue | 🔴 CriticalProtected routes cannot be accessed via API keys due to missing auth middleware wiring and incompatible permission checks.
All newly protected routes (users, organizations, roles, api-keys, audit, webhooks) are gated by
JWTAuthmiddleware, which only acceptsAuthorization: Bearer <JWT>tokens. TheAPIKeyAuthmiddleware exists but is never wired into the router, making it impossible for service-to-service API key clients to reach any of these endpoints. Even ifAPIKeyAuthwere added, theRequirePermissionmiddleware requiresContextKeyUserIDto be present in the context—butAPIKeyAuthonly setsContextKeyOrgID, causing authorization to fail.Additionally, API key scopes are stored in the database (line 30 of
internal/service/apikey.go) but are never validated or enforced anywhere in the middleware chain or service layer, violating the requirement that "API key scopes must control access. Always verify scopes in the service layer before allowing operations."To fix:
- Create a combined auth middleware that accepts both JWT and API keys, or integrate
APIKeyAuthinto the protected route group.- Modify
RequirePermissionto handle API key–authenticated requests (with scope-based access control instead of user-based RBAC).- Implement scope validation in the service layer before allowing API key operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/router.go` around lines 98 - 142, Protected routes are currently only using middleware.JWTAuth so API keys can't authenticate and RequirePermission assumes a user ID in context, while APIKeyAuth only sets ContextKeyOrgID and API key scopes (from internal/service/apikey.go) are never enforced; fix by wiring a combined authenticator (or apply both middleware.JWTAuth and middleware.APIKeyAuth) to the protected router group so requests can authenticate with either method, update middleware.RequirePermission to detect when ContextKeyOrgID/API key is present and perform scope-based checks (falling back to user-based RBAC when ContextKeyUserID is set), and enforce scope validation in the service layer before sensitive operations (implement explicit scope checks in apikey service methods and in services handling users/roles/webhooks/audit/api-keys such as apiKeyH.CreateAPIKey, apiKeyH.RevokeAPIKey, webhook handlers and corresponding service methods) to ensure API key scopes control access.
🧹 Nitpick comments (3)
internal/repository/postgres/roles.go (1)
189-190: Use the standard wrapped-error prefix here.Please switch this to the repo format, e.g.
fmt.Errorf("db.UserHasPermission: %w", err), so downstream logging and error traces stay consistent.As per coding guidelines, "Wrap errors with context using
fmt.Errorf()in the format:fmt.Errorf("<package>.<function>: %w", <error>)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repository/postgres/roles.go` around lines 189 - 190, The error wrap in the row.Scan call uses a freeform message; update it to use the repo standard package+function prefix. Replace the current fmt.Errorf("check user permission: %w", err) with fmt.Errorf("db.UserHasPermission: %w", err) inside the UserHasPermission implementation in roles.go so downstream logging/traces follow the standard format.internal/api/router_authorization_test.go (2)
24-25: Missingtesting.Short()guard for database-dependent tests.Per coding guidelines, DB-dependent tests must include a
testing.Short()guard to allow skipping in short mode. All tests in this file depend on the database viatestutil.RequireTestStore(t).♻️ Proposed fix: add Short() guard at the start of each test
func TestRouterRequiresPermissionsOnPrivilegedRoutes(t *testing.T) { + if testing.Short() { + t.Skip("skipping database-dependent test in short mode") + } store := testutil.RequireTestStore(t)Apply the same pattern to
TestRouterAllowsSelfServiceEndpointsWithoutRBACPermissions,TestRouterAllowsGrantedPermissions, andTestRouterAllowsSuperuserBypass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/router_authorization_test.go` around lines 24 - 25, Each DB-dependent test must skip in short mode: at the top of TestRouterRequiresPermissionsOnPrivilegedRoutes, TestRouterAllowsSelfServiceEndpointsWithoutRBACPermissions, TestRouterAllowsGrantedPermissions, and TestRouterAllowsSuperuserBypass add a guard like if testing.Short() { t.Skip("skipping DB test in short mode") } before calling testutil.RequireTestStore(t) so the tests are skipped when running with -short.
72-72: Unused context variable.The
ctxvariable is created but only referenced in a no-op statement_ = ctx. Either use it or remove it.♻️ Proposed fix: remove unused context
- ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) - defer cancel() - org := testutil.CreateOrganization(t, store, "router-forbidden-org") user := testutil.CreateUser(t, store, org.ID, "router-forbidden-user") accessToken := issueAccessToken(t, user.ID, org.ID) // ... test cases ... - - _ = ctx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/router_authorization_test.go` at line 72, The test declares an unused context variable `ctx` that is only referenced via `_ = ctx`; remove the unused `ctx` declaration (or replace its usage with a real use if intended) so there are no no-op assignments left in router_authorization_test.go; look for the `ctx` variable near the `_ = ctx` line and delete the declaration and the `_ = ctx` statement (or wire `ctx` into the test call that requires a context such as any function that accepts `context.Context`).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/router_authorization_test.go`:
- Around line 178-183: The test is passing nil for the scopes parameter to
CreateAPIKey which violates the DB NOT NULL constraint; update the path closure
in router_authorization_test.go so the call to store.CreateAPIKey(ctx, orgID,
"router-delete-key", "uni", uuid.NewString(), nil, nil) supplies an empty slice
for scopes (e.g., []string{} or make([]string,0)) instead of nil; keep all other
args the same and ensure any other CreateAPIKey calls in this test use a non-nil
empty slice for scopes.
In `@internal/repository/postgres/roles.go`:
- Around line 176-185: The authorization query in UserHasPermission is filtering
on ur.org_id but user_roles only has user_id and role_id; change the query to
scope by the organization via the roles table instead—join roles (e.g., alias r)
or reference roles.org_id and replace ur.org_id with r.org_id in the WHERE
clause so the EXISTS check correctly enforces multi-tenant isolation; update the
QueryRow parameters if needed to still pass orgID, userID, permission to match
the new WHERE predicate in UserHasPermission.
In `@internal/repository/postgres/tenant_scope_test.go`:
- Around line 227-229: The test TestStoreUserHasPermissionHonorsOrgScope
currently calls RequireTestStore immediately; add a testing.Short() guard at the
top of that test so it skips when tests are run with -short. Specifically, check
testing.Short() at the start of TestStoreUserHasPermissionHonorsOrgScope and
call t.Skip("skipping DB test in short mode") before calling RequireTestStore to
avoid requiring the database in short runs.
In `@internal/service/rbac_scope_test.go`:
- Around line 59-61: In TestRBACServiceAuthorize add a testing.Short() guard
before calling RequireTestStore so the DB-backed test is skipped in short mode;
specifically, at the start of the TestRBACServiceAuthorize function check if
testing.Short() { t.Skip("skipping DB test in short mode") } and only then call
testutil.RequireTestStore and NewRBACService/NewAuditService to create the store
and services.
---
Outside diff comments:
In `@internal/api/router.go`:
- Around line 98-142: Protected routes are currently only using
middleware.JWTAuth so API keys can't authenticate and RequirePermission assumes
a user ID in context, while APIKeyAuth only sets ContextKeyOrgID and API key
scopes (from internal/service/apikey.go) are never enforced; fix by wiring a
combined authenticator (or apply both middleware.JWTAuth and
middleware.APIKeyAuth) to the protected router group so requests can
authenticate with either method, update middleware.RequirePermission to detect
when ContextKeyOrgID/API key is present and perform scope-based checks (falling
back to user-based RBAC when ContextKeyUserID is set), and enforce scope
validation in the service layer before sensitive operations (implement explicit
scope checks in apikey service methods and in services handling
users/roles/webhooks/audit/api-keys such as apiKeyH.CreateAPIKey,
apiKeyH.RevokeAPIKey, webhook handlers and corresponding service methods) to
ensure API key scopes control access.
---
Nitpick comments:
In `@internal/api/router_authorization_test.go`:
- Around line 24-25: Each DB-dependent test must skip in short mode: at the top
of TestRouterRequiresPermissionsOnPrivilegedRoutes,
TestRouterAllowsSelfServiceEndpointsWithoutRBACPermissions,
TestRouterAllowsGrantedPermissions, and TestRouterAllowsSuperuserBypass add a
guard like if testing.Short() { t.Skip("skipping DB test in short mode") }
before calling testutil.RequireTestStore(t) so the tests are skipped when
running with -short.
- Line 72: The test declares an unused context variable `ctx` that is only
referenced via `_ = ctx`; remove the unused `ctx` declaration (or replace its
usage with a real use if intended) so there are no no-op assignments left in
router_authorization_test.go; look for the `ctx` variable near the `_ = ctx`
line and delete the declaration and the `_ = ctx` statement (or wire `ctx` into
the test call that requires a context such as any function that accepts
`context.Context`).
In `@internal/repository/postgres/roles.go`:
- Around line 189-190: The error wrap in the row.Scan call uses a freeform
message; update it to use the repo standard package+function prefix. Replace the
current fmt.Errorf("check user permission: %w", err) with
fmt.Errorf("db.UserHasPermission: %w", err) inside the UserHasPermission
implementation in roles.go so downstream logging/traces follow the standard
format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e5ddc56-8f1f-457c-b8a7-342e52538dda
📒 Files selected for processing (21)
docs/docs.godocs/security-report.mddocs/swagger.jsondocs/swagger.yamlinternal/api/handlers/apikeys.gointernal/api/handlers/audit.gointernal/api/handlers/organizations.gointernal/api/handlers/roles.gointernal/api/handlers/users.gointernal/api/handlers/webhooks.gointernal/api/middleware/authorization.gointernal/api/router.gointernal/api/router_authorization_test.gointernal/domain/role.gointernal/repository/postgres/roles.gointernal/repository/postgres/tenant_scope_test.gointernal/service/rbac.gointernal/service/rbac_scope_test.gomigrations/000003_organization_permissions.down.sqlmigrations/000003_organization_permissions.up.sqlsql/schema.sql
| func (s *Store) UserHasPermission(ctx context.Context, orgID, userID uuid.UUID, permission string) (bool, error) { | ||
| row := s.pool.QueryRow(ctx, | ||
| `SELECT EXISTS ( | ||
| SELECT 1 | ||
| FROM user_roles ur | ||
| JOIN role_permissions rp ON rp.role_id = ur.role_id | ||
| JOIN permissions p ON p.id = rp.permission_id | ||
| WHERE ur.org_id = $1 AND ur.user_id = $2 AND p.name = $3 | ||
| )`, | ||
| orgID, userID, permission, |
There was a problem hiding this comment.
user_roles.org_id will break this authorization query.
Line 183 filters on ur.org_id, but user_roles only stores user_id and role_id. That means this new RBAC path will fail with a SQL error instead of returning allow/deny. Scope through roles.org_id here.
🐛 Proposed fix
func (s *Store) UserHasPermission(ctx context.Context, orgID, userID uuid.UUID, permission string) (bool, error) {
row := s.pool.QueryRow(ctx,
`SELECT EXISTS (
SELECT 1
FROM user_roles ur
+ JOIN roles r ON r.id = ur.role_id
JOIN role_permissions rp ON rp.role_id = ur.role_id
JOIN permissions p ON p.id = rp.permission_id
- WHERE ur.org_id = $1 AND ur.user_id = $2 AND p.name = $3
+ WHERE r.org_id = $1 AND ur.user_id = $2 AND p.name = $3
)`,
orgID, userID, permission,
)As per coding guidelines, "Multi-tenancy isolation must be enforced: every repository query must filter by org_id. No exceptions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/repository/postgres/roles.go` around lines 176 - 185, The
authorization query in UserHasPermission is filtering on ur.org_id but
user_roles only has user_id and role_id; change the query to scope by the
organization via the roles table instead—join roles (e.g., alias r) or reference
roles.org_id and replace ur.org_id with r.org_id in the WHERE clause so the
EXISTS check correctly enforces multi-tenant isolation; update the QueryRow
parameters if needed to still pass orgID, userID, permission to match the new
WHERE predicate in UserHasPermission.
| func TestStoreUserHasPermissionHonorsOrgScope(t *testing.T) { | ||
| store := testutil.RequireTestStore(t) | ||
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) |
There was a problem hiding this comment.
Skip this DB-backed test in short mode.
Line 228 hits RequireTestStore immediately, so go test -short ./... will still require the database. Please add a testing.Short() guard before creating the store.
💡 Minimal fix
func TestStoreUserHasPermissionHonorsOrgScope(t *testing.T) {
+ if testing.Short() {
+ t.Skip("skipping DB-dependent test in short mode")
+ }
store := testutil.RequireTestStore(t)
ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestStoreUserHasPermissionHonorsOrgScope(t *testing.T) { | |
| store := testutil.RequireTestStore(t) | |
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) | |
| func TestStoreUserHasPermissionHonorsOrgScope(t *testing.T) { | |
| if testing.Short() { | |
| t.Skip("skipping DB-dependent test in short mode") | |
| } | |
| store := testutil.RequireTestStore(t) | |
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/repository/postgres/tenant_scope_test.go` around lines 227 - 229,
The test TestStoreUserHasPermissionHonorsOrgScope currently calls
RequireTestStore immediately; add a testing.Short() guard at the top of that
test so it skips when tests are run with -short. Specifically, check
testing.Short() at the start of TestStoreUserHasPermissionHonorsOrgScope and
call t.Skip("skipping DB test in short mode") before calling RequireTestStore to
avoid requiring the database in short runs.
| func TestRBACServiceAuthorize(t *testing.T) { | ||
| store := testutil.RequireTestStore(t) | ||
| svc := NewRBACService(store, NewAuditService(store, testutil.DiscardLogger())) |
There was a problem hiding this comment.
Skip this DB-backed test in short mode.
Line 60 calls RequireTestStore unconditionally, so go test -short ./... still needs Postgres. Add a testing.Short() guard before the store is created.
💡 Minimal fix
func TestRBACServiceAuthorize(t *testing.T) {
+ if testing.Short() {
+ t.Skip("skipping DB-dependent test in short mode")
+ }
store := testutil.RequireTestStore(t)
svc := NewRBACService(store, NewAuditService(store, testutil.DiscardLogger()))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestRBACServiceAuthorize(t *testing.T) { | |
| store := testutil.RequireTestStore(t) | |
| svc := NewRBACService(store, NewAuditService(store, testutil.DiscardLogger())) | |
| func TestRBACServiceAuthorize(t *testing.T) { | |
| if testing.Short() { | |
| t.Skip("skipping DB-dependent test in short mode") | |
| } | |
| store := testutil.RequireTestStore(t) | |
| svc := NewRBACService(store, NewAuditService(store, testutil.DiscardLogger())) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/rbac_scope_test.go` around lines 59 - 61, In
TestRBACServiceAuthorize add a testing.Short() guard before calling
RequireTestStore so the DB-backed test is skipped in short mode; specifically,
at the start of the TestRBACServiceAuthorize function check if testing.Short() {
t.Skip("skipping DB test in short mode") } and only then call
testutil.RequireTestStore and NewRBACService/NewAuditService to create the store
and services.
1 participant
Summary
Testing
go test ./...docker compose build appdocker compose up -d000003applied and new organization permissions exist in Postgres403for low-privilege users,200after grantingusers:read, and continued403for unrelated protected routesSummary by CodeRabbit
New Features
Bug Fixes
Documentation