diff --git a/frontend/src/__tests__/permissions.test.ts b/frontend/src/__tests__/permissions.test.ts index dc598cb1f..7bdd76b0f 100644 --- a/frontend/src/__tests__/permissions.test.ts +++ b/frontend/src/__tests__/permissions.test.ts @@ -4,9 +4,12 @@ * 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). @@ -14,7 +17,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, 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); + }); }); }); diff --git a/frontend/src/permissions.ts b/frontend/src/permissions.ts index 111f07eb2..9e76e5cd2 100644 --- a/frontend/src/permissions.ts +++ b/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 = 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 = 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 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(); } diff --git a/internal/api/ri_exchange_carveout_test.go b/internal/api/ri_exchange_carveout_test.go new file mode 100644 index 000000000..16aecef30 --- /dev/null +++ b/internal/api/ri_exchange_carveout_test.go @@ -0,0 +1,170 @@ +package api + +import ( + "context" + "testing" + + "github.com/LeanerCloud/CUDly/internal/auth" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Handler-level coverage for the execute:ri-exchange carve-out (issue #1644). +// +// Before this change admin:* granted execute:ri-exchange outright, so both +// routed execute endpoints -- POST /api/ri-exchange/execute (executeExchange) +// and POST /api/ri-exchange/azure-instances/exchange (executeAzureExchange) -- +// were reachable by any admin with no explicit grant, and the provider, +// region and MaxPurchaseAmount dimensions were skipped with it. +// +// Both directions are covered on purpose. A refusal-only test passes just as +// well against a handler that refuses everyone, which would hide the seeded +// RI Exchanger group failing to grant the verb at all. + +func riExchangeCarveoutRequest() *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer exchange-token"}, + } +} + +func riExchangeCarveoutHandler(t *testing.T, perms []auth.Permission) *Handler { + t.Helper() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"} + mockAuth.On("ValidateSession", context.Background(), "exchange-token").Return(session, nil) + mockAuth.grantPermissions(perms) + return &Handler{auth: mockAuth} +} + +// TestRIExchangeCarveOut_PlainAdminIsRefused pins the refusal direction: a +// principal holding only {admin, *} must not pass the execute:ri-exchange +// gate. This is the assertion that fails if the verb is dropped from +// adminCarvedOuts. +func TestRIExchangeCarveOut_PlainAdminIsRefused(t *testing.T) { + ctx := context.Background() + h := riExchangeCarveoutHandler(t, []auth.Permission{ + {Action: auth.ActionAdmin, Resource: auth.ResourceAll}, + }) + + got, err := h.requirePermission(ctx, riExchangeCarveoutRequest(), auth.ActionExecute, auth.ResourceRIExchange) + + require.Error(t, err, "admin:* must NOT grant execute:ri-exchange (issue #1644)") + assert.Nil(t, got) + ce, ok := IsClientError(err) + require.True(t, ok, "a carve-out denial must be a client error, not a 500") + assert.Equal(t, 403, ce.code) + assert.Contains(t, err.Error(), auth.ActionExecute) + assert.Contains(t, err.Error(), auth.ResourceRIExchange) +} + +// TestRIExchangeCarveOut_ExchangerIsAllowed pins the other direction: a +// principal holding execute:ri-exchange explicitly -- what membership in the +// seeded RI Exchanger group (migration 000096) confers -- passes. Without +// this, a handler that refused everyone would satisfy the test above. +func TestRIExchangeCarveOut_ExchangerIsAllowed(t *testing.T) { + ctx := context.Background() + h := riExchangeCarveoutHandler(t, []auth.Permission{ + {Action: auth.ActionAdmin, Resource: auth.ResourceAll}, + {Action: auth.ActionExecute, Resource: auth.ResourceRIExchange}, + }) + + got, err := h.requirePermission(ctx, riExchangeCarveoutRequest(), auth.ActionExecute, auth.ResourceRIExchange) + + require.NoError(t, err, "an explicit execute:ri-exchange grant must pass the gate") + assert.NotNil(t, got) +} + +// TestRIExchangeCarveOut_AdminKeepsNonCarvedVerbs is the scope control: the +// carve-out must remove exactly one pair, not narrow admin:* generally. It +// includes view:purchases, which the RI-exchange handlers themselves gate on, +// so a carve-out that over-reached would strand the read paths too. +func TestRIExchangeCarveOut_AdminKeepsNonCarvedVerbs(t *testing.T) { + ctx := context.Background() + for _, verb := range [][2]string{ + {auth.ActionView, auth.ResourcePurchases}, + {auth.ActionView, auth.ResourceRIExchange}, + {auth.ActionUpdate, auth.ResourceConfig}, + } { + action, resource := verb[0], verb[1] + t.Run(action+":"+resource, func(t *testing.T) { + h := riExchangeCarveoutHandler(t, []auth.Permission{ + {Action: auth.ActionAdmin, Resource: auth.ResourceAll}, + }) + got, err := h.requirePermission(ctx, riExchangeCarveoutRequest(), action, resource) + require.NoError(t, err, "admin:* must still grant %s:%s", action, resource) + assert.NotNil(t, got) + }) + } +} + +// TestExecuteExchange_PlainAdminIsRefused drives the REAL routed handler, +// not the permission predicate. +// +// The three tests above call requirePermission directly, which proves the +// carve-out set refuses the pair; it does not prove the endpoint does. Those +// are different claims, and #1757 is the standing example of the gap: a test +// named "...MUST be required" passed while calling a predicate the real +// dispatch never consults. This exercises executeExchange itself, the handler +// behind POST /api/ri-exchange/execute. +// +// The discriminating assertion is the error's IDENTITY, not its wording: +// requirePermission's carve-out denial is a *clientError with code 403 +// (handler.go's requireSessionPermission), and executeExchange returns it +// unwrapped (handler_ri_exchange.go:1713-1716). Asserting substrings of the +// message ("execute", "ri-exchange") is fragile in the wrong direction: if +// the carve-out is dropped, executeExchange proceeds into +// exchange.ExecuteExchange, which builds its own AWS clients from ambient +// credentials and fails at AWS credential/STS resolution before ever +// touching the store -- an error whose wording has nothing to do with +// permissions. A prior version of this test relied on that STS error's +// message happening not to contain "execute"/"ri-exchange", which discovers +// a regression only by accident and stops working the moment that wording +// changes. Asserting the 403 ClientError identity instead means the test +// fails for the right reason regardless of what the downstream AWS error +// says. +// +// mockStore's AssertNotCalled is defense-in-depth, not the guard that +// currently fires: executeExchange's success path never calls +// SaveRIExchangeRecord at all (only the scheduled auto-exchange path in +// pkg/exchange/auto.go does), so this assertion holds unconditionally on +// this handler and does not discriminate the mutation. It stays in case a +// future change routes this handler through the store. +// +// t.Setenv("AWS_EC2_METADATA_DISABLED", "true") bounds the AWS SDK client +// construction inside exchange.ExecuteExchange, reached only if the +// carve-out regresses (#1644), to fail fast against an unreachable +// credential source instead of depending on what credentials happen to be +// configured in whichever environment re-runs this test (tracked more +// broadly as #1760: executeExchange builds AWS clients from ambient +// credentials with no injected seam, unlike internal/server's +// riExchangeClients). +func TestExecuteExchange_PlainAdminIsRefused(t *testing.T) { + t.Setenv("AWS_EC2_METADATA_DISABLED", "true") + + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "exchange-token").Return(session, nil) + mockAuth.grantPermissions([]auth.Permission{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}) + + h := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer exchange-token"}, + Body: `{"ri_ids":["ri-1"],"target_offering_id":"off-1","region":"us-east-1","target_count":1,"max_payment_due_usd":"100.00"}`, + } + + result, err := h.executeExchange(ctx, req) + + require.Error(t, err, "a plain admin must not be able to execute an RI exchange (#1644)") + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "a carve-out denial must be a client error, not a downstream AWS failure") + assert.Equal(t, 403, ce.code, "must be refused at the permission gate, not fail later for an unrelated reason") + mockStore.AssertNotCalled(t, "SaveRIExchangeRecord", mock.Anything, mock.Anything) +} diff --git a/internal/auth/types.go b/internal/auth/types.go index cab2240a1..d39e15655 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -116,6 +116,12 @@ var adminCarvedOuts = map[[2]string]bool{ {ActionExecute, ResourcePurchases}: true, {ActionApproveAny, ResourcePurchases}: true, {ActionRetryAny, ResourcePurchases}: true, + // execute:ri-exchange (issue #1644). An RI exchange consumes existing + // commitments and buys replacements, and the provider APIs have no + // rollback once submitted, so it belongs under the same separation of + // duties as execute:purchases. Membership in the seeded RI Exchanger + // group (migration 000096) is what grants it. + {ActionExecute, ResourceRIExchange}: true, } // HasPermission checks if the auth context has a specific permission. @@ -320,6 +326,13 @@ const DefaultAdminGroupID = "00000000-0000-5000-8000-000000000001" // admin:* wildcard (issue #923). const DefaultPurchaserGroupID = "00000000-0000-5000-8000-000000000007" +// DefaultRIExchangerGroupID is the fixed UUID of the RI Exchanger group +// seeded by migration 000096. It holds execute:ri-exchange, carved out of +// the admin:* wildcard by issue #1644. 000008 is the next free id in the +// seeded namespace (000005 Standard Users, 000006 Read-Only Users, +// 000007 Purchaser). +const DefaultRIExchangerGroupID = "00000000-0000-5000-8000-000000000008" + // GroupPurchaser is the canonical name of the system-managed Purchaser // group. MUST match the literal name inserted by migration // 000059_seed_purchaser_group.up.sql so name-based lookups agree with diff --git a/internal/database/postgres/migrations/000096_seed_ri_exchanger_group.down.sql b/internal/database/postgres/migrations/000096_seed_ri_exchanger_group.down.sql new file mode 100644 index 000000000..353586e05 --- /dev/null +++ b/internal/database/postgres/migrations/000096_seed_ri_exchanger_group.down.sql @@ -0,0 +1,18 @@ +-- Reverse 000096: detach every user from RI Exchanger, then drop the group. +-- +-- Order matters: group_ids is a plain UUID array with no FK, so a dropped +-- group would otherwise leave dangling ids that collectGroupsAndAccounts +-- silently skips, making the rollback look clean while leaving debris. + +DO $$ +DECLARE + v_uuid UUID := '00000000-0000-5000-8000-000000000008'; +BEGIN + UPDATE users + SET group_ids = array_remove(COALESCE(group_ids, '{}'), v_uuid) + WHERE v_uuid = ANY(COALESCE(group_ids, '{}')); + + -- Only remove the seeded row. A group an operator renamed onto this id + -- is not ours to drop. + DELETE FROM groups WHERE id = v_uuid AND name = 'RI Exchanger'; +END $$; diff --git a/internal/database/postgres/migrations/000096_seed_ri_exchanger_group.up.sql b/internal/database/postgres/migrations/000096_seed_ri_exchanger_group.up.sql new file mode 100644 index 000000000..b15d1c4e9 --- /dev/null +++ b/internal/database/postgres/migrations/000096_seed_ri_exchanger_group.up.sql @@ -0,0 +1,96 @@ +-- Seed the RI Exchanger system-managed group (issue #1644). +-- +-- execute:ri-exchange is added to adminCarvedOuts in the same change, so +-- admin:* alone no longer grants it. Without a group that DOES grant it the +-- verb would be unreachable for every principal: PR #1737's grant ceiling +-- refuses to add a carved-out verb to any group through the API, and no +-- migration seeded it. Both routed execute endpoints +-- (POST /api/ri-exchange/execute, POST /api/ri-exchange/azure-instances/exchange) +-- would then 403 for everyone. This migration is what keeps the verb +-- grantable, mirroring what 000059/000064 do for execute:purchases (#923). +-- +-- 000008 is the next free id in the seeded namespace: +-- 000005 = Standard Users, 000006 = Read-Only Users, 000007 = Purchaser. +-- Keep DefaultRIExchangerGroupID (internal/auth/types.go) and +-- RI_EXCHANGER_GROUP_ID (frontend/src/permissions.ts) in step with it. + +DO $$ +DECLARE + v_uuid UUID := '00000000-0000-5000-8000-000000000008'; + v_admin_uuid UUID := '00000000-0000-5000-8000-000000000001'; + v_occupant TEXT; +BEGIN + -- Guard: fail hard rather than silently no-op if the target UUID is + -- already held by a different group. Migration 000059 shipped a bare + -- ON CONFLICT (id) DO NOTHING onto an occupied id and the seed was + -- skipped on every database, which is the bug 000064 had to repair + -- (#942). Fail loudly instead. + SELECT name INTO v_occupant + FROM groups + WHERE id = v_uuid AND name <> 'RI Exchanger'; + + IF FOUND THEN + RAISE EXCEPTION + 'migration 000096: UUID % is already claimed by group ''%''; ' + 'choose a different UUID for RI Exchanger before applying this migration', + v_uuid, v_occupant; + END IF; + + -- Same guard on the name: a pre-existing "RI Exchanger" under a + -- different id would leave two rows competing for the same meaning. + IF EXISTS (SELECT 1 FROM groups WHERE name = 'RI Exchanger' AND id <> v_uuid) THEN + RAISE EXCEPTION + 'migration 000096: a group named ''RI Exchanger'' already exists with a different id; ' + 'rename it before applying this migration so the seeded id (%) can be created', + v_uuid; + END IF; + + -- The grant is deliberately UNCONSTRAINED (no providers/regions/ + -- MaxPurchaseAmount). A migration cannot know an operator's accounts, + -- regions or spend ceiling, and an over-narrow seed would refuse + -- legitimate exchanges on upgrade. The consequence is stated plainly in + -- the PR and issue #1644: permissionsAllow short-circuits on an + -- unconstrained grant, so members bypass the constraint dimensions + -- exactly as admin:* does today. Operators who want those dimensions + -- enforced must grant execute:ri-exchange through a custom group with + -- constraints instead of relying on this seed. + INSERT INTO groups (id, name, description, permissions, allowed_accounts, system_managed) + VALUES ( + v_uuid, + 'RI Exchanger', + 'Execute RI exchanges. Membership is required even for admins, because an exchange consumes existing commitments and cannot be rolled back (separation of duties, issues #923 and #1644).', + '[ + {"action":"execute","resource":"ri-exchange"}, + {"action":"view","resource":"recommendations"}, + {"action":"view","resource":"purchases"}, + {"action":"view","resource":"history"} + ]'::jsonb, + ARRAY['*'], + TRUE + ) + ON CONFLICT (id) DO NOTHING; + + -- Admin-backfill: every member of Administrators also joins RI Exchanger. + -- + -- Without this, every existing admin loses RI-exchange execute the moment + -- this deploys, discovered in production. That mirrors 000059/000064 for + -- Purchaser, and keeps the two carve-outs in the same set behaving the + -- same way. It does mean the carve-out changes little operationally for + -- the existing admin population; what it does buy is that membership is + -- now revocable and auditable independently of the admin role, that new + -- principals need a deliberate grant rather than inheriting the verb from + -- admin:*, and that the API grant path is closed. + -- + -- Idempotent: the NOT (...) guard skips existing members and + -- DISTINCT(unnest) deduplicates. The EXISTS guard leaves no orphaned + -- array entries if the INSERT above was skipped. + UPDATE users + SET group_ids = ARRAY( + SELECT DISTINCT unnest( + COALESCE(group_ids, '{}') || ARRAY[v_uuid] + ) + ) + WHERE v_admin_uuid = ANY(COALESCE(group_ids, '{}')) + AND NOT (v_uuid = ANY(COALESCE(group_ids, '{}'))) + AND EXISTS (SELECT 1 FROM groups WHERE id = v_uuid); +END $$; diff --git a/internal/database/postgres/migrations/000096_seed_ri_exchanger_group_test.go b/internal/database/postgres/migrations/000096_seed_ri_exchanger_group_test.go new file mode 100644 index 000000000..95d4403d1 --- /dev/null +++ b/internal/database/postgres/migrations/000096_seed_ri_exchanger_group_test.go @@ -0,0 +1,142 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" + "github.com/LeanerCloud/CUDly/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// riExchangerGroupIDTest is the UUID migration 000096 seeds the RI Exchanger +// group at (issue #1644). Must match DefaultRIExchangerGroupID in +// internal/auth/types.go and RI_EXCHANGER_GROUP_ID in frontend/src/permissions.ts. +const riExchangerGroupIDTest = "00000000-0000-5000-8000-000000000008" + +// TestMigration_SeedRIExchangerGroup covers issue #1644. +// +// The carve-out and this seed are only correct together: adding +// execute:ri-exchange to adminCarvedOuts without a group that grants it +// leaves the verb unreachable for everyone, because PR #1737's grant ceiling +// refuses to add a carved-out verb through the API. A seed migration that +// silently no-ops therefore does not merely miss a nice-to-have, it takes +// both routed execute endpoints offline -- which is exactly how 000059 failed +// (#942), by running ON CONFLICT DO NOTHING onto an already-occupied UUID. +// These assertions exist so that failure is loud. +func TestMigration_SeedRIExchangerGroup(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + + t.Run("RI Exchanger seeded, system-managed, granting execute:ri-exchange", func(t *testing.T) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + + var name string + var systemManaged bool + err = pool.QueryRow(ctx, + `SELECT name, system_managed FROM groups WHERE id = $1`, riExchangerGroupIDTest). + Scan(&name, &systemManaged) + require.NoError(t, err, "RI Exchanger group must exist at UUID %s", riExchangerGroupIDTest) + assert.Equal(t, "RI Exchanger", name) + assert.True(t, systemManaged, + "the group must be system-managed so the API cannot edit or delete it") + + // Round-trip by name, so a second row under a different id would fail. + var id string + err = pool.QueryRow(ctx, `SELECT id FROM groups WHERE name = 'RI Exchanger'`).Scan(&id) + require.NoError(t, err, "exactly one group named 'RI Exchanger' must exist") + assert.Equal(t, riExchangerGroupIDTest, id) + + // The verb itself. Asserting the group exists is not enough: a seed + // that created the row with the wrong permission list would leave the + // carve-out ungrantable just as surely as no row at all. + var grantsExecute bool + err = pool.QueryRow(ctx, ` + SELECT EXISTS ( + SELECT 1 FROM groups + WHERE id = $1 + AND permissions @> '[{"action":"execute","resource":"ri-exchange"}]'::jsonb + )`, riExchangerGroupIDTest).Scan(&grantsExecute) + require.NoError(t, err) + assert.True(t, grantsExecute, + "RI Exchanger must grant execute:ri-exchange, or the carved-out verb is unreachable for every principal") + }) + + t.Run("admin-backfill: Administrators members land in RI Exchanger", func(t *testing.T) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + // Pin just below 000096 so the backfill is observed firing, rather + // than inferred from the end state. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 95)) + + const adminEmail = "admin-riexchanger-test@test.example" + _, err = pool.Exec(ctx, ` + INSERT INTO users (id, email, password_hash, salt, active, group_ids, created_at, updated_at) + VALUES (gen_random_uuid(), $1, '', '', true, ARRAY[$2::uuid], NOW(), NOW()) + `, adminEmail, adminGroupIDForPurchaserTest) + require.NoError(t, err) + + before := queryGroupIDsByEmail(t, ctx, pool, adminEmail) + require.NotContains(t, before, riExchangerGroupIDTest, + "precondition: the admin must not already be an RI Exchanger") + + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + + after := queryGroupIDsByEmail(t, ctx, pool, adminEmail) + assert.Contains(t, after, riExchangerGroupIDTest, + "every Administrators member must be backfilled into RI Exchanger, or admins lose RI-exchange execute on upgrade") + assert.Contains(t, after, adminGroupIDForPurchaserTest, + "the backfill must add a group, not replace the user's existing membership") + }) + + t.Run("backfill is idempotent and leaves no duplicate entry", func(t *testing.T) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + + const adminEmail = "admin-riexchanger-idempotent@test.example" + _, err = pool.Exec(ctx, ` + INSERT INTO users (id, email, password_hash, salt, active, group_ids, created_at, updated_at) + VALUES (gen_random_uuid(), $1, '', '', true, ARRAY[$2::uuid, $3::uuid], NOW(), NOW()) + `, adminEmail, adminGroupIDForPurchaserTest, riExchangerGroupIDTest) + require.NoError(t, err) + + // Re-running migrations.RunMigrations here would NOT exercise the + // idempotency guards below: m.Up() returns ErrNoChange once the + // database is already at the latest version, so the migration body + // never runs a second time and this subtest would pass unconditionally + // regardless of whether the DO block's guards work. Reading the up + // migration file and executing its SQL directly re-applies the DO + // block for real, the same way 000095's re-run test does. + upSQL, err := os.ReadFile(filepath.Join(migrationsPath, "000096_seed_ri_exchanger_group.up.sql")) + require.NoError(t, err, "the up migration file must be readable") + _, err = pool.Exec(ctx, string(upSQL)) + require.NoError(t, err, "re-running 000096 on an already-seeded database must be a no-op, not an error") + + after := queryGroupIDsByEmail(t, ctx, pool, adminEmail) + count := 0 + for _, id := range after { + if id == riExchangerGroupIDTest { + count++ + } + } + assert.Equal(t, 1, count, "RI Exchanger must appear exactly once in group_ids") + }) +}