feat(api/auth): flip mutating-route gate to handler-level requirePermission (PR-A of #660) - #726
Conversation
…ission (PR-A of #660) Every mutating route in the plans, purchases, planned-purchases, and ri-exchange groups was previously locked at AuthAdmin at the router level, which meant the per-handler requirePermission(action, resource) call was unreachable for non-admin users. This PR-A fix: 1. Router (internal/api/router.go): flips the Auth field on all mutating routes that already have a handler-level requirePermission call from AuthAdmin to AuthUser. The router-level gate now only asserts "must be signed in" (401 for anonymous callers); the real permission gate fires inside each handler. Routes flipped: - POST /api/plans (create:plans) - POST /api/plans/{id}/purchases (create:plans) - PUT /api/plans/{id}/accounts (update:plans) - PUT /api/plans/{id} (update:plans) - PATCH /api/plans/{id} (update:plans) - DELETE /api/plans/{id} (delete:plans) - POST /api/purchases/execute (execute:purchases) - POST /api/purchases/planned/{id}/pause (update:purchases) - POST /api/purchases/planned/{id}/resume (update:purchases) - POST /api/purchases/planned/{id}/run (execute:purchases) - DELETE /api/purchases/planned/{id} (delete:purchases) - POST /api/ri-exchange/quote (view:purchases) - POST /api/ri-exchange/execute (execute:purchases) - PUT /api/ri-exchange/config (update:config) 2. Defaults (internal/auth/types.go): adds delete:plans and update:purchases to DefaultUserPermissions so regular users get the two most common operator verbs by default, per the design comment on #660. PR-B (execute:purchases default grant) and PR-C (frontend) are deferred per the design comment. Refs #660 (PR-A; PR-B and PR-C deferred per design comment)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughBaseline user permissions expand to include ChangesPermission Definition and Router Enforcement Flip
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…faults Refs #660 (PR-A). Updates 4 test assertions in permissions.test.ts and plans-permissions.test.ts to reflect the 2 new entries in DefaultUserPermissions (delete:plans and update:purchases). - getRolePermissions user-role test: expected array 9 -> 11 entries - canAccess user-role denied test: delete:plans now toBe(true), not false - plans-permissions user-role card test: delete-plan button now shown - plans-permissions user-role row test: disable button now shown
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/api/router_660_permission_flips_test.go (1)
82-96: ⚡ Quick win
assertNotForbiddenis too permissive for “permission gate passed” assertions.This helper treats any non-401/403 as success, so granted-path tests can pass even when execution fails before proving meaningful post-gate behavior. Consider adding a stronger signal per test (e.g., assert expected downstream call occurred, or assert
HasPermissionAPIwas actually invoked for non-admin paths).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/router_660_permission_flips_test.go` around lines 82 - 96, The current assertNotForbidden helper (function assertNotForbidden) is too permissive because it treats any non-401/403 ClientError as proof the permission gate passed; change it to require an explicit positive signal that the post-gate path executed: either (A) modify assertNotForbidden signature to accept an extra boolean or interface (e.g., verified bool or a ctx/matcher) that callers set when the downstream action or HasPermissionAPI mock was actually invoked, or (B) make assertNotForbidden check a supplied mock flag/assertion method (e.g., validate that HasPermissionAPI was called on the permission mock) in addition to using IsClientError/ce.code; update all callers to provide that signal so tests fail if execution never reached the post-gate logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/api/router_660_permission_flips_test.go`:
- Around line 82-96: The current assertNotForbidden helper (function
assertNotForbidden) is too permissive because it treats any non-401/403
ClientError as proof the permission gate passed; change it to require an
explicit positive signal that the post-gate path executed: either (A) modify
assertNotForbidden signature to accept an extra boolean or interface (e.g.,
verified bool or a ctx/matcher) that callers set when the downstream action or
HasPermissionAPI mock was actually invoked, or (B) make assertNotForbidden check
a supplied mock flag/assertion method (e.g., validate that HasPermissionAPI was
called on the permission mock) in addition to using IsClientError/ce.code;
update all callers to provide that signal so tests fail if execution never
reached the post-gate logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77243e1b-898b-42fe-a54f-a21d5d1896f3
📒 Files selected for processing (9)
frontend/src/__tests__/permissions.test.tsfrontend/src/__tests__/plans-permissions.test.tsfrontend/src/permissions.generated.tsinternal/api/router.gointernal/api/router_660_permission_flips_test.gointernal/auth/service_group_test.gointernal/auth/service_test.gointernal/auth/types.gointernal/auth/types_test.go
…ignal
Register t.Cleanup(func(){ m.AssertExpectations(t) }) in both authForUserWith
and authForAdmin so testify verifies every On() expectation was actually invoked.
This catches cases where the permission gate is bypassed before HasPermissionAPI
runs: the granted-path sub-test would have passed assertNotForbidden even if
the gate were never reached.
Also removes two incorrect GetAllowedAccountsAPI and one GetExecutionByID
expectation from admin-bypass sub-tests. Admin sessions short-circuit in
getAllowedAccounts (role == "admin" -> nil, nil) so IsUnrestrictedAccess returns
true and requirePlanAccess/requireExecutionAccess return nil immediately without
calling GetAllowedAccountsAPI or GetExecutionByID. Registering those expectations
with AssertExpectations enforced correctly surfaced them as bugs.
|
Addressed the assertNotForbidden nitpick (option B). What changed (
All 1301 tests in @coderabbitai review |
|
Triggering a full review now. ✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Refs #660 (PR-A; PR-B frontend visibility + PR-C optional admin grant UI deferred per design comment).
What changed
The
Permission{Action, Resource, Constraints}schema andHasPermissioncheck have been in place for a while, plus handler-levelrequirePermission(action, resource)calls were wired into every mutating handler. The only chokepoint wasinternal/api/router.go— every mutating route hadAuth: AuthAdmin, so non-admins got rejected atrouter.go:321before the handler verb check ever fired.This PR flips 14 mutating routes from
AuthAdmintoAuthUserso handler-levelrequirePermissionbecomes the actual gate. Each was verified to already have itsrequirePermissioncall.Routes flipped
Defaults (
internal/auth/types.go DefaultUserPermissions)Added
delete:plansandupdate:purchases(count 9 → 11). Other defaults unchanged.Tests (+28 subtests across 7 test functions)
TestRouter_MutatingRoutes_RequireAuth— 7 routes, asserts unauthenticated callers still get 401 after the flipauth/types_test.go,service_group_test.go,service_test.gobumped 9→11 (and 11→13 for user+groups)Frontend
frontend/src/permissions.generated.tsauto-regenerated by the pre-commit hook to include the 2 new defaults inUSER_PERMS.1798 tests pass across
internal/apiandinternal/auth.Summary by CodeRabbit
New Features
Tests