diff --git a/frontend/src/permissions.ts b/frontend/src/permissions.ts index 75a9d318e..fecf06d46 100644 --- a/frontend/src/permissions.ts +++ b/frontend/src/permissions.ts @@ -77,13 +77,14 @@ export type Resource = export const ADMINISTRATORS_GROUP_ID = '00000000-0000-5000-8000-000000000001'; /** - * Well-known group UUID for the Purchaser group seeded by migration - * 000058 (issue #923). The three money-spending verbs - * (execute:purchases, approve-any:purchases, retry-any:purchases) are - * carved out of the admin:* wildcard and require explicit membership - * in this group (or a custom group that grants the same verbs). + * Well-known group UUID for the Purchaser group relocated by migration + * 000064 (issue #942; originally seeded for issue #923). The three + * money-spending verbs (execute:purchases, approve-any:purchases, + * retry-any:purchases) are carved out of the admin:* wildcard and + * require explicit membership in this group (or a custom group that + * grants the same verbs). */ -export const PURCHASER_GROUP_ID = '00000000-0000-5000-8000-000000000005'; +export const PURCHASER_GROUP_ID = '00000000-0000-5000-8000-000000000007'; /** * The set of (action, resource) pairs carved out of the admin:* diff --git a/internal/auth/types.go b/internal/auth/types.go index 827dfa075..4958182d7 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -310,10 +310,11 @@ const ( // group so the group card shows members on a fresh install. const DefaultAdminGroupID = "00000000-0000-5000-8000-000000000001" -// DefaultPurchaserGroupID is the fixed UUID of the Purchaser group seeded -// by migration 000054. It holds the three money-spending verbs carved out -// of the admin:* wildcard (issue #923). -const DefaultPurchaserGroupID = "00000000-0000-5000-8000-000000000005" +// DefaultPurchaserGroupID is the fixed UUID of the Purchaser group, relocated +// by migration 000064 to resolve the UUID collision with "Standard Users" +// (issue #942). It holds the three money-spending verbs carved out of the +// admin:* wildcard (issue #923). +const DefaultPurchaserGroupID = "00000000-0000-5000-8000-000000000007" // GroupPurchaser is the canonical name of the system-managed Purchaser // group. MUST match the literal name inserted by migration diff --git a/internal/database/postgres/migrations/000064_relocate_purchaser_group.down.sql b/internal/database/postgres/migrations/000064_relocate_purchaser_group.down.sql new file mode 100644 index 000000000..5ee928b7e --- /dev/null +++ b/internal/database/postgres/migrations/000064_relocate_purchaser_group.down.sql @@ -0,0 +1,13 @@ +-- Intentionally a no-op. +-- +-- Reversing the UUID relocation of the Purchaser group is not safely +-- possible: external references (user group_ids, audit logs, API tokens, +-- and application constants) may have been written against the new UUID +-- (00000000-0000-5000-8000-000000000007) since this migration applied. +-- Rolling back to the old UUID (00000000-0000-5000-8000-000000000005) would +-- silently break every one of those references. +-- +-- To revert manually: remove Purchaser memberships from all users, delete +-- the row at 00000000-0000-5000-8000-000000000007, and revert the constants +-- in internal/auth/types.go and frontend/src/permissions.ts. +SELECT 1; diff --git a/internal/database/postgres/migrations/000064_relocate_purchaser_group.up.sql b/internal/database/postgres/migrations/000064_relocate_purchaser_group.up.sql new file mode 100644 index 000000000..61d5c8f91 --- /dev/null +++ b/internal/database/postgres/migrations/000064_relocate_purchaser_group.up.sql @@ -0,0 +1,102 @@ +-- Relocate the Purchaser system-managed group to a free UUID. +-- +-- Root cause (issue #942): migration 000057_drop_user_role_to_groups.up.sql +-- claimed UUID 00000000-0000-5000-8000-000000000005 for "Standard Users". +-- Migration 000059_seed_purchaser_group.up.sql tried to seed "Purchaser" +-- at the same UUID and ended with ON CONFLICT (id) DO NOTHING, so the +-- Purchaser INSERT was silently no-op'd on every database that ran both +-- migrations. The admin-backfill in 000059 then attached admins to +-- "Standard Users" (the row at ...000005), not to Purchaser, leaving full +-- admins without execute:purchases / approve-any:purchases / retry-any:purchases +-- despite holding admin:*. +-- +-- Fix: assign Purchaser to the next free UUID in the seeded namespace: +-- 00000000-0000-5000-8000-000000000007 +-- (000005 = Standard Users, 000006 = Read-Only Users, 000007 = Purchaser) +-- Update DefaultPurchaserGroupID in internal/auth/types.go and +-- PURCHASER_GROUP_ID in frontend/src/permissions.ts to match. + +DO $$ +DECLARE + v_new_uuid UUID := '00000000-0000-5000-8000-000000000007'; + v_old_uuid UUID; + v_occupant TEXT; +BEGIN + -- Guard: fail hard if the target UUID is already taken by a + -- non-Purchaser row. This prevents the same class of silent UUID + -- collision that caused the original bug. + SELECT name INTO v_occupant + FROM groups + WHERE id = v_new_uuid AND name <> 'Purchaser'; + + IF FOUND THEN + RAISE EXCEPTION + 'migration 000064: UUID % is already claimed by group ''%''; ' + 'choose a different UUID for Purchaser before applying this migration', + v_new_uuid, v_occupant; + END IF; + + -- Case 1: "Purchaser" exists but at a non-target UUID (e.g. a partial + -- repair attempt). Capture the current id, swap user references, then + -- relocate the group row to the canonical UUID. + SELECT id INTO v_old_uuid + FROM groups + WHERE name = 'Purchaser' AND id <> v_new_uuid; + + IF FOUND THEN + -- Swap the stale UUID for the canonical one in every user's + -- group_ids array before updating the groups PK to avoid FK issues. + UPDATE users + SET group_ids = ARRAY( + SELECT DISTINCT unnest( + array_remove(COALESCE(group_ids, '{}'), v_old_uuid) || + ARRAY[v_new_uuid] + ) + ) + WHERE v_old_uuid = ANY(COALESCE(group_ids, '{}')); + + UPDATE groups SET id = v_new_uuid WHERE id = v_old_uuid; + + -- Case 2: No "Purchaser" row exists at all (the common bug-case on + -- any DB that ran 000057 before 000059). Insert a fresh one. + ELSIF NOT EXISTS (SELECT 1 FROM groups WHERE name = 'Purchaser') THEN + INSERT INTO groups (id, name, description, permissions, allowed_accounts, system_managed) + VALUES ( + v_new_uuid, + 'Purchaser', + 'Execute, approve, and retry purchases. Membership is required even for admins to spend money (separation of duties, issue #923).', + '[ + {"action":"execute","resource":"purchases"}, + {"action":"approve-any","resource":"purchases"}, + {"action":"retry-any","resource":"purchases"}, + {"action":"view","resource":"recommendations"}, + {"action":"view","resource":"plans"}, + {"action":"view","resource":"purchases"}, + {"action":"view","resource":"history"} + ]'::jsonb, + ARRAY['*'], + TRUE + ); + + END IF; + + -- Case 3: "Purchaser" already sits at v_new_uuid. + -- Nothing to relocate; fall through to the backfill below. + + -- Admin-backfill: ensure every member of Administrators + -- (00000000-0000-5000-8000-000000000001) is also in Purchaser. + -- Idempotent: the NOT (v_new_uuid = ANY(...)) guard skips rows that are + -- already members; DISTINCT(unnest) deduplicates any remaining overlap. + -- The EXISTS guard skips the UPDATE entirely if Purchaser still does not + -- exist for some reason, leaving no orphaned array entries. + UPDATE users + SET group_ids = ARRAY( + SELECT DISTINCT unnest( + COALESCE(group_ids, '{}') || ARRAY[v_new_uuid] + ) + ) + WHERE '00000000-0000-5000-8000-000000000001'::UUID = ANY(COALESCE(group_ids, '{}')) + AND NOT (v_new_uuid = ANY(COALESCE(group_ids, '{}'))) + AND EXISTS (SELECT 1 FROM groups WHERE id = v_new_uuid); + +END $$; diff --git a/internal/database/postgres/migrations/000064_relocate_purchaser_group_test.go b/internal/database/postgres/migrations/000064_relocate_purchaser_group_test.go new file mode 100644 index 000000000..40901e3f7 --- /dev/null +++ b/internal/database/postgres/migrations/000064_relocate_purchaser_group_test.go @@ -0,0 +1,134 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "sort" + "testing" + + "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" + "github.com/LeanerCloud/CUDly/internal/database/postgres/testhelpers" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + // purchaserGroupIDTest is the new UUID assigned to the Purchaser group + // by migration 000064 (relocated from 000005 to fix issue #942). + purchaserGroupIDTest = "00000000-0000-5000-8000-000000000007" + // adminGroupIDForPurchaserTest mirrors defaultAdminGroupIDTest; copied + // here to make the test self-contained without depending on declaration + // order across test files in the same package. + adminGroupIDForPurchaserTest = "00000000-0000-5000-8000-000000000001" +) + +// queryGroupIDsByName returns the sorted group_ids of a user with the given +// email, as UUID strings. +func queryGroupIDsByEmail(t *testing.T, ctx context.Context, pool *pgxpool.Pool, email string) []string { + t.Helper() + var ids []string + err := pool.QueryRow(ctx, ` + SELECT COALESCE(ARRAY(SELECT id::text FROM unnest(group_ids) AS id), '{}') + FROM users WHERE email = $1 + `, email).Scan(&ids) + require.NoError(t, err, "user row for %s must exist", email) + sort.Strings(ids) + return ids +} + +// TestMigration_RelocatePurchaserGroup covers issue #942: migration 000064 +// must ensure the Purchaser group exists at UUID 00000000-0000-5000-8000-000000000007 +// on databases where migration 000059 was silently no-op'd due to the UUID +// collision with Standard Users (000057), and must backfill all Administrators +// group members into Purchaser. +func TestMigration_RelocatePurchaserGroup(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + + t.Run("Purchaser seeded at new UUID after full migration run", 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, "", "")) + + // Purchaser must exist at the new UUID. + var name string + err = pool.QueryRow(ctx, `SELECT name FROM groups WHERE id = $1`, purchaserGroupIDTest).Scan(&name) + require.NoError(t, err, "Purchaser group must exist at UUID %s", purchaserGroupIDTest) + assert.Equal(t, "Purchaser", name, "group at new UUID must be named 'Purchaser'") + + // Round-trip: lookup by name must return the new UUID. + var id string + err = pool.QueryRow(ctx, `SELECT id FROM groups WHERE name = 'Purchaser'`).Scan(&id) + require.NoError(t, err, "a group named 'Purchaser' must exist") + assert.Equal(t, purchaserGroupIDTest, id, "Purchaser name must resolve to the new UUID") + }) + + t.Run("admin-backfill: Administrators members land in Purchaser", func(t *testing.T) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + // Apply all migrations, then roll back to before 000064 so we + // can seed an admin and verify the backfill fires on re-apply. + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + require.NoError(t, migrations.RollbackMigrations(ctx, pool, migrationsPath, 1)) + + // Seed an admin user in the Administrators group but NOT in Purchaser. + const adminEmail = "admin-purchaser-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) + + ids := queryGroupIDsByEmail(t, ctx, pool, adminEmail) + require.NotContains(t, ids, purchaserGroupIDTest, + "test setup: admin must not be in Purchaser before migration 000064 runs") + + // Re-apply 000064. + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + + // Admin must now be in Purchaser. + ids = queryGroupIDsByEmail(t, ctx, pool, adminEmail) + assert.Contains(t, ids, purchaserGroupIDTest, + "migration 000064 admin-backfill must add Administrators members to Purchaser") + }) + + t.Run("idempotent: re-running migrations does not duplicate Purchaser membership", func(t *testing.T) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + // Seed an admin before first run so 000064 backfill fires. + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + + const adminEmail = "idempotent-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, $3::uuid], NOW(), NOW()) + `, adminEmail, adminGroupIDForPurchaserTest, purchaserGroupIDTest) + require.NoError(t, err) + + // Run migrations again (no-op at DB level, but 000064's DO block runs again). + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + + ids := queryGroupIDsByEmail(t, ctx, pool, adminEmail) + count := 0 + for _, id := range ids { + if id == purchaserGroupIDTest { + count++ + } + } + assert.Equal(t, 1, count, "Purchaser UUID must appear exactly once in group_ids after two runs") + }) +}