From 5de544983323e90d619cd758b7fa63d53f7ed679 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 20 May 2026 18:35:23 +0200 Subject: [PATCH 1/4] test(database): #440 route admin group backfill log to stderr + cover it The #440 stdout-leak fix routed the per-user admin messages to the stdlib logger (stderr) but left the group_ids backfill line in assignAdminGroupAndWarn on fmt.Printf, which writes to stdout. The existing unit regression test could not catch it because it uses an unreachable pool, so the backfill branch never runs. - migrate.go: switch the "Backfilled ..." line from fmt.Printf to log.Printf so every admin-activity message in the file stays on the stderr-bound logger. - Add integration regression test TestAssignAdminGroup_BackfillLogsToStderr_NotStdout that seeds a drifted admin against a real container, runs the real ensureAdminUser path, and asserts the backfill message lands on stderr and never on stdout. Verified to fail when the line is reverted to fmt.Printf. Refs #545 --- .../database/postgres/migrations/migrate.go | 5 +- .../migrate_security_integration_test.go | 99 +++++++++++++++++++ 2 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 internal/database/postgres/migrations/migrate_security_integration_test.go 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..2cbc825b2 --- /dev/null +++ b/internal/database/postgres/migrations/migrate_security_integration_test.go @@ -0,0 +1,99 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "bytes" + "context" + "log" + "os" + "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" +) + +// captureStdoutIntegration redirects os.Stdout to a pipe and returns a function +// that closes the pipe, restores stdout, and returns everything written to it. +func captureStdoutIntegration(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() + } +} + +// captureLogOutputIntegration redirects the standard logger to a buffer for the +// duration of the test, restoring the original flags and writer on cleanup. +func captureLogOutputIntegration(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 +} + +// 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 := captureStdoutIntegration(t) + logBuf := captureLogOutputIntegration(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) +} From 15ff0bbd78bd1eaa184eeece298397fa1714f507 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 20 May 2026 18:36:36 +0200 Subject: [PATCH 2/4] test(database): #351 add SQL-level admin group_ids backfill migration + test Issue #351 acceptance criterion 2 asked for a migration-level idempotent backfill. PR #533 backfilled only in Go (assignAdminGroupAndWarn), which fires only when RunMigrations gets a non-empty admin email. A DB restored from a backup, or migrated without ADMIN_EMAIL set, never re-applied the backfill to pre-existing drifted admin rows. Migration 000024's backfill is one-shot at its version and does not re-run on an already-migrated DB. - Add migration 000053_backfill_admin_group_ids: the same idempotent backfill as 000024 (DISTINCT unnest dedupe, EXISTS guard, only touches empty group_ids so operator customisation is preserved), applied at migrate time regardless of how the deployment invokes migrations. Down is a documented no-op (additive backfill has no safe reverse). - Add integration test TestMigration_BackfillAdminGroupIDs covering the restore / no-admin-email path: it runs migrations with NO admin email so the Go backfill cannot fire, proving the SQL migration repairs a drifted admin row, and asserts idempotency on re-apply. Verified to fail when 000053 is neutered. The #351 group-assignment invariant already runs in default CI: ci.yml's integration-tests job runs `go test -tags=integration ./...` against a postgres service and is required by the ci-success gate, so TestEnsureAdminUser_GroupAssignment is exercised on every PR. Refs #546 --- .../000053_backfill_admin_group_ids.down.sql | 8 +++ .../000053_backfill_admin_group_ids.up.sql | 25 +++++++ .../backfill_admin_group_ids_test.go | 66 +++++++++++++++++++ 3 files changed, 99 insertions(+) create mode 100644 internal/database/postgres/migrations/000053_backfill_admin_group_ids.down.sql create mode 100644 internal/database/postgres/migrations/000053_backfill_admin_group_ids.up.sql create mode 100644 internal/database/postgres/migrations/backfill_admin_group_ids_test.go diff --git a/internal/database/postgres/migrations/000053_backfill_admin_group_ids.down.sql b/internal/database/postgres/migrations/000053_backfill_admin_group_ids.down.sql new file mode 100644 index 000000000..bc7d25071 --- /dev/null +++ b/internal/database/postgres/migrations/000053_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/000053_backfill_admin_group_ids.up.sql b/internal/database/postgres/migrations/000053_backfill_admin_group_ids.up.sql new file mode 100644 index 000000000..f2ff1a59d --- /dev/null +++ b/internal/database/postgres/migrations/000053_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..424b0add9 --- /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 000053) 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 (000053) so the DB sits at version 52, 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 000053), then roll back 000053 -> version 52. + 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 000053; 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 000053 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 000053 must not duplicate the Administrators group entry") +} From 985d1749555933abcd5cd473cb4b6dd6914834a5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 15:13:30 +0200 Subject: [PATCH 3/4] test(database): renumber backfill_admin_group_ids migration 000053 -> 000056 PR #614 merged 000053_executions_account_fk_restrict on the base branch while this PR was open. Renumber the backfill migration to 000056 (next free slot after 000055_add_paused_status) to clear the collision, and update the test comment references to track the new number. The migration files are renamed via `git mv` to preserve history. The SQL contents are unchanged. --- ....sql => 000056_backfill_admin_group_ids.down.sql} | 0 ...up.sql => 000056_backfill_admin_group_ids.up.sql} | 0 .../migrations/backfill_admin_group_ids_test.go | 12 ++++++------ 3 files changed, 6 insertions(+), 6 deletions(-) rename internal/database/postgres/migrations/{000053_backfill_admin_group_ids.down.sql => 000056_backfill_admin_group_ids.down.sql} (100%) rename internal/database/postgres/migrations/{000053_backfill_admin_group_ids.up.sql => 000056_backfill_admin_group_ids.up.sql} (100%) diff --git a/internal/database/postgres/migrations/000053_backfill_admin_group_ids.down.sql b/internal/database/postgres/migrations/000056_backfill_admin_group_ids.down.sql similarity index 100% rename from internal/database/postgres/migrations/000053_backfill_admin_group_ids.down.sql rename to internal/database/postgres/migrations/000056_backfill_admin_group_ids.down.sql diff --git a/internal/database/postgres/migrations/000053_backfill_admin_group_ids.up.sql b/internal/database/postgres/migrations/000056_backfill_admin_group_ids.up.sql similarity index 100% rename from internal/database/postgres/migrations/000053_backfill_admin_group_ids.up.sql rename to internal/database/postgres/migrations/000056_backfill_admin_group_ids.up.sql diff --git a/internal/database/postgres/migrations/backfill_admin_group_ids_test.go b/internal/database/postgres/migrations/backfill_admin_group_ids_test.go index 424b0add9..0ca79a805 100644 --- a/internal/database/postgres/migrations/backfill_admin_group_ids_test.go +++ b/internal/database/postgres/migrations/backfill_admin_group_ids_test.go @@ -14,13 +14,13 @@ import ( ) // TestMigration_BackfillAdminGroupIDs covers issue #546 acceptance criterion 2: -// the SQL-level idempotent backfill (migration 000053) must repair a drifted +// 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 (000053) so the DB sits at version 52, seed a drifted admin, then +// 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) { @@ -33,7 +33,7 @@ func TestMigration_BackfillAdminGroupIDs(t *testing.T) { defer container.Cleanup(ctx) pool := container.DB.Pool() - // Up to head (includes 000053), then roll back 000053 -> version 52. + // 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)) @@ -48,19 +48,19 @@ func TestMigration_BackfillAdminGroupIDs(t *testing.T) { 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 000053; the + // 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 000053 must backfill the Administrators group onto a drifted admin row even without an admin email") + "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 000053 must not duplicate the Administrators group entry") + "re-applying migration 000056 must not duplicate the Administrators group entry") } From 69ba7c44f636762e963d127b3c20056749685ee7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 15:15:05 +0200 Subject: [PATCH 4/4] test(database): consolidate stdout/log capture helpers into helpers_test.go Address CodeRabbit nitpick on PR #579: drop the duplicate captureStdoutIntegration / captureLogOutputIntegration helpers from migrate_security_integration_test.go and centralise the integration-tag copies in helpers_test.go (same package, same build tag). The duplication with migrate_security_test.go's captureStdout / captureLogOutput is forced by a package boundary (that file lives in `package migrations`, while integration tests live in `package migrations_test`), so the helpers cannot be shared across files; the new copies in helpers_test.go reuse the same names and carry a comment pointing at the unit-test originals. --- .../postgres/migrations/helpers_test.go | 50 +++++++++++++++++++ .../migrate_security_integration_test.go | 42 +--------------- 2 files changed, 52 insertions(+), 40 deletions(-) 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_security_integration_test.go b/internal/database/postgres/migrations/migrate_security_integration_test.go index 2cbc825b2..84bea65f0 100644 --- a/internal/database/postgres/migrations/migrate_security_integration_test.go +++ b/internal/database/postgres/migrations/migrate_security_integration_test.go @@ -4,10 +4,7 @@ package migrations_test import ( - "bytes" "context" - "log" - "os" "testing" "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" @@ -16,41 +13,6 @@ import ( "github.com/stretchr/testify/require" ) -// captureStdoutIntegration redirects os.Stdout to a pipe and returns a function -// that closes the pipe, restores stdout, and returns everything written to it. -func captureStdoutIntegration(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() - } -} - -// captureLogOutputIntegration redirects the standard logger to a buffer for the -// duration of the test, restoring the original flags and writer on cleanup. -func captureLogOutputIntegration(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 -} - // 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 @@ -80,8 +42,8 @@ func TestAssignAdminGroup_BackfillLogsToStderr_NotStdout(t *testing.T) { `, adminEmail) require.NoError(t, err) - readStdout := captureStdoutIntegration(t) - logBuf := captureLogOutputIntegration(t) + 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.