From ac50fd9f2017709aefe31aaddbb8b7c3a8875339 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 06:26:37 +0200 Subject: [PATCH 1/5] sec(auth): carve execute:ri-exchange out of admin:* and seed the granting group execute:ri-exchange was absent from adminCarvedOuts, so permissionsAllow short-circuited on the admin:* wildcard and returned true unconditionally. Both routed execute endpoints, POST /api/ri-exchange/execute and POST /api/ri-exchange/azure-instances/exchange, were reachable by any admin with no explicit grant, and the provider/region/MaxPurchaseAmount dimensions were skipped along with the verb. An exchange consumes existing commitments and buys replacements with no rollback path, which is the same rationale that put execute:purchases behind separation of duties in #923. The carve-out alone would have been an outage rather than a partial fix. PR #1737's grant ceiling refuses to add a carved-out verb to any group through the API, and no migration seeded one, so the verb would have been grantable to nobody and both endpoints would 403 for every principal. execute:purchases survives its own carve-out only because 000059/000064 seed Purchaser and backfill admins into it. Migration 000096 does the same here: it seeds a system-managed RI Exchanger group at the next free namespace UUID and backfills every Administrators member, so no admin loses the capability on upgrade. The seeded grant is deliberately unconstrained, because a migration cannot know an operator's accounts, regions or spend ceiling and an over-narrow seed would refuse legitimate exchanges. Verified by execution against permissionsAllow: an unconstrained grant still short-circuits the constraint dimensions, so backfilled members bypass them exactly as admin:* does today. What the carve-out does buy is that the verb can no longer be granted through the API, that new principals need a deliberate grant instead of inheriting it from the wildcard, and that membership is revocable and auditable independently of the admin role. Constraint enforcement for the seeded population remains open on #1644. Coverage runs both directions, because a refusal-only test passes equally well against a handler that refuses everyone: a plain admin:* principal is refused, a holder of an explicit execute:ri-exchange grant is allowed, and a scope control pins that admin:* still grants the neighbouring non-carved verbs including view:ri-exchange. Mutation-verified per test, run alone: removing the pair from adminCarvedOuts fails the refusal test and leaves the other two passing, which is the correct dependency shape. The frontend ADMIN_CARVED_OUTS mirror is updated in the same change; drift there would offer an action in the UI that the backend then refuses. Closes #1644 --- frontend/src/permissions.ts | 14 ++ internal/api/ri_exchange_carveout_test.go | 100 +++++++++++++ internal/auth/types.go | 13 ++ .../000096_seed_ri_exchanger_group.down.sql | 18 +++ .../000096_seed_ri_exchanger_group.up.sql | 96 +++++++++++++ .../000096_seed_ri_exchanger_group_test.go | 131 ++++++++++++++++++ 6 files changed, 372 insertions(+) create mode 100644 internal/api/ri_exchange_carveout_test.go create mode 100644 internal/database/postgres/migrations/000096_seed_ri_exchanger_group.down.sql create mode 100644 internal/database/postgres/migrations/000096_seed_ri_exchanger_group.up.sql create mode 100644 internal/database/postgres/migrations/000096_seed_ri_exchanger_group_test.go diff --git a/frontend/src/permissions.ts b/frontend/src/permissions.ts index 111f07eb2..4bf781184 100644 --- a/frontend/src/permissions.ts +++ b/frontend/src/permissions.ts @@ -153,6 +153,15 @@ 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. @@ -163,6 +172,11 @@ 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', ]); /** diff --git a/internal/api/ri_exchange_carveout_test.go b/internal/api/ri_exchange_carveout_test.go new file mode 100644 index 000000000..2e2acaedc --- /dev/null +++ b/internal/api/ri_exchange_carveout_test.go @@ -0,0 +1,100 @@ +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/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) + }) + } +} 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..d703e144e --- /dev/null +++ b/internal/database/postgres/migrations/000096_seed_ri_exchanger_group_test.go @@ -0,0 +1,131 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "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 the seed must not duplicate the array entry. + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + + 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") + }) +} From f7df33e96dcc3e3b30f0a3237db21d15b7e33b69 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 06:36:19 +0200 Subject: [PATCH 2/5] test(api): drive the routed execute handler, not just the permission predicate The three existing carve-out tests call requirePermission directly. That proves adminCarvedOuts refuses the pair; it does not prove the endpoint refuses, and those are different claims. #1757 is the standing example of the gap in this same file: a test named "...MUST be required" passes while calling a predicate the real dispatch never consults. Adds coverage that exercises executeExchange itself, the handler behind POST /api/ri-exchange/execute, with a plain admin:* principal. mockStore is stubbed with no expectations so a regression reaches the store and panics rather than quietly returning 200, and the AssertNotCalled passes two matchers because SaveRIExchangeRecord hands two arguments to m.Called (a name-only form could never fail: #1595/#1740). The request body is the real ExchangeExecuteRequestBody shape. The first draft used invented field names, and the mutation run exposed it: with the carve-out removed the handler stopped at "ri_ids is required" rather than proceeding, so the no-state-change assertion was satisfied by body validation instead of by the guard under test -- the semantically-inert fixture shape from #1735. With a valid body the mutation now carries execution past the gate and into the handler proper, so the assertion discriminates. Refs #1644 --- internal/api/ri_exchange_carveout_test.go | 41 +++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/internal/api/ri_exchange_carveout_test.go b/internal/api/ri_exchange_carveout_test.go index 2e2acaedc..8f400400c 100644 --- a/internal/api/ri_exchange_carveout_test.go +++ b/internal/api/ri_exchange_carveout_test.go @@ -7,6 +7,7 @@ import ( "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" ) @@ -98,3 +99,43 @@ func TestRIExchangeCarveOut_AdminKeepsNonCarvedVerbs(t *testing.T) { }) } } + +// 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. +// +// mockStore is stubbed with no expectations, so if the carve-out ever stops +// refusing, the handler reaches the store and testify panics rather than +// quietly returning a 200. The explicit per-parameter matchers keep the +// AssertNotCalled non-vacuous either way (#1595/#1740): SaveRIExchangeRecord +// hands two arguments to m.Called, so two matchers are required. +func TestExecuteExchange_PlainAdminIsRefused(t *testing.T) { + 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) + assert.Contains(t, err.Error(), auth.ActionExecute) + assert.Contains(t, err.Error(), auth.ResourceRIExchange) + mockStore.AssertNotCalled(t, "SaveRIExchangeRecord", mock.Anything, mock.Anything) +} From 1de31a49fd80663797740d03ef6185e5b7137091 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 07:28:42 +0200 Subject: [PATCH 3/5] fix(frontend): route each carved-out verb through the group that grants it back canAccess()'s loading-race fallback (effectivePermissions not yet loaded) hardcoded every carved-out verb to isPurchaser(), so it agreed with the backend for the three money-spending verbs but not for execute:ri-exchange (issue #1644): admin+Purchaser (not RI Exchanger) wrongly passed, and admin+RI-Exchanger (not Purchaser) wrongly failed. Adds isRIExchanger(), mirroring isPurchaser()'s shape, and routes both through a CARVE_OUT_FALLBACK_CHECK map keyed by carved-out verb instead of a single hardcoded predicate. Also fixes isPurchaser() itself, which iterated the full ADMIN_CARVED_OUTS set (now including execute:ri-exchange) instead of the three money-spending verbs it's actually meant to gate -- holding execute:ri-exchange alone would have wrongly satisfied the "can spend money" predicate the no-Purchaser first-run prompt keys off. Fixes the stale permissions.test.ts assertion that still expected execute:ri-exchange to pass admin:* during the fallback (pre-carve-out behaviour), and adds both-direction coverage for the fix plus regression tests pinning the two carve-outs' groups as disjoint. UX-only gate; the backend enforces on every request regardless of this fallback's answer. --- frontend/src/__tests__/permissions.test.ts | 134 +++++++++++++++++++-- frontend/src/permissions.ts | 98 ++++++++++++--- 2 files changed, 206 insertions(+), 26 deletions(-) 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 4bf781184..9e76e5cd2 100644 --- a/frontend/src/permissions.ts +++ b/frontend/src/permissions.ts @@ -164,9 +164,11 @@ 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', @@ -179,6 +181,19 @@ const ADMIN_CARVED_OUTS: ReadonlySet = new Set([ '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', +]); + /** * Return true when the current session user is a member of the * Administrators group. This replaces the former `user.role === "admin"` @@ -218,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); @@ -234,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 @@ -282,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(); } From e73b13e8e6d22d1973a0bcc6a6cca82e540c0470 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 07:30:05 +0200 Subject: [PATCH 4/5] test(migrations): make the 000096 idempotency subtest actually re-run the migration The "backfill is idempotent" subtest called migrations.RunMigrations a second time expecting it to re-exercise the DO block's guards. It does not: m.Up() returns migrate.ErrNoChange once the database is already at the latest version, so the migration body never runs again and the subtest passed regardless of whether the guards worked. Proved by mutation: stripping both idempotency devices from the up migration (the WHERE NOT guard and the DISTINCT dedup on the backfill UPDATE) still left the old test green. Rewrites the subtest to read 000096_seed_ri_exchanger_group.up.sql and execute its SQL directly a second time, the same pattern 000095_purchase_history_account_id_width_test.go already uses for its re-run assertion. Re-verified by the same mutation: with the fix, stripping the guards now fails the subtest (duplicate group_ids entry), and restoring them passes it again. The migration itself was already correctly idempotent; only the test coverage was empty. --- .../000096_seed_ri_exchanger_group_test.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) 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 index d703e144e..95d4403d1 100644 --- a/internal/database/postgres/migrations/000096_seed_ri_exchanger_group_test.go +++ b/internal/database/postgres/migrations/000096_seed_ri_exchanger_group_test.go @@ -5,6 +5,8 @@ package migrations_test import ( "context" + "os" + "path/filepath" "testing" "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" @@ -116,8 +118,17 @@ func TestMigration_SeedRIExchangerGroup(t *testing.T) { `, adminEmail, adminGroupIDForPurchaserTest, riExchangerGroupIDTest) require.NoError(t, err) - // Re-running the seed must not duplicate the array entry. - require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + // 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 From 4eab2b599814542ad2ad2d9c7f6c0516a5bb2e86 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 07:44:52 +0200 Subject: [PATCH 5/5] test(api): assert the 403 identity, not error-string tokens, in the execute:ri-exchange handler test TestExecuteExchange_PlainAdminIsRefused discriminated a dropped carve-out by checking that the error message did NOT contain "execute"/"ri-exchange". Under mutation the error actually returned is an AWS credential/STS failure from exchange.ExecuteExchange (reached only once the carve-out stops refusing), whose wording has nothing to do with permissions -- the test caught the regression by accident, because that unrelated error string happened not to contain those two tokens. If the wording of that AWS error ever changed, the test would stop discriminating while staying green. Replaces the substring assertions with an identity check: the carve-out denial from requirePermission is a *clientError with code 403 (requireSessionPermission in handler.go), returned unwrapped by executeExchange, so asserting IsClientError + code 403 fails for the actual reason -- refused at the gate vs. failed downstream for something else. Verified by mutation, run in a hard-sandboxed AWS environment (nulled credentials/config files, disabled profile, IMDS disabled) so the mutated test cannot reach live AWS: with adminCarvedOuts stripped of the execute:ri-exchange pair, the new 403-identity assertion is the one that fails (0.00s, no AWS contact); restoring the pair passes again. Also corrects the docstring, which claimed the unstubbed mockStore backstop would panic if the carve-out regressed. It cannot: executeExchange's success path never calls SaveRIExchangeRecord at all (only the scheduled auto-exchange path in pkg/exchange/auto.go does), so AssertNotCalled holds unconditionally here and does not discriminate this test today. Kept as defense-in-depth for a future change that routes this handler through the store, documented as such. Adds t.Setenv("AWS_EC2_METADATA_DISABLED", "true") so the credential resolution failure this test depends on under mutation is fast and deterministic regardless of what AWS credentials happen to be configured in whichever environment re-runs it later, rather than depending on IMDS being unreachable by chance. executeExchange builds its AWS clients from ambient credentials with no injected seam (unlike internal/server's riExchangeClients) -- that structural gap is tracked separately as #1760; this is containment for the test, not a fix to the seam. --- internal/api/ri_exchange_carveout_test.go | 43 +++++++++++++++++++---- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/internal/api/ri_exchange_carveout_test.go b/internal/api/ri_exchange_carveout_test.go index 8f400400c..16aecef30 100644 --- a/internal/api/ri_exchange_carveout_test.go +++ b/internal/api/ri_exchange_carveout_test.go @@ -110,12 +110,40 @@ func TestRIExchangeCarveOut_AdminKeepsNonCarvedVerbs(t *testing.T) { // dispatch never consults. This exercises executeExchange itself, the handler // behind POST /api/ri-exchange/execute. // -// mockStore is stubbed with no expectations, so if the carve-out ever stops -// refusing, the handler reaches the store and testify panics rather than -// quietly returning a 200. The explicit per-parameter matchers keep the -// AssertNotCalled non-vacuous either way (#1595/#1740): SaveRIExchangeRecord -// hands two arguments to m.Called, so two matchers are required. +// 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) @@ -135,7 +163,8 @@ func TestExecuteExchange_PlainAdminIsRefused(t *testing.T) { require.Error(t, err, "a plain admin must not be able to execute an RI exchange (#1644)") assert.Nil(t, result) - assert.Contains(t, err.Error(), auth.ActionExecute) - assert.Contains(t, err.Error(), auth.ResourceRIExchange) + 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) }