From a36c266dc40b952ec06acd4e15b7505001c4f71a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 21:28:32 +0200 Subject: [PATCH] fix(frontend): surface Cancel button for cancel-any operators (closes #158) Replace the user.role === 'admin' check in canCancelPendingRow with the canAccess predicate pattern from PR #590: canAccess('admin', '*') || canAccess('cancel-any', 'purchases') || (canAccess('cancel-own', 'purchases') && row.created_by_user_id === user.id). Removes the deferred-work caveat block. Adds history-cancel-permissions.test.ts covering the full 6-case permission matrix. --- .../__tests__/history-approve-button.test.ts | 18 +- .../__tests__/history-cancel-button.test.ts | 18 +- .../history-cancel-permissions.test.ts | 255 ++++++++++++++++++ frontend/src/history.ts | 24 +- 4 files changed, 297 insertions(+), 18 deletions(-) create mode 100644 frontend/src/__tests__/history-cancel-permissions.test.ts diff --git a/frontend/src/__tests__/history-approve-button.test.ts b/frontend/src/__tests__/history-approve-button.test.ts index b3f57fb76..827543bc9 100644 --- a/frontend/src/__tests__/history-approve-button.test.ts +++ b/frontend/src/__tests__/history-approve-button.test.ts @@ -66,7 +66,23 @@ import { getCurrentUser } from '../state'; import { ADMINISTRATORS_GROUP_ID } from '../permissions'; const ADMIN_USER = { id: 'admin-uuid', email: 'admin@example.com', groups: [ADMINISTRATORS_GROUP_ID] }; -const REG_USER = { id: 'user-uuid', email: 'user@example.com', groups: [] }; +// REG_USER carries the default-user effective permission set (approve-own +// + cancel-own + retry-own on purchases) so canAccess returns true for +// own-row actions without needing the bootstrap fetch. The previous +// `groups: []` shape relied on the pre-#917 role-string gate that no +// longer exists; the post-rebase canAccess only honors the loaded +// effectivePermissions, so the test fixture must populate it. +const REG_USER = { + id: 'user-uuid', + email: 'user@example.com', + groups: [], + effectivePermissions: [ + { action: 'approve-own', resource: 'purchases' }, + { action: 'cancel-own', resource: 'purchases' }, + { action: 'retry-own', resource: 'purchases' }, + { action: 'view', resource: 'history' }, + ], +}; const OTHER_UUID = 'other-uuid'; function setupDOM(): void { diff --git a/frontend/src/__tests__/history-cancel-button.test.ts b/frontend/src/__tests__/history-cancel-button.test.ts index d642e3dc6..bf3649a60 100644 --- a/frontend/src/__tests__/history-cancel-button.test.ts +++ b/frontend/src/__tests__/history-cancel-button.test.ts @@ -63,7 +63,23 @@ import { getCurrentUser } from '../state'; import { ADMINISTRATORS_GROUP_ID } from '../permissions'; const ADMIN_USER = { id: 'admin-uuid', email: 'admin@example.com', groups: [ADMINISTRATORS_GROUP_ID] }; -const REG_USER = { id: 'user-uuid', email: 'user@example.com', groups: [] }; +// REG_USER carries the default-user effective permission set (cancel-own +// + approve-own + retry-own on purchases) so canAccess('cancel-own', +// 'purchases') returns true without needing the bootstrap fetch. The +// previous `groups: []` shape relied on the pre-#917 role-string gate +// that no longer exists; the post-rebase canAccess only honors the +// loaded effectivePermissions, so the test fixture must populate it. +const REG_USER = { + id: 'user-uuid', + email: 'user@example.com', + groups: [], + effectivePermissions: [ + { action: 'cancel-own', resource: 'purchases' }, + { action: 'approve-own', resource: 'purchases' }, + { action: 'retry-own', resource: 'purchases' }, + { action: 'view', resource: 'history' }, + ], +}; const OTHER_UUID = 'other-uuid'; function setupDOM(): void { diff --git a/frontend/src/__tests__/history-cancel-permissions.test.ts b/frontend/src/__tests__/history-cancel-permissions.test.ts new file mode 100644 index 000000000..306462780 --- /dev/null +++ b/frontend/src/__tests__/history-cancel-permissions.test.ts @@ -0,0 +1,255 @@ +/** + * History inline Cancel button permission-gating tests (issue #158). + * + * Verifies that canCancelPendingRow surfaces the Cancel button for every + * session the backend authorizeSessionCancel would allow through, not just + * admins and cancel-own holders. + * + * Tested matrix (mirrors history-cancel-button.test.ts for the admin / + * cancel-own cases; adds the cancel-any operator-role case from issue #158): + * 1. admin sees Cancel on any pending row regardless of creator. + * 2. cancel-any:purchases non-admin sees Cancel on any pending row (NEW). + * 3. cancel-own:purchases non-admin sees Cancel only on their own row. + * 4. cancel-own:purchases non-admin does NOT see Cancel on another user's row. + * 5. no-cancel-permission user sees no Cancel buttons. + * 6. anonymous (no current user) sees no Cancel buttons. + * + * The permissions module is mocked so we can inject arbitrary permission sets + * without adding a built-in role that carries cancel-any. + */ + +import { loadHistory } from '../history'; + +jest.mock('../api', () => ({ + getHistory: jest.fn(), + cancelPurchase: jest.fn(), +})); + +jest.mock('../navigation', () => ({ + switchTab: jest.fn(), +})); + +jest.mock('../utils', () => ({ + formatCurrency: jest.fn((val) => `$${val || 0}`), + formatDate: jest.fn((val) => (val ? new Date(val).toLocaleDateString() : '')), + formatTerm: jest.fn((years) => (years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`)), + escapeHtml: jest.fn((str) => str || ''), + populateAccountFilter: jest.fn(() => Promise.resolve()), +})); + +jest.mock('../confirmDialog', () => ({ + confirmDialog: jest.fn(), +})); + +jest.mock('../toast', () => ({ + showToast: jest.fn(), +})); + +jest.mock('../state', () => ({ + getCurrentUser: jest.fn(), + getCurrentProvider: jest.fn().mockReturnValue(''), + setCurrentProvider: jest.fn(), + getCurrentAccountIDs: jest.fn().mockReturnValue([]), + setCurrentAccountIDs: jest.fn(), + subscribeProvider: jest.fn().mockReturnValue(() => {}), + subscribeAccount: jest.fn().mockReturnValue(() => {}), +})); + +// Mock permissions so we can inject arbitrary permission sets, including +// cancel-any which belongs to no built-in role in the current defaults. +// isAdmin is included because history.ts still relies on it for the +// approve / retry helpers (out of scope of this PR); leaving it out would +// surface as a TypeError inside renderApprovalQueue and short-circuit the +// render path the cancel-permission assertions depend on. +jest.mock('../permissions', () => ({ + canAccess: jest.fn(), + isAdmin: jest.fn().mockReturnValue(false), +})); + +import * as api from '../api'; +import { getCurrentUser } from '../state'; +import { canAccess, isAdmin } from '../permissions'; + +const ADMIN_ID = 'admin-uuid'; +const USER_ID = 'user-uuid'; +const OTHER_ID = 'other-uuid'; + +// Helper: configure canAccess to behave like a specific permission set. +// Also configures isAdmin to follow the same admin:* signal so the +// out-of-scope approve / retry helpers (which still call isAdmin) stay +// consistent with the canAccess gate this test exercises. +function setPermissions(perms: string[]): void { + const isAdminSet = perms.includes('admin:*'); + (canAccess as jest.Mock).mockImplementation((action: string, resource: string) => { + const key = `${action}:${resource}`; + if (isAdminSet) return true; + return perms.includes(key); + }); + (isAdmin as jest.Mock).mockReturnValue(isAdminSet); +} + +function setupDOM(): void { + while (document.body.firstChild) document.body.removeChild(document.body.firstChild); + + const mkInput = (id: string): HTMLInputElement => { + const el = document.createElement('input'); + el.type = 'date'; + el.id = id; + return el; + }; + const mkSelect = (id: string): HTMLSelectElement => { + const el = document.createElement('select'); + el.id = id; + const opt = document.createElement('option'); + opt.value = ''; + opt.textContent = 'All'; + el.appendChild(opt); + return el; + }; + const mkDiv = (id: string): HTMLDivElement => { + const el = document.createElement('div'); + el.id = id; + return el; + }; + + document.body.appendChild(mkInput('history-start')); + document.body.appendChild(mkInput('history-end')); + document.body.appendChild(mkSelect('history-provider-filter')); + document.body.appendChild(mkSelect('history-account-filter')); + document.body.appendChild(mkDiv('history-summary')); + document.body.appendChild(mkDiv('history-list')); + document.body.appendChild(mkDiv('purchases-approval-queue')); +} + +function makeRow(overrides: Record) { + return { + purchase_id: 'exec-1', + timestamp: '2024-01-15T00:00:00Z', + provider: 'aws', + service: 'ec2', + resource_type: 't3.medium', + region: 'us-east-1', + count: 1, + term: 1, + upfront_cost: 100, + estimated_savings: 50, + plan_name: '', + status: 'pending', + ...overrides, + }; +} + +describe('History Cancel button permission gating (issue #158)', () => { + beforeEach(() => { + setupDOM(); + jest.clearAllMocks(); + }); + + test('admin sees Cancel on every pending row regardless of creator', async () => { + (getCurrentUser as jest.Mock).mockReturnValue({ id: ADMIN_ID, email: 'admin@example.com', role: 'admin' }); + setPermissions(['admin:*']); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ purchase_id: 'exec-mine', created_by_user_id: ADMIN_ID }), + makeRow({ purchase_id: 'exec-other', created_by_user_id: OTHER_ID }), + makeRow({ purchase_id: 'exec-legacy', created_by_user_id: undefined }), + ], + }); + + await loadHistory(); + + const list = document.getElementById('history-list')!; + const ids = Array.from(list.querySelectorAll('.history-cancel-btn')) + .map((b) => b.dataset['cancelId']); + expect(ids).toEqual(expect.arrayContaining(['exec-mine', 'exec-other', 'exec-legacy'])); + expect(ids).toHaveLength(3); + }); + + test('cancel-any:purchases non-admin sees Cancel on any pending row (issue #158)', async () => { + (getCurrentUser as jest.Mock).mockReturnValue({ id: USER_ID, email: 'operator@example.com', role: 'operator' }); + setPermissions(['cancel-any:purchases']); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ purchase_id: 'exec-mine', created_by_user_id: USER_ID }), + makeRow({ purchase_id: 'exec-other', created_by_user_id: OTHER_ID }), + makeRow({ purchase_id: 'exec-legacy', created_by_user_id: undefined }), + ], + }); + + await loadHistory(); + + const list = document.getElementById('history-list')!; + const ids = Array.from(list.querySelectorAll('.history-cancel-btn')) + .map((b) => b.dataset['cancelId']); + expect(ids).toEqual(expect.arrayContaining(['exec-mine', 'exec-other', 'exec-legacy'])); + expect(ids).toHaveLength(3); + }); + + test('cancel-own:purchases user sees Cancel only on their own pending row', async () => { + (getCurrentUser as jest.Mock).mockReturnValue({ id: USER_ID, email: 'user@example.com', role: 'user' }); + setPermissions(['cancel-own:purchases']); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ purchase_id: 'exec-mine', created_by_user_id: USER_ID }), + makeRow({ purchase_id: 'exec-other', created_by_user_id: OTHER_ID }), + makeRow({ purchase_id: 'exec-legacy', created_by_user_id: undefined }), + ], + }); + + await loadHistory(); + + const list = document.getElementById('history-list')!; + const ids = Array.from(list.querySelectorAll('.history-cancel-btn')) + .map((b) => b.dataset['cancelId']); + expect(ids).toEqual(['exec-mine']); + }); + + test('cancel-own:purchases user does not see Cancel on another user row', async () => { + (getCurrentUser as jest.Mock).mockReturnValue({ id: USER_ID, email: 'user@example.com', role: 'user' }); + setPermissions(['cancel-own:purchases']); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ purchase_id: 'exec-other', created_by_user_id: OTHER_ID }), + ], + }); + + await loadHistory(); + + const list = document.getElementById('history-list')!; + const buttons = list.querySelectorAll('.history-cancel-btn'); + expect(buttons).toHaveLength(0); + }); + + test('no-cancel-permission user sees no Cancel buttons', async () => { + (getCurrentUser as jest.Mock).mockReturnValue({ id: USER_ID, email: 'readonly@example.com', role: 'readonly' }); + setPermissions(['view:history', 'view:purchases']); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ purchase_id: 'exec-mine', created_by_user_id: USER_ID }), + makeRow({ purchase_id: 'exec-other', created_by_user_id: OTHER_ID }), + ], + }); + + await loadHistory(); + + expect(document.querySelectorAll('.history-cancel-btn')).toHaveLength(0); + }); + + test('anonymous session (no current user) sees no Cancel buttons', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(null); + (canAccess as jest.Mock).mockReturnValue(false); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ created_by_user_id: USER_ID })], + }); + + await loadHistory(); + + expect(document.querySelectorAll('.history-cancel-btn')).toHaveLength(0); + }); +}); diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 1d25f6bac..89029291c 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -11,7 +11,7 @@ import { confirmDialog } from './confirmDialog'; import { buildApprovalDetailsBody } from './approval-details'; import { showToast } from './toast'; import { getCurrentUser } from './state'; -import { isAdmin } from './permissions'; +import { isAdmin, canAccess } from './permissions'; import { showSkeletonRows, teardownSkeleton } from './lib/skeleton'; import { getAccountName } from './recommendations'; @@ -381,29 +381,21 @@ function providerCell(p: HistoryPurchase): string { // 403 and the click handler turns that into a "Failed to cancel" toast. // // Heuristic: -// * admin → always yes; -// * non-admin matching the row's created_by_user_id → yes (cancel-own); -// * anyone else → no. -// -// Caveat: a non-admin role explicitly granted cancel-any:purchases (no -// such role exists by default; the verb is reserved for future operator -// roles) WILL be allowed by the backend but hidden by this helper. We -// don't surface that case because the frontend doesn't currently fetch -// the user's permission list, and adding a /me/permissions round-trip -// just to enable a button for a role nobody has is wasteful. If/when an -// operator role lands, extend User to carry permissions and broaden this -// check accordingly. +// * admin → always yes (canAccess('admin', '*')); +// * cancel-any:purchases → yes (operator roles, issue #158); +// * cancel-own:purchases + matching created_by_user_id → yes; +// * anyone else, or legacy row with no created_by_user_id → no. function canCancelPendingRow(p: HistoryPurchase): boolean { const status = (p.status || '').toLowerCase(); if (status !== 'pending' && status !== 'notified') return false; const user = getCurrentUser(); if (!user) return false; - if (isAdmin()) return true; - // Non-admin: only the original creator. Legacy rows with no + if (canAccess('admin', '*') || canAccess('cancel-any', 'purchases')) return true; + // cancel-own: only the original creator. Legacy rows with no // created_by_user_id can't be cancelled via this UI; the email-token // path remains the escape hatch. if (!p.created_by_user_id) return false; - return p.created_by_user_id === user.id; + return canAccess('cancel-own', 'purchases') && p.created_by_user_id === user.id; } // canApprovePendingRow returns true when the current session is permitted