diff --git a/cmd/gen-permissions/main.go b/cmd/gen-permissions/main.go index 1f883b621..ef5bf8730 100644 --- a/cmd/gen-permissions/main.go +++ b/cmd/gen-permissions/main.go @@ -1,6 +1,7 @@ // gen-permissions generates frontend/src/permissions.generated.ts from the // backend's DefaultAdminPermissions / DefaultUserPermissions / -// DefaultReadOnlyPermissions constants in internal/auth/types.go. +// DefaultReadOnlyPermissions / DefaultPurchaserPermissions constants in +// internal/auth/types.go. // // The generated file is imported by the hand-written // frontend/src/permissions.ts wrapper so the small data surface that @@ -61,8 +62,9 @@ func main() { buf.WriteString(`// CODE GENERATED by ` + "`go run ./cmd/gen-permissions`" + `. DO NOT EDIT MANUALLY. // // Source of truth: internal/auth/types.go (DefaultAdminPermissions, -// DefaultUserPermissions, DefaultReadOnlyPermissions). To regenerate after -// editing the Go defaults, run: +// DefaultUserPermissions, DefaultReadOnlyPermissions, +// DefaultPurchaserPermissions). To regenerate after editing the Go +// defaults, run: // // go run ./cmd/gen-permissions // @@ -81,6 +83,8 @@ func main() { render("USER_PERMS", collect(auth.DefaultUserPermissions()), &buf) buf.WriteString("\n") render("READONLY_PERMS", collect(auth.DefaultReadOnlyPermissions()), &buf) + buf.WriteString("\n") + render("PURCHASER_PERMS", collect(auth.DefaultPurchaserPermissions()), &buf) // Resolve the output path relative to the repo root. The generator is // always invoked from the repo root (the comment block on the package diff --git a/frontend/src/__tests__/history-approve-button.test.ts b/frontend/src/__tests__/history-approve-button.test.ts index 827543bc9..a6f5c4922 100644 --- a/frontend/src/__tests__/history-approve-button.test.ts +++ b/frontend/src/__tests__/history-approve-button.test.ts @@ -63,9 +63,20 @@ import * as api from '../api'; import { confirmDialog } from '../confirmDialog'; import { showToast } from '../toast'; import { getCurrentUser } from '../state'; -import { ADMINISTRATORS_GROUP_ID } from '../permissions'; - -const ADMIN_USER = { id: 'admin-uuid', email: 'admin@example.com', groups: [ADMINISTRATORS_GROUP_ID] }; +import { ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID } from '../permissions'; + +// Admin user includes Purchaser membership (mirrors the auto-migration for +// existing admins on first deploy of issue #923). approve-any:purchases is +// carved out of admin:* and requires Purchaser group membership. +const ADMIN_USER = { id: 'admin-uuid', email: 'admin@example.com', groups: [ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID] }; +// Admin without Purchaser membership and without effectivePermissions +// for any carved-out spending verb. Issue #923 explicitly carves +// approve-any:purchases / retry-any:purchases / execute:purchases OUT +// of admin:*, so this user MUST NOT see Approve / Retry buttons on +// rows they did not create. Regression guard for CR #924 F5 — if a +// future refactor reintroduces isAdmin() as the gate, this test +// catches it. +const ADMIN_NO_PURCH = { id: 'admin-no-purch-uuid', email: 'admin-no-purch@example.com', groups: [ADMINISTRATORS_GROUP_ID] }; // 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 @@ -367,4 +378,33 @@ describe('History inline Approve button (issue #286)', () => { expect(cancelBtn?.disabled).toBe(false); expect(showToast).toHaveBeenCalledWith(expect.objectContaining({ kind: 'error' })); }); + + test('admin WITHOUT Purchaser membership does not see Approve on rows they did not create (CR #924 F5)', async () => { + // Issue #923 + CR #924 F5: approve-any:purchases is carved out of + // admin:*. canApprovePendingRow must gate on + // canAccess('approve-any', 'purchases'), NOT on isAdmin() or + // isPurchaser() group membership alone. A bare admin (no Purchaser + // group, no effectivePermissions yet) is exactly the case where + // the carve-out matters: the legacy implementation would have + // shown Approve on every pending row. + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_NO_PURCH); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ purchase_id: 'exec-mine', created_by_user_id: ADMIN_NO_PURCH.id }), + makeRow({ purchase_id: 'exec-other', created_by_user_id: OTHER_UUID }), + makeRow({ purchase_id: 'exec-legacy', created_by_user_id: undefined }), + ], + }); + + await loadHistory(); + + // Scope to the history list (not the approval queue card). + const list = document.getElementById('history-list')!; + const buttons = list.querySelectorAll('.history-approve-btn'); + const ids = Array.from(buttons).map((b) => b.dataset['approveId']); + // Approve renders only via the approve-own fallback (matching + // created_by_user_id), NOT approve-any. + expect(ids).toEqual(['exec-mine']); + }); }); diff --git a/frontend/src/__tests__/history-retry-button.test.ts b/frontend/src/__tests__/history-retry-button.test.ts index 6a56fbe8e..ba4f0cc15 100644 --- a/frontend/src/__tests__/history-retry-button.test.ts +++ b/frontend/src/__tests__/history-retry-button.test.ts @@ -69,10 +69,18 @@ import * as api from '../api'; import { confirmDialog } from '../confirmDialog'; import { showToast } from '../toast'; import { getCurrentUser } from '../state'; -import { ADMINISTRATORS_GROUP_ID } from '../permissions'; +import { ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID } from '../permissions'; -const ADMIN_USER = { id: 'admin-uuid', email: 'admin@example.com', groups: [ADMINISTRATORS_GROUP_ID] }; +// Admin user includes Purchaser membership (mirrors the auto-migration for +// existing admins on first deploy of issue #923). retry-any:purchases is +// carved out of admin:* and requires Purchaser group membership. +const ADMIN_USER = { id: 'admin-uuid', email: 'admin@example.com', groups: [ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID] }; const REG_USER = { id: 'user-uuid', email: 'user@example.com', groups: [] }; +// Admin WITHOUT Purchaser membership. retry-any:purchases is carved +// out of admin:* by issue #923, so this user MUST NOT see Retry on +// rows they did not create. Regression guard for CR #924 F5 -- if a +// future refactor reintroduces isAdmin() as the gate, this catches it. +const ADMIN_NO_PURCH = { id: 'admin-no-purch-uuid', email: 'admin-no-purch@example.com', groups: [ADMINISTRATORS_GROUP_ID] }; const OTHER_UUID = 'other-uuid'; function setupDOM(): void { @@ -430,4 +438,31 @@ describe('History inline Retry button (issue #47)', () => { expect(showToast).toHaveBeenCalledWith(expect.objectContaining({ kind: 'error' })); expect(btn?.disabled).toBe(false); }); + + test('admin WITHOUT Purchaser membership does not see Retry on rows they did not create (CR #924 F5)', async () => { + // Issue #923 + CR #924 F5: retry-any:purchases is carved out of + // admin:*. canRetryFailedRow must gate on + // canAccess('retry-any', 'purchases'), NOT on isAdmin() or + // isPurchaser() group membership alone. A bare admin (no Purchaser + // group, no effectivePermissions yet) is exactly the case where + // the carve-out matters: the legacy implementation would have + // shown Retry on every failed row. + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_NO_PURCH); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ purchase_id: 'fail-mine', created_by_user_id: ADMIN_NO_PURCH.id }), + makeRow({ purchase_id: 'fail-other', created_by_user_id: OTHER_UUID }), + makeRow({ purchase_id: 'fail-legacy', created_by_user_id: undefined }), + ], + }); + + await loadHistory(); + + const buttons = document.querySelectorAll('.history-retry-btn'); + const ids = Array.from(buttons).map((b) => b.dataset['retryId']); + // Retry renders only via the retry-own fallback (matching + // created_by_user_id), NOT retry-any. + expect(ids).toEqual(['fail-mine']); + }); }); diff --git a/frontend/src/__tests__/permissions.test.ts b/frontend/src/__tests__/permissions.test.ts index 3052d961d..dd4ab9292 100644 --- a/frontend/src/__tests__/permissions.test.ts +++ b/frontend/src/__tests__/permissions.test.ts @@ -4,7 +4,9 @@ * Issue #917: canAccess() now consults user.effectivePermissions when * populated (fetched from GET /api/auth/me/permissions on bootstrap). * When effectivePermissions is absent (loading race) it falls back to - * the group-membership admin check: admin passes, others block. + * group-membership checks: admin passes everywhere EXCEPT the three + * money-spending verbs carved out by issue #923, which require + * explicit Purchaser-group membership. * * isAdmin() returns true when the current user is a member of the * Administrators group (UUID 00000000-0000-5000-8000-000000000001). @@ -12,7 +14,7 @@ * getRolePermissions() is kept for the effective-permissions display in * the admin Users page and still returns the same sets as before. */ -import { canAccess, getRolePermissions, isAdmin, ADMINISTRATORS_GROUP_ID } from '../permissions'; +import { canAccess, getRolePermissions, isAdmin, isPurchaser, ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID } from '../permissions'; import type { PermissionEntry } from '../api/types'; jest.mock('../state', () => ({ @@ -22,7 +24,11 @@ jest.mock('../state', () => ({ import * as state from '../state'; const ADMIN_GID = ADMINISTRATORS_GROUP_ID; -const STD_GID = '00000000-0000-5000-8000-000000000005'; +// Standard Users group seeded by migration 000057. Must NOT collide with +// PURCHASER_GROUP_ID ('...005'); otherwise the fallback path (effectivePermissions +// absent) would treat the standard-user fixture as a Purchaser and let the +// carved-out money-spending verbs through (CR finding, PR #924). +const STD_GID = '00000000-0000-5000-8000-000000000002'; const RO_GID = '00000000-0000-5000-8000-000000000006'; const mockUserWithGroups = (groups: string[], effectivePermissions?: PermissionEntry[]) => { @@ -126,16 +132,45 @@ describe('permissions', () => { }); describe('canAccess - fallback (effectivePermissions absent)', () => { - test('Administrators group member passes all checks via group-membership fallback', () => { + test('Administrators group member passes non-spending checks via group-membership fallback', () => { mockUserWithGroups([ADMIN_GID]); expect(canAccess('admin', '*')).toBe(true); expect(canAccess('view', 'users')).toBe(true); expect(canAccess('delete', 'plans')).toBe(true); - expect(canAccess('execute', 'purchases')).toBe(true); + expect(canAccess('view', 'accounts')).toBe(true); + // execute:ri-exchange is NOT carved out of admin:* (issue #660 split it + // from execute:purchases), so an admin-only member still passes it. expect(canAccess('execute', 'ri-exchange')).toBe(true); + // execute:purchases is carved out of admin:* and requires Purchaser membership. + expect(canAccess('execute', 'purchases')).toBe(false); + expect(canAccess('approve-any', 'purchases')).toBe(false); + expect(canAccess('retry-any', 'purchases')).toBe(false); + }); + + test('Administrators + Purchaser group member passes all checks including spending', () => { + mockUserWithGroups([ADMIN_GID, PURCHASER_GROUP_ID]); + expect(canAccess('admin', '*')).toBe(true); + expect(canAccess('view', 'users')).toBe(true); + expect(canAccess('delete', 'plans')).toBe(true); + expect(canAccess('execute', 'purchases')).toBe(true); + expect(canAccess('approve-any', 'purchases')).toBe(true); + expect(canAccess('retry-any', 'purchases')).toBe(true); expect(canAccess('view', 'accounts')).toBe(true); }); + test('Purchaser-only (no admin) passes carved-out verbs but not other admin actions', () => { + mockUserWithGroups([PURCHASER_GROUP_ID]); + // Purchaser group grants the three carved-out verbs. + expect(canAccess('execute', 'purchases')).toBe(true); + expect(canAccess('approve-any', 'purchases')).toBe(true); + expect(canAccess('retry-any', 'purchases')).toBe(true); + // But Purchaser membership alone is not admin -- non-spending admin + // actions remain denied during the fallback path. + expect(canAccess('admin', '*')).toBe(false); + expect(canAccess('view', 'users')).toBe(false); + expect(canAccess('delete', 'plans')).toBe(false); + }); + test('Standard Users group member blocked during loading (effectivePermissions undefined)', () => { // Before /me/permissions returns, non-admins are blocked (fails closed). mockUserWithGroups([STD_GID]); @@ -219,7 +254,11 @@ describe('permissions', () => { expect(canAccess('execute', 'purchases')).toBe(false); }); - test('admin wildcard in effectivePermissions grants everything', () => { + test('admin wildcard in effectivePermissions grants everything except carved-out spending verbs', () => { + // Mirror the backend (issue #923): admin:* covers every check EXCEPT + // execute/approve-any/retry-any on purchases. Those require an + // explicit (action, resource) entry from a non-admin group such + // as Purchaser. const perms: PermissionEntry[] = [ { action: 'admin', resource: '*' }, ]; @@ -227,7 +266,57 @@ describe('permissions', () => { expect(canAccess('admin', '*')).toBe(true); expect(canAccess('delete', 'plans')).toBe(true); expect(canAccess('view', 'users')).toBe(true); + // Carved-out verbs deny even with admin:* in the effective set. + expect(canAccess('execute', 'purchases')).toBe(false); + expect(canAccess('approve-any', 'purchases')).toBe(false); + expect(canAccess('retry-any', 'purchases')).toBe(false); + }); + + test('admin wildcard plus explicit Purchaser grants in effectivePermissions cover everything', () => { + // What the backend returns for an Administrators + Purchaser user: + // admin:* (from Administrators) PLUS the three explicit verbs (from + // Purchaser). The explicit entries cover the carve-out. + const perms: PermissionEntry[] = [ + { action: 'admin', resource: '*' }, + { action: 'execute', resource: 'purchases' }, + { action: 'approve-any', resource: 'purchases' }, + { action: 'retry-any', resource: 'purchases' }, + ]; + mockUserWithGroups([ADMIN_GID, PURCHASER_GROUP_ID], perms); + expect(canAccess('admin', '*')).toBe(true); + expect(canAccess('delete', 'plans')).toBe(true); + expect(canAccess('view', 'users')).toBe(true); + expect(canAccess('execute', 'purchases')).toBe(true); + expect(canAccess('approve-any', 'purchases')).toBe(true); + expect(canAccess('retry-any', 'purchases')).toBe(true); + }); + + test('Purchaser explicit grants in effectivePermissions allow spending without admin:*', () => { + // A Purchaser-only user (no admin) gets just the seven verbs in + // DefaultPurchaserPermissions. canAccess must allow the spending + // verbs and the four view verbs, and deny everything else. + const perms: PermissionEntry[] = [ + { action: 'execute', resource: 'purchases' }, + { action: 'approve-any', resource: 'purchases' }, + { action: 'retry-any', resource: 'purchases' }, + { action: 'view', resource: 'recommendations' }, + { action: 'view', resource: 'plans' }, + { action: 'view', resource: 'purchases' }, + { action: 'view', resource: 'history' }, + ]; + mockUserWithGroups([PURCHASER_GROUP_ID], perms); expect(canAccess('execute', 'purchases')).toBe(true); + expect(canAccess('approve-any', 'purchases')).toBe(true); + expect(canAccess('retry-any', 'purchases')).toBe(true); + expect(canAccess('view', 'recommendations')).toBe(true); + expect(canAccess('view', 'plans')).toBe(true); + expect(canAccess('view', 'purchases')).toBe(true); + expect(canAccess('view', 'history')).toBe(true); + // Non-Purchaser admin actions remain denied. + expect(canAccess('admin', '*')).toBe(false); + expect(canAccess('delete', 'plans')).toBe(false); + expect(canAccess('view', 'users')).toBe(false); + expect(canAccess('cancel-any', 'purchases')).toBe(false); }); test('empty effectivePermissions array denies everything', () => { @@ -254,5 +343,117 @@ describe('permissions', () => { expect(canAccess('view', 'recommendations')).toBe(false); expect(canAccess('admin', '*')).toBe(false); }); + + test('custom (non-seeded) group with carved-out grants in effectivePermissions allows spending', () => { + // CR #924 F4 + F5 regression: a custom group that does not include + // PURCHASER_GROUP_ID but DOES carry execute/approve-any/retry-any + // on purchases must satisfy canAccess for those verbs. The seeded + // Purchaser group is not the only legitimate source of the + // carve-out; the backend already allows this shape via the + // group's effectivePermissions, so the frontend must agree. + const customGid = '00000000-0000-5000-8000-00000000abcd'; + const perms: PermissionEntry[] = [ + { action: 'execute', resource: 'purchases' }, + { action: 'approve-any', resource: 'purchases' }, + { action: 'retry-any', resource: 'purchases' }, + { action: 'view', resource: 'recommendations' }, + { action: 'view', resource: 'plans' }, + { action: 'view', resource: 'purchases' }, + { action: 'view', resource: 'history' }, + ]; + mockUserWithGroups([customGid], perms); + // The three carved-out spending verbs are granted by the explicit + // entries even without PURCHASER_GROUP_ID membership. + expect(canAccess('execute', 'purchases')).toBe(true); + expect(canAccess('approve-any', 'purchases')).toBe(true); + expect(canAccess('retry-any', 'purchases')).toBe(true); + expect(canAccess('view', 'recommendations')).toBe(true); + expect(canAccess('view', 'history')).toBe(true); + // Non-purchaser admin actions remain denied. + expect(canAccess('admin', '*')).toBe(false); + expect(canAccess('delete', 'plans')).toBe(false); + expect(canAccess('view', 'users')).toBe(false); + expect(canAccess('cancel-any', 'purchases')).toBe(false); + }); + }); + + describe('isPurchaser', () => { + test('seeded Purchaser group member (no effectivePermissions yet) returns true via fallback', () => { + // Pre-bootstrap loading window: effectivePermissions not yet + // populated. The helper falls back to seeded group membership. + mockUserWithGroups([PURCHASER_GROUP_ID]); + expect(isPurchaser()).toBe(true); + }); + + test('user without Purchaser group and no effectivePermissions returns false', () => { + mockUserWithGroups([ADMIN_GID]); // admin only + expect(isPurchaser()).toBe(false); + }); + + test('explicit execute:purchases grant in effectivePermissions (custom group) returns true', () => { + // CR #924 F4: isPurchaser() must reflect effective permissions, + // not just seeded group ID. A user whose custom group grants + // execute:purchases is a spender even without PURCHASER_GROUP_ID. + const customGid = '00000000-0000-5000-8000-00000000beef'; + mockUserWithGroups([customGid], [ + { action: 'execute', resource: 'purchases' }, + { action: 'view', resource: 'recommendations' }, + ]); + expect(isPurchaser()).toBe(true); + }); + + test('explicit approve-any:purchases grant returns true', () => { + const customGid = '00000000-0000-5000-8000-00000000cafe'; + mockUserWithGroups([customGid], [ + { action: 'approve-any', resource: 'purchases' }, + ]); + expect(isPurchaser()).toBe(true); + }); + + test('explicit retry-any:purchases grant returns true', () => { + const customGid = '00000000-0000-5000-8000-00000000face'; + mockUserWithGroups([customGid], [ + { action: 'retry-any', resource: 'purchases' }, + ]); + expect(isPurchaser()).toBe(true); + }); + + test('wildcard resource on a carved-out action grants Purchaser (matches canAccess semantics)', () => { + // canAccess('execute', 'purchases') returns true for a + // {execute, *} entry. isPurchaser() must agree so the two helpers + // do not diverge when an unusual-but-legal permission shape + // arrives from the backend. + const customGid = '00000000-0000-5000-8000-00000000d00d'; + mockUserWithGroups([customGid], [ + { action: 'execute', resource: '*' }, + ]); + expect(isPurchaser()).toBe(true); + }); + + test('admin:* in effectivePermissions WITHOUT explicit carved-out grants returns false', () => { + // The backend carves the three spending verbs OUT of admin:*. + // isPurchaser() must reflect that carve-out: a user whose + // effectivePermissions are admin:* only (no explicit carve-out + // entries from Purchaser membership) is NOT a spender. Note that + // {admin, *} does NOT match {execute, *} / {approve-any, *} / + // {retry-any, *} because the actions differ. + mockUserWithGroups([ADMIN_GID], [ + { action: 'admin', resource: '*' }, + ]); + expect(isPurchaser()).toBe(false); + }); + + test('empty effectivePermissions array returns false even with PURCHASER_GROUP_ID', () => { + // Loading is complete (effectivePermissions is defined and + // empty); group membership without backend confirmation is not + // enough. + mockUserWithGroups([PURCHASER_GROUP_ID], []); + expect(isPurchaser()).toBe(false); + }); + + test('null user (logged out) returns false', () => { + mockNoUser(); + expect(isPurchaser()).toBe(false); + }); }); }); diff --git a/frontend/src/__tests__/recommendations-permissions.test.ts b/frontend/src/__tests__/recommendations-permissions.test.ts index 7ef6fdcca..fab581dcb 100644 --- a/frontend/src/__tests__/recommendations-permissions.test.ts +++ b/frontend/src/__tests__/recommendations-permissions.test.ts @@ -80,11 +80,26 @@ jest.mock('../plans', () => ({ })); import * as state from '../state'; -import { ADMINISTRATORS_GROUP_ID } from '../permissions'; +import { ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID } from '../permissions'; const mockUser = (role: string | null) => { + // 'admin' represents a fully-capable admin: Administrators + Purchaser + // (mirrors the auto-migration that adds existing admins to Purchaser on + // first deploy of issue #923). Tests that want to assert admin-alone + // behaviour (no spending access) should call mockUserWithGroups directly. (state.getCurrentUser as jest.Mock).mockReturnValue( - role === null ? null : { id: 'u', email: 'u@example.com', groups: role === 'admin' ? [ADMINISTRATORS_GROUP_ID] : [] }, + role === null + ? null + : { id: 'u', email: 'u@example.com', groups: role === 'admin' ? [ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID] : [] }, + ); +}; + +// Direct group-set mocking for tests that need to assert specific +// group combinations (e.g. admin WITHOUT Purchaser for the carve-out +// regression checks per CR #924 F5). +const mockUserWithGroups = (groups: string[], effectivePermissions?: { action: string; resource: string }[]) => { + (state.getCurrentUser as jest.Mock).mockReturnValue( + { id: 'u', email: 'u@example.com', groups, effectivePermissions }, ); }; @@ -170,6 +185,49 @@ describe('Recommendations action-box permission gating (issue #365)', () => { expect(plan.hidden).toBe(true); }); + test('admin WITHOUT Purchaser hides Purchase, keeps Create Plan, shows no-Purchaser notice (CR #924 F3)', async () => { + // Issue #923 + CR #924 F3: execute:purchases is carved out of + // admin:*. A bare admin (no Purchaser group, no effectivePermissions + // yet) sees the Create Plan button (create:plans is NOT carved out) + // but NOT the Purchase one-off button. The no-Purchaser banner must + // appear, driven by canAccess('execute', 'purchases') so it stays + // in lockstep with the Purchase CTA. + mockUserWithGroups([ADMINISTRATORS_GROUP_ID]); + await loadRecommendations(); + const purchase = document.getElementById('bulk-purchase-btn') as HTMLButtonElement; + const plan = document.getElementById('create-plan-btn') as HTMLButtonElement; + expect(purchase).not.toBeNull(); + expect(plan).not.toBeNull(); + expect(purchase.hidden).toBe(true); + expect(plan.hidden).toBe(false); + // No-Purchaser banner present. + const actionBox = document.getElementById('recommendations-action-box')!; + const banners = actionBox.querySelectorAll('.info-banner'); + expect(banners.length).toBe(1); + expect(banners[0]?.textContent).toContain('You can view but not execute purchases'); + }); + + test('custom (non-seeded) group with explicit execute:purchases shows Purchase + no banner (CR #924 F3/F4)', async () => { + // CR #924 F3 + F4: the banner must NOT appear when a custom group + // (not PURCHASER_GROUP_ID) carries execute:purchases via + // effectivePermissions. The previous isPurchaser()-only predicate + // would have shown the contradictory "you can view but not execute" + // notice alongside a live Purchase CTA. + const customGid = '00000000-0000-5000-8000-00000000abcd'; + mockUserWithGroups([customGid], [ + { action: 'execute', resource: 'purchases' }, + { action: 'create', resource: 'plans' }, + { action: 'view', resource: 'recommendations' }, + ]); + await loadRecommendations(); + const purchase = document.getElementById('bulk-purchase-btn') as HTMLButtonElement; + expect(purchase.hidden).toBe(false); + // No banner -- the predicate matches the live CTA. + const actionBox = document.getElementById('recommendations-action-box')!; + const banners = actionBox.querySelectorAll('.info-banner'); + expect(banners.length).toBe(0); + }); + test('the action-box capacity input stays visible for all sessions', async () => { // Non-mutating elements stay visible regardless of group membership; // only the action CTAs gate on permissions. diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index a8a1a80b5..a8634a6b0 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -416,6 +416,9 @@ export interface APIGroup { description: string; permissions: Permission[]; allowed_accounts?: string[]; + // system_managed groups are seeded by migrations; they cannot be + // renamed or deleted via the UI (only membership can change). + system_managed?: boolean; created_at?: string; updated_at?: string; } diff --git a/frontend/src/history.ts b/frontend/src/history.ts index ac3029260..c0a204d80 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, canAccess } from './permissions'; +import { canAccess } from './permissions'; import { showSkeletonRows, teardownSkeleton } from './lib/skeleton'; import { getAccountName } from './recommendations'; @@ -406,22 +406,26 @@ function canCancelPendingRow(p: HistoryPurchase): boolean { // false-positive here surfaces as a 403 toast on click rather than a // successful approve. // -// Heuristic mirrors canCancelPendingRow: +// Heuristic: // * status must be "pending" or "notified"; -// * admin → always yes; -// * non-admin matching the row's created_by_user_id → yes (approve-own); -// * legacy rows with NULL created_by_user_id → no (the email-token path -// remains the escape hatch). -// -// As with canCancelPendingRow, we don't surface the approve-any verb -// because no default role grants it; if/when an operator role lands -// with approve-any, broaden this check accordingly. +// * any session with approve-any:purchases (carved-out admin verb, +// seeded on Purchaser group; can also come from a custom group via +// effectivePermissions) → approve-any; +// * otherwise the row's created_by_user_id must match the current +// user (approve-own); +// * legacy rows with NULL created_by_user_id → no (the email-token +// path remains the escape hatch). function canApprovePendingRow(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; + // approve-any:purchases is carved out of admin:* (issue #923) and is + // granted by the seeded Purchaser group OR any custom group that + // explicitly lists the verb in effectivePermissions. Gate on the + // verb directly so a non-seeded role with the same grant still + // approves rows the backend would also let through. + if (canAccess('approve-any', 'purchases')) return true; if (!p.created_by_user_id) return false; return p.created_by_user_id === user.id; } @@ -431,15 +435,18 @@ function canApprovePendingRow(p: HistoryPurchase): boolean { // (issue #47). UX gate only — the backend authorizeSessionRetry in // internal/api/handler_purchases.go remains the security boundary. // -// Heuristic mirrors canCancelPendingRow: +// Heuristic: // * status must be "failed"; // * row must NOT carry an ops_hint (persistent failure → no retry, // show the hint instead); // * row must NOT already have a retry_execution_id (we don't allow // retrying the same failure twice — the user should retry the // latest descendant in the chain); -// * admin → always yes; -// * non-admin matching the row's created_by_user_id → yes (retry-own). +// * any session with retry-any:purchases (carved-out admin verb, +// seeded on Purchaser group; can also come from a custom group via +// effectivePermissions) → retry-any; +// * otherwise the row's created_by_user_id must match the current +// user (retry-own). function canRetryFailedRow(p: HistoryPurchase): boolean { const status = (p.status || '').toLowerCase(); if (status !== 'failed') return false; @@ -447,7 +454,12 @@ function canRetryFailedRow(p: HistoryPurchase): boolean { if (p.retry_execution_id) return false; // already retried — user should act on the descendant const user = getCurrentUser(); if (!user) return false; - if (isAdmin()) return true; + // retry-any:purchases is carved out of admin:* (issue #923) and is + // granted by the seeded Purchaser group OR any custom group that + // explicitly lists the verb in effectivePermissions. Gate on the + // verb directly so a non-seeded role with the same grant still + // retries rows the backend would also let through. + if (canAccess('retry-any', 'purchases')) return true; if (!p.created_by_user_id) return false; return p.created_by_user_id === user.id; } @@ -579,6 +591,31 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { lastPurchases = purchases; + // Issue #923: inject a read-only notice for sessions that lack the + // carved-out spending verbs the buttons in this table need. Approve / + // Retry are gated by canApprovePendingRow / canRetryFailedRow which + // call canAccess('approve-any','purchases') and + // canAccess('retry-any','purchases'); use the same predicate here so + // the banner stays in lockstep with the visible buttons (a user who + // holds either verb via a custom group sees no contradictory + // notice). + const canApproveAny = canAccess('approve-any', 'purchases'); + const canRetryAny = canAccess('retry-any', 'purchases'); + const hasAnyCarvedOut = canApproveAny || canRetryAny; + const existingBanner = document.getElementById('history-no-purchaser-banner'); + if (!existingBanner && !hasAnyCarvedOut) { + const banner = document.createElement('div'); + banner.id = 'history-no-purchaser-banner'; + banner.className = 'info-banner'; + banner.setAttribute('role', 'note'); + banner.textContent = + 'You can view but not execute purchases. ' + + 'Ask an admin to add you to the Purchaser group, or add yourself in Settings → Users.'; + container.parentElement?.insertBefore(banner, container); + } else if (existingBanner && hasAnyCarvedOut) { + existingBanner.remove(); + } + // Reset the filter when the dataset changes so the user isn't stuck on an // empty "Cancelled" slice after reloading with a fresh query. if (activeStatusFilter !== 'all' && !purchases.some(p => { diff --git a/frontend/src/permissions.generated.ts b/frontend/src/permissions.generated.ts index 3065e83b9..8137a23c8 100644 --- a/frontend/src/permissions.generated.ts +++ b/frontend/src/permissions.generated.ts @@ -1,8 +1,9 @@ // CODE GENERATED by `go run ./cmd/gen-permissions`. DO NOT EDIT MANUALLY. // // Source of truth: internal/auth/types.go (DefaultAdminPermissions, -// DefaultUserPermissions, DefaultReadOnlyPermissions). To regenerate after -// editing the Go defaults, run: +// DefaultUserPermissions, DefaultReadOnlyPermissions, +// DefaultPurchaserPermissions). To regenerate after editing the Go +// defaults, run: // // go run ./cmd/gen-permissions // @@ -37,3 +38,13 @@ export const READONLY_PERMS: ReadonlySet = new Set([ 'view:plans', 'view:recommendations', ]); + +export const PURCHASER_PERMS: ReadonlySet = new Set([ + 'approve-any:purchases', + 'execute:purchases', + 'retry-any:purchases', + 'view:history', + 'view:plans', + 'view:purchases', + 'view:recommendations', +]); diff --git a/frontend/src/permissions.ts b/frontend/src/permissions.ts index 998ccba9a..24946a7da 100644 --- a/frontend/src/permissions.ts +++ b/frontend/src/permissions.ts @@ -74,6 +74,27 @@ export type Resource = */ export const ADMINISTRATORS_GROUP_ID = '00000000-0000-5000-8000-000000000001'; +/** + * Well-known group UUID for the Purchaser group seeded by migration + * 000058 (issue #923). The three money-spending verbs + * (execute:purchases, approve-any:purchases, retry-any:purchases) are + * carved out of the admin:* wildcard and require explicit membership + * in this group (or a custom group that grants the same verbs). + */ +export const PURCHASER_GROUP_ID = '00000000-0000-5000-8000-000000000005'; + +/** + * The set of (action, resource) pairs carved out of the admin:* + * wildcard. Mirrors adminCarvedOuts in internal/auth/types.go. + * Admin-group members must also be in the Purchaser group to pass + * these checks. + */ +const ADMIN_CARVED_OUTS: ReadonlySet = new Set([ + 'execute:purchases', + 'approve-any:purchases', + 'retry-any:purchases', +]); + /** * Return true when the current session user is a member of the * Administrators group. This replaces the former `user.role === "admin"` @@ -87,38 +108,101 @@ export function isAdmin(): boolean { return Array.isArray(user.groups) && user.groups.includes(ADMINISTRATORS_GROUP_ID); } +/** + * Return true when the current session is authorised to execute the + * three carved-out money-spending verbs (execute:purchases, + * approve-any:purchases, retry-any:purchases). When the backend has + * delivered effectivePermissions (post-bootstrap) we drive off the + * permission set itself so a user who holds any of those verbs via a + * custom group (not just the seeded Purchaser group) also returns + * true. While effectivePermissions is still loading we fall back to + * seeded-group membership so the helper agrees with the canAccess() + * carve-out fallback in the same window. + * + * Callers that need a hard verb-specific gate should prefer + * canAccess('execute', 'purchases'). isPurchaser() is the + * verb-agnostic "can spend money at all" predicate (true if ANY of + * the three carved-out verbs is granted), which is what the + * no-Purchaser banners use. + */ +export function isPurchaser(): boolean { + const user = state.getCurrentUser(); + if (!user) return false; + if (user.effectivePermissions) { + // Match canAccess()'s semantics: a permission entry with + // resource '*' satisfies the carved-out verb on 'purchases' the + // same way the backend's HasPermission accepts ResourceAll. Walk + // each carved-out key and accept either an exact match or a + // wildcard-resource match on the same action. + for (const key of ADMIN_CARVED_OUTS) { + const colon = key.indexOf(':'); + if (colon < 0) continue; + const action = key.slice(0, colon); + const resource = key.slice(colon + 1); + for (const p of user.effectivePermissions) { + if (p.action === action && (p.resource === resource || p.resource === '*')) { + return true; + } + } + } + return false; + } + return Array.isArray(user.groups) && user.groups.includes(PURCHASER_GROUP_ID); +} + /** * Returns true when the current session's effective permissions grant * the specified action on the specified resource. * * When effectivePermissions is populated (fetched from * GET /api/auth/me/permissions on login/bootstrap) the set is - * consulted directly: admin:* grants everything; otherwise an exact - * action:resource match is required. + * consulted directly: admin:* grants everything EXCEPT the three + * money-spending verbs carved out of admin:* by the backend + * (issue #923) -- those require an explicit (action, resource) entry + * in effectivePermissions (which the backend only returns when the + * user is in the Purchaser group or a custom group that grants the + * verb directly). For non-admin entries an exact action:resource + * match (or matching action with resource '*') is required. * * While effectivePermissions is not yet loaded (e.g. during the first * render before the async fetch completes) the function falls back to - * the group-membership admin check so Administrators-group members - * aren't locked out during bootstrap. Non-admins see buttons hidden - * briefly -- acceptable because the full set loads immediately after - * login. + * group-membership checks: Administrators-group members pass every + * check EXCEPT the carved-out money-spending verbs, which require + * Purchaser-group membership. This mirrors the backend's + * HasPermission carve-out so UX and enforcement agree on the same + * verbs whether or not effectivePermissions has loaded yet. * - * UX-only gate. The backend still enforces on every request. + * UX-only gate. The backend still enforces on every request; a + * wrong-positive surfaces as a 403 on click, a wrong-negative just + * hides a button. */ export function canAccess(action: Action, resource: Resource): boolean { const user = state.getCurrentUser(); if (!user) return false; + const key = `${action}:${resource}`; + const isCarvedOut = ADMIN_CARVED_OUTS.has(key); + // Use the server-provided effective permission set when available. if (user.effectivePermissions) { for (const p of user.effectivePermissions) { - if (p.action === 'admin' && p.resource === '*') return true; - if (p.action === action && (p.resource === resource || p.resource === '*')) return true; + // admin:* covers everything EXCEPT the carved-out verbs. + if (p.action === 'admin' && p.resource === '*' && !isCarvedOut) { + return true; + } + if (p.action === action && (p.resource === resource || p.resource === '*')) { + return true; + } } return false; } - // Fallback while permissions are still loading: admins pass, others block. + // Fallback while permissions are still loading. Mirror the backend's + // carve-out: admin grants everything except the money-spending verbs, + // which require explicit Purchaser-group membership. + if (isCarvedOut) { + return isPurchaser(); + } return isAdmin(); } diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 4de23ba0e..4c62ca3fa 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -3441,11 +3441,12 @@ function mountBottomActionBox(): HTMLElement | null { box.appendChild(capacityLabel); // Purchase one-off (preserved ID). Issue #365: hide for sessions - // that lack `execute:purchases` (admin only by default; user and - // readonly never see the button). The element stays in the DOM so - // the click handler stays wired and the existing `updateBottomAction - // Box` updates still flow through; `.hidden` toggles via the HTML - // hidden attribute which renders as `display: none`. + // that lack `execute:purchases`. After issue #923, execute:purchases + // requires Purchaser-group membership; Administrators-group alone is + // no longer sufficient. The element stays in the DOM so the click + // handler stays wired and the existing `updateBottomActionBox` + // updates still flow through; `.hidden` toggles via the HTML hidden + // attribute which renders as `display: none`. const purchaseBtn = document.createElement('button'); purchaseBtn.type = 'button'; purchaseBtn.className = 'btn btn-primary'; @@ -3455,6 +3456,23 @@ function mountBottomActionBox(): HTMLElement | null { purchaseBtn.hidden = !canAccess('execute', 'purchases'); box.appendChild(purchaseBtn); + // Issue #923: show an informational banner when the session can view + // recommendations but cannot execute purchases. The predicate MUST + // mirror the Purchase CTA's gate (canAccess('execute', 'purchases')) + // so a custom-role user who holds execute:purchases via a non-seeded + // group sees the live button without a contradictory "you can view + // but not execute" notice. Pure read-only users won't reach this + // page's action area anyway. + if (!canAccess('execute', 'purchases')) { + const noPurchaseBanner = document.createElement('div'); + noPurchaseBanner.className = 'info-banner'; + noPurchaseBanner.setAttribute('role', 'note'); + noPurchaseBanner.textContent = + 'You can view but not execute purchases. ' + + 'Ask an admin to add you to the Purchaser group, or add yourself in Settings → Users.'; + box.appendChild(noPurchaseBanner); + } + // Create Purchase Plan (relocated from old top bar). Issue #365: // hide for sessions that lack `create:plans` (readonly loses it; // admin + user keep it). diff --git a/frontend/src/styles/components.css b/frontend/src/styles/components.css index 68c5559d6..b69b59507 100644 --- a/frontend/src/styles/components.css +++ b/frontend/src/styles/components.css @@ -877,6 +877,20 @@ tr.recommendation-row:hover { color: #444; } +/* Informational notice banner used on pages where the current session can + * view but not execute purchases (issue #923: Purchaser group separation + * of duties). Reuses the accent border pattern from .self-account-banner + * in settings.css. */ +.info-banner { + border-left: 3px solid var(--accent, #4a9eff); + background: var(--cudly-info-bg, #eaf4ff); + padding: 0.5rem 0.75rem; + margin-bottom: 0.75rem; + border-radius: 0 4px 4px 0; + font-size: 0.9em; + color: #333; +} + /* Cell summary row (collapsed state) */ tr.rec-cell-summary-row { background: #f3f6fb; diff --git a/frontend/src/users/userActions.ts b/frontend/src/users/userActions.ts index aaacdadcf..7a87f6d95 100644 --- a/frontend/src/users/userActions.ts +++ b/frontend/src/users/userActions.ts @@ -4,6 +4,7 @@ import * as api from '../api'; import { getCurrentUser } from '../state'; +import { isAdmin, isPurchaser, PURCHASER_GROUP_ID } from '../permissions'; import { allUsers, filteredUsers, @@ -94,6 +95,31 @@ export async function loadUsers(): Promise { if (matrixContainer) { renderPermissionMatrix(groups, matrixContainer); } + + // Issue #923: first-run prompt for admins not in the Purchaser group. + // Show once per browser (stored in localStorage). The Purchaser group + // must exist (migration 000058) before the prompt is relevant, so we + // check that availableGroups contains it before surfacing the dialog. + const PROMPT_KEY = 'cudly:purchaser-prompt-dismissed'; + const purchaserGroupExists = groups.some(g => g.id === PURCHASER_GROUP_ID); + if ( + purchaserGroupExists && + isAdmin() && + !isPurchaser() && + !localStorage.getItem(PROMPT_KEY) + ) { + localStorage.setItem(PROMPT_KEY, '1'); + // Use the existing confirmDialog as a non-destructive notification. + void confirmDialog({ + title: 'Purchaser group: separation of duties', + body: + 'Recommended: add yourself to the Purchaser group only if no separate ' + + 'finance team will execute purchases. Otherwise leave it to dedicated ' + + 'Purchaser user(s). You can manage membership in the Groups panel below.', + confirmLabel: 'Got it', + destructive: false, + }); + } } catch (error) { console.error('Failed to load users/groups:', error); showError('Failed to load users and groups'); diff --git a/internal/auth/store_postgres.go b/internal/auth/store_postgres.go index 8b66f3708..efdf1bfca 100644 --- a/internal/auth/store_postgres.go +++ b/internal/auth/store_postgres.go @@ -417,7 +417,7 @@ func (s *PostgresStore) CreateAdminIfNone(ctx context.Context, user *User) (bool func (s *PostgresStore) GetGroup(ctx context.Context, groupID string) (*Group, error) { query := ` SELECT id, name, description, permissions, allowed_accounts, - created_at, updated_at, created_by + system_managed, created_at, updated_at, created_by FROM groups WHERE id = $1 ` @@ -538,7 +538,7 @@ func (s *PostgresStore) ListGroups(ctx context.Context) ([]Group, error) { // Pagination support should be added if this limit proves insufficient. query := ` SELECT id, name, description, permissions, allowed_accounts, - created_at, updated_at, created_by + system_managed, created_at, updated_at, created_by FROM groups ORDER BY created_at DESC LIMIT 10000 @@ -926,6 +926,7 @@ func (s *PostgresStore) scanGroup(scanner Scanner) (*Group, error) { &group.Description, &permissionsJSON, &allowedAccounts, + &group.SystemManaged, &group.CreatedAt, &group.UpdatedAt, &createdBy, diff --git a/internal/auth/store_postgres_test.go b/internal/auth/store_postgres_test.go index f6b0ae937..cfd06d00f 100644 --- a/internal/auth/store_postgres_test.go +++ b/internal/auth/store_postgres_test.go @@ -695,20 +695,22 @@ func TestPostgresStore_CleanupExpiredSessions(t *testing.T) { func createMockRowWithGroup(group *Group) *MockRow { return &MockRow{ scanFunc: func(dest ...interface{}) error { - if len(dest) >= 8 { + if len(dest) >= 9 { *dest[0].(*string) = group.ID *dest[1].(*string) = group.Name *dest[2].(*string) = group.Description // dest[3] is permissions JSON *dest[3].(*[]byte) = []byte(`[]`) *dest[4].(*[]string) = group.AllowedAccounts - *dest[5].(*time.Time) = group.CreatedAt - *dest[6].(*time.Time) = group.UpdatedAt - // dest[7] is sql.NullString for CreatedBy + // dest[5] is system_managed (issue #923) + *dest[5].(*bool) = group.SystemManaged + *dest[6].(*time.Time) = group.CreatedAt + *dest[7].(*time.Time) = group.UpdatedAt + // dest[8] is sql.NullString for CreatedBy if group.CreatedBy != "" { - *dest[7].(*sql.NullString) = sql.NullString{String: group.CreatedBy, Valid: true} + *dest[8].(*sql.NullString) = sql.NullString{String: group.CreatedBy, Valid: true} } else { - *dest[7].(*sql.NullString) = sql.NullString{Valid: false} + *dest[8].(*sql.NullString) = sql.NullString{Valid: false} } } return nil diff --git a/internal/auth/types.go b/internal/auth/types.go index 0601637d3..e322fa82d 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -48,9 +48,13 @@ type Group struct { Description string `json:"description,omitempty" dynamodbav:"Description"` Permissions []Permission `json:"permissions" dynamodbav:"Permissions"` AllowedAccounts []string `json:"allowed_accounts,omitempty" dynamodbav:"AllowedAccounts"` - CreatedAt time.Time `json:"created_at" dynamodbav:"CreatedAt"` - UpdatedAt time.Time `json:"updated_at" dynamodbav:"UpdatedAt"` - CreatedBy string `json:"created_by" dynamodbav:"CreatedBy"` + // SystemManaged marks groups that are seeded by migrations and + // should not be renamed or deleted via the API. Only membership + // can change for system-managed groups. + SystemManaged bool `json:"system_managed,omitempty" dynamodbav:"SystemManaged"` + CreatedAt time.Time `json:"created_at" dynamodbav:"CreatedAt"` + UpdatedAt time.Time `json:"updated_at" dynamodbav:"UpdatedAt"` + CreatedBy string `json:"created_by" dynamodbav:"CreatedBy"` } // Permission defines what actions a group can perform @@ -106,15 +110,36 @@ type AuthContext struct { Permissions []Permission // Computed from group memberships } +// adminCarvedOuts is the set of (action, resource) pairs that the admin:* +// wildcard does NOT cover. Each pair requires explicit membership in a group +// that holds the matching permission (e.g. the Purchaser group). This +// implements separation-of-duties for money-spending operations (issue #923): +// a compromised admin account alone cannot drain commitments. +var adminCarvedOuts = map[[2]string]bool{ + {ActionExecute, ResourcePurchases}: true, + {ActionApproveAny, ResourcePurchases}: true, + {ActionRetryAny, ResourcePurchases}: true, +} + // HasPermission checks if the auth context has a specific permission. // Authorization is derived purely from group-granted permissions: a user // who is a member of the Administrators group holds {ActionAdmin, ResourceAll} // and therefore passes any check; a user with no groups holds no permissions // and is denied everything (fail closed). +// +// The admin:* wildcard is intentionally narrow for the three carved-out +// money-spending verbs (execute:purchases, approve-any:purchases, +// retry-any:purchases). Those require explicit membership in a group that +// grants them directly (e.g. the Purchaser group seeded by migration 000054). func (ctx *AuthContext) HasPermission(action, resource string) bool { for _, perm := range ctx.Permissions { - // Admin permission grants all access + // Admin permission grants all access EXCEPT the carved-out + // money-spending verbs (separation of duties, issue #923). if perm.Action == ActionAdmin && perm.Resource == ResourceAll { + if adminCarvedOuts[[2]string{action, resource}] { + // Fall through to explicit-permission check below. + continue + } return true } @@ -285,6 +310,17 @@ const ( // group so the group card shows members on a fresh install. const DefaultAdminGroupID = "00000000-0000-5000-8000-000000000001" +// DefaultPurchaserGroupID is the fixed UUID of the Purchaser group seeded +// by migration 000054. It holds the three money-spending verbs carved out +// of the admin:* wildcard (issue #923). +const DefaultPurchaserGroupID = "00000000-0000-5000-8000-000000000005" + +// GroupPurchaser is the canonical name of the system-managed Purchaser +// group. MUST match the literal name inserted by migration +// 000058_seed_purchaser_group.up.sql so name-based lookups agree with +// the seeded row. +const GroupPurchaser = "Purchaser" + // Predefined actions const ( ActionView = "view" @@ -451,3 +487,21 @@ func DefaultReadOnlyPermissions() []Permission { {Action: ActionView, Resource: ResourceHistory}, } } + +// DefaultPurchaserPermissions returns the permissions for the system-managed +// Purchaser group (issue #923). The three execute/approve-any/retry-any verbs +// are carved out of the admin:* wildcard; a user must hold them explicitly +// (via this group or a custom group that includes them) to spend money. +func DefaultPurchaserPermissions() []Permission { + return []Permission{ + // Money-spending verbs (carved out of admin:* wildcard). + {Action: ActionExecute, Resource: ResourcePurchases}, + {Action: ActionApproveAny, Resource: ResourcePurchases}, + {Action: ActionRetryAny, Resource: ResourcePurchases}, + // Read access so Purchaser members can navigate to the relevant pages. + {Action: ActionView, Resource: ResourceRecommendations}, + {Action: ActionView, Resource: ResourcePlans}, + {Action: ActionView, Resource: ResourcePurchases}, + {Action: ActionView, Resource: ResourceHistory}, + } +} diff --git a/internal/auth/types_test.go b/internal/auth/types_test.go index 430b9ad3c..38e01d206 100644 --- a/internal/auth/types_test.go +++ b/internal/auth/types_test.go @@ -50,4 +50,100 @@ func TestDefaultPermissions(t *testing.T) { assert.Equal(t, ActionView, p.Action) } }) + + t.Run("DefaultPurchaserPermissions contains carved verbs and view grants", func(t *testing.T) { + perms := DefaultPurchaserPermissions() + // 3 money-spending verbs + 4 view grants = 7. + assert.Len(t, perms, 7) + + actions := make(map[string]bool) + for _, p := range perms { + actions[p.Action+":"+p.Resource] = true + } + + assert.True(t, actions[ActionExecute+":"+ResourcePurchases]) + assert.True(t, actions[ActionApproveAny+":"+ResourcePurchases]) + assert.True(t, actions[ActionRetryAny+":"+ResourcePurchases]) + assert.True(t, actions[ActionView+":"+ResourceRecommendations]) + assert.True(t, actions[ActionView+":"+ResourcePlans]) + assert.True(t, actions[ActionView+":"+ResourcePurchases]) + assert.True(t, actions[ActionView+":"+ResourceHistory]) + }) +} + +// TestAdminWildcardCarveOuts verifies that the admin:* permission does NOT +// cover the three money-spending verbs carved out for separation of duties +// (issue #923). +func TestAdminWildcardCarveOuts(t *testing.T) { + adminCtx := &AuthContext{ + User: &User{}, + Permissions: []Permission{ + {Action: ActionAdmin, Resource: ResourceAll}, + }, + } + + // Admin wildcard must NOT cover the three carved-out verbs. + assert.False(t, adminCtx.HasPermission(ActionExecute, ResourcePurchases), + "admin:* must not cover execute:purchases (issue #923)") + assert.False(t, adminCtx.HasPermission(ActionApproveAny, ResourcePurchases), + "admin:* must not cover approve-any:purchases (issue #923)") + assert.False(t, adminCtx.HasPermission(ActionRetryAny, ResourcePurchases), + "admin:* must not cover retry-any:purchases (issue #923)") + + // Admin wildcard MUST still cover everything else. + assert.True(t, adminCtx.HasPermission(ActionView, ResourcePurchases)) + assert.True(t, adminCtx.HasPermission(ActionCreate, ResourcePlans)) + assert.True(t, adminCtx.HasPermission(ActionDelete, ResourceUsers)) + assert.True(t, adminCtx.HasPermission(ActionCancelAny, ResourcePurchases), + "cancel-any stays on admin (cleanup, not money-out)") +} + +// TestPurchaserGroupCoversExecutePurchases verifies that a user who holds +// the Purchaser group permissions (but not admin:*) can execute, approve-any, +// and retry-any purchases. +func TestPurchaserGroupCoversExecutePurchases(t *testing.T) { + purchaserCtx := &AuthContext{ + User: &User{}, + Permissions: DefaultPurchaserPermissions(), + } + + assert.True(t, purchaserCtx.HasPermission(ActionExecute, ResourcePurchases)) + assert.True(t, purchaserCtx.HasPermission(ActionApproveAny, ResourcePurchases)) + assert.True(t, purchaserCtx.HasPermission(ActionRetryAny, ResourcePurchases)) + + // But not admin-only operations. + assert.False(t, purchaserCtx.HasPermission(ActionDelete, ResourceUsers)) + assert.False(t, purchaserCtx.HasPermission(ActionAdmin, ResourceAll)) +} + +// TestAdminWithoutPurchaserCannotExecutePurchases verifies that admin:* alone +// (without the Purchaser group permissions) is denied execute:purchases. +func TestAdminWithoutPurchaserCannotExecutePurchases(t *testing.T) { + adminOnlyCtx := &AuthContext{ + User: &User{}, + Permissions: []Permission{ + {Action: ActionAdmin, Resource: ResourceAll}, + }, + } + + assert.False(t, adminOnlyCtx.HasPermission(ActionExecute, ResourcePurchases), + "admin-only context must be denied execute:purchases") +} + +// TestAdminAndPurchaserCanExecutePurchases verifies that a user in both the +// Administrators group and the Purchaser group can execute purchases. +func TestAdminAndPurchaserCanExecutePurchases(t *testing.T) { + combinedPerms := append( + []Permission{{Action: ActionAdmin, Resource: ResourceAll}}, + DefaultPurchaserPermissions()..., + ) + ctx := &AuthContext{ + User: &User{}, + Permissions: combinedPerms, + } + + assert.True(t, ctx.HasPermission(ActionExecute, ResourcePurchases)) + assert.True(t, ctx.HasPermission(ActionApproveAny, ResourcePurchases)) + assert.True(t, ctx.HasPermission(ActionRetryAny, ResourcePurchases)) + assert.True(t, ctx.HasPermission(ActionDelete, ResourceUsers)) } diff --git a/internal/database/postgres/migrations/000058_seed_purchaser_group.down.sql b/internal/database/postgres/migrations/000058_seed_purchaser_group.down.sql new file mode 100644 index 000000000..db99b7289 --- /dev/null +++ b/internal/database/postgres/migrations/000058_seed_purchaser_group.down.sql @@ -0,0 +1,14 @@ +-- Reverse migration: remove Purchaser group memberships from users, +-- then delete the Purchaser group, then drop the system_managed column. + +-- Remove the Purchaser group from all user group_ids arrays. +UPDATE users +SET group_ids = array_remove(group_ids, '00000000-0000-5000-8000-000000000005'::UUID) +WHERE '00000000-0000-5000-8000-000000000005'::UUID = ANY(group_ids); + +-- Delete the Purchaser group. +DELETE FROM groups +WHERE id = '00000000-0000-5000-8000-000000000005'; + +-- Drop the system_managed column (rolls back the ALTER TABLE above). +ALTER TABLE groups DROP COLUMN IF EXISTS system_managed; diff --git a/internal/database/postgres/migrations/000058_seed_purchaser_group.up.sql b/internal/database/postgres/migrations/000058_seed_purchaser_group.up.sql new file mode 100644 index 000000000..cd6a7b2dd --- /dev/null +++ b/internal/database/postgres/migrations/000058_seed_purchaser_group.up.sql @@ -0,0 +1,76 @@ +-- Seed the Purchaser system-managed group with a fixed UUID so the +-- seeding is idempotent and the ID is stable across deployments. +-- The three money-spending verbs (execute, approve-any, retry-any on +-- purchases) are carved out of the admin:* wildcard by the backend +-- HasPermission change in this same PR (issue #923). A user must be +-- a member of this group (or a custom group granting these verbs) to +-- spend money, even if they are an administrator. +-- +-- The groups table does not yet have a system_managed column, so we +-- add it here and backfill existing seed groups as system-managed. + +ALTER TABLE groups ADD COLUMN IF NOT EXISTS system_managed BOOLEAN NOT NULL DEFAULT FALSE; + +-- Mark the four existing seed groups as system-managed. +UPDATE groups +SET system_managed = TRUE +WHERE id IN ( + '00000000-0000-5000-8000-000000000001', + '00000000-0000-5000-8000-000000000002', + '00000000-0000-5000-8000-000000000003', + '00000000-0000-5000-8000-000000000004' +); + +-- Fail closed if a pre-existing group already owns the name "Purchaser" +-- under a different UUID. Without this guard the bare ON CONFLICT DO +-- NOTHING below would silently skip the seed and leave +-- DefaultPurchaserGroupID absent, breaking the admin-backfill UPDATE +-- further down and every callsite that keys off the fixed UUID. +DO $$ +BEGIN + IF EXISTS ( + SELECT 1 + FROM groups + WHERE name = 'Purchaser' + AND id <> '00000000-0000-5000-8000-000000000005' + ) THEN + RAISE EXCEPTION + 'migration 000058: a group named ''Purchaser'' already exists with a different id; rename it before applying this migration so the seeded id (00000000-0000-5000-8000-000000000005) can be created'; + END IF; +END $$; + +-- Insert the Purchaser group. Idempotent on the seeded id only -- a +-- name-collision on a different id is caught by the DO block above so +-- the seed never silently goes missing. +INSERT INTO groups (id, name, description, permissions, allowed_accounts, system_managed) +VALUES ( + '00000000-0000-5000-8000-000000000005', + 'Purchaser', + 'Execute, approve, and retry purchases. Membership is required even for admins to spend money (separation of duties, issue #923).', + '[ + {"action":"execute","resource":"purchases"}, + {"action":"approve-any","resource":"purchases"}, + {"action":"retry-any","resource":"purchases"}, + {"action":"view","resource":"recommendations"}, + {"action":"view","resource":"plans"}, + {"action":"view","resource":"purchases"}, + {"action":"view","resource":"history"} + ]'::jsonb, + ARRAY['*'], + TRUE +) +ON CONFLICT (id) DO NOTHING; + +-- Auto-assign every existing admin-group member to the Purchaser group +-- so upgrade preserves current behavior. Admins can later remove +-- themselves to enforce strict separation of duties. +-- We drive off group membership (Administrators group UUID) rather than +-- the legacy role column (which has been dropped in migration 000057). +UPDATE users +SET group_ids = ARRAY( + SELECT DISTINCT unnest( + COALESCE(group_ids, '{}') || ARRAY['00000000-0000-5000-8000-000000000005']::UUID[] + ) +) +WHERE '00000000-0000-5000-8000-000000000001'::UUID = ANY(COALESCE(group_ids, '{}')) + AND EXISTS (SELECT 1 FROM groups WHERE id = '00000000-0000-5000-8000-000000000005');