diff --git a/internal/database/postgres/migrations/000056_backfill_admin_group_ids.down.sql b/internal/database/postgres/migrations/000056_backfill_admin_group_ids.down.sql new file mode 100644 index 000000000..bc7d25071 --- /dev/null +++ b/internal/database/postgres/migrations/000056_backfill_admin_group_ids.down.sql @@ -0,0 +1,8 @@ +-- No-op rollback. The up migration is an additive, idempotent backfill that +-- only appends the Administrators group to admin rows whose group_ids were +-- empty. There is no safe reverse: once applied, a backfilled row is +-- indistinguishable from a row an operator deliberately assigned to the +-- Administrators group, so removing the group on rollback could revoke +-- legitimately-assigned permissions. Migration 000024's UPDATE is reversed by +-- its own down migration; this follow-up backfill leaves the data in place. +SELECT 1; diff --git a/internal/database/postgres/migrations/000056_backfill_admin_group_ids.up.sql b/internal/database/postgres/migrations/000056_backfill_admin_group_ids.up.sql new file mode 100644 index 000000000..f2ff1a59d --- /dev/null +++ b/internal/database/postgres/migrations/000056_backfill_admin_group_ids.up.sql @@ -0,0 +1,25 @@ +-- Idempotent SQL-level backfill of the Administrators group onto any admin +-- user whose group_ids drifted to empty (issue #351, follow-up #546). +-- +-- Migration 000024 already runs this same backfill, but only once at its own +-- version. A database restored from a backup, or upgraded without the +-- ADMIN_EMAIL env var set, never re-runs the Go-level backfill in +-- assignAdminGroupAndWarn (it only fires when RunMigrations is called with a +-- non-empty admin email). This migration closes that path: any admin row with +-- empty group_ids gets the Administrators group on the next `migrate up`, +-- regardless of how the deployment invokes migrations. +-- +-- Idempotent: DISTINCT(unnest(...)) dedupes so a re-run never duplicates the +-- entry, and the WHERE clause only touches rows that are actually empty, so +-- operator-customised group_ids are left untouched. The EXISTS guard makes the +-- statement a no-op if the Administrators group row is somehow absent. +UPDATE users +SET group_ids = ARRAY( + SELECT DISTINCT unnest( + COALESCE(group_ids, '{}') || ARRAY['00000000-0000-5000-8000-000000000001']::UUID[] + ) +), + updated_at = NOW() +WHERE role = 'admin' + AND (group_ids IS NULL OR cardinality(group_ids) = 0) + AND EXISTS (SELECT 1 FROM groups WHERE id = '00000000-0000-5000-8000-000000000001'); diff --git a/internal/database/postgres/migrations/backfill_admin_group_ids_test.go b/internal/database/postgres/migrations/backfill_admin_group_ids_test.go new file mode 100644 index 000000000..0ca79a805 --- /dev/null +++ b/internal/database/postgres/migrations/backfill_admin_group_ids_test.go @@ -0,0 +1,66 @@ +//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" +) + +// TestMigration_BackfillAdminGroupIDs covers issue #546 acceptance criterion 2: +// the SQL-level idempotent backfill (migration 000056) must repair a drifted +// admin row even when migrations run WITHOUT an admin email (the restore / +// no-ADMIN_EMAIL deployment path, where the Go-level assignAdminGroupAndWarn +// never fires). +// +// Mechanism mirrors split_savingsplans_test: run all migrations, roll back the +// last one (000056) so the DB sits at version 55, seed a drifted admin, then +// re-run migrations with NO admin email so only the SQL migration can repair +// the row. A pass therefore proves the migration (not the Go path) did it. +func TestMigration_BackfillAdminGroupIDs(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + const adminEmail = "restore-path@test.example" + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + // Up to head (includes 000056), then roll back 000056 -> version 55. + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + require.NoError(t, migrations.RollbackMigrations(ctx, pool, migrationsPath, 1)) + + // Simulate a restored/manually-seeded admin whose group_ids drifted to + // empty (the bug pattern from issue #351). + _, 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) + + drifted := queryAdminGroupIDs(t, ctx, pool, adminEmail) + require.Empty(t, drifted, "test setup: admin should start with empty group_ids") + + // Re-run migrations with NO admin email. m.Up() re-applies 000056; the + // Go-level assignAdminGroupAndWarn does NOT run (empty email), so any + // repair is attributable solely to the SQL migration. + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + + got := queryAdminGroupIDs(t, ctx, pool, adminEmail) + assert.Equal(t, []string{defaultAdminGroupIDTest}, got, + "migration 000056 must backfill the Administrators group onto a drifted admin row even without an admin email") + + // Idempotent: re-running the migration path again must not duplicate. + require.NoError(t, migrations.RollbackMigrations(ctx, pool, migrationsPath, 1)) + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + got = queryAdminGroupIDs(t, ctx, pool, adminEmail) + assert.Equal(t, []string{defaultAdminGroupIDTest}, got, + "re-applying migration 000056 must not duplicate the Administrators group entry") +} diff --git a/internal/database/postgres/migrations/helpers_test.go b/internal/database/postgres/migrations/helpers_test.go index 9a76f93b0..8b7700607 100644 --- a/internal/database/postgres/migrations/helpers_test.go +++ b/internal/database/postgres/migrations/helpers_test.go @@ -4,8 +4,14 @@ package migrations_test import ( + "bytes" + "log" + "os" "path/filepath" "runtime" + "testing" + + "github.com/stretchr/testify/require" ) // getMigrationsPath resolves the migrations directory relative to this test @@ -15,3 +21,47 @@ func getMigrationsPath() string { _, filename, _, _ := runtime.Caller(0) return filepath.Dir(filename) } + +// captureStdout redirects os.Stdout to a pipe and returns a function that +// closes the pipe, restores stdout, and returns everything written to it. +// +// Mirrors the helper of the same name in migrate_security_test.go; the +// duplication is forced by the package boundary (that file lives in +// `package migrations`, while integration tests live in `package +// migrations_test`). Centralising this copy here keeps every integration +// test that needs the helper pointed at one definition. +func captureStdout(t *testing.T) func() string { + t.Helper() + origStdout := os.Stdout + r, w, err := os.Pipe() + require.NoError(t, err, "os.Pipe must succeed") + os.Stdout = w + t.Cleanup(func() { os.Stdout = origStdout }) + return func() string { + _ = w.Close() + os.Stdout = origStdout + var buf bytes.Buffer + _, _ = buf.ReadFrom(r) + _ = r.Close() + return buf.String() + } +} + +// captureLogOutput redirects the standard logger to a buffer for the duration +// of the test, restoring the original flags and writer on cleanup. +// +// Mirrors the helper of the same name in migrate_security_test.go for the +// same package-boundary reason described on captureStdout. +func captureLogOutput(t *testing.T) *bytes.Buffer { + t.Helper() + var buf bytes.Buffer + origFlags := log.Flags() + origOutput := log.Writer() + log.SetFlags(0) + log.SetOutput(&buf) + t.Cleanup(func() { + log.SetFlags(origFlags) + log.SetOutput(origOutput) + }) + return &buf +} diff --git a/internal/database/postgres/migrations/migrate.go b/internal/database/postgres/migrations/migrate.go index a434b97b5..3e1b7c531 100644 --- a/internal/database/postgres/migrations/migrate.go +++ b/internal/database/postgres/migrations/migrate.go @@ -210,7 +210,10 @@ func assignAdminGroupAndWarn(ctx context.Context, pool *pgxpool.Pool, groupID st return fmt.Errorf("failed to backfill admin group_ids: %w", err) } if n := res.RowsAffected(); n > 0 { - fmt.Printf("Backfilled group_ids for %d admin user(s) to include Administrators group\n", n) + // 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) } // Invariant check: any admin still missing group_ids after the diff --git a/internal/database/postgres/migrations/migrate_security_integration_test.go b/internal/database/postgres/migrations/migrate_security_integration_test.go new file mode 100644 index 000000000..84bea65f0 --- /dev/null +++ b/internal/database/postgres/migrations/migrate_security_integration_test.go @@ -0,0 +1,61 @@ +//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" +) + +// TestAssignAdminGroup_BackfillLogsToStderr_NotStdout is a regression test for +// issue #545 (a follow-up to #440): the admin group_ids backfill in +// assignAdminGroupAndWarn must log to stderr (via log.Printf), never stdout +// (via fmt.Printf). The earlier #440 fix routed the per-user admin messages to +// the stdlib logger but left the "Backfilled ..." line on fmt.Printf, which +// the unit test could not catch because it uses an unreachable pool so the +// backfill branch never runs. This exercises the branch against a real DB. +func TestAssignAdminGroup_BackfillLogsToStderr_NotStdout(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + const adminEmail = "stderr-backfill@test.example" + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + // Migrate to head with no admin email. Migration 000024 seeds the + // Administrators group; no admin user is inserted yet. + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + + // Seed a drifted admin with empty group_ids so the next ensureAdminUser + // run triggers the backfill (and thus the log line under test). + _, 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) + + readStdout := captureStdout(t) + logBuf := captureLogOutput(t) + + // Re-run with the admin email so ensureAdminUser -> assignAdminGroupAndWarn + // fires and backfills the drifted row, emitting the "Backfilled" message. + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, adminEmail, "")) + + stdout := readStdout() + logged := logBuf.String() + + assert.Contains(t, logged, "Backfilled", + "the backfill message must be emitted on the stderr-bound log path") + assert.NotContains(t, stdout, "Backfilled", + "the backfill message must not be written to stdout (issue #440/#545)") + assert.Empty(t, stdout, + "admin group backfill must not write anything to stdout; found: %q", stdout) +}