Skip to content

feat(api/auth): flip mutating-route gate to handler-level requirePermission (PR-A of #660) #726

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristim merged 3 commits into from
May 27, 2026
Merged

feat(api/auth): flip mutating-route gate to handler-level requirePermission (PR-A of #660) #726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b2c61b6
feat(api/auth): flip mutating-route gate to handler-level requirePerm…
cristim May 25, 2026
a47247e
test(frontend/perms): update permission test fixtures for new user de…
cristim May 25, 2026
425ace4
test(api): tighten assertNotForbidden to require positive post-gate s…
cristim May 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion frontend/src/__tests__/permissions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ describe('permissions', () => {
'view:history',
'create:plans',
'update:plans',
'delete:plans',
'update:purchases',
'cancel-own:purchases',
'retry-own:purchases',
'approve-own:purchases',
Expand Down Expand Up @@ -91,7 +93,9 @@ describe('permissions', () => {

test('user role: denied for admin-gated actions', () => {
mockUser('user');
expect(canAccess('delete', 'plans')).toBe(false);
// delete:plans is now a default user permission (PR #660); no longer admin-gated.
expect(canAccess('delete', 'plans')).toBe(true);
// execute:purchases was NOT added to user defaults; remains admin-only.
expect(canAccess('execute', 'purchases')).toBe(false);
expect(canAccess('admin', '*')).toBe(false);
expect(canAccess('view', 'users')).toBe(false);
Expand Down
8 changes: 4 additions & 4 deletions frontend/src/__tests__/plans-permissions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,24 +140,24 @@ describe('Plans page permission gating (issue #365)', () => {
expect(btn.hidden).toBe(false);
});

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

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

Expand Down
2 changes: 2 additions & 0 deletions frontend/src/permissions.generated.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@ export const USER_PERMS: ReadonlySet<string> = new Set([
'approve-own:purchases',
'cancel-own:purchases',
'create:plans',
'delete:plans',
'retry-own:purchases',
'update:plans',
'update:purchases',
'view:history',
'view:plans',
'view:purchases',
Expand Down
54 changes: 32 additions & 22 deletions internal/api/router.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,20 +140,28 @@ func (r *Router) registerRoutes() {
{PathPrefix: "/api/recommendations/", PathSuffix: "/detail", Method: "GET", Handler: r.getRecommendationDetailHandler, Auth: AuthUser},

// Purchase plans endpoints — GETs are AuthUser (anyone signed in
// can see plans they're entitled to), writes stay AuthAdmin.
// can see plans they're entitled to). Mutating routes are also
// AuthUser now (PR-A of #660): the router-level gate drops to
// "must be signed in" and each handler calls requirePermission with
// the specific verb+resource so the per-handler check is the real
// gate. Without the flip the requirePermission call inside the
// handler is never reached for non-admins.
{ExactPath: "/api/plans", Method: "GET", Handler: r.listPlansHandler, Auth: AuthUser},
{ExactPath: "/api/plans", Method: "POST", Handler: r.createPlanHandler, Auth: AuthAdmin},
{ExactPath: "/api/plans", Method: "POST", Handler: r.createPlanHandler, Auth: AuthUser},
// Suffix routes must precede generic prefix routes so they are matched first.
{PathPrefix: "/api/plans/", PathSuffix: "/purchases", Method: "POST", Handler: r.createPlannedPurchasesHandler, Auth: AuthAdmin},
{PathPrefix: "/api/plans/", PathSuffix: "/purchases", Method: "POST", Handler: r.createPlannedPurchasesHandler, Auth: AuthUser},
{PathPrefix: "/api/plans/", PathSuffix: "/accounts", Method: "GET", Handler: r.listPlanAccountsHandler, Auth: AuthUser},
{PathPrefix: "/api/plans/", PathSuffix: "/accounts", Method: "PUT", Handler: r.setPlanAccountsHandler, Auth: AuthAdmin},
{PathPrefix: "/api/plans/", PathSuffix: "/accounts", Method: "PUT", Handler: r.setPlanAccountsHandler, Auth: AuthUser},
{PathPrefix: "/api/plans/", Method: "GET", Handler: r.getPlanHandler, Auth: AuthUser},
{PathPrefix: "/api/plans/", Method: "PUT", Handler: r.updatePlanHandler, Auth: AuthAdmin},
{PathPrefix: "/api/plans/", Method: "PATCH", Handler: r.patchPlanHandler, Auth: AuthAdmin},
{PathPrefix: "/api/plans/", Method: "DELETE", Handler: r.deletePlanHandler, Auth: AuthAdmin},

// Purchase actions
{ExactPath: "/api/purchases/execute", Method: "POST", Handler: r.executePurchaseHandler, Auth: AuthAdmin},
{PathPrefix: "/api/plans/", Method: "PUT", Handler: r.updatePlanHandler, Auth: AuthUser},
{PathPrefix: "/api/plans/", Method: "PATCH", Handler: r.patchPlanHandler, Auth: AuthUser},
{PathPrefix: "/api/plans/", Method: "DELETE", Handler: r.deletePlanHandler, Auth: AuthUser},

// Purchase actions. AuthUser so requirePermission inside each
// handler is the real gate (PR-A of #660 — same rationale as plans
// above). Approve + Cancel stay AuthPublic (token-based paths that
// are deliberately unauthenticated).
{ExactPath: "/api/purchases/execute", Method: "POST", Handler: r.executePurchaseHandler, Auth: AuthUser},
// Approve + Cancel also accept GET so the one-click links in the
// approval email (rendered as <a href>) land on the correct
// handler instead of falling through to the catch-all 401 that
Expand All @@ -170,14 +178,14 @@ func (r *Router) registerRoutes() {
// the retry-any/retry-own RBAC matrix.
{PathPrefix: "/api/purchases/retry/", Method: "POST", Handler: r.retryPurchaseHandler, Auth: AuthUser},

// Planned purchases endpoints (must come before generic /api/purchases/{id})
// — GET list is AuthUser; the action endpoints (pause/resume/run/delete)
// stay AuthAdmin.
// Planned purchases endpoints (must come before generic /api/purchases/{id}).
// All now AuthUser (PR-A of #660): handler-level requirePermission
// is the actual gate for pause/resume/run/delete.
{ExactPath: "/api/purchases/planned", Method: "GET", Handler: r.getPlannedPurchasesHandler, Auth: AuthUser},
{PathPrefix: "/api/purchases/planned/", PathSuffix: "/pause", Method: "POST", Handler: r.pausePlannedPurchaseHandler, Auth: AuthAdmin},
{PathPrefix: "/api/purchases/planned/", PathSuffix: "/resume", Method: "POST", Handler: r.resumePlannedPurchaseHandler, Auth: AuthAdmin},
{PathPrefix: "/api/purchases/planned/", PathSuffix: "/run", Method: "POST", Handler: r.runPlannedPurchaseHandler, Auth: AuthAdmin},
{PathPrefix: "/api/purchases/planned/", Method: "DELETE", Handler: r.deletePlannedPurchaseHandler, Auth: AuthAdmin},
{PathPrefix: "/api/purchases/planned/", PathSuffix: "/pause", Method: "POST", Handler: r.pausePlannedPurchaseHandler, Auth: AuthUser},
{PathPrefix: "/api/purchases/planned/", PathSuffix: "/resume", Method: "POST", Handler: r.resumePlannedPurchaseHandler, Auth: AuthUser},
{PathPrefix: "/api/purchases/planned/", PathSuffix: "/run", Method: "POST", Handler: r.runPlannedPurchaseHandler, Auth: AuthUser},
{PathPrefix: "/api/purchases/planned/", Method: "DELETE", Handler: r.deletePlannedPurchaseHandler, Auth: AuthUser},

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

// RI Exchange endpoints — GETs are AuthUser (Convertible RIs,
// Reshape Recommendations, Exchange History pages all need this);
// quote / execute / config writes stay AuthAdmin.
// Reshape Recommendations, Exchange History pages all need this).
// quote / execute / config writes are now AuthUser (PR-A of #660):
// each handler calls requirePermission so the per-handler check is
// the real gate. approve/reject stay AuthPublic (token-based).
{ExactPath: "/api/ri-exchange/azure-instances", Method: "GET", Handler: r.listExchangeableAzureRIsHandler, Auth: AuthUser},
{ExactPath: "/api/ri-exchange/instances", Method: "GET", Handler: r.listConvertibleRIsHandler, Auth: AuthUser},
{ExactPath: "/api/ri-exchange/target-offerings", Method: "GET", Handler: r.listTargetOfferingsHandler, Auth: AuthUser},
{ExactPath: "/api/ri-exchange/utilization", Method: "GET", Handler: r.getRIUtilizationHandler, Auth: AuthUser},
{ExactPath: "/api/ri-exchange/reshape-recommendations", Method: "GET", Handler: r.getReshapeRecommendationsHandler, Auth: AuthUser},
{ExactPath: "/api/ri-exchange/quote", Method: "POST", Handler: r.getExchangeQuoteHandler, Auth: AuthAdmin},
{ExactPath: "/api/ri-exchange/execute", Method: "POST", Handler: r.executeExchangeHandler, Auth: AuthAdmin},
{ExactPath: "/api/ri-exchange/quote", Method: "POST", Handler: r.getExchangeQuoteHandler, Auth: AuthUser},
{ExactPath: "/api/ri-exchange/execute", Method: "POST", Handler: r.executeExchangeHandler, Auth: AuthUser},
{ExactPath: "/api/ri-exchange/config", Method: "GET", Handler: r.getRIExchangeConfigHandler, Auth: AuthUser},
{ExactPath: "/api/ri-exchange/config", Method: "PUT", Handler: r.updateRIExchangeConfigHandler, Auth: AuthAdmin},
{ExactPath: "/api/ri-exchange/config", Method: "PUT", Handler: r.updateRIExchangeConfigHandler, Auth: AuthUser},
{ExactPath: "/api/ri-exchange/history", Method: "GET", Handler: r.getRIExchangeHistoryHandler, Auth: AuthUser},
{PathPrefix: "/api/ri-exchange/approve/", Method: "POST", Handler: r.approveRIExchangeHandler, Auth: AuthPublic},
{PathPrefix: "/api/ri-exchange/reject/", Method: "POST", Handler: r.rejectRIExchangeHandler, Auth: AuthPublic},
Expand Down
Loading
Loading