diff --git a/frontend/src/__tests__/dashboard-ownership-950.test.ts b/frontend/src/__tests__/dashboard-ownership-950.test.ts index f69c46119..d4fc01ef9 100644 --- a/frontend/src/__tests__/dashboard-ownership-950.test.ts +++ b/frontend/src/__tests__/dashboard-ownership-950.test.ts @@ -101,14 +101,27 @@ const legacyPurchase = { created_by_user_id: undefined as string | undefined, }; -type StubUserOpts = { updateAny?: boolean; admin?: boolean; deletePurchases?: boolean }; +type StubUserOpts = { + updateAny?: boolean; + admin?: boolean; + deletePurchases?: boolean; + cancelOwn?: boolean; + cancelAny?: boolean; +}; const setUser = (id: string, opts: StubUserOpts = {}) => { const effectivePermissions: Array<{ action: string; resource: string }> = []; if (opts.admin) { effectivePermissions.push({ action: 'admin', resource: '*' }); - } else if (opts.deletePurchases !== false) { - // Standard user holds delete:purchases (PR #660 default). - effectivePermissions.push({ action: 'delete', resource: 'purchases' }); + } else { + if (opts.deletePurchases) { + effectivePermissions.push({ action: 'delete', resource: 'purchases' }); + } + if (opts.cancelOwn) { + effectivePermissions.push({ action: 'cancel-own', resource: 'purchases' }); + } + if (opts.cancelAny) { + effectivePermissions.push({ action: 'cancel-any', resource: 'purchases' }); + } } if (opts.updateAny) { effectivePermissions.push({ action: 'update-any', resource: 'purchases' }); @@ -141,19 +154,19 @@ describe('Dashboard upcoming-purchase ownership gating (issue #950)', () => { setupDom(); }); - test('creator sees the Cancel button on their own scheduled purchase', async () => { - setUser(CREATOR_ID); + test('delete:purchases creator sees the Cancel button on their own scheduled purchase', async () => { + setUser(CREATOR_ID, { deletePurchases: true }); (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [ownedPurchase] }); await loadDashboard(); expect(cancelBtns()).toHaveLength(1); }); - test('non-creator with the same verbs sees NO Cancel button (the bug)', async () => { - setUser(OTHER_ID); + test('delete:purchases non-creator sees NO Cancel button', async () => { + setUser(OTHER_ID, { deletePurchases: true }); (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [ownedPurchase] }); await loadDashboard(); expect(cancelBtns()).toHaveLength(0); - // The card still renders -- the operator sees the row and can click + // The card still renders -- the user sees the row and can click // "View Details", which is intentionally unrestricted. const viewBtns = document.querySelectorAll('[data-action="view-purchase"]'); expect(viewBtns).toHaveLength(1); @@ -166,7 +179,7 @@ describe('Dashboard upcoming-purchase ownership gating (issue #950)', () => { expect(cancelBtns()).toHaveLength(1); }); - test('admin sees Cancel on every row (admin:* covers delete:purchases)', async () => { + test('admin sees Cancel on every row (admin:* covers all verbs)', async () => { setUser(OTHER_ID, { admin: true }); (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [ownedPurchase, { ...ownedPurchase, execution_id: 'exec-2' }], @@ -175,17 +188,15 @@ describe('Dashboard upcoming-purchase ownership gating (issue #950)', () => { expect(cancelBtns()).toHaveLength(2); }); - test('legacy NULL-creator row shows no Cancel for a non-update-any user', async () => { - setUser(CREATOR_ID); + test('legacy NULL-creator row shows no Cancel for a non-full-scope user', async () => { + setUser(CREATOR_ID, { cancelOwn: true }); (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [legacyPurchase] }); await loadDashboard(); expect(cancelBtns()).toHaveLength(0); }); - test('user without delete:purchases sees no Cancel even on their own row', async () => { - // Read-only style: holds neither delete:purchases nor admin nor - // update-any. Even on their own row the button must stay hidden - // because the backend would reject the click on verb grounds. + test('user with no cancel/delete verb sees no Cancel even on their own row', async () => { + // Read-only style: holds only view:purchases -- no cancel-own, no delete. (state.getCurrentUser as jest.Mock).mockReturnValue({ id: CREATOR_ID, email: 'ro@example.com', @@ -197,3 +208,43 @@ describe('Dashboard upcoming-purchase ownership gating (issue #950)', () => { expect(cancelBtns()).toHaveLength(0); }); }); + +describe('Dashboard upcoming-purchase cancel-own gating (issue #1400)', () => { + beforeEach(() => { + jest.clearAllMocks(); + setupDom(); + }); + + test('Standard user (cancel-own) sees Cancel on their own scheduled purchase', async () => { + // Standard users hold cancel-own:purchases in DefaultUserPermissions. + // They must see Cancel on rows they created. + setUser(CREATOR_ID, { cancelOwn: true }); + (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [ownedPurchase] }); + await loadDashboard(); + expect(cancelBtns()).toHaveLength(1); + }); + + test('Standard user (cancel-own) does NOT see Cancel on another user\'s purchase', async () => { + setUser(OTHER_ID, { cancelOwn: true }); + (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [ownedPurchase] }); + await loadDashboard(); + expect(cancelBtns()).toHaveLength(0); + // View Details is still visible. + const viewBtns = document.querySelectorAll('[data-action="view-purchase"]'); + expect(viewBtns).toHaveLength(1); + }); + + test('Standard user (cancel-own) does NOT see Cancel on legacy NULL-creator row', async () => { + setUser(CREATOR_ID, { cancelOwn: true }); + (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [legacyPurchase] }); + await loadDashboard(); + expect(cancelBtns()).toHaveLength(0); + }); + + test('cancel-any holder sees Cancel on any row (full scope)', async () => { + setUser(OTHER_ID, { cancelAny: true }); + (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [ownedPurchase] }); + await loadDashboard(); + expect(cancelBtns()).toHaveLength(1); + }); +}); diff --git a/frontend/src/dashboard.ts b/frontend/src/dashboard.ts index ffab5b4db..409114741 100644 --- a/frontend/src/dashboard.ts +++ b/frontend/src/dashboard.ts @@ -389,27 +389,36 @@ export const __test__ = { sparklinePoints, attachSparkline, computeServiceStats // canCancelUpcomingPurchase returns true when the current session is // permitted to cancel the given upcoming purchase via the Dashboard -// widget (issue #950). UX gate only -- the backend -// authorizeExecutionManagement in internal/api/handler_purchases.go +// widget (issues #950, #1400). UX gate only -- the backend +// authorizePlannedPurchaseCancel in internal/api/handler_purchases.go // remains the security boundary; a false-positive surfaces as a 403 // toast rather than a successful mutation. // -// Mirrors canManageScheduledPurchase in plans.ts so the Plans page and -// the Dashboard widget agree on which Cancel buttons appear: -// * admin (admin:*) or update-any:purchases -> can cancel any row; -// * otherwise the row's created_by_user_id must match the current user; -// * legacy / scheduler-tick rows with undefined created_by_user_id -> -// no Cancel button for non-privileged users (out of reach without -// update-any). -// Additionally requires the base delete:purchases verb the backend -// handler asks for, mirroring the gate on the Plans page disable button. +// Full-scope (any row): admin (admin:*), cancel-any:purchases, or +// update-any:purchases. +// Creator-scope (own row only): delete:purchases OR cancel-own:purchases, +// PLUS non-null created_by_user_id matching the current user. +// delete:purchases mirrors the original ownership gate (issue #950). +// cancel-own:purchases enables Standard users to cancel their own +// upcoming purchases from the Home page (issue #1400). +// Legacy / scheduler-tick rows with undefined created_by_user_id -> +// no Cancel button for non-privileged users (out of reach without a +// full-scope verb). export function canCancelUpcomingPurchase(purchase: UpcomingPurchase): boolean { - if (!canAccess('delete', 'purchases')) return false; - if (canAccess('admin', '*') || canAccess('update-any', 'purchases')) return true; + // Full-scope: admin, cancel-any, or update-any grant unrestricted row access. + if ( + canAccess('admin', '*') || + canAccess('cancel-any', 'purchases') || + canAccess('update-any', 'purchases') + ) { + return true; + } + // Creator-scope: delete:purchases or cancel-own:purchases + creator match. const user = state.getCurrentUser(); if (!user) return false; if (!purchase.created_by_user_id) return false; - return purchase.created_by_user_id === user.id; + if (purchase.created_by_user_id !== user.id) return false; + return canAccess('delete', 'purchases') || canAccess('cancel-own', 'purchases'); } function renderUpcomingPurchases(purchases: UpcomingPurchase[]): void { diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index d09f16e09..96207c78b 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -335,12 +335,94 @@ func (h *Handler) runPlannedPurchase(ctx context.Context, req *events.LambdaFunc }, nil } +// requireDeleteOrCancelPurchasePermission validates the request session and +// returns it when the caller holds any of: delete:purchases (management), +// cancel-any:purchases (operator-scope cancel), or cancel-own:purchases +// (creator-scope cancel, issue #1400). Ownership is enforced separately by +// authorizePlannedPurchaseCancel; this gate is the minimum-verb check. +func (h *Handler) requireDeleteOrCancelPurchasePermission(ctx context.Context, req *events.LambdaFunctionURLRequest) (*Session, error) { + apiKey := extractAPIKey(req) + if h.checkAdminAPIKey(apiKey) { + return &Session{UserID: apiKeyAdminUserID}, nil + } + if h.auth == nil { + return nil, fmt.Errorf("authentication service not configured") + } + token := h.extractBearerToken(req) + if token == "" { + return nil, NewClientError(401, "no authorization token provided") + } + session, err := h.auth.ValidateSession(ctx, token) + if err != nil { + return nil, NewClientError(401, "invalid session") + } + for _, verb := range []string{auth.ActionDelete, auth.ActionCancelAny, auth.ActionCancelOwn} { + has, checkErr := h.auth.HasPermissionAPI(ctx, session.UserID, verb, auth.ResourcePurchases) + if checkErr != nil { + return nil, fmt.Errorf("permission check failed: %w", checkErr) + } + if has { + return session, nil + } + } + return nil, NewClientError(403, "permission denied: requires delete, cancel-any, or cancel-own on purchases") +} + +// authorizePlannedPurchaseCancel enforces the ownership + cancel permission +// check for DELETE /api/purchases/planned/{id} (issue #1400). It extends +// authorizeExecutionManagement to also accept cancel-any:purchases for +// full-scope cancel (without requiring delete:purchases), and allows the +// creator-scope path without a prior delete:purchases check. +// +// Gate logic: +// - stateless admin API key: always permitted. +// - update-any:purchases or cancel-any:purchases: permitted without an +// owner check (full-scope management and cancel-any operator paths). +// - otherwise: fetch the execution and require creator match. +// Legacy rows with a NULL creator are out of reach for non-privileged +// users (matching cancel-own / approve-own / retry-own behavior). +func (h *Handler) authorizePlannedPurchaseCancel(ctx context.Context, session *Session, executionID string) error { + if session.UserID == apiKeyAdminUserID { + return nil + } + if h.auth == nil { + return NewClientError(500, "authentication service not configured") + } + // update-any and cancel-any both grant full-scope access without an owner check. + for _, verb := range []string{auth.ActionUpdateAny, auth.ActionCancelAny} { + has, err := h.auth.HasPermissionAPI(ctx, session.UserID, verb, auth.ResourcePurchases) + if err != nil { + return fmt.Errorf("permission check failed: %w", err) + } + if has { + return nil + } + } + execution, err := h.config.GetExecutionByID(ctx, executionID) + if errors.Is(err, config.ErrNotFound) { + return errNotFound + } + if err != nil { + return fmt.Errorf("failed to get execution: %w", err) + } + // requireDeleteOrCancelPurchasePermission already validated the session; + // UserID is either the API-key sentinel (handled above) or a real user UUID + // from ValidateSession. The nil/empty guard here is therefore defensive. + if execution.CreatedByUserID == nil || *execution.CreatedByUserID != session.UserID { + return NewClientError(403, "permission denied: cannot cancel another user's scheduled purchase") + } + return nil +} + func (h *Handler) deletePlannedPurchase(ctx context.Context, req *events.LambdaFunctionURLRequest, executionID string) (*StatusResponse, error) { if err := validateUUID(executionID); err != nil { return nil, err } - session, err := h.requirePermission(ctx, req, "delete", "purchases") + // Accept delete:purchases (management), cancel-any:purchases, or + // cancel-own:purchases (issue #1400: Standard users must be able to + // cancel their own upcoming purchases from the Home page). + session, err := h.requireDeleteOrCancelPurchasePermission(ctx, req) if err != nil { return nil, err } @@ -348,7 +430,7 @@ func (h *Handler) deletePlannedPurchase(ctx context.Context, req *events.LambdaF if err != nil { return nil, err } - err = h.authorizeExecutionManagement(ctx, session, executionID) + err = h.authorizePlannedPurchaseCancel(ctx, session, executionID) if err != nil { return nil, err } diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 8efd3c374..23c2c07e1 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -3642,6 +3642,32 @@ func buildManageHandler(userID, creatorID string, hasUpdateAny bool) (*Handler, mockAuth.On("HasPermissionAPI", mock.Anything, userID, "execute", "purchases").Return(true, nil).Maybe() mockAuth.On("HasPermissionAPI", mock.Anything, userID, "delete", "purchases").Return(true, nil).Maybe() mockAuth.On("HasPermissionAPI", mock.Anything, userID, "update-any", "purchases").Return(hasUpdateAny, nil).Maybe() + // cancel-any is checked by authorizePlannedPurchaseCancel on the delete + // path when update-any is false; register with false so callers that + // use buildManageHandler for the delete path don't get an unexpected-call + // panic (issue #1400). + mockAuth.On("HasPermissionAPI", mock.Anything, userID, "cancel-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, userID).Return([]string{}, nil).Maybe() + + creator := creatorID + exec := &config.PurchaseExecution{ExecutionID: ownExecID, Status: "pending", CreatedByUserID: &creator} + mockConfig := new(MockConfigStore) + mockConfig.On("GetExecutionByID", mock.Anything, ownExecID).Return(exec, nil) + + return &Handler{config: mockConfig, auth: mockAuth}, mockConfig, mockAuth +} + +// buildCancelOwnHandler wires a Standard-user "user-token" session that holds +// cancel-own:purchases but NOT delete:purchases. Used to verify the issue #1400 +// fix: creator-scope planned-purchase cancel without the management delete verb. +func buildCancelOwnHandler(userID, creatorID string) (*Handler, *MockConfigStore, *MockAuthService) { //nolint:unparam // userID param intentional for future callers + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "user-token").Return(&Session{UserID: userID}, nil) + // Standard user: cancel-own only -- no delete, no cancel-any, no update-any. + mockAuth.On("HasPermissionAPI", mock.Anything, userID, "delete", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", mock.Anything, userID, "cancel-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", mock.Anything, userID, "cancel-own", "purchases").Return(true, nil).Maybe() + mockAuth.On("HasPermissionAPI", mock.Anything, userID, "update-any", "purchases").Return(false, nil).Maybe() mockAuth.On("GetAllowedAccountsAPI", mock.Anything, userID).Return([]string{}, nil).Maybe() creator := creatorID @@ -3696,6 +3722,34 @@ func TestHandler_deletePlannedPurchase_NonOwner_Rejected(t *testing.T) { mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } +// TestHandler_deletePlannedPurchase_CancelOwn_Creator_Allowed verifies that a +// Standard user with cancel-own:purchases (but NOT delete:purchases) can cancel +// their own planned purchase from the Home page (issue #1400). +func TestHandler_deletePlannedPurchase_CancelOwn_Creator_Allowed(t *testing.T) { + handler, mockConfig, _ := buildCancelOwnHandler(ownUserA, ownUserA) + canceled := &config.PurchaseExecution{ExecutionID: ownExecID, PlanID: "", Status: "canceled"} + mockConfig.On("TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "paused"}, "canceled", mock.Anything). + Return(canceled, nil) + + result, err := handler.deletePlannedPurchase(context.Background(), manageReq(), ownExecID) + require.NoError(t, err) + assert.Equal(t, "canceled", result.Status) +} + +// TestHandler_deletePlannedPurchase_CancelOwn_NonCreator_Rejected verifies +// that cancel-own:purchases does not let a Standard user cancel another user's +// planned purchase (issue #1400). +func TestHandler_deletePlannedPurchase_CancelOwn_NonCreator_Rejected(t *testing.T) { + handler, mockConfig, _ := buildCancelOwnHandler(ownUserA, ownUserB) + + _, err := handler.deletePlannedPurchase(context.Background(), manageReq(), ownExecID) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + func TestHandler_runPlannedPurchase_NonOwner_Rejected(t *testing.T) { handler, mockConfig, _ := buildManageHandler(ownUserA, ownUserB, false) diff --git a/internal/api/router_660_permission_flips_test.go b/internal/api/router_660_permission_flips_test.go index 0045d0d5b..1fcc8685d 100644 --- a/internal/api/router_660_permission_flips_test.go +++ b/internal/api/router_660_permission_flips_test.go @@ -289,6 +289,9 @@ func TestDeletePlannedPurchase_PermissionGate(t *testing.T) { mockAuth := authForUserWith(ctx, t, userID, "delete", "purchases", true) mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) mockAuth.On("HasPermissionAPI", ctx, userID, "update-any", "purchases").Return(false, nil) + // authorizePlannedPurchaseCancel also checks cancel-any when update-any + // is false (issue #1400); register it so the mock does not panic. + mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-any", "purchases").Return(false, nil) creator := userID mockStore := new(MockConfigStore) mockStore.On("GetExecutionByID", ctx, execID). @@ -305,6 +308,9 @@ func TestDeletePlannedPurchase_PermissionGate(t *testing.T) { mockAuth := authForUserWith(ctx, t, userID, "delete", "purchases", true) mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) mockAuth.On("HasPermissionAPI", ctx, userID, "update-any", "purchases").Return(false, nil) + // authorizePlannedPurchaseCancel also checks cancel-any when update-any + // is false (issue #1400); register it so the mock does not panic. + mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-any", "purchases").Return(false, nil) otherCreator := "99999999-9999-9999-9999-999999999999" mockStore := new(MockConfigStore) mockStore.On("GetExecutionByID", ctx, execID). @@ -317,11 +323,59 @@ func TestDeletePlannedPurchase_PermissionGate(t *testing.T) { }) t.Run("user without delete:purchases is rejected with 403", func(t *testing.T) { + // Without delete, cancel-any, or cancel-own the first gate rejects. mockAuth := authForUserWith(ctx, t, userID, "delete", "purchases", false) + mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-any", "purchases").Return(false, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-own", "purchases").Return(false, nil) h := &Handler{auth: mockAuth, config: new(MockConfigStore)} _, err := h.deletePlannedPurchase(ctx, reqWithBearer("user-token"), execID) assert403(t, err) }) + + t.Run("Standard user with cancel-own:purchases can cancel their own planned purchase (issue #1400)", func(t *testing.T) { + // Standard users hold cancel-own but not delete:purchases. They must be + // able to cancel their own upcoming purchases from the Home page widget. + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "user-token").Return(userSessionFixture(userID), nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "delete", "purchases").Return(false, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-any", "purchases").Return(false, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-own", "purchases").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "update-any", "purchases").Return(false, nil) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + creator := userID + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending", CreatedByUserID: &creator}, nil) + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "canceled", mock.Anything). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "canceled"}, nil) + + h := &Handler{auth: mockAuth, config: mockStore} + _, err := h.deletePlannedPurchase(ctx, reqWithBearer("user-token"), execID) + assertNotForbidden(t, err) + }) + + t.Run("Standard user with cancel-own:purchases cannot cancel another user's purchase (issue #1400)", func(t *testing.T) { + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "user-token").Return(userSessionFixture(userID), nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "delete", "purchases").Return(false, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-any", "purchases").Return(false, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-own", "purchases").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "update-any", "purchases").Return(false, nil) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + otherCreator := "99999999-9999-9999-9999-999999999999" + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending", CreatedByUserID: &otherCreator}, nil) + + h := &Handler{auth: mockAuth, config: mockStore} + _, err := h.deletePlannedPurchase(ctx, reqWithBearer("user-token"), execID) + assert403(t, err) + mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) } // ---- RI Exchange ----------------------------------------------------------