sec(auth): carve execute:ri-exchange out of admin:* and seed the granting group

frontend/src/__tests__/permissions.test.ts

@@ -4,17 +4,20 @@
  * 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
- * group-membership checks: admin passes everywhere EXCEPT the three
- * money-spending verbs carved out by issue #923, which require
- * explicit Purchaser-group membership.
+ * group-membership checks: admin passes everywhere EXCEPT the carved-out
+ * verbs -- the three money-spending verbs from issue #923 (require
+ * explicit Purchaser-group membership) and execute:ri-exchange from issue
+ * #1644 (requires explicit RI-Exchanger-group membership). Each carved-out
+ * verb is gated by the specific group that grants it back, not a single
+ * hardcoded group (PR #1758 review).
  *
  * isAdmin() returns true when the current user is a member of the
  * Administrators group (UUID 00000000-0000-5000-8000-000000000001).
  *
  * 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, isPurchaser, ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID } from '../permissions';
+import { canAccess, getRolePermissions, isAdmin, isPurchaser, isRIExchanger, ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID, RI_EXCHANGER_GROUP_ID } from '../permissions';
 import type { PermissionEntry } from '../api/types';
 
 jest.mock('../state', () => ({
@@ -156,9 +159,11 @@ describe('permissions', () => {
       expect(canAccess('view', 'users')).toBe(true);
       expect(canAccess('delete', 'plans')).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:ri-exchange is carved out of admin:* (issue #1644) and
+      // requires explicit RI-Exchanger-group membership; an admin-only
+      // member (no RI Exchanger membership) is refused during the
+      // fallback path just like the money-spending verbs.
+      expect(canAccess('execute', 'ri-exchange')).toBe(false);
       // execute:purchases is carved out of admin:* and requires Purchaser membership.
       expect(canAccess('execute', 'purchases')).toBe(false);
       expect(canAccess('approve-any', 'purchases')).toBe(false);
@@ -189,6 +194,38 @@ describe('permissions', () => {
       expect(canAccess('delete', 'plans')).toBe(false);
     });
 
+    test('Administrators + Purchaser (NOT RI Exchanger) is refused execute:ri-exchange', () => {
+      // PR #1758 F2: the fallback must consult the group that actually
+      // grants execute:ri-exchange, not fall through to Purchaser just
+      // because Purchaser grants the neighbouring money-spending verbs.
+      mockUserWithGroups([ADMIN_GID, PURCHASER_GROUP_ID]);
+      expect(canAccess('execute', 'ri-exchange')).toBe(false);
+      // Confirm the fix didn't regress the Purchaser verbs it shares a
+      // code path with.
+      expect(canAccess('execute', 'purchases')).toBe(true);
+    });
+
+    test('Administrators + RI Exchanger (NOT Purchaser) is allowed execute:ri-exchange but not purchases', () => {
+      mockUserWithGroups([ADMIN_GID, RI_EXCHANGER_GROUP_ID]);
+      expect(canAccess('execute', 'ri-exchange')).toBe(true);
+      // RI Exchanger membership must not also unlock the money-spending
+      // verbs -- the two carve-outs are granted by disjoint groups.
+      expect(canAccess('execute', 'purchases')).toBe(false);
+      expect(canAccess('approve-any', 'purchases')).toBe(false);
+      expect(canAccess('retry-any', 'purchases')).toBe(false);
+      // Non-carved-out admin actions remain available via admin:*.
+      expect(canAccess('view', 'users')).toBe(true);
+      expect(canAccess('delete', 'plans')).toBe(true);
+    });
+
+    test('RI Exchanger-only (no admin) passes execute:ri-exchange but not other admin actions', () => {
+      mockUserWithGroups([RI_EXCHANGER_GROUP_ID]);
+      expect(canAccess('execute', 'ri-exchange')).toBe(true);
+      expect(canAccess('admin', '*')).toBe(false);
+      expect(canAccess('view', 'users')).toBe(false);
+      expect(canAccess('execute', 'purchases')).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]);
@@ -473,5 +510,88 @@ describe('permissions', () => {
       mockNoUser();
       expect(isPurchaser()).toBe(false);
     });
+
+    test('explicit execute:ri-exchange grant alone does NOT make isPurchaser() true', () => {
+      // isPurchaser() must consult only the three money-spending verbs
+      // (PURCHASER_CARVED_OUTS), not the full carved-out set. Holding
+      // execute:ri-exchange (issue #1644, a disjoint carve-out with its
+      // own group) is not "can spend money" -- if isPurchaser() looped
+      // over every carved-out key it would wrongly return true here and
+      // the "add yourself to Purchaser" first-run prompt (userActions.ts)
+      // would wrongly stay hidden for an RI-Exchanger-only admin.
+      const customGid = '00000000-0000-5000-8000-00000000fade';
+      mockUserWithGroups([customGid], [
+        { action: 'execute', resource: 'ri-exchange' },
+      ]);
+      expect(isPurchaser()).toBe(false);
+    });
+
+    test('RI Exchanger group membership alone (no effectivePermissions) does NOT make isPurchaser() true', () => {
+      mockUserWithGroups([RI_EXCHANGER_GROUP_ID]);
+      expect(isPurchaser()).toBe(false);
+    });
+  });
+
+  describe('isRIExchanger', () => {
+    test('seeded RI Exchanger 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([RI_EXCHANGER_GROUP_ID]);
+      expect(isRIExchanger()).toBe(true);
+    });
+
+    test('user without RI Exchanger group and no effectivePermissions returns false', () => {
+      mockUserWithGroups([ADMIN_GID]); // admin only
+      expect(isRIExchanger()).toBe(false);
+    });
+
+    test('Purchaser group membership alone does NOT make isRIExchanger() true', () => {
+      // Inverse of the isPurchaser regression test above: the two
+      // carve-outs are granted by disjoint groups in both directions.
+      mockUserWithGroups([PURCHASER_GROUP_ID]);
+      expect(isRIExchanger()).toBe(false);
+    });
+
+    test('explicit execute:ri-exchange grant in effectivePermissions (custom group) returns true', () => {
+      const customGid = '00000000-0000-5000-8000-00000000b00c';
+      mockUserWithGroups([customGid], [
+        { action: 'execute', resource: 'ri-exchange' },
+        { action: 'view', resource: 'recommendations' },
+      ]);
+      expect(isRIExchanger()).toBe(true);
+    });
+
+    test('wildcard resource on execute grants RI Exchanger (matches canAccess semantics)', () => {
+      const customGid = '00000000-0000-5000-8000-00000000c0de';
+      mockUserWithGroups([customGid], [
+        { action: 'execute', resource: '*' },
+      ]);
+      expect(isRIExchanger()).toBe(true);
+    });
+
+    test('explicit execute:purchases grant does NOT make isRIExchanger() true', () => {
+      const customGid = '00000000-0000-5000-8000-00000000da7a';
+      mockUserWithGroups([customGid], [
+        { action: 'execute', resource: 'purchases' },
+      ]);
+      expect(isRIExchanger()).toBe(false);
+    });
+
+    test('admin:* in effectivePermissions WITHOUT explicit carved-out grant returns false', () => {
+      mockUserWithGroups([ADMIN_GID], [
+        { action: 'admin', resource: '*' },
+      ]);
+      expect(isRIExchanger()).toBe(false);
+    });
+
+    test('empty effectivePermissions array returns false even with RI_EXCHANGER_GROUP_ID', () => {
+      mockUserWithGroups([RI_EXCHANGER_GROUP_ID], []);
+      expect(isRIExchanger()).toBe(false);
+    });
+
+    test('null user (logged out) returns false', () => {
+      mockNoUser();
+      expect(isRIExchanger()).toBe(false);
+    });
   });
 });

frontend/src/permissions.ts

@@ -153,16 +153,45 @@ export const ADMINISTRATORS_GROUP_ID = '00000000-0000-5000-8000-000000000001';
  */
 export const PURCHASER_GROUP_ID = '00000000-0000-5000-8000-000000000007';
 
+/**
+ * Well-known group UUID for the RI Exchanger group seeded by migration
+ * 000096 (issue #1644). execute:ri-exchange is carved out of the admin:*
+ * wildcard and requires explicit membership in this group (or a custom
+ * group granting the same verb). Mirrors DefaultRIExchangerGroupID in
+ * internal/auth/types.go.
+ */
+export const RI_EXCHANGER_GROUP_ID = '00000000-0000-5000-8000-000000000008';
+
 /**
  * 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.
+ * wildcard. Mirrors adminCarvedOuts in internal/auth/types.go. Which
+ * group's membership grants each key back during the fallback path
+ * (effectivePermissions not yet loaded) is NOT uniform across this set --
+ * see CARVE_OUT_FALLBACK_CHECK below, which every entry here must also
+ * appear in.
  */
 const ADMIN_CARVED_OUTS: ReadonlySet<string> = new Set([
   'execute:purchases',
   'approve-any:purchases',
   'retry-any:purchases',
+  // execute:ri-exchange is carved out by issue #1644 and granted by the
+  // seeded RI Exchanger group (migration 000096), not by admin:*. If this
+  // set drifts from adminCarvedOuts the UI offers an action the backend
+  // then refuses with a 403.
+  'execute:ri-exchange',
+]);
+
+/**
+ * Subset of ADMIN_CARVED_OUTS specific to the three money-spending purchase
+ * verbs (issue #923). isPurchaser() consults only these -- NOT the full
+ * ADMIN_CARVED_OUTS set -- so that holding execute:ri-exchange alone (issue
+ * #1644, a disjoint carve-out with its own group) does not also satisfy the
+ * "can spend money" predicate the no-Purchaser banners key off.
+ */
+const PURCHASER_CARVED_OUTS: ReadonlySet<string> = new Set([
+  'execute:purchases',
+  'approve-any:purchases',
+  'retry-any:purchases',
 ]);
 
 /**
@@ -204,7 +233,7 @@ export function isPurchaser(): boolean {
     // 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) {
+    for (const key of PURCHASER_CARVED_OUTS) {
       const colon = key.indexOf(':');
       if (colon < 0) continue;
       const action = key.slice(0, colon);
@@ -220,27 +249,71 @@ export function isPurchaser(): boolean {
   return Array.isArray(user.groups) && user.groups.includes(PURCHASER_GROUP_ID);
 }
 
+/**
+ * Return true when the current session is authorised for the
+ * execute:ri-exchange carved-out verb (issue #1644). Mirrors isPurchaser()'s
+ * shape: when effectivePermissions has loaded, drive off the permission set
+ * itself so a user granted the verb via a custom group (not just the seeded
+ * RI Exchanger group) also returns true; while it is still loading, fall
+ * back to seeded RI-Exchanger-group membership so this helper agrees with
+ * canAccess()'s fallback in the same window.
+ */
+export function isRIExchanger(): boolean {
+  const user = state.getCurrentUser();
+  if (!user) return false;
+  if (user.effectivePermissions) {
+    for (const p of user.effectivePermissions) {
+      if (p.action === 'execute' && (p.resource === 'ri-exchange' || p.resource === '*')) {
+        return true;
+      }
+    }
+    return false;
+  }
+  return Array.isArray(user.groups) && user.groups.includes(RI_EXCHANGER_GROUP_ID);
+}
+
+/**
+ * Maps each carved-out (action:resource) key to the predicate that grants it
+ * back during the fallback (effectivePermissions not yet loaded) path.
+ * ADMIN_CARVED_OUTS mirrors the backend's *set* of carved-out verbs; this map
+ * mirrors which group's membership grants each one back, which is NOT
+ * uniform (Purchaser for the three money-spending verbs, RI Exchanger for
+ * execute:ri-exchange). A carved-out key missing from this map would be
+ * silently hardcoded to the wrong predicate here, which is exactly the bug
+ * this map replaces: canAccess() used to route every carved-out verb through
+ * isPurchaser() regardless of which group actually granted it (PR #1758
+ * review).
+ */
+const CARVE_OUT_FALLBACK_CHECK: ReadonlyMap<string, () => boolean> = new Map([
+  ['execute:purchases', isPurchaser],
+  ['approve-any:purchases', isPurchaser],
+  ['retry-any:purchases', isPurchaser],
+  ['execute:ri-exchange', isRIExchanger],
+]);
+
 /**
  * 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 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.
+ * consulted directly: admin:* grants everything EXCEPT the verbs carved
+ * out of admin:* by the backend (the three money-spending verbs from
+ * issue #923, plus execute:ri-exchange from issue #1644) -- those require
+ * an explicit (action, resource) entry in effectivePermissions (which the
+ * backend only returns when the user is in the group that grants the verb,
+ * or a custom group that grants it 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
- * 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.
+ * group-membership checks via CARVE_OUT_FALLBACK_CHECK: Administrators-
+ * group members pass every check EXCEPT the carved-out verbs, each of which
+ * requires membership in the specific group that grants it (Purchaser for
+ * the money-spending verbs, RI Exchanger for execute:ri-exchange). 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; a
  * wrong-positive surfaces as a 403 on click, a wrong-negative just
@@ -268,10 +341,11 @@ export function canAccess(action: Action, resource: Resource): boolean {
   }
 
   // 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.
+  // carve-out: admin grants everything except the carved-out verbs, each of
+  // which requires membership in the specific group that grants it back.
   if (isCarvedOut) {
-    return isPurchaser();
+    const check = CARVE_OUT_FALLBACK_CHECK.get(key);
+    return check !== undefined && check();
   }
   return isAdmin();
 }