fix(db/migrate): admin-upsert SQL parity with post-057 schema + decouple bootstrap from migration step (closes #945)

internal/api/handler_purchases.go

@@ -510,14 +510,18 @@ func (h *Handler) authorizeSessionApprove(ctx context.Context, session *Session,
 // resolveCreatorUserID before this call; "" on non-human or legacy rows).
 //
 // Gate logic (mirrors authorizeSessionApprove / authorizeSessionCancel):
-//   - admin role: always permitted.
-//   - execute-any: permitted regardless of creator.
+//   - stateless admin API key: always permitted (apiKeyAdminUserID sentinel).
+//   - execute-any: permitted regardless of creator. Administrators-group users
+//     pass here because {admin, *} matches ActionExecuteAny.
 //   - execute-own: permitted only when creatorID == session.UserID and both
 //     are non-empty (prevents an empty-string collision from granting access).
 //   - no matching grant: 403 fail-closed; nil auth component is a 500 as
 //     per feedback_fail_closed_middleware.md.
 func (h *Handler) authorizeSessionExecuteDirect(ctx context.Context, session *Session, creatorID string) error {
-	if session.Role == "admin" {
+	// Stateless admin API key: full access, no user row. Administrators-group
+	// users pass via the execute-any HasPermissionAPI check below, since
+	// {admin, *} matches any requested permission.
+	if session.UserID == apiKeyAdminUserID {
 		return nil
 	}
 	if h.auth == nil {

internal/api/handler_purchases_test.go

@@ -3080,7 +3080,6 @@ func TestHandler_executePurchase_DirectExec_NoPermission(t *testing.T) {
 	userSession := &Session{
 		UserID: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb",
 		Email:  "user@example.com",
-		Role:   "user",
 	}
 	mockAuth.On("ValidateSession", ctx, "user-token").Return(userSession, nil)
 	// Base execute:purchases grant — passes the validateExecutePurchaseRequest
@@ -3120,11 +3119,17 @@ func TestHandler_executePurchase_DirectExec_ExecuteAny(t *testing.T) {
 	adminSession := &Session{
 		UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa",
 		Email:  "admin@example.com",
-		Role:   "admin",
 	}
 	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
-	// Admin role short-circuits the permission matrix (no HasPermissionAPI call
-	// expected) but we still need ApproveAndExecute on the purchase mock.
+	// execute-any grant covers admin users (Administrators-group {admin,*}
+	// wildcard matches ActionExecuteAny). The old session.Role shortcut was
+	// removed in issue #940 — HasPermissionAPI is now always consulted.
+	// First, the outer gate: requirePermission("execute","purchases").
+	mockAuth.On("HasPermissionAPI", ctx, adminSession.UserID, "execute", "purchases").Return(true, nil)
+	// Then the direct-execute gate: authorizeSessionExecuteDirect("execute-any").
+	mockAuth.On("HasPermissionAPI", ctx, adminSession.UserID, "execute-any", "purchases").Return(true, nil)
+	// Scope check: no allowed_accounts restriction for this test.
+	mockAuth.On("GetAllowedAccountsAPI", ctx, adminSession.UserID).Return([]string{}, nil)
 	mockPurchase.On("ApproveAndExecute", ctx, mock.AnythingOfType("string"), adminSession.Email).Return(nil)
 	setupDirectExecMocks(ctx, mockStore)
 
@@ -3157,7 +3162,6 @@ func TestHandler_executePurchase_DirectExec_ExecuteOwn_Owner(t *testing.T) {
 	ownerSession := &Session{
 		UserID: ownerID,
 		Email:  "owner@example.com",
-		Role:   "user",
 	}
 	mockAuth.On("ValidateSession", ctx, "owner-token").Return(ownerSession, nil)
 	mockAuth.On("HasPermissionAPI", ctx, ownerID, "execute", "purchases").Return(true, nil)
@@ -3198,7 +3202,7 @@ func TestHandler_authorizeSessionExecuteDirect_ExecuteOwn_NonOwner(t *testing.T)
 
 	sessionUserID := "dddddddd-dddd-dddd-dddd-dddddddddddd"
 	differentCreatorID := "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee"
-	session := &Session{UserID: sessionUserID, Role: "user"}
+	session := &Session{UserID: sessionUserID}
 
 	mockAuth.On("HasPermissionAPI", ctx, sessionUserID, "execute-any", "purchases").Return(false, nil)
 	mockAuth.On("HasPermissionAPI", ctx, sessionUserID, "execute-own", "purchases").Return(true, nil)
@@ -3211,3 +3215,60 @@ func TestHandler_authorizeSessionExecuteDirect_ExecuteOwn_NonOwner(t *testing.T)
 	assert.Equal(t, 403, ce.code)
 	assert.Contains(t, ce.Error(), "execute-own requires you to be the creator")
 }
+
+// TestHandler_authorizeSessionExecuteDirect_AdminGroupViaExecuteAny verifies
+// that an Administrators-group user whose {admin,*} wildcard resolves to
+// execute-any is PERMITTED by the HasPermissionAPI path (not a dead
+// session.Role shortcut that was removed in issue #940).
+func TestHandler_authorizeSessionExecuteDirect_AdminGroupViaExecuteAny(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	t.Cleanup(func() { mockAuth.AssertExpectations(t) })
+
+	adminUserID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"
+	creatorID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"
+	session := &Session{UserID: adminUserID}
+
+	// Administrators-group wildcard {admin,*} covers execute-any.
+	mockAuth.On("HasPermissionAPI", ctx, adminUserID, "execute-any", "purchases").Return(true, nil)
+
+	handler := &Handler{auth: mockAuth}
+	err := handler.authorizeSessionExecuteDirect(ctx, session, creatorID)
+	require.NoError(t, err)
+}
+
+// TestHandler_authorizeSessionExecuteDirect_NoGrant verifies that a session
+// without execute-any or execute-own on purchases is rejected with 403.
+func TestHandler_authorizeSessionExecuteDirect_NoGrant(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	t.Cleanup(func() { mockAuth.AssertExpectations(t) })
+
+	userID := "cccccccc-cccc-cccc-cccc-cccccccccccc"
+	creatorID := "dddddddd-dddd-dddd-dddd-dddddddddddd"
+	session := &Session{UserID: userID}
+
+	mockAuth.On("HasPermissionAPI", ctx, userID, "execute-any", "purchases").Return(false, nil)
+	mockAuth.On("HasPermissionAPI", ctx, userID, "execute-own", "purchases").Return(false, nil)
+
+	handler := &Handler{auth: mockAuth}
+	err := handler.authorizeSessionExecuteDirect(ctx, session, creatorID)
+	require.Error(t, err)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected a clientError")
+	assert.Equal(t, 403, ce.code)
+}
+
+// TestHandler_authorizeSessionExecuteDirect_NilAuth verifies that a nil auth
+// component returns 500 (fail-closed per feedback_fail_closed_middleware.md).
+func TestHandler_authorizeSessionExecuteDirect_NilAuth(t *testing.T) {
+	ctx := context.Background()
+	session := &Session{UserID: "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee"}
+
+	handler := &Handler{auth: nil}
+	err := handler.authorizeSessionExecuteDirect(ctx, session, "")
+	require.Error(t, err)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected a clientError")
+	assert.Equal(t, 500, ce.code)
+}

internal/auth/types.go

@@ -317,7 +317,7 @@ const DefaultPurchaserGroupID = "00000000-0000-5000-8000-000000000005"
 
 // GroupPurchaser is the canonical name of the system-managed Purchaser
 // group. MUST match the literal name inserted by migration
-// 000058_seed_purchaser_group.up.sql so name-based lookups agree with
+// 000059_seed_purchaser_group.up.sql so name-based lookups agree with
 // the seeded row.
 const GroupPurchaser = "Purchaser"
 

internal/database/postgres/migrations/000058_seed_purchaser_group.up.sql → internal/database/postgres/migrations/000059_seed_purchaser_group.up.sql

@@ -35,7 +35,7 @@ BEGIN
           AND id <> '00000000-0000-5000-8000-000000000005'
     ) THEN
         RAISE EXCEPTION
-            'migration 000058: a group named ''Purchaser'' already exists with a different id; rename it before applying this migration so the seeded id (00000000-0000-5000-8000-000000000005) can be created';
+            'migration 000059: a group named ''Purchaser'' already exists with a different id; rename it before applying this migration so the seeded id (00000000-0000-5000-8000-000000000005) can be created';
     END IF;
 END $$;
 

internal/database/postgres/migrations/ensure_admin_user_test.go

@@ -72,39 +72,34 @@ func TestEnsureAdminUser_GroupAssignment(t *testing.T) {
 			"fresh bootstrap admin (with-password path) must have the Administrators group assigned")
 	})
 
-	t.Run("post-migration drift repair", func(t *testing.T) {
+	t.Run("re-run with existing correct admin (post-057 idempotency)", func(t *testing.T) {
+		// Migration 000057 added a CHECK constraint (users_min_one_group)
+		// that prevents group_ids from being NULL or empty, so the
+		// pre-057 drift scenario (empty group_ids on an existing admin row)
+		// is structurally impossible post-057. This sub-test verifies that
+		// re-running RunMigrations when the admin already exists with the
+		// correct group_ids is a no-op that neither fails nor doubles the
+		// group entry (issue #945).
 		container, err := testhelpers.SetupPostgresContainer(ctx, t)
 		require.NoError(t, err)
 		defer container.Cleanup(ctx)
 		pool := container.DB.Pool()
 
-		// First pass: run migrations only, no admin yet. Migration 000024
-		// seeds the Administrators group row but does not insert any
-		// admin user.
-		require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", ""))
-
-		// Simulate an out-of-band manual DB seed that inserted an admin
-		// without group_ids (the bug pattern from issue #351 - happens
-		// when an operator runs INSERT INTO users directly).
-		_, err = pool.Exec(ctx, `
-			INSERT INTO users (id, email, password_hash, salt, role, active, group_ids, created_at, updated_at)
-			VALUES (gen_random_uuid(), $1, '', '', 'admin', false, '{}', NOW(), NOW())
-		`, adminEmail)
-		require.NoError(t, err)
+		// First pass: create admin via the normal path (migrations + bootstrap).
+		require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, adminEmail, ""))
 
-		// Verify the drift state before re-running RunMigrations.
-		drifted := queryAdminGroupIDs(t, ctx, pool, adminEmail)
-		require.Empty(t, drifted, "test setup: admin should start with empty group_ids")
+		initial := queryAdminGroupIDs(t, ctx, pool, adminEmail)
+		require.Equal(t, []string{defaultAdminGroupIDTest}, initial,
+			"test setup: admin must have the Administrators group after first run")
 
-		// Second pass: re-run RunMigrations with the admin email. The
-		// m.Up() inside is a no-op (already at head, returns
-		// ErrNoChange) but ensureAdminUser still fires and triggers
-		// the code-level backfill via assignAdminGroupAndWarn.
+		// Second pass: re-run RunMigrations with the same admin email.
+		// ensureAdminUser fires again; ON CONFLICT DO NOTHING skips the
+		// insert; assignAdminGroupAndWarn is a no-op (group_ids is non-empty).
 		require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, adminEmail, ""))
 
 		got := queryAdminGroupIDs(t, ctx, pool, adminEmail)
 		assert.Equal(t, []string{defaultAdminGroupIDTest}, got,
-			"post-migration drift must self-heal on the next ensureAdminUser run")
+			"re-running RunMigrations on an existing correctly-grouped admin must be a no-op")
 	})
 
 	t.Run("idempotency", func(t *testing.T) {
@@ -149,7 +144,7 @@ func TestEnsureAdminUser_GroupAssignment(t *testing.T) {
 
 		_, err = pool.Exec(ctx, `
 			UPDATE users SET group_ids = ARRAY[$1]::UUID[]
-			WHERE email = $2 AND role = 'admin'
+			WHERE email = $2
 		`, customGroupID, adminEmail)
 		require.NoError(t, err)
 

internal/database/postgres/migrations/migrate.go

@@ -70,10 +70,15 @@ func RunMigrations(ctx context.Context, pool *pgxpool.Pool, migrationsPath strin
 
 	log.Printf("Database migrations completed successfully (version: %d)", version)
 
-	// Create admin user if email provided (after migrations complete)
+	// Create admin user if email provided (after migrations complete).
+	// Admin-bootstrap failure is non-fatal: the schema migration step
+	// succeeded and the app must not be prevented from starting because
+	// of a bootstrap-only error (e.g. a stale migrate.go referencing a
+	// dropped column). The failure is logged clearly so operators can
+	// distinguish it from a migration-step failure (issue #945).
 	if adminEmail != "" {
 		if err := ensureAdminUser(ctx, pool, adminEmail, adminPassword); err != nil {
-			return fmt.Errorf("failed to create admin user: %w", err)
+			log.Printf("WARNING: admin bootstrap failed (schema migration completed successfully): %v", err)
 		}
 	}
 
@@ -90,6 +95,9 @@ func RunMigrations(ctx context.Context, pool *pgxpool.Pool, migrationsPath strin
 // admin row whose group_ids drifted to empty (e.g. from an out-of-band
 // manual DB seed), and warns operators if the drift cannot be repaired
 // (e.g. groups table not yet populated). See issue #351.
+//
+// Note: the `role` column was dropped by migration 000057; this INSERT
+// intentionally omits it (issue #945).
 func ensureAdminUser(ctx context.Context, pool *pgxpool.Pool, email string, password string) error {
 	if password != "" {
 		return ensureAdminUserWithPassword(ctx, pool, email, password)
@@ -101,11 +109,12 @@ func ensureAdminUser(ctx context.Context, pool *pgxpool.Pool, email string, pass
 	// Use ON CONFLICT to prevent race conditions when multiple instances run migrations.
 	// group_ids is seeded with the Administrators group so a fresh bootstrap admin
 	// has group-based permissions from the start (issue #351).
+	// The `role` column was removed by migration 000057 (issue #945).
 	result, err := pool.Exec(ctx, `
 		INSERT INTO users (
-			id, email, password_hash, salt, role, active, group_ids, created_at, updated_at
+			id, email, password_hash, salt, active, group_ids, created_at, updated_at
 		) VALUES (
-			gen_random_uuid(), $1, '', '', 'admin', false, ARRAY[$2]::UUID[], NOW(), NOW()
+			gen_random_uuid(), $1, '', '', false, ARRAY[$2]::UUID[], NOW(), NOW()
 		)
 		ON CONFLICT (email) DO NOTHING
 	`, email, defaultAdminGroupID)
@@ -120,10 +129,10 @@ func ensureAdminUser(ctx context.Context, pool *pgxpool.Pool, email string, pass
 		log.Printf("Admin user already exists: %s", email)
 	}
 
-	// Idempotent backfill + invariant check on any pre-existing admin
-	// row whose group_ids drifted to empty after migration 000024's
-	// one-shot backfill already ran.
-	if err := assignAdminGroupAndWarn(ctx, pool, defaultAdminGroupID); err != nil {
+	// Idempotent backfill + invariant check on the bootstrap admin row
+	// only. Scoping by email prevents the backfill from touching
+	// non-admin users in pre-057/rollback states (issue #945).
+	if err := assignAdminGroupAndWarn(ctx, pool, defaultAdminGroupID, email); err != nil {
 		return fmt.Errorf("failed to assign admin group: %w", err)
 	}
 	return nil
@@ -138,6 +147,9 @@ func ensureAdminUser(ctx context.Context, pool *pgxpool.Pool, email string, pass
 // post-insert assignAdminGroupAndWarn handles drift uniformly without
 // coupling that semantics to the password-empty WHERE clause. See
 // issue #351.
+//
+// Note: the `role` column was dropped by migration 000057; this INSERT
+// intentionally omits it (issue #945).
 func ensureAdminUserWithPassword(ctx context.Context, pool *pgxpool.Pool, email string, password string) error {
 	log.Printf("Ensuring admin user exists with password: %s", email)
 
@@ -150,11 +162,12 @@ func ensureAdminUserWithPassword(ctx context.Context, pool *pgxpool.Pool, email
 	// Only overwrite password_hash/active when the existing hash is empty,
 	// so we never clobber a password that was already set via the UI.
 	// group_ids is seeded with the Administrators group on insert (issue #351).
+	// The `role` column was removed by migration 000057 (issue #945).
 	result, err := pool.Exec(ctx, `
 		INSERT INTO users (
-			id, email, password_hash, salt, role, active, group_ids, created_at, updated_at
+			id, email, password_hash, salt, active, group_ids, created_at, updated_at
 		) VALUES (
-			gen_random_uuid(), $1, $2, '', 'admin', true, ARRAY[$3]::UUID[], NOW(), NOW()
+			gen_random_uuid(), $1, $2, '', true, ARRAY[$3]::UUID[], NOW(), NOW()
 		)
 		ON CONFLICT (email) DO UPDATE
 			SET password_hash = $2, active = true, updated_at = NOW()
@@ -171,29 +184,36 @@ func ensureAdminUserWithPassword(ctx context.Context, pool *pgxpool.Pool, email
 		log.Printf("Admin user already has a password set: %s (skipping)", email)
 	}
 
-	// Idempotent backfill + invariant check on any pre-existing admin
-	// row whose group_ids drifted to empty after migration 000024's
-	// one-shot backfill already ran.
-	if err := assignAdminGroupAndWarn(ctx, pool, defaultAdminGroupID); err != nil {
+	// Idempotent backfill + invariant check on the bootstrap admin row
+	// only. Scoping by email prevents the backfill from touching
+	// non-admin users in pre-057/rollback states (issue #945).
+	if err := assignAdminGroupAndWarn(ctx, pool, defaultAdminGroupID, email); err != nil {
 		return fmt.Errorf("failed to assign admin group: %w", err)
 	}
 	return nil
 }
 
 // assignAdminGroupAndWarn runs an idempotent backfill that appends
-// groupID to any admin row whose group_ids is empty (NULL or
-// zero-length). The DISTINCT(unnest(...)) dedupe makes the UPDATE
+// groupID to the bootstrap admin row (identified by adminEmail) when
+// its group_ids is empty (NULL or zero-length). Scoping by email
+// prevents the backfill from touching non-admin users in pre-057 or
+// rollback states. The DISTINCT(unnest(...)) dedupe makes the UPDATE
 // safe to run repeatedly. After the backfill, a defensive SELECT
-// counts any admin rows still showing empty group_ids and logs a
-// WARN so operators see drift in container logs rather than only via
-// a broken UI. This is the "defence-in-depth invariant" described
-// in issue #351.
+// checks whether the admin row still has empty group_ids and logs a
+// WARN so operators see drift in container logs.
+//
+// Post-migration-000057: the `users_min_one_group` CHECK constraint
+// prevents group_ids from being NULL or zero-length, so this backfill
+// is a no-op in normal operation. It remains as defence-in-depth for
+// pre-057 schemas (rollback scenarios) and any future drift. The `role`
+// column was removed by migration 000057 (issue #945) and must not be
+// referenced here.
 //
 // The EXISTS guard on the groups table makes the backfill a no-op
 // when migration 000024 hasn't yet seeded the Administrators group -
 // defence-in-depth, since in practice this function is invoked
 // after RunMigrations -> m.Up() completes.
-func assignAdminGroupAndWarn(ctx context.Context, pool *pgxpool.Pool, groupID string) error {
+func assignAdminGroupAndWarn(ctx context.Context, pool *pgxpool.Pool, groupID string, adminEmail string) error {
 	res, err := pool.Exec(ctx, `
 		UPDATE users
 		SET group_ids = ARRAY(
@@ -202,33 +222,35 @@ func assignAdminGroupAndWarn(ctx context.Context, pool *pgxpool.Pool, groupID st
 			)
 		),
 			updated_at = NOW()
-		WHERE role = 'admin'
+		WHERE email = $2
 		  AND (group_ids IS NULL OR cardinality(group_ids) = 0)
 		  AND EXISTS (SELECT 1 FROM groups WHERE id = $1::UUID)
-	`, groupID)
+	`, groupID, adminEmail)
 	if err != nil {
 		return fmt.Errorf("failed to backfill admin group_ids: %w", err)
 	}
 	if n := res.RowsAffected(); n > 0 {
 		// Route to the stdlib logger (stderr) like every other admin-activity
 		// message in this file. fmt.Printf would echo this to stdout, which
 		// issue #440 explicitly forbids for admin-account operations.
-		log.Printf("Backfilled group_ids for %d admin user(s) to include Administrators group", n)
+		log.Printf("Backfilled group_ids for %d user(s) to include Administrators group", n)
 	}
 
-	// Invariant check: any admin still missing group_ids after the
-	// backfill (e.g. EXISTS guard failed because the Administrators
-	// group is missing) is logged loudly so operators notice.
+	// Invariant check: if the bootstrap admin row still has empty
+	// group_ids after the backfill (e.g. EXISTS guard failed because
+	// the Administrators group is missing), log loudly so operators notice.
+	// Post-057, the users_min_one_group CHECK means this count is
+	// always 0 unless a rollback is in progress.
 	var remaining int
 	if err := pool.QueryRow(ctx, `
 		SELECT COUNT(*) FROM users
-		WHERE role = 'admin'
+		WHERE email = $1
 		  AND (group_ids IS NULL OR cardinality(group_ids) = 0)
-	`).Scan(&remaining); err != nil {
+	`, adminEmail).Scan(&remaining); err != nil {
 		return fmt.Errorf("failed to check admin group_ids invariant: %w", err)
 	}
 	if remaining > 0 {
-		log.Printf("WARN: %d admin user(s) have empty group_ids and the Administrators group could not be assigned. Permissions may not work as expected. Check that migration 000024_seed_default_groups has run successfully.", remaining)
+		log.Printf("WARN: bootstrap admin %s has empty group_ids and the Administrators group could not be assigned. Permissions may not work as expected. Check that migration 000024_seed_default_groups has run successfully.", adminEmail)
 	}
 	return nil
 }