Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
83 changes: 67 additions & 16 deletions frontend/src/__tests__/dashboard-ownership-950.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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' });
Expand Down Expand Up @@ -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<HTMLButtonElement>('[data-action="view-purchase"]');
expect(viewBtns).toHaveLength(1);
Expand All @@ -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' }],
Expand All @@ -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',
Expand All @@ -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<HTMLButtonElement>('[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);
});
});
37 changes: 23 additions & 14 deletions frontend/src/dashboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
86 changes: 84 additions & 2 deletions internal/api/handler_purchases.go
Original file line number Diff line number Diff line change
Expand Up @@ -335,20 +335,102 @@ 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
}
err = h.requireExecutionAccess(ctx, session, executionID)
if err != nil {
return nil, err
}
err = h.authorizeExecutionManagement(ctx, session, executionID)
err = h.authorizePlannedPurchaseCancel(ctx, session, executionID)
if err != nil {
return nil, err
}
Expand Down
54 changes: 54 additions & 0 deletions internal/api/handler_purchases_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand Down
Loading
Loading