fix(auth): auto-assign bootstrap admin to Administrators group (closes #351)#533
fix(auth): auto-assign bootstrap admin to Administrators group (closes #351)#533cristim wants to merge 1 commit into
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)
📝 WalkthroughWalkthroughThis PR fixes a gap in admin user bootstrap: admin rows created via the migration helper did not receive the default Administrators group. The change adds ChangesAdmin Group Assignment in Bootstrap
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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 review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
group_idswith the Administrators group UUID on bothensureAdminUserINSERT paths (no-password and with-password) so a bootstrap admin has group-based permissions from the first boot.assignAdminGroupAndWarnhelper that runs an idempotent backfillUPDATEafter eachensureAdminUsercall, repairing any pre-existing admin rows whosegroup_idsdrifted to empty (e.g. from an out-of-band manual DB seed).WARNafter the backfill if any admin rows still have emptygroup_ids(signals the Administrators group row is missing from the DB), giving operators visibility in container logs.The
defaultAdminGroupIDconstant is duplicated as a package-private literal instead of imported frominternal/authto preserve the correct dependency direction (auth depends on DB, not the reverse).Integration test (
ensure_admin_user_test.go, build tagintegration) covers five scenarios: fresh insert no-password, fresh insert with-password, post-migration drift repair, idempotency under repeated boots, and operator-customisation preservation.Test plan
go test -tags integration ./internal/database/postgres/migrations/... -run TestEnsureAdminUser_GroupAssignmentpasses (requires a running postgres)go build ./...passesgo vet ./...passesCloses #351
Summary by CodeRabbit
Release Notes
Tests
Bug Fixes