fix(auth): auto-assign bootstrap admin to Administrators group (closes #351) - #393
Conversation
ensureAdminUser and ensureAdminUserWithPassword in internal/database/postgres/migrations/migrate.go insert admin rows without populating group_ids. A bootstrap admin (via ADMIN_EMAIL + ADMIN_PASSWORD_SECRET) ended up with role='admin' but empty group_ids, so the permissions system saw no group memberships and group-based features (frontend rendering, group-based authorisation) behaved incorrectly. Migration 000024_seed_default_groups already backfills existing admins at migration time, but it runs only once. The bootstrap path fires on every container boot, AFTER migrations are at head, so any admin inserted by ensureAdminUser bypassed the backfill entirely. Fix: - INSERT statements in both ensureAdminUser variants now seed group_ids with the Administrators group UUID (00000000-0000-5000-8000-000000000001). - A new assignAdminGroupAndWarn helper runs an idempotent backfill UPDATE after each ensureAdminUser call. It targets any admin row whose group_ids drifted to empty (NULL or zero-length) - e.g. from an out-of-band manual DB seed - so post-migration drift self-heals on the next container boot. The DISTINCT(unnest(...)) dedupe makes the UPDATE safe to run repeatedly. - After the backfill, a defensive SELECT counts admins 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 the issue body. - Operator customisation (non-empty group_ids that deliberately omits the default admin group) is preserved - the WHERE clause is gated on cardinality(group_ids) = 0. The defaultAdminGroupID constant is duplicated as a package-private literal rather than imported from internal/auth, because the migrations package must not depend on the auth package - auth depends on the DB, not the reverse. Integration test (ensure_admin_user_test.go, build tag 'integration') covers five scenarios: fresh insert (no-password), fresh insert (with-password), post-migration drift repair, idempotency under repeated boots, and operator-customisation preservation. All five pass against a postgres:16-alpine test container. Closes #351
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMigration bootstrap now guarantees admin users are seeded with the Administrators group on creation via ChangesAdmin Group Assignment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
CR pass 1 result: "No actionable comments were generated in the recent review. 🎉" - zero Actionable items, zero Nitpicks, all 5 pre-merge checks (Description, Title, Linked Issues, Out of Scope, Docstring Coverage) passed. CR loop reaches silence on the first pass. CI status: pre-commit ✅ success, AWS Sanity ✅ success, Azure Sanity ✅ success. Ready for human review. Not self-merging per repo policy. |
Summary
Closes #351.
ensureAdminUserandensureAdminUserWithPassword(
internal/database/postgres/migrations/migrate.go) insert adminrows without populating
group_ids. A bootstrap admin (viaADMIN_EMAIL+ADMIN_PASSWORD_SECRET) ended up withrole='admin'but emptygroup_ids, so the permissions system sawno group memberships and group-based features broke.
Migration
000024_seed_default_groupsalready backfills existingadmins at migration time, but it runs only once. The bootstrap path
fires on every container boot, AFTER migrations are at head, so any
admin inserted by
ensureAdminUserbypassed the backfill entirely.What changed
ensureAdminUservariants now seedgroup_idswith theAdministrators group UUID on
INSERT.ON CONFLICT (email) DO NOTHING/
DO UPDATE WHERE password_hash = ''semantics are preserved soan operator's customised
group_idsis never overwritten.assignAdminGroupAndWarnruns an idempotent backfillon any admin row whose
group_idsdrifted to empty. TheDISTINCT(unnest(...))dedupe makes the UPDATE safe to run onevery boot. Operator customisation (non-empty
group_idsthatdeliberately omits the default admin group) is preserved via the
cardinality(group_ids) = 0guard.SELECTcounts admins stillshowing empty
group_idsand logs aWARNso operators seedrift in container logs rather than only via a broken UI. This
is the "defence-in-depth invariant" described in the issue body.
Notes
defaultAdminGroupIDis duplicated as a package-private literalin
migrate.gorather than imported frominternal/auth, becausethe migrations package must not depend on the auth package
(
authdepends on the DB, not the reverse). Kept in sync withauth.DefaultAdminGroupIDand the literal in the000024migration via cross-referencing comments.
DO UPDATEclause inensureAdminUserWithPasswordisdeliberately NOT extended to touch
group_ids- the post-insertassignAdminGroupAndWarnhandles drift uniformly withoutcoupling that semantics to the password-empty
WHEREclause.Test plan
Integration test
ensure_admin_user_test.go(build tagintegration) covers five sub-cases against apostgres:16-alpinetestcontainer:
group_ids = [DefaultAdminGroupID].out-of-band with empty
group_ids, runRunMigrationsagain,assert the row self-heals.
RunMigrationscalls do notduplicate the admin group ID.
custom group (deliberately removing the default admin group)
survives subsequent boots unchanged.
All five pass locally:
go build ./...andgo vet ./...clean.Summary by CodeRabbit
Tests
Bug Fixes