Skip to content

Commit 4c62718

Browse files
cristimcristim
authored
feat(api/auth): flip mutating-route gate to handler-level requirePermission (PR-A of #660) (#726)
* feat(api/auth): flip mutating-route gate to handler-level requirePermission (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) * test(frontend/perms): update permission test fixtures for new user defaults 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 * test(api): tighten assertNotForbidden to require positive post-gate signal 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.
1 parent cb735f2 commit 4c62718

9 files changed

Lines changed: 447 additions & 50 deletions

File tree

‎frontend/src/__tests__/permissions.test.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ describe('permissions', () => {
4040
'view:history',
4141
'create:plans',
4242
'update:plans',
43+
'delete:plans',
44+
'update:purchases',
4345
'cancel-own:purchases',
4446
'retry-own:purchases',
4547
'approve-own:purchases',
@@ -91,7 +93,9 @@ describe('permissions', () => {
9193

9294
test('user role: denied for admin-gated actions', () => {
9395
mockUser('user');
94-
expect(canAccess('delete', 'plans')).toBe(false);
96+
// delete:plans is now a default user permission (PR #660); no longer admin-gated.
97+
expect(canAccess('delete', 'plans')).toBe(true);
98+
// execute:purchases was NOT added to user defaults; remains admin-only.
9599
expect(canAccess('execute', 'purchases')).toBe(false);
96100
expect(canAccess('admin', '*')).toBe(false);
97101
expect(canAccess('view', 'users')).toBe(false);

‎frontend/src/__tests__/plans-permissions.test.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -140,24 +140,24 @@ describe('Plans page permission gating (issue #365)', () => {
140140
expect(btn.hidden).toBe(false);
141141
});
142142

143-
test('shows manage actions but hides Delete (user lacks delete:plans)', async () => {
143+
test('shows manage actions including Delete (user has delete:plans since PR #660)', async () => {
144144
await loadPlans();
145145
const list = document.getElementById('plans-list') as HTMLElement;
146146
const html = list.innerHTML;
147147
expect(html).toContain('data-action="add-purchases"');
148148
expect(html).toContain('data-action="edit-plan"');
149149
expect(html).toContain('data-action="toggle-plan"');
150-
expect(html).not.toContain('data-action="delete-plan"');
150+
expect(html).toContain('data-action="delete-plan"');
151151
});
152152

153-
test('shows row Run/Pause/Edit but hides Disable on planned purchases', async () => {
153+
test('shows row Run/Pause/Edit/Disable on planned purchases (user has delete:plans since PR #660)', async () => {
154154
await loadPlans();
155155
const pp = document.getElementById('planned-purchases-list') as HTMLElement;
156156
const html = pp.innerHTML;
157157
expect(html).toContain('data-action="run"');
158158
expect(html).toContain('data-action="pause"');
159159
expect(html).toContain('data-action="edit"');
160-
expect(html).not.toContain('data-action="disable"');
160+
expect(html).toContain('data-action="disable"');
161161
});
162162
});
163163

‎frontend/src/permissions.generated.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,10 @@ export const USER_PERMS: ReadonlySet<string> = new Set([
2222
'approve-own:purchases',
2323
'cancel-own:purchases',
2424
'create:plans',
25+
'delete:plans',
2526
'retry-own:purchases',
2627
'update:plans',
28+
'update:purchases',
2729
'view:history',
2830
'view:plans',
2931
'view:purchases',

‎internal/api/router.go‎

Lines changed: 32 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -140,20 +140,28 @@ func (r *Router) registerRoutes() {
140140
{PathPrefix: "/api/recommendations/", PathSuffix: "/detail", Method: "GET", Handler: r.getRecommendationDetailHandler, Auth: AuthUser},
141141

142142
// Purchase plans endpoints — GETs are AuthUser (anyone signed in
143-
// can see plans they're entitled to), writes stay AuthAdmin.
143+
// can see plans they're entitled to). Mutating routes are also
144+
// AuthUser now (PR-A of #660): the router-level gate drops to
145+
// "must be signed in" and each handler calls requirePermission with
146+
// the specific verb+resource so the per-handler check is the real
147+
// gate. Without the flip the requirePermission call inside the
148+
// handler is never reached for non-admins.
144149
{ExactPath: "/api/plans", Method: "GET", Handler: r.listPlansHandler, Auth: AuthUser},
145-
{ExactPath: "/api/plans", Method: "POST", Handler: r.createPlanHandler, Auth: AuthAdmin},
150+
{ExactPath: "/api/plans", Method: "POST", Handler: r.createPlanHandler, Auth: AuthUser},
146151
// Suffix routes must precede generic prefix routes so they are matched first.
147-
{PathPrefix: "/api/plans/", PathSuffix: "/purchases", Method: "POST", Handler: r.createPlannedPurchasesHandler, Auth: AuthAdmin},
152+
{PathPrefix: "/api/plans/", PathSuffix: "/purchases", Method: "POST", Handler: r.createPlannedPurchasesHandler, Auth: AuthUser},
148153
{PathPrefix: "/api/plans/", PathSuffix: "/accounts", Method: "GET", Handler: r.listPlanAccountsHandler, Auth: AuthUser},
149-
{PathPrefix: "/api/plans/", PathSuffix: "/accounts", Method: "PUT", Handler: r.setPlanAccountsHandler, Auth: AuthAdmin},
154+
{PathPrefix: "/api/plans/", PathSuffix: "/accounts", Method: "PUT", Handler: r.setPlanAccountsHandler, Auth: AuthUser},
150155
{PathPrefix: "/api/plans/", Method: "GET", Handler: r.getPlanHandler, Auth: AuthUser},
151-
{PathPrefix: "/api/plans/", Method: "PUT", Handler: r.updatePlanHandler, Auth: AuthAdmin},
152-
{PathPrefix: "/api/plans/", Method: "PATCH", Handler: r.patchPlanHandler, Auth: AuthAdmin},
153-
{PathPrefix: "/api/plans/", Method: "DELETE", Handler: r.deletePlanHandler, Auth: AuthAdmin},
154-
155-
// Purchase actions
156-
{ExactPath: "/api/purchases/execute", Method: "POST", Handler: r.executePurchaseHandler, Auth: AuthAdmin},
156+
{PathPrefix: "/api/plans/", Method: "PUT", Handler: r.updatePlanHandler, Auth: AuthUser},
157+
{PathPrefix: "/api/plans/", Method: "PATCH", Handler: r.patchPlanHandler, Auth: AuthUser},
158+
{PathPrefix: "/api/plans/", Method: "DELETE", Handler: r.deletePlanHandler, Auth: AuthUser},
159+
160+
// Purchase actions. AuthUser so requirePermission inside each
161+
// handler is the real gate (PR-A of #660 — same rationale as plans
162+
// above). Approve + Cancel stay AuthPublic (token-based paths that
163+
// are deliberately unauthenticated).
164+
{ExactPath: "/api/purchases/execute", Method: "POST", Handler: r.executePurchaseHandler, Auth: AuthUser},
157165
// Approve + Cancel also accept GET so the one-click links in the
158166
// approval email (rendered as <a href>) land on the correct
159167
// handler instead of falling through to the catch-all 401 that
@@ -170,14 +178,14 @@ func (r *Router) registerRoutes() {
170178
// the retry-any/retry-own RBAC matrix.
171179
{PathPrefix: "/api/purchases/retry/", Method: "POST", Handler: r.retryPurchaseHandler, Auth: AuthUser},
172180

173-
// Planned purchases endpoints (must come before generic /api/purchases/{id})
174-
// — GET list is AuthUser; the action endpoints (pause/resume/run/delete)
175-
// stay AuthAdmin.
181+
// Planned purchases endpoints (must come before generic /api/purchases/{id}).
182+
// All now AuthUser (PR-A of #660): handler-level requirePermission
183+
// is the actual gate for pause/resume/run/delete.
176184
{ExactPath: "/api/purchases/planned", Method: "GET", Handler: r.getPlannedPurchasesHandler, Auth: AuthUser},
177-
{PathPrefix: "/api/purchases/planned/", PathSuffix: "/pause", Method: "POST", Handler: r.pausePlannedPurchaseHandler, Auth: AuthAdmin},
178-
{PathPrefix: "/api/purchases/planned/", PathSuffix: "/resume", Method: "POST", Handler: r.resumePlannedPurchaseHandler, Auth: AuthAdmin},
179-
{PathPrefix: "/api/purchases/planned/", PathSuffix: "/run", Method: "POST", Handler: r.runPlannedPurchaseHandler, Auth: AuthAdmin},
180-
{PathPrefix: "/api/purchases/planned/", Method: "DELETE", Handler: r.deletePlannedPurchaseHandler, Auth: AuthAdmin},
185+
{PathPrefix: "/api/purchases/planned/", PathSuffix: "/pause", Method: "POST", Handler: r.pausePlannedPurchaseHandler, Auth: AuthUser},
186+
{PathPrefix: "/api/purchases/planned/", PathSuffix: "/resume", Method: "POST", Handler: r.resumePlannedPurchaseHandler, Auth: AuthUser},
187+
{PathPrefix: "/api/purchases/planned/", PathSuffix: "/run", Method: "POST", Handler: r.runPlannedPurchaseHandler, Auth: AuthUser},
188+
{PathPrefix: "/api/purchases/planned/", Method: "DELETE", Handler: r.deletePlannedPurchaseHandler, Auth: AuthUser},
181189

182190
// Generic purchase details (must come after more specific routes)
183191
// — read-only; AuthUser so the history detail view works for everyone.
@@ -255,17 +263,19 @@ func (r *Router) registerRoutes() {
255263
{ExactPath: "/api/inventory/commitments", Method: "GET", Handler: r.listInventoryCommitmentsHandler, Auth: AuthUser},
256264

257265
// RI Exchange endpoints — GETs are AuthUser (Convertible RIs,
258-
// Reshape Recommendations, Exchange History pages all need this);
259-
// quote / execute / config writes stay AuthAdmin.
266+
// Reshape Recommendations, Exchange History pages all need this).
267+
// quote / execute / config writes are now AuthUser (PR-A of #660):
268+
// each handler calls requirePermission so the per-handler check is
269+
// the real gate. approve/reject stay AuthPublic (token-based).
260270
{ExactPath: "/api/ri-exchange/azure-instances", Method: "GET", Handler: r.listExchangeableAzureRIsHandler, Auth: AuthUser},
261271
{ExactPath: "/api/ri-exchange/instances", Method: "GET", Handler: r.listConvertibleRIsHandler, Auth: AuthUser},
262272
{ExactPath: "/api/ri-exchange/target-offerings", Method: "GET", Handler: r.listTargetOfferingsHandler, Auth: AuthUser},
263273
{ExactPath: "/api/ri-exchange/utilization", Method: "GET", Handler: r.getRIUtilizationHandler, Auth: AuthUser},
264274
{ExactPath: "/api/ri-exchange/reshape-recommendations", Method: "GET", Handler: r.getReshapeRecommendationsHandler, Auth: AuthUser},
265-
{ExactPath: "/api/ri-exchange/quote", Method: "POST", Handler: r.getExchangeQuoteHandler, Auth: AuthAdmin},
266-
{ExactPath: "/api/ri-exchange/execute", Method: "POST", Handler: r.executeExchangeHandler, Auth: AuthAdmin},
275+
{ExactPath: "/api/ri-exchange/quote", Method: "POST", Handler: r.getExchangeQuoteHandler, Auth: AuthUser},
276+
{ExactPath: "/api/ri-exchange/execute", Method: "POST", Handler: r.executeExchangeHandler, Auth: AuthUser},
267277
{ExactPath: "/api/ri-exchange/config", Method: "GET", Handler: r.getRIExchangeConfigHandler, Auth: AuthUser},
268-
{ExactPath: "/api/ri-exchange/config", Method: "PUT", Handler: r.updateRIExchangeConfigHandler, Auth: AuthAdmin},
278+
{ExactPath: "/api/ri-exchange/config", Method: "PUT", Handler: r.updateRIExchangeConfigHandler, Auth: AuthUser},
269279
{ExactPath: "/api/ri-exchange/history", Method: "GET", Handler: r.getRIExchangeHistoryHandler, Auth: AuthUser},
270280
{PathPrefix: "/api/ri-exchange/approve/", Method: "POST", Handler: r.approveRIExchangeHandler, Auth: AuthPublic},
271281
{PathPrefix: "/api/ri-exchange/reject/", Method: "POST", Handler: r.rejectRIExchangeHandler, Auth: AuthPublic},

0 commit comments

Comments
 (0)