fix(db/migrate): admin-upsert SQL parity with post-057 schema + decouple bootstrap from migration step (closes #945) - #947
Merged
cristim merged 4 commits intoJun 4, 2026
Conversation
…ect gate (closes #940) PR #912 (#907) removed the Role field from api.Session, leaving authorizeSessionExecuteDirect with a dead `if session.Role == "admin"` reference that prevented the base branch from compiling. Remove the three-line shortcut and update the gate-logic doc comment to match the sibling authorizeSessionApprove/authorizeSessionCancel pattern: admins pass via the execute-any HasPermissionAPI check because the Administrators group carries the {admin,*} wildcard permission. Also fix all test Session struct literals that still set Role (now an unknown field), update the admin-role-shortcut test to properly stub the HasPermissionAPI path, and add three regression tests: - AdminGroupViaExecuteAny: confirms admin users still pass via execute-any - NoGrant: confirms non-admin without execute-any/own gets 403 - NilAuth: confirms nil auth component returns 500 (fail-closed)
…000059 (base collision from #803+#924) The base feat/multicloud-web-frontend merged two 000058 migrations from the recent wave, which the check-migration-conflicts pre-commit hook rejects: - 000058_purchase_executions_direct_execute_audit (#803, schema change) - 000058_seed_purchaser_group (#924, data seed) Keep the schema-change audit migration at 000058 and renumber the seed migration to the next free slot, 000059 (base jumps 000058 -> 000063, so 000059-000062 are open). golang-migrate discovers migrations by filename glob, so no embed list needs updating; only two self-references were adjusted: - auth/types.go GroupPurchaser doc comment pointing at the filename - the self-referencing error message inside the up.sql Safe to renumber without applied-state reconciliation: the base has not compiled since the #803/#907 break, so neither 000058 migration has been applied to any database yet.
…ple bootstrap from migration step (closes #945) Migration 000057 dropped the users.role column and sessions.role column (PR #912). The Go bootstrap functions in migrate.go were not updated at the time, leaving three SQL statements that reference the dropped column: - ensureAdminUser: INSERT INTO users included `role` in column list / VALUES - ensureAdminUserWithPassword: same INSERT pattern - assignAdminGroupAndWarn: two queries used WHERE role = 'admin' This caused every Lambda cold start to fail the admin-upsert step with "column role of relation users does not exist (SQLSTATE 42703)". The admin-upsert failure also masked a second deployment gap: migration 000058 (purchase_executions direct-execute audit columns) was missing from the deployed image, causing all purchase-execution reads/writes to fail with column-not-found errors. The image rebuild (ops action) will apply 000058 on the next cold start. Changes: - Remove `role` from INSERT column list and VALUES in both ensureAdminUser and ensureAdminUserWithPassword - Replace `WHERE role = 'admin'` in assignAdminGroupAndWarn with `WHERE (group_ids IS NULL OR cardinality(group_ids) = 0)`; post-057, the users_min_one_group CHECK makes this a permanent no-op so the backfill path is defence-in-depth for rollback scenarios only - Decouple admin-bootstrap failure from RunMigrations return value: if m.Up() succeeds but ensureAdminUser fails, RunMigrations now logs a WARNING and returns nil so the health endpoint correctly reports migration success and the app boots cleanly - Update ensure_admin_user_test.go: the pre-057 drift scenario (empty group_ids on an existing admin row) is structurally impossible post-057 due to the CHECK constraint; replace with an idempotency test that verifies re-running RunMigrations on an already-correct admin row is a no-op - Update migrate_security_integration_test.go: the same drift simulation (INSERT with role + empty group_ids after migrations) is impossible post-057; test now exercises the "Admin user created" log path instead
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR transitions authorization in the purchases handler from role-based checks to a stateless admin API key sentinel combined with group-membership RBAC matrix consultation. Corresponding migration changes remove role column references and make admin bootstrap failures non-fatal. Tests are updated to remove Role dependency and verify idempotency. ChangesAuthorization & Admin Bootstrap Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/database/postgres/migrations/ensure_admin_user_test.go (1)
145-148:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest SQL still references dropped
users.rolecolumn.The update predicate uses
role = 'admin', butusers.rolewas removed by migration000057. This can fail the integration test on current schema.✅ Proposed fix
_, err = pool.Exec(ctx, ` UPDATE users SET group_ids = ARRAY[$1]::UUID[] - WHERE email = $2 AND role = 'admin' + WHERE email = $2 `, customGroupID, adminEmail)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/postgres/migrations/ensure_admin_user_test.go` around lines 145 - 148, The SQL update in the Exec call still filters by the removed column `role = 'admin'`; remove that predicate and update the WHERE clause to use only the email (i.e., change the SQL in the pool.Exec call that sets group_ids to "WHERE email = $2" using the existing variables customGroupID and adminEmail). This keeps the test aligned with the post-migration schema; if you need to assert admin membership instead, replace the predicate with the new authorization model (e.g., join or check appropriate user_groups/group_ids field) rather than referencing `role`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auth/types.go`:
- Line 320: Update the migration comment string from "000058" to "000059"
wherever the frontend references the backend migration: specifically change the
comment above the exported PERMISSIONS constant in permissions.ts and the
comment above the user actions (exported actions in userActions.ts, e.g.,
createUser/fetchUsers action helpers) to "000059" so the frontend comments match
the backend migration renumbering.
In `@internal/database/postgres/migrations/migrate.go`:
- Around line 224-226: The UPDATE/COUNT SQL currently affects all users with
empty group_ids; restrict it to only the bootstrap admin user by adding a user
id predicate and passing the bootstrap admin's UUID as a second parameter.
Update the SQL fragments that use groupID (the two places around the shown query
and the similar block at lines ~245-250) to include "AND id = $2::UUID" (or the
equivalent user id column) and pass the bootstrapAdminID (or adminUserID) as the
new second argument so only the bootstrap admin gets the Administrators group
assigned / counted.
---
Outside diff comments:
In `@internal/database/postgres/migrations/ensure_admin_user_test.go`:
- Around line 145-148: The SQL update in the Exec call still filters by the
removed column `role = 'admin'`; remove that predicate and update the WHERE
clause to use only the email (i.e., change the SQL in the pool.Exec call that
sets group_ids to "WHERE email = $2" using the existing variables customGroupID
and adminEmail). This keeps the test aligned with the post-migration schema; if
you need to assert admin membership instead, replace the predicate with the new
authorization model (e.g., join or check appropriate user_groups/group_ids
field) rather than referencing `role`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c263ab93-76f0-438a-afd7-49234bf83807
📒 Files selected for processing (8)
internal/api/handler_purchases.gointernal/api/handler_purchases_test.gointernal/auth/types.gointernal/database/postgres/migrations/000059_seed_purchaser_group.down.sqlinternal/database/postgres/migrations/000059_seed_purchaser_group.up.sqlinternal/database/postgres/migrations/ensure_admin_user_test.gointernal/database/postgres/migrations/migrate.gointernal/database/postgres/migrations/migrate_security_integration_test.go
| // GroupPurchaser is the canonical name of the system-managed Purchaser | ||
| // group. MUST match the literal name inserted by migration | ||
| // 000058_seed_purchaser_group.up.sql so name-based lookups agree with | ||
| // 000059_seed_purchaser_group.up.sql so name-based lookups agree with |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find frontend files still referencing the old migration number 000058 for the Purchaser group.
# Search for references to migration 000058 in frontend files
rg -n --type=ts --type=tsx -C2 '000058.*[Pp]urchas|[Pp]urchas.*000058' frontend/ || echo "No matches found"Repository: LeanerCloud/CUDly
Length of output: 124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show where frontend directory exists (if at all)
ls -la | head -n 50
ls -la frontend 2>/dev/null || true
# Locate the targeted files
echo "== Locate permissions.ts and userActions.ts =="
fd -e ts -e tsx -e js -e jsx 'permissions.ts|userActions.ts' . || true
fd -e ts -e tsx -e js -e jsx 'permissions.ts$|userActions.ts$' . || true
# Search for raw migration number occurrences in frontend (no invalid --type flags)
echo "== Search for 000058 in frontend/ (all file types) =="
rg -n '000058' frontend/ 2>/dev/null || echo "No matches found in frontend/"
# Search for purchaser-related + 000058 patterns in repo (broader, avoids relying on filetype flags)
echo "== Search for purchaser + 000058 across repo =="
rg -n -i '000058.*purchas|purchas.*000058' . || echo "No matches found for purchaser + 000058"
# Check the two specific files if found
echo "== If permissions.ts exists, search inside it =="
file_permissions="$(fd 'permissions\.ts$' . || true | head -n 1)"
if [ -n "${file_permissions}" ]; then
rg -n '000058|000059' "${file_permissions}" || true
else
echo "permissions.ts not found"
fi
echo "== If userActions.ts exists, search inside it =="
file_useractions="$(fd 'userActions\.ts$' . || true | head -n 1)"
if [ -n "${file_useractions}" ]; then
rg -n '000058|000059' "${file_useractions}" || true
else
echo "userActions.ts not found"
fiRepository: LeanerCloud/CUDly
Length of output: 4249
Update frontend migration comment references (000058 → 000059)
Frontend still references migration 000058 in:
frontend/src/permissions.ts:81frontend/src/users/userActions.ts:101
Update these comments to 000059 to match the backend migration renumbering.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/auth/types.go` at line 320, Update the migration comment string from
"000058" to "000059" wherever the frontend references the backend migration:
specifically change the comment above the exported PERMISSIONS constant in
permissions.ts and the comment above the user actions (exported actions in
userActions.ts, e.g., createUser/fetchUsers action helpers) to "000059" so the
frontend comments match the backend migration renumbering.
Comment on lines
224
to
226
| WHERE (group_ids IS NULL OR cardinality(group_ids) = 0) | ||
| AND EXISTS (SELECT 1 FROM groups WHERE id = $1::UUID) | ||
| `, groupID) |
Contributor
There was a problem hiding this comment.
Scope the admin-group backfill to the bootstrap admin user only.
This UPDATE/COUNT currently targets all users with empty group_ids. In pre-057/rollback states, that can assign the Administrators group to non-admin users.
🔧 Proposed fix
@@
- if err := assignAdminGroupAndWarn(ctx, pool, defaultAdminGroupID); err != nil {
+ if err := assignAdminGroupAndWarn(ctx, pool, defaultAdminGroupID, email); err != nil {
return fmt.Errorf("failed to assign admin group: %w", err)
}
@@
- if err := assignAdminGroupAndWarn(ctx, pool, defaultAdminGroupID); err != nil {
+ if err := assignAdminGroupAndWarn(ctx, pool, defaultAdminGroupID, email); err != nil {
return fmt.Errorf("failed to assign admin group: %w", err)
}
@@
-func assignAdminGroupAndWarn(ctx context.Context, pool *pgxpool.Pool, groupID string) error {
+func assignAdminGroupAndWarn(ctx context.Context, pool *pgxpool.Pool, groupID string, adminEmail string) error {
res, err := pool.Exec(ctx, `
UPDATE users
@@
- WHERE (group_ids IS NULL OR cardinality(group_ids) = 0)
+ WHERE email = $2
+ AND (group_ids IS NULL OR cardinality(group_ids) = 0)
AND EXISTS (SELECT 1 FROM groups WHERE id = $1::UUID)
- `, groupID)
+ `, groupID, adminEmail)
@@
if err := pool.QueryRow(ctx, `
SELECT COUNT(*) FROM users
- WHERE (group_ids IS NULL OR cardinality(group_ids) = 0)
- `).Scan(&remaining); err != nil {
+ WHERE email = $1
+ AND (group_ids IS NULL OR cardinality(group_ids) = 0)
+ `, adminEmail).Scan(&remaining); err != nil {
return fmt.Errorf("failed to check admin group_ids invariant: %w", err)
}Also applies to: 245-250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/database/postgres/migrations/migrate.go` around lines 224 - 226, The
UPDATE/COUNT SQL currently affects all users with empty group_ids; restrict it
to only the bootstrap admin user by adding a user id predicate and passing the
bootstrap admin's UUID as a second parameter. Update the SQL fragments that use
groupID (the two places around the shown query and the similar block at lines
~245-250) to include "AND id = $2::UUID" (or the equivalent user id column) and
pass the bootstrapAdminID (or adminUserID) as the new second argument so only
the bootstrap admin gets the Administrators group assigned / counted.
- Scope assignAdminGroupAndWarn to the bootstrap admin email only; the previous UPDATE/COUNT targeted all users with empty group_ids, which would assign the Administrators group to non-admin users in pre-057 or rollback states (CR finding, major severity) - Remove dropped `role = 'admin'` predicate from test SQL in the operator-customisation sub-case; the column was dropped by migration 000057 so the WHERE clause would always return zero rows on the current schema (CR outside-diff finding) - Deferred: frontend 000058->000059 comment update is out of PR scope; filed as issue #955
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hypothesis Settled: Hypothesis (a) is PARTIALLY correct, but the primary gap is a missing image rebuild
Settled on: The deployed Lambda image was built from a commit that predates migration 000058.
m.Up()on the running image has no 000058 SQL file to apply, so the three audit columns were never added topurchase_executions. The admin-upsert failure (bug 2) is a separate regression: migration 000057 droppedusers.rolein PR #912 butmigrate.gowas not updated at the time, causing every post-057 cold start to fail the admin-bootstrap step withcolumn "role" does not exist. Both failures are independently present in the deployed environment.Two Bugs Fixed
Bug 1 -- Admin-upsert SQL references dropped
users.rolecolumn (every cold start)Migration 000057 dropped
users.roleandsessions.role. The bootstrap functions inmigrate.gowere not updated:ensureAdminUser: INSERT INTO users includedrolein column list and VALUESensureAdminUserWithPassword: same INSERT patternassignAdminGroupAndWarn: two queries usedWHERE role = 'admin'CloudWatch evidence (every cold start):
Fix: remove
rolefrom both INSERT statements; replaceWHERE role = 'admin'withWHERE (group_ids IS NULL OR cardinality(group_ids) = 0)inassignAdminGroupAndWarn. Post-057, theusers_min_one_groupCHECK makes the backfill a permanent no-op (empty group_ids is structurally impossible), so this is defence-in-depth only.Bug 2 -- Migration 000058 not applied (deployed image predates the commit)
Migration
000058_purchase_executions_direct_execute_audit.up.sqladds three audit columns topurchase_executions. The deployed image was built before this migration was merged, so its/app/migrationsdirectory does not include the 000058 SQL files.m.Up()cannot apply a migration whose file does not exist. The Go scanner inscanExecutionRows(store_postgres.go) expects 25 columns; the table has 22.CloudWatch evidence:
This is an ops action, not a code change: once this PR merges and the Lambda image is rebuilt from the updated branch,
m.Up()will apply 000058 on the next cold start, adding the three missing columns.Additional fix -- Decouple admin-bootstrap failure from
RunMigrationsreturn valuePreviously, if
ensureAdminUserfailed,RunMigrationsreturned an error, making the health endpoint report migration failure even when the schema (m.Up()) was fully current. Now: ifm.Up()succeeds but admin-bootstrap fails,RunMigrationslogsWARNING: admin bootstrap failed (schema migration completed successfully)and returns nil. This makes cold-start diagnostics unambiguous and prevents a stalemigrate.gofrom causing the app to misreport its own health.Migration 000058 Application Path
Image rebuild required. After this PR merges:
/app/migrations)m.Up()will detect version 57 inschema_migrationsand apply 000058 (and 000059/000063 if also missing)Database migrations completed successfully (version: 63)or similar and zerocolumn "executed_by_user_id" does not existerrorsIf
schema_migrationsshows a dirty state (unlikely given the soft-continue boot path), set env varCUDLY_FORCE_MIGRATION_VERSION=57on the Lambda, redeploy once to clear the dirty flag, then remove the env var on the next deploy.Test Plan
go build ./...passesgo test ./internal/database/postgres/migrations/passes (4 unit tests)go test ./internal/server/...passes (334 tests)go test -tags integration ./internal/database/postgres/migrations/...(requires Docker for the testcontainers)column "role" of relation "users"errors on cold startcolumn "executed_by_user_id" does not existerrorsOps Follow-up Required
Yes. The deployed image must be rebuilt from the current branch and redeployed to apply migration 000058. The code fix in this PR alone does not resolve the
purchase_executionscolumn errors - it only fixes the cold-start admin-upsert failure. A new deploy is required to ship 000058's SQL file inside the Lambda image.Follow-up issue: track the image rebuild + redeploy as a separate ops action against issue #945.
Summary by CodeRabbit
Bug Fixes
Tests