diff --git a/frontend/src/__tests__/permissions.test.ts b/frontend/src/__tests__/permissions.test.ts index 8fa264a44..7e00b7c09 100644 --- a/frontend/src/__tests__/permissions.test.ts +++ b/frontend/src/__tests__/permissions.test.ts @@ -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', @@ -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); diff --git a/frontend/src/__tests__/plans-permissions.test.ts b/frontend/src/__tests__/plans-permissions.test.ts index 071f0eaf6..cc0c3a0d2 100644 --- a/frontend/src/__tests__/plans-permissions.test.ts +++ b/frontend/src/__tests__/plans-permissions.test.ts @@ -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"'); }); }); diff --git a/frontend/src/permissions.generated.ts b/frontend/src/permissions.generated.ts index bf5b90983..3065e83b9 100644 --- a/frontend/src/permissions.generated.ts +++ b/frontend/src/permissions.generated.ts @@ -22,8 +22,10 @@ export const USER_PERMS: ReadonlySet = 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', diff --git a/internal/api/router.go b/internal/api/router.go index 0ada3859e..4ad2e6b05 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -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 ) land on the correct // handler instead of falling through to the catch-all 401 that @@ -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. @@ -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}, diff --git a/internal/api/router_660_permission_flips_test.go b/internal/api/router_660_permission_flips_test.go new file mode 100644 index 000000000..a5033cb73 --- /dev/null +++ b/internal/api/router_660_permission_flips_test.go @@ -0,0 +1,357 @@ +package api + +// Tests for PR-A of issue #660: mutating-route gate flip from AuthAdmin to +// AuthUser + handler-level requirePermission as the real gate. +// +// Each sub-test follows the same pattern: +// 1. User session with role "user" (not admin). +// 2. HasPermissionAPI returns true -> request should reach the handler and +// pass (or fail with a domain error, never a 401/403). +// 3. HasPermissionAPI returns false -> request must be rejected with 403. +// 4. Admin session -> request must pass regardless of the +// HasPermissionAPI mock (admin bypasses the permission check). +// +// Handler mocks are kept minimal: we only stub what the first line of +// each handler touches so the test terminates quickly. A 403 is caught +// before any store call; a handler success (or a domain-level 404/conflict) +// proves the permission gate was cleared. + +import ( + "context" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/CUDly/internal/config" +) + +// --- helpers --------------------------------------------------------------- + +func userSessionFixture(userID string) *Session { + return &Session{UserID: userID, Role: "user"} +} + +func adminSessionFixture() *Session { + return &Session{UserID: "admin-uid", Role: "admin"} +} + +// authForUserWith returns a MockAuthService that: +// - validates "user-token" as the user session fixture +// - responds to HasPermissionAPI(ctx, userID, action, resource) with granted +// +// AssertExpectations is registered as a t.Cleanup so that every granted-path +// sub-test fails if HasPermissionAPI was never called (i.e. the gate was +// bypassed before the permission check ran). +func authForUserWith(ctx context.Context, t *testing.T, userID, action, resource string, granted bool) *MockAuthService { + t.Helper() + m := new(MockAuthService) + m.On("ValidateSession", ctx, "user-token").Return(userSessionFixture(userID), nil) + m.On("HasPermissionAPI", ctx, userID, action, resource).Return(granted, nil) + t.Cleanup(func() { m.AssertExpectations(t) }) + return m +} + +// authForAdmin returns a MockAuthService that validates "admin-token" as admin. +// AssertExpectations is registered as a t.Cleanup to verify ValidateSession was called. +func authForAdmin(ctx context.Context, t *testing.T) *MockAuthService { + t.Helper() + m := new(MockAuthService) + m.On("ValidateSession", ctx, "admin-token").Return(adminSessionFixture(), nil) + t.Cleanup(func() { m.AssertExpectations(t) }) + return m +} + +func reqWithBearer(token string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer " + token}, + } +} + +func reqWithBearerAndBody(token, body string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer " + token}, + Body: body, + } +} + +// assert403 verifies the error is a 403 ClientError. +func assert403(t *testing.T, err error) { + t.Helper() + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 403, ce.code, "expected 403, got %d: %s", ce.code, ce.message) +} + +// assertNotForbidden checks the error is nil or is NOT a 401/403. It allows +// domain-level errors (404, 409, 500) that prove the permission gate was +// cleared and the handler logic ran. +func assertNotForbidden(t *testing.T, err error) { + t.Helper() + if err == nil { + return + } + ce, ok := IsClientError(err) + if !ok { + return // non-ClientError domain error is fine + } + assert.NotEqual(t, 401, ce.code, "unexpected 401: permission gate should have passed") + assert.NotEqual(t, 403, ce.code, "unexpected 403: permission gate should have passed") +} + +// ---- Plans ---------------------------------------------------------------- + +// TestDeletePlan_PermissionGate verifies that DELETE /api/plans/{id} +// (previously AuthAdmin; now AuthUser + requirePermission("delete","plans")) +// correctly gates on the handler-level permission. +func TestDeletePlan_PermissionGate(t *testing.T) { + ctx := context.Background() + const userID = "11111111-1111-1111-1111-111111111111" + const planID = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + + t.Run("user with delete:plans can delete a plan", func(t *testing.T) { + mockAuth := authForUserWith(ctx, t, userID, "delete", "plans", true) + // requirePlanAccess calls GetAllowedAccountsAPI; stub it to allow all. + mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) + mockStore := new(MockConfigStore) + // After the permission gate the handler calls GetPurchasePlan then + // DeletePurchasePlan. Seed both so we don't crash on unexpected calls. + mockStore.GetPurchasePlanFn = func(_ context.Context, id string) (*config.PurchasePlan, error) { + return &config.PurchasePlan{ID: id}, nil + } + mockStore.On("DeletePurchasePlan", ctx, planID).Return(nil) + + h := &Handler{auth: mockAuth, config: mockStore} + _, err := h.deletePlan(ctx, reqWithBearer("user-token"), planID) + assertNotForbidden(t, err) + }) + + t.Run("user without delete:plans is rejected with 403", func(t *testing.T) { + mockAuth := authForUserWith(ctx, t, userID, "delete", "plans", false) + h := &Handler{auth: mockAuth, config: new(MockConfigStore)} + _, err := h.deletePlan(ctx, reqWithBearer("user-token"), planID) + assert403(t, err) + }) + + t.Run("admin bypasses permission check", func(t *testing.T) { + // Admin sessions short-circuit in getAllowedAccounts (role == "admin" returns + // nil without calling GetAllowedAccountsAPI), so do NOT register that expectation. + mockAuth := authForAdmin(ctx, t) + mockStore := new(MockConfigStore) + mockStore.GetPurchasePlanFn = func(_ context.Context, id string) (*config.PurchasePlan, error) { + return &config.PurchasePlan{ID: id}, nil + } + mockStore.On("DeletePurchasePlan", ctx, planID).Return(nil) + + h := &Handler{auth: mockAuth, config: mockStore} + _, err := h.deletePlan(ctx, reqWithBearer("admin-token"), planID) + assertNotForbidden(t, err) + }) +} + +// TestUpdatePlan_PermissionGate covers PUT /api/plans/{id}. +func TestUpdatePlan_PermissionGate(t *testing.T) { + ctx := context.Background() + const userID = "22222222-2222-2222-2222-222222222222" + const planID = "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" + const body = `{"name":"Updated","enabled":true,"provider":"aws","service":"ec2"}` + + t.Run("user with update:plans can update a plan", func(t *testing.T) { + mockAuth := authForUserWith(ctx, t, userID, "update", "plans", true) + mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) + mockStore := new(MockConfigStore) + mockStore.GetPurchasePlanFn = func(_ context.Context, id string) (*config.PurchasePlan, error) { + return &config.PurchasePlan{ID: id, Name: "Old"}, nil + } + // Use AnythingOfType because the plan struct is built inside updatePlan with + // computed timestamps that we can't predict here. + mockStore.On("UpdatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil) + + h := &Handler{auth: mockAuth, config: mockStore} + _, err := h.updatePlan(ctx, reqWithBearerAndBody("user-token", body), planID) + assertNotForbidden(t, err) + }) + + t.Run("user without update:plans is rejected with 403", func(t *testing.T) { + mockAuth := authForUserWith(ctx, t, userID, "update", "plans", false) + h := &Handler{auth: mockAuth, config: new(MockConfigStore)} + _, err := h.updatePlan(ctx, reqWithBearerAndBody("user-token", body), planID) + assert403(t, err) + }) +} + +// ---- Planned Purchases ---------------------------------------------------- + +// TestPausePlannedPurchase_PermissionGate covers POST /api/purchases/planned/{id}/pause. +// The handler requires update:purchases. This is the "update:purchases" example +// from the design comment. +func TestPausePlannedPurchase_PermissionGate(t *testing.T) { + ctx := context.Background() + const userID = "33333333-3333-3333-3333-333333333333" + const execID = "cccccccc-cccc-cccc-cccc-cccccccccccc" + + t.Run("user with update:purchases can pause a planned purchase", func(t *testing.T) { + mockAuth := authForUserWith(ctx, t, userID, "update", "purchases", true) + mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) + mockStore := new(MockConfigStore) + // requireExecutionAccess calls GetExecutionByID; stub a minimal row. + mockStore.On("GetExecutionByID", ctx, execID). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending"}, nil) + // TransitionExecutionStatus is called next; stub it. + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "running"}, "paused"). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "paused"}, nil) + + h := &Handler{auth: mockAuth, config: mockStore} + _, err := h.pausePlannedPurchase(ctx, reqWithBearer("user-token"), execID) + assertNotForbidden(t, err) + }) + + t.Run("user without update:purchases is rejected with 403", func(t *testing.T) { + mockAuth := authForUserWith(ctx, t, userID, "update", "purchases", false) + h := &Handler{auth: mockAuth, config: new(MockConfigStore)} + _, err := h.pausePlannedPurchase(ctx, reqWithBearer("user-token"), execID) + assert403(t, err) + }) + + t.Run("admin bypasses permission check", func(t *testing.T) { + // Admin sessions short-circuit in getAllowedAccounts (role == "admin" returns + // nil, IsUnrestrictedAccess returns true, requireExecutionAccess returns nil + // immediately without calling GetAllowedAccountsAPI or GetExecutionByID). + mockAuth := authForAdmin(ctx, t) + mockStore := new(MockConfigStore) + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "running"}, "paused"). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "paused"}, nil) + + h := &Handler{auth: mockAuth, config: mockStore} + _, err := h.pausePlannedPurchase(ctx, reqWithBearer("admin-token"), execID) + assertNotForbidden(t, err) + }) +} + +// TestDeletePlannedPurchase_PermissionGate covers DELETE /api/purchases/planned/{id}. +// The handler requires delete:purchases. +func TestDeletePlannedPurchase_PermissionGate(t *testing.T) { + ctx := context.Background() + const userID = "44444444-4444-4444-4444-444444444444" + const execID = "dddddddd-dddd-dddd-dddd-dddddddddddd" + + t.Run("user with delete:purchases can delete a planned purchase", func(t *testing.T) { + mockAuth := authForUserWith(ctx, t, userID, "delete", "purchases", true) + mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending"}, nil) + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled"). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "cancelled"}, nil) + + h := &Handler{auth: mockAuth, config: mockStore} + _, err := h.deletePlannedPurchase(ctx, reqWithBearer("user-token"), execID) + assertNotForbidden(t, err) + }) + + t.Run("user without delete:purchases is rejected with 403", func(t *testing.T) { + mockAuth := authForUserWith(ctx, t, userID, "delete", "purchases", false) + h := &Handler{auth: mockAuth, config: new(MockConfigStore)} + _, err := h.deletePlannedPurchase(ctx, reqWithBearer("user-token"), execID) + assert403(t, err) + }) +} + +// ---- RI Exchange ---------------------------------------------------------- + +// TestExecuteExchange_PermissionGate covers POST /api/ri-exchange/execute. +// The handler requires execute:purchases. +func TestExecuteExchange_PermissionGate(t *testing.T) { + ctx := context.Background() + const userID = "55555555-5555-5555-5555-555555555555" + + t.Run("user without execute:purchases is rejected with 403", func(t *testing.T) { + mockAuth := authForUserWith(ctx, t, userID, "execute", "purchases", false) + h := &Handler{auth: mockAuth} + body := `{"ri_ids":["ri-abc"],"targets":[{"offering_id":"of-1"}],"max_payment_due_usd":"1000"}` + _, err := h.executeExchange(ctx, reqWithBearerAndBody("user-token", body)) + assert403(t, err) + }) + + // Note: a full "granted" sub-test for executeExchange would require wiring + // the real AWS exchange client. We verify the permission gate passes by + // checking the error is NOT a 403 when the permission is granted; the + // AWS SDK call will fail with a non-403 (connection refused / 500). + t.Run("user with execute:purchases clears 403 gate", func(t *testing.T) { + mockAuth := authForUserWith(ctx, t, userID, "execute", "purchases", true) + h := &Handler{auth: mockAuth} + body := `{"ri_ids":["ri-abc"],"targets":[{"offering_id":"of-1"}],"max_payment_due_usd":"1000"}` + _, err := h.executeExchange(ctx, reqWithBearerAndBody("user-token", body)) + // The error will be from the AWS SDK (not a 403), proving the gate passed. + assertNotForbidden(t, err) + }) +} + +// TestUpdateRIExchangeConfig_PermissionGate covers PUT /api/ri-exchange/config. +// The handler requires update:config. +func TestUpdateRIExchangeConfig_PermissionGate(t *testing.T) { + ctx := context.Background() + const userID = "66666666-6666-6666-6666-666666666666" + + t.Run("user without update:config is rejected with 403", func(t *testing.T) { + mockAuth := authForUserWith(ctx, t, userID, "update", "config", false) + h := &Handler{auth: mockAuth} + body := `{"auto_exchange_enabled":false}` + _, err := h.updateRIExchangeConfig(ctx, reqWithBearerAndBody("user-token", body)) + assert403(t, err) + }) + + t.Run("user with update:config clears 403 gate", func(t *testing.T) { + mockAuth := authForUserWith(ctx, t, userID, "update", "config", true) + mockStore := new(MockConfigStore) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) + mockStore.On("SaveGlobalConfig", ctx, &config.GlobalConfig{}).Return(nil).Maybe() + h := &Handler{auth: mockAuth, config: mockStore} + body := `{"auto_exchange_enabled":false,"mode":"recommend","utilization_threshold":80,"max_payment_per_exchange_usd":"0","max_payment_daily_usd":"0"}` + _, err := h.updateRIExchangeConfig(ctx, reqWithBearerAndBody("user-token", body)) + assertNotForbidden(t, err) + }) +} + +// ---- Router-level gate (defence-in-depth) --------------------------------- + +// TestRouter_MutatingRoutes_RequireAuth confirms that the AuthUser check at +// the router level (defence-in-depth) still rejects completely unauthenticated +// callers before they reach the handler — even after the flip from AuthAdmin. +func TestRouter_MutatingRoutes_RequireAuth(t *testing.T) { + ctx := context.Background() + + routes := []struct { + method string + path string + }{ + {"POST", "/api/plans"}, + {"DELETE", "/api/plans/aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"}, + {"POST", "/api/purchases/execute"}, + {"POST", "/api/purchases/planned/bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb/pause"}, + {"DELETE", "/api/purchases/planned/cccccccc-cccc-cccc-cccc-cccccccccccc"}, + {"POST", "/api/ri-exchange/execute"}, + {"PUT", "/api/ri-exchange/config"}, + } + + for _, rt := range routes { + rt := rt + t.Run(rt.method+" "+rt.path, func(t *testing.T) { + mockAuth := new(MockAuthService) + h := &Handler{auth: mockAuth} + r := NewRouter(h) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{}, // no credentials + } + _, err := r.Route(ctx, rt.method, rt.path, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError for %s %s, got %T: %v", rt.method, rt.path, err, err) + assert.Equal(t, 401, ce.code, "unauthenticated request to %s %s should return 401", rt.method, rt.path) + }) + } +} diff --git a/internal/auth/service_group_test.go b/internal/auth/service_group_test.go index 27e9d2678..10d34308d 100644 --- a/internal/auth/service_group_test.go +++ b/internal/auth/service_group_test.go @@ -255,9 +255,11 @@ func TestService_GetUserPermissions(t *testing.T) { permissions, err := service.GetUserPermissions(ctx, "user-123") require.NoError(t, err) - // 9 = 6 read/plan-author + cancel-own:purchases (issue #46) + - // retry-own:purchases (issue #47) + approve-own:purchases (issue #286). - assert.Len(t, permissions, 9) + // 11 = 6 read/plan-author + delete:plans (PR-A #660) + // + update:purchases (PR-A #660) + // + cancel-own:purchases (issue #46) + // + retry-own:purchases (issue #47) + approve-own:purchases (issue #286). + assert.Len(t, permissions, 11) mockStore.AssertExpectations(t) }) @@ -314,9 +316,10 @@ func TestService_GetUserPermissions(t *testing.T) { permissions, err := service.GetUserPermissions(ctx, "user-123") require.NoError(t, err) - // 9 user (incl. cancel-own (#46) + retry-own (#47) + - // approve-own (#286):purchases) + 1 group1 + 1 group2 = 11 - assert.Len(t, permissions, 11) + // 11 user (incl. delete:plans (PR-A #660) + update:purchases (PR-A #660) + // + cancel-own (#46) + retry-own (#47) + approve-own (#286):purchases) + // + 1 group1 + 1 group2 = 13 + assert.Len(t, permissions, 13) mockStore.AssertExpectations(t) }) @@ -353,9 +356,11 @@ func TestService_GetUserPermissions(t *testing.T) { permissions, err := service.GetUserPermissions(ctx, "user-123") require.NoError(t, err) // Should have only user permissions, missing group is skipped. - // 9 = 6 read/plan-author + cancel-own:purchases (issue #46) + - // retry-own:purchases (issue #47) + approve-own:purchases (issue #286). - assert.Len(t, permissions, 9) + // 11 = 6 read/plan-author + delete:plans (PR-A #660) + // + update:purchases (PR-A #660) + // + cancel-own:purchases (issue #46) + // + retry-own:purchases (issue #47) + approve-own:purchases (issue #286). + assert.Len(t, permissions, 11) mockStore.AssertExpectations(t) }) @@ -432,9 +437,10 @@ func TestService_BuildAuthContext(t *testing.T) { assert.Contains(t, authCtx.AllowedAccounts, "111111111111") assert.Contains(t, authCtx.AllowedAccounts, "222222222222") assert.Contains(t, authCtx.AllowedAccounts, "333333333333") - // 9 user perms (incl. cancel-own (#46) + retry-own (#47) + - // approve-own (#286):purchases) + 1 group1 + 1 group2 = 11 - assert.Len(t, authCtx.Permissions, 11) + // 11 user perms (incl. delete:plans (PR-A #660) + update:purchases (PR-A #660) + // + cancel-own (#46) + retry-own (#47) + approve-own (#286):purchases) + // + 1 group1 + 1 group2 = 13 + assert.Len(t, authCtx.Permissions, 13) mockStore.AssertExpectations(t) }) @@ -456,10 +462,11 @@ func TestService_BuildAuthContext(t *testing.T) { require.NoError(t, err) assert.NotNil(t, authCtx) assert.Empty(t, authCtx.AllowedAccounts) - // 6 read/plan-author + cancel-own:purchases (issue #46) + - // retry-own:purchases (issue #47) + approve-own:purchases - // (issue #286) = 9. Only role-based permissions. - assert.Len(t, authCtx.Permissions, 9) + // 6 read/plan-author + delete:plans (PR-A #660) + update:purchases (PR-A #660) + // + cancel-own:purchases (issue #46) + // + retry-own:purchases (issue #47) + approve-own:purchases + // (issue #286) = 11. Only role-based permissions. + assert.Len(t, authCtx.Permissions, 11) mockStore.AssertExpectations(t) }) diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index f4ef5c732..bc6e4fc6e 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -661,9 +661,11 @@ func TestService_ErrorPaths(t *testing.T) { permissions, err := service.GetUserPermissions(ctx, "user-123") require.NoError(t, err) // Should still return user permissions even if group fetch fails. - // 9 = 6 read/plan-author + cancel-own:purchases (issue #46) + - // retry-own:purchases (issue #47) + approve-own:purchases (issue #286). - assert.Len(t, permissions, 9) + // 11 = 6 read/plan-author + delete:plans (PR-A #660) + // + update:purchases (PR-A #660) + // + cancel-own:purchases (issue #46) + // + retry-own:purchases (issue #47) + approve-own:purchases (issue #286). + assert.Len(t, permissions, 11) mockStore.AssertExpectations(t) }) diff --git a/internal/auth/types.go b/internal/auth/types.go index 6f5e419ec..5519cd763 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -394,6 +394,17 @@ func DefaultUserPermissions() []Permission { {Action: ActionView, Resource: ResourceHistory}, {Action: ActionCreate, Resource: ResourcePlans}, {Action: ActionUpdate, Resource: ResourcePlans}, + // delete:plans — every authenticated user can delete plans they + // have access to (PR-A of #660). The handler still requires + // requirePermission("delete", "plans") and the plan-access scope + // check, so only plans in the user's allowed accounts are + // reachable. + {Action: ActionDelete, Resource: ResourcePlans}, + // update:purchases — every authenticated user can pause, resume, + // and update planned purchase executions (PR-A of #660). The + // handler still requires requirePermission("update", "purchases") + // and the execution-access scope check. + {Action: ActionUpdate, Resource: ResourcePurchases}, // cancel-own:purchases — every authenticated user can cancel // pending purchase executions they created themselves (issue #46). // The handler still requires the execution to be in a cancellable diff --git a/internal/auth/types_test.go b/internal/auth/types_test.go index d83165a5f..430b9ad3c 100644 --- a/internal/auth/types_test.go +++ b/internal/auth/types_test.go @@ -16,10 +16,12 @@ func TestDefaultPermissions(t *testing.T) { t.Run("DefaultUserPermissions returns user access", func(t *testing.T) { perms := DefaultUserPermissions() - // 6 read/plan-author perms + cancel-own:purchases (issue #46) - // + retry-own:purchases (issue #47) + approve-own:purchases - // (issue #286) = 9. - assert.Len(t, perms, 9) + // 6 read/plan-author perms + delete:plans (PR-A #660) + // + update:purchases (PR-A #660) + // + cancel-own:purchases (issue #46) + // + retry-own:purchases (issue #47) + // + approve-own:purchases (issue #286) = 11. + assert.Len(t, perms, 11) actions := make(map[string]bool) for _, p := range perms { @@ -32,6 +34,8 @@ func TestDefaultPermissions(t *testing.T) { assert.True(t, actions[ActionView+":"+ResourceHistory]) assert.True(t, actions[ActionCreate+":"+ResourcePlans]) assert.True(t, actions[ActionUpdate+":"+ResourcePlans]) + assert.True(t, actions[ActionDelete+":"+ResourcePlans]) + assert.True(t, actions[ActionUpdate+":"+ResourcePurchases]) assert.True(t, actions[ActionCancelOwn+":"+ResourcePurchases]) assert.True(t, actions[ActionRetryOwn+":"+ResourcePurchases]) assert.True(t, actions[ActionApproveOwn+":"+ResourcePurchases])