From c72869b53f78294440fa9cd707e7d0096c379995 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 2 Jun 2026 20:01:07 +0200 Subject: [PATCH 1/3] feat(auth): add Purchaser group + carve execute/approve-any/retry-any out of admin wildcard (closes #923) Implements separation of duties for money-spending operations. The three carved verbs (execute:purchases, approve-any:purchases, retry-any:purchases) are removed from the admin:* wildcard in HasPermission; a user must hold them via explicit group membership (the new Purchaser group or any custom group that grants them). Migration 000058 seeds the Purchaser group (UUID 000000000005, system_managed) and auto-assigns existing Administrators-group members so upgrades preserve current behavior. Adds system_managed column to the groups table. Frontend: canAccess respects the carve-out; isPurchaser() checks Purchaser group membership; info banners on Recommendations and History pages for sessions lacking execute:purchases; first-run dialog for admins not yet in the Purchaser group. --- cmd/gen-permissions/main.go | 10 +- frontend/src/api/types.ts | 3 + frontend/src/history.ts | 44 ++++++--- frontend/src/permissions.generated.ts | 15 ++- frontend/src/permissions.ts | 76 +++++++++++++-- frontend/src/recommendations.ts | 28 ++++-- frontend/src/styles/components.css | 14 +++ frontend/src/users/userActions.ts | 26 +++++ internal/auth/store_postgres.go | 5 +- internal/auth/store_postgres_test.go | 14 +-- internal/auth/types.go | 59 +++++++++++- internal/auth/types_test.go | 96 +++++++++++++++++++ .../000058_seed_purchaser_group.down.sql | 14 +++ .../000058_seed_purchaser_group.up.sql | 56 +++++++++++ 14 files changed, 414 insertions(+), 46 deletions(-) create mode 100644 internal/database/postgres/migrations/000058_seed_purchaser_group.down.sql create mode 100644 internal/database/postgres/migrations/000058_seed_purchaser_group.up.sql 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/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..2e5968a32 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 { isAdmin, canAccess, isPurchaser } from './permissions'; import { showSkeletonRows, teardownSkeleton } from './lib/skeleton'; import { getAccountName } from './recommendations'; @@ -406,22 +406,20 @@ 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); +// * Purchaser-group member → approve-any (issue #923: approve-any is +// carved out of admin:* and requires explicit Purchaser membership); +// * non-Purchaser matching the row's created_by_user_id → 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. 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; + // Purchaser group membership grants approve-any (issue #923). + if (isPurchaser()) return true; if (!p.created_by_user_id) return false; return p.created_by_user_id === user.id; } @@ -431,15 +429,16 @@ 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). +// * Purchaser-group member → retry-any (issue #923: retry-any is +// carved out of admin:* and requires explicit Purchaser membership); +// * non-Purchaser matching the row's created_by_user_id → retry-own. function canRetryFailedRow(p: HistoryPurchase): boolean { const status = (p.status || '').toLowerCase(); if (status !== 'failed') return false; @@ -447,7 +446,8 @@ 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; + // Purchaser group membership grants retry-any (issue #923). + if (isPurchaser()) return true; if (!p.created_by_user_id) return false; return p.created_by_user_id === user.id; } @@ -579,6 +579,24 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { lastPurchases = purchases; + // Issue #923: inject a read-only notice for sessions that lack Purchaser + // group membership. The Approve / Retry buttons in the table are gated by + // canApprovePendingRow / canRetryFailedRow (which use isPurchaser), so this + // banner explains why those buttons are absent for admin-only sessions. + const existingBanner = document.getElementById('history-no-purchaser-banner'); + if (!existingBanner && !isPurchaser()) { + 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 && isPurchaser()) { + 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..717c36147 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,73 @@ export function isAdmin(): boolean { return Array.isArray(user.groups) && user.groups.includes(ADMINISTRATORS_GROUP_ID); } +/** + * Return true when the current session user is a member of the + * Purchaser group. The three money-spending verbs + * (execute:purchases, approve-any:purchases, retry-any:purchases) are + * carved out of the admin:* wildcard on the backend (issue #923) and + * require explicit Purchaser-group membership (or any custom group + * that grants the same verbs via effectivePermissions). + */ +export function isPurchaser(): boolean { + const user = state.getCurrentUser(); + if (!user) 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..030b99536 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -22,7 +22,7 @@ import type { AccountServiceOverride } from './api/accounts'; import type { RecommendationsResponse, LocalRecommendation, RecommendationsSummary, GlobalConfig } from './types'; import { openModal } from './modal'; import { showSkeletonRows, teardownSkeleton } from './lib/skeleton'; -import { canAccess } from './permissions'; +import { canAccess, isPurchaser } from './permissions'; // Issue #869: true when the current session can take any action on // recommendations (purchase or plan). Readonly/viewer sessions have neither @@ -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,21 @@ 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 (not in Purchaser group). + // The banner is visible only to users who have view access but lack + // the Purchaser group membership -- pure read-only users won't reach + // this page's action area anyway. + if (!isPurchaser()) { + 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..e7b6892e8 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,14 @@ 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. +const GroupPurchaser = "purchaser" + // Predefined actions const ( ActionView = "view" @@ -451,3 +484,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..18a86621a --- /dev/null +++ b/internal/database/postgres/migrations/000058_seed_purchaser_group.up.sql @@ -0,0 +1,56 @@ +-- 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' +); + +-- Insert the Purchaser group (idempotent on both id and name conflicts). +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 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'); From fd13e70905161a69b370340f43f2b00272c51dd6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 2 Jun 2026 20:47:37 +0200 Subject: [PATCH 2/3] fix(auth/tests): align permission tests with admin carve-out for purchase execution The Purchaser-group carve-out (issue #923) removed execute:purchases, approve-any:purchases, and retry-any:purchases from the admin:* wildcard. Four tests were written before that contract existed and expected admin-alone membership to grant those verbs. - permissions.test.ts: split the "Administrators group member passes all checks" test into two -- admin-alone now asserts execute:purchases === false (regression guard for the carve-out), admin+Purchaser asserts all three carved-out verbs pass - recommendations-permissions.test.ts: mockUser('admin') now includes Purchaser group membership, matching the auto-migration path for existing admins - history-approve-button.test.ts: ADMIN_USER gains Purchaser membership so approve-any:purchases is granted (approve button visible on all pending rows) - history-retry-button.test.ts: ADMIN_USER gains Purchaser membership so retry-any:purchases is granted (retry button visible on all failed rows) --- .../__tests__/history-approve-button.test.ts | 7 +- .../__tests__/history-retry-button.test.ts | 7 +- frontend/src/__tests__/permissions.test.ts | 101 ++++++++++++++++-- .../recommendations-permissions.test.ts | 10 +- 4 files changed, 113 insertions(+), 12 deletions(-) diff --git a/frontend/src/__tests__/history-approve-button.test.ts b/frontend/src/__tests__/history-approve-button.test.ts index 827543bc9..958266124 100644 --- a/frontend/src/__tests__/history-approve-button.test.ts +++ b/frontend/src/__tests__/history-approve-button.test.ts @@ -63,9 +63,12 @@ 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). 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] }; // 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 diff --git a/frontend/src/__tests__/history-retry-button.test.ts b/frontend/src/__tests__/history-retry-button.test.ts index 6a56fbe8e..9ca21efe7 100644 --- a/frontend/src/__tests__/history-retry-button.test.ts +++ b/frontend/src/__tests__/history-retry-button.test.ts @@ -69,9 +69,12 @@ 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: [] }; const OTHER_UUID = 'other-uuid'; diff --git a/frontend/src/__tests__/permissions.test.ts b/frontend/src/__tests__/permissions.test.ts index 3052d961d..8c52ccc8b 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, 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', () => { diff --git a/frontend/src/__tests__/recommendations-permissions.test.ts b/frontend/src/__tests__/recommendations-permissions.test.ts index 7ef6fdcca..944fbe5f5 100644 --- a/frontend/src/__tests__/recommendations-permissions.test.ts +++ b/frontend/src/__tests__/recommendations-permissions.test.ts @@ -80,11 +80,17 @@ 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] : [] }, ); }; From ba8f8d2d732ea803eba050323fe7e63ba69b038d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 3 Jun 2026 15:09:48 +0200 Subject: [PATCH 3/3] fix(auth): align Purchaser carve-out with CR review on PR #924 Address the five Major findings from CodeRabbit's pass on PR #924 so the frontend/backend authorization contract for the carved-out money-spending verbs (execute / approve-any / retry-any on purchases) stays consistent end-to-end. F1 (migration 000058): the seed used a bare `ON CONFLICT DO NOTHING` which treated "row with seeded UUID already exists" and "some other row already owns the name Purchaser" the same way. The latter case left DefaultPurchaserGroupID absent and silently broke the admin backfill. Detect a name-collision under a different UUID and RAISE EXCEPTION; narrow the conflict target to (id) so only the intended re-run is tolerated. F2 (internal/auth/types.go): `GroupPurchaser` constant was the lowercase string "purchaser" while the seeded row carries "Purchaser". Any name-based lookup would have missed the row. Align the constant with the seeded literal and document the contract in the comment. F3 (frontend/src/recommendations.ts): the no-Purchaser banner was predicated on `isPurchaser()` while the Purchase CTA used `canAccess('execute', 'purchases')`. Custom roles that hold execute:purchases via a non-seeded group saw both a live Purchase button AND the contradictory "you can view but not execute" notice. Drive the banner from the same predicate as the CTA. F4 (frontend/src/permissions.ts): `isPurchaser()` only checked PURCHASER_GROUP_ID membership, so the helper denied users who hold the carved-out verbs via custom groups. Extend it to consult effectivePermissions when present (true if any of the three carved-out verbs is granted), keeping the seeded-group fallback for the loading window so it agrees with canAccess()'s carve-out path. F5 (frontend/src/history.ts): canApprovePendingRow / canRetryFailedRow and the read-only banner all hard-coded isPurchaser(). A user granted approve-any:purchases or retry-any:purchases via another group was rejected by the UI even though the backend allowed the action. Gate each path on the exact canAccess(verb, 'purchases') the buttons need, and derive the banner from the same predicate so UI and enforcement stay in lockstep. Regression coverage: * permissions.test.ts: new isPurchaser() suite (seeded-group fallback, explicit-grant via execute / approve-any / retry-any in effectivePermissions, admin-without-explicit-carve-out denied, empty-effective-permissions denied, null-user denied) + custom-group carve-out coverage in the canAccess suite. * history-approve-button.test.ts: admin-without-Purchaser MUST NOT see Approve on rows they did not create (catches a future regression to isAdmin() / isPurchaser() group-only gates). * history-retry-button.test.ts: matching admin-without-Purchaser retry-any negative case. * recommendations-permissions.test.ts: admin-without-Purchaser hides Purchase, keeps Create Plan, shows the no-Purchaser banner; custom non-seeded group with explicit execute:purchases shows Purchase and no banner (F3 + F4 together). --- .../__tests__/history-approve-button.test.ts | 37 ++++++ .../__tests__/history-retry-button.test.ts | 32 +++++ frontend/src/__tests__/permissions.test.ts | 114 +++++++++++++++++- .../recommendations-permissions.test.ts | 52 ++++++++ frontend/src/history.ts | 57 ++++++--- frontend/src/permissions.ts | 40 +++++- frontend/src/recommendations.ts | 14 ++- internal/auth/types.go | 7 +- .../000058_seed_purchaser_group.up.sql | 24 +++- 9 files changed, 341 insertions(+), 36 deletions(-) diff --git a/frontend/src/__tests__/history-approve-button.test.ts b/frontend/src/__tests__/history-approve-button.test.ts index 958266124..a6f5c4922 100644 --- a/frontend/src/__tests__/history-approve-button.test.ts +++ b/frontend/src/__tests__/history-approve-button.test.ts @@ -69,6 +69,14 @@ import { ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID } from '../permissions'; // 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 @@ -370,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 9ca21efe7..ba4f0cc15 100644 --- a/frontend/src/__tests__/history-retry-button.test.ts +++ b/frontend/src/__tests__/history-retry-button.test.ts @@ -76,6 +76,11 @@ import { ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID } from '../permissions'; // 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 { @@ -433,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 8c52ccc8b..dd4ab9292 100644 --- a/frontend/src/__tests__/permissions.test.ts +++ b/frontend/src/__tests__/permissions.test.ts @@ -14,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, PURCHASER_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', () => ({ @@ -343,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 944fbe5f5..fab581dcb 100644 --- a/frontend/src/__tests__/recommendations-permissions.test.ts +++ b/frontend/src/__tests__/recommendations-permissions.test.ts @@ -94,6 +94,15 @@ const mockUser = (role: string | null) => { ); }; +// 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 }, + ); +}; + const setupDom = () => { const recsTab = document.createElement('div'); recsTab.id = 'opportunities-tab'; @@ -176,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/history.ts b/frontend/src/history.ts index 2e5968a32..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, isPurchaser } from './permissions'; +import { canAccess } from './permissions'; import { showSkeletonRows, teardownSkeleton } from './lib/skeleton'; import { getAccountName } from './recommendations'; @@ -408,18 +408,24 @@ function canCancelPendingRow(p: HistoryPurchase): boolean { // // Heuristic: // * status must be "pending" or "notified"; -// * Purchaser-group member → approve-any (issue #923: approve-any is -// carved out of admin:* and requires explicit Purchaser membership); -// * non-Purchaser matching the row's created_by_user_id → approve-own; -// * legacy rows with NULL created_by_user_id → no (the email-token path -// remains the escape hatch). +// * 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; - // Purchaser group membership grants approve-any (issue #923). - if (isPurchaser()) 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; } @@ -436,9 +442,11 @@ function canApprovePendingRow(p: HistoryPurchase): boolean { // * 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); -// * Purchaser-group member → retry-any (issue #923: retry-any is -// carved out of admin:* and requires explicit Purchaser membership); -// * non-Purchaser matching the row's created_by_user_id → 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; @@ -446,8 +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; - // Purchaser group membership grants retry-any (issue #923). - if (isPurchaser()) 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,12 +591,19 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { lastPurchases = purchases; - // Issue #923: inject a read-only notice for sessions that lack Purchaser - // group membership. The Approve / Retry buttons in the table are gated by - // canApprovePendingRow / canRetryFailedRow (which use isPurchaser), so this - // banner explains why those buttons are absent for admin-only sessions. + // 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 && !isPurchaser()) { + if (!existingBanner && !hasAnyCarvedOut) { const banner = document.createElement('div'); banner.id = 'history-no-purchaser-banner'; banner.className = 'info-banner'; @@ -593,7 +612,7 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { '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 && isPurchaser()) { + } else if (existingBanner && hasAnyCarvedOut) { existingBanner.remove(); } diff --git a/frontend/src/permissions.ts b/frontend/src/permissions.ts index 717c36147..24946a7da 100644 --- a/frontend/src/permissions.ts +++ b/frontend/src/permissions.ts @@ -109,16 +109,44 @@ export function isAdmin(): boolean { } /** - * Return true when the current session user is a member of the - * Purchaser group. The three money-spending verbs - * (execute:purchases, approve-any:purchases, retry-any:purchases) are - * carved out of the admin:* wildcard on the backend (issue #923) and - * require explicit Purchaser-group membership (or any custom group - * that grants the same verbs via effectivePermissions). + * 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); } diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 030b99536..4c62ca3fa 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -22,7 +22,7 @@ import type { AccountServiceOverride } from './api/accounts'; import type { RecommendationsResponse, LocalRecommendation, RecommendationsSummary, GlobalConfig } from './types'; import { openModal } from './modal'; import { showSkeletonRows, teardownSkeleton } from './lib/skeleton'; -import { canAccess, isPurchaser } from './permissions'; +import { canAccess } from './permissions'; // Issue #869: true when the current session can take any action on // recommendations (purchase or plan). Readonly/viewer sessions have neither @@ -3457,11 +3457,13 @@ function mountBottomActionBox(): HTMLElement | null { box.appendChild(purchaseBtn); // Issue #923: show an informational banner when the session can view - // recommendations but cannot execute purchases (not in Purchaser group). - // The banner is visible only to users who have view access but lack - // the Purchaser group membership -- pure read-only users won't reach - // this page's action area anyway. - if (!isPurchaser()) { + // 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'); diff --git a/internal/auth/types.go b/internal/auth/types.go index e7b6892e8..e322fa82d 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -315,8 +315,11 @@ const DefaultAdminGroupID = "00000000-0000-5000-8000-000000000001" // of the admin:* wildcard (issue #923). const DefaultPurchaserGroupID = "00000000-0000-5000-8000-000000000005" -// GroupPurchaser is the canonical name of the system-managed Purchaser group. -const GroupPurchaser = "purchaser" +// 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 ( diff --git a/internal/database/postgres/migrations/000058_seed_purchaser_group.up.sql b/internal/database/postgres/migrations/000058_seed_purchaser_group.up.sql index 18a86621a..cd6a7b2dd 100644 --- a/internal/database/postgres/migrations/000058_seed_purchaser_group.up.sql +++ b/internal/database/postgres/migrations/000058_seed_purchaser_group.up.sql @@ -21,7 +21,27 @@ WHERE id IN ( '00000000-0000-5000-8000-000000000004' ); --- Insert the Purchaser group (idempotent on both id and name conflicts). +-- 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', @@ -39,7 +59,7 @@ VALUES ( ARRAY['*'], TRUE ) -ON CONFLICT DO NOTHING; +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