test(database): exercise #440 stdout-leak guard + SQL backfill for #351 admin group (closes #545, #546)

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;

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');

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")
+}

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
+}

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

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)
+}