diff --git a/frontend/src/__tests__/history-retry-button.test.ts b/frontend/src/__tests__/history-retry-button.test.ts index 3e113e7af..72d86282a 100644 --- a/frontend/src/__tests__/history-retry-button.test.ts +++ b/frontend/src/__tests__/history-retry-button.test.ts @@ -82,7 +82,19 @@ import { ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID } from '../permissions'; // 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: [] }; +// REG_USER carries retry-own:purchases in effectivePermissions so canAccess +// returns the correct value. The old shape (groups: [], no effectivePermissions) +// silently allowed the creator check to bypass the permission check -- exactly +// the bug canRetryFailedRow now fixes (issue #1418). +const REG_USER = { + id: 'user-uuid', + email: 'user@example.com', + groups: [], + effectivePermissions: [ + { action: 'retry-own', resource: 'purchases' }, + { action: 'cancel-own', resource: 'purchases' }, + ], +}; // 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 @@ -473,3 +485,56 @@ describe('History inline Retry button (issue #47)', () => { expect(ids).toEqual(['fail-mine']); }); }); + +// Issue #1418 regression: user without retry-own:purchases sees no Retry button +// even on rows they created (closes the same pattern fixed for approve-own by +// PR #1422 / issue #1407). +describe('History retry-own permission gate (issue #1418)', () => { + beforeEach(() => { + setupDOM(); + jest.clearAllMocks(); + }); + + test('user without retry-own:purchases sees no Retry button on own failed row', async () => { + // A Plan Authors-style user: holds plan verbs only, no purchase retry perm. + const planAuthorUser = { + id: 'plan-author-uuid', + email: 'plan-author@example.com', + groups: [], + effectivePermissions: [ + { action: 'create', resource: 'plans' }, + { action: 'update', resource: 'plans' }, + { action: 'delete', resource: 'plans' }, + ], + }; + (getCurrentUser as jest.Mock).mockReturnValue(planAuthorUser); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ purchase_id: 'own-fail', created_by_user_id: planAuthorUser.id }), + ], + }); + + await loadHistory(); + + // Fail-before: without the retry-own check the button appeared; now it must not. + expect(document.querySelectorAll('.history-retry-btn')).toHaveLength(0); + }); + + test('user with retry-own:purchases sees Retry on own failed rows', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(REG_USER); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ purchase_id: 'own-fail', created_by_user_id: REG_USER.id }), + makeRow({ purchase_id: 'other-fail', created_by_user_id: OTHER_UUID }), + ], + }); + + await loadHistory(); + + const buttons = document.querySelectorAll('.history-retry-btn'); + const ids = Array.from(buttons).map((b) => b.dataset['retryId']); + expect(ids).toEqual(['own-fail']); + }); +}); diff --git a/frontend/src/__tests__/plans-permissions.test.ts b/frontend/src/__tests__/plans-permissions.test.ts index 7e97929dd..383bf3699 100644 --- a/frontend/src/__tests__/plans-permissions.test.ts +++ b/frontend/src/__tests__/plans-permissions.test.ts @@ -226,3 +226,64 @@ describe('Plans page permission gating (issue #365)', () => { }); }); }); + +// Issue #1406 / #1418: "Add Purchases" button on a plan card must be gated on +// update:purchases in addition to update:plans. Plan Authors hold the plan verbs +// but not the purchase verb -- they must NOT see the Add Purchases button. +describe('Plans page Add Purchases permission gate (issue #1406 / #1418)', () => { + beforeEach(() => { + jest.clearAllMocks(); + setupDom(); + (api.getPlans as jest.Mock).mockResolvedValue({ plans: [samplePlan] }); + (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); + }); + + test('Plan Authors (update:plans only, no update:purchases) see Edit Plan but not Add Purchases', async () => { + // Simulate a Plan Authors group: plan management verbs, no purchase verbs. + (state.getCurrentUser as jest.Mock).mockReturnValue({ + id: 'plan-author-id', + email: 'plan-author@example.com', + groups: [], + effectivePermissions: [ + { action: 'create', resource: 'plans' }, + { action: 'update', resource: 'plans' }, + { action: 'delete', resource: 'plans' }, + { action: 'view', resource: 'plans' }, + { action: 'view', resource: 'purchases' }, + ], + }); + + await loadPlans(); + const html = (document.getElementById('plans-list') as HTMLElement).innerHTML; + + // Fail-before: Add Purchases was shown; pass-after: it must be absent. + expect(html).not.toContain('data-action="add-purchases"'); + // Edit Plan is still shown (update:plans is sufficient for it). + expect(html).toContain('data-action="edit-plan"'); + // Delete Plan is still shown (delete:plans is sufficient). + expect(html).toContain('data-action="delete-plan"'); + }); + + test('Standard User (update:plans + update:purchases) sees both Edit Plan and Add Purchases', async () => { + (state.getCurrentUser as jest.Mock).mockReturnValue({ + id: 'std-user-id', + email: 'std-user@example.com', + groups: [], + effectivePermissions: [ + { action: 'create', resource: 'plans' }, + { action: 'update', resource: 'plans' }, + { action: 'delete', resource: 'plans' }, + { action: 'update', resource: 'purchases' }, + { action: 'view', resource: 'plans' }, + { action: 'view', resource: 'purchases' }, + ], + }); + + await loadPlans(); + const html = (document.getElementById('plans-list') as HTMLElement).innerHTML; + + expect(html).toContain('data-action="add-purchases"'); + expect(html).toContain('data-action="edit-plan"'); + expect(html).toContain('data-action="delete-plan"'); + }); +}); diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 96bf6b526..92678e7e3 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -535,6 +535,14 @@ function canRetryFailedRow(p: HistoryPurchase): boolean { // 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; + // Mirror canCancelPendingRow / canApprovePendingRow (issue #1418): require + // the retry-own permission explicitly before checking creator match. + // Without this gate a role that holds NO retry permission at all (e.g. a + // Plan Authors group with only plan verbs) would still see the Retry button + // on rows they created, because the creator check alone was a sufficient + // condition. The backend authorizeSessionRetry is the real security boundary; + // this closes the UX gap. + if (!canAccess('retry-own', 'purchases')) return false; if (!p.created_by_user_id) return false; return p.created_by_user_id === user.id; } diff --git a/frontend/src/plans.ts b/frontend/src/plans.ts index 3a7df75a4..ebda11194 100644 --- a/frontend/src/plans.ts +++ b/frontend/src/plans.ts @@ -942,7 +942,7 @@ async function loadPlanAccountNames(planId: string, cardEl: Element): Promise
- ${canManagePlan && !isUnassigned ? `` : ''} + ${canAddPurchases && !isUnassigned ? `` : ''} ${canManagePlan && !isUnassigned ? `` : ''} ${canDeletePlan ? `` : ''} @@ -1030,6 +1030,11 @@ function renderPlans(plans: LocalPlan[]): void { // 600 times. Action buttons hidden for sessions that lack the verb. const canManagePlan = canAccess('update', 'plans'); const canDeletePlan = canAccess('delete', 'plans'); + // Issue #1406 / #1418: "Add Purchases" requires BOTH plan-management AND + // purchase-write permission. Plan Authors hold update:plans but not + // update:purchases; Standard Users hold both. Gate the button on the + // conjunction so Plan Authors cannot schedule purchases. + const canAddPurchases = canManagePlan && canAccess('update', 'purchases'); // Issue #973: split plans into assigned (have plan_accounts rows) and // unassigned (legacy plans with zero plan_accounts rows, flagged by the @@ -1039,13 +1044,13 @@ function renderPlans(plans: LocalPlan[]): void { const unassignedPlans = plans.filter(p => (p as unknown as BackendPlan).unassigned); const assignedHtml = assignedPlans.map(rawPlan => - renderPlanCard(rawPlan as unknown as BackendPlan, canManagePlan, canDeletePlan) + renderPlanCard(rawPlan as unknown as BackendPlan, canManagePlan, canDeletePlan, canAddPurchases) ).join(''); let unassignedHtml = ''; if (unassignedPlans.length > 0) { const cards = unassignedPlans.map(rawPlan => - renderPlanCard(rawPlan as unknown as BackendPlan, canManagePlan, canDeletePlan) + renderPlanCard(rawPlan as unknown as BackendPlan, canManagePlan, canDeletePlan, canAddPurchases) ).join(''); unassignedHtml = `
diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 95851a68d..4f9e2f066 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -269,7 +269,12 @@ func (h *Handler) createPlannedPurchases(ctx context.Context, httpReq *events.La return nil, err } - session, err := h.requirePermission(ctx, httpReq, "create", "plans") + // Require update:purchases — creating scheduled purchase rows is a + // purchase-write operation. Standard Users (DefaultUserPermissions) + // hold this verb; Plan Authors (create/update/delete:plans only) + // do not, closing issue #1406 / #1418. The prior gate ("create","plans") + // incorrectly admitted Plan Authors because they hold create:plans. + session, err := h.requirePermission(ctx, httpReq, "update", "purchases") if err != nil { return nil, err } diff --git a/internal/api/router_660_permission_flips_test.go b/internal/api/router_660_permission_flips_test.go index 1fcc8685d..465e8c338 100644 --- a/internal/api/router_660_permission_flips_test.go +++ b/internal/api/router_660_permission_flips_test.go @@ -185,6 +185,60 @@ func TestUpdatePlan_PermissionGate(t *testing.T) { }) } +// TestCreatePlannedPurchases_PermissionGate covers +// POST /api/plans/{id}/purchases (issue #1406 / #1418). +// +// The handler was previously gated on create:plans, which incorrectly admitted +// Plan Authors (create/update/delete:plans only, no purchase verbs). The fix +// changes the gate to update:purchases: Standard Users hold it; Plan Authors do +// not. Regression test: user without update:purchases is rejected with 403. +func TestCreatePlannedPurchases_PermissionGate(t *testing.T) { + ctx := context.Background() + const userID = "44444444-4444-4444-4444-444444444444" + const planID = "dddddddd-dddd-dddd-dddd-dddddddddddd" + // Minimal valid body: count in [1,52] and a parseable YYYY-MM-DD date. + const body = `{"count":1,"start_date":"2026-07-01"}` + + t.Run("user with update:purchases can add planned purchases", func(t *testing.T) { + mockAuth := authForUserWith(ctx, t, userID, "update", "purchases", true) + mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) + mockStore := new(MockConfigStore) + // After the permission + plan-access gates pass, the handler fetches the + // plan. Returning nil, nil yields a 404 ClientError — a domain error that + // proves the permission gate was cleared without wiring up the full write path. + mockStore.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) { + return nil, nil + } + + h := &Handler{auth: mockAuth, config: mockStore} + _, err := h.createPlannedPurchases(ctx, reqWithBearerAndBody("user-token", body), planID) + assertNotForbidden(t, err) + }) + + t.Run("user without update:purchases (Plan Authors) is rejected with 403", func(t *testing.T) { + // This is the regression case for issue #1406: a Plan Authors user holds + // create/update/delete:plans but not update:purchases. Before the fix the + // gate was create:plans (which Plan Authors pass); after the fix it is + // update:purchases (which Plan Authors do not hold). + mockAuth := authForUserWith(ctx, t, userID, "update", "purchases", false) + h := &Handler{auth: mockAuth, config: new(MockConfigStore)} + _, err := h.createPlannedPurchases(ctx, reqWithBearerAndBody("user-token", body), planID) + assert403(t, err) + }) + + t.Run("admin bypasses permission check", func(t *testing.T) { + mockAuth := authForAdmin(ctx, t) + mockStore := new(MockConfigStore) + mockStore.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) { + return nil, nil + } + + h := &Handler{auth: mockAuth, config: mockStore} + _, err := h.createPlannedPurchases(ctx, reqWithBearerAndBody("admin-token", body), planID) + assertNotForbidden(t, err) + }) +} + // ---- Planned Purchases ---------------------------------------------------- // TestPausePlannedPurchase_PermissionGate covers POST /api/purchases/planned/{id}/pause.