Skip to content

fix(auth/seed): relocate Purchaser group to free UUID + recover admin-backfill (closes #942) - #943

Merged
cristim merged 2 commits into
feat/multicloud-web-frontendfrom
fix/purchaser-uuid-collision
Jun 4, 2026
Merged

fix(auth/seed): relocate Purchaser group to free UUID + recover admin-backfill (closes #942)#943
cristim merged 2 commits into
feat/multicloud-web-frontendfrom
fix/purchaser-uuid-collision

Conversation

@cristim

@cristim cristim commented Jun 4, 2026

Copy link
Copy Markdown
Member

Root cause

Migration 000057_drop_user_role_to_groups.up.sql:18 seeds "Standard Users" at UUID 00000000-0000-5000-8000-000000000005. Migration 000059_seed_purchaser_group.up.sql:45-62 later tries to seed "Purchaser" at the same UUID. Because 000057 runs first and 000059 ends with ON CONFLICT (id) DO NOTHING, the Purchaser INSERT is silently no-op'd on every database that applied both migrations. The guard in 000059 only catches a name collision, not a UUID collision from the other direction.

The admin-backfill UPDATE in 000059 then attaches admins to "Standard Users" (the actual occupant of ...000005), not to Purchaser. Full admins end up without execute:purchases, approve-any:purchases, and retry-any:purchases -- all three verbs carved out of admin:* by issue #923.

Closes #942.

Fix

Migration 000064_relocate_purchaser_group.up.sql relocates Purchaser to 00000000-0000-5000-8000-000000000007 (the next free UUID: ...000005 = Standard Users, ...000006 = Read-Only Users, ...000007 = Purchaser).

The DO $$ block handles all three states a database can be in:

  • Case 1 - Purchaser exists at a wrong UUID (partial repair attempt): swap user group_ids references then relocate the group row.
  • Case 2 - No Purchaser row at all (the common bug-case): insert a fresh row with the full permission set from 000059.
  • Case 3 - Purchaser already at the target UUID (greenfield or pre-repaired): fall through to the backfill.

A pre-run guard RAISE EXCEPTION fires if ...000007 is taken by a different group, preventing the same class of silent UUID collision.

The admin-backfill UPDATE re-runs idempotently so all Administrators group members join Purchaser, restoring the intent of 000059's backfill.

DefaultPurchaserGroupID in internal/auth/types.go and PURCHASER_GROUP_ID in frontend/src/permissions.ts are updated to ...000007.

Risk surface

  • Existing databases with the bug: Case 2 inserts the missing Purchaser row and backfills all admins. On the next login the auth layer sees the correct group and grants the carved-out verbs.
  • Greenfield databases: all migrations run sequentially; 000064 executes Case 2 for the same reason 000059 failed. No difference from a user perspective.
  • Databases with a partial repair: Case 1 handles the relocation cleanly.
  • No data loss: the system_managed column added by 000059 is preserved; the group row moves, it is not deleted and re-inserted (Case 1). Case 2 inserts with system_managed = TRUE.
  • Frontend: PURCHASER_GROUP_ID is imported symbolically in all tests; no test hardcodes the old UUID for Purchaser, so the update is safe.

Test plan

  • Integration test TestMigration_RelocatePurchaserGroup/Purchaser_seeded_at_new_UUID_after_full_migration_run asserts Purchaser exists at ...000007 after all migrations run.
  • TestMigration_RelocatePurchaserGroup/admin-backfill:_Administrators_members_land_in_Purchaser verifies the backfill fires when 000064 is applied.
  • TestMigration_RelocatePurchaserGroup/idempotent:_re-running_migrations_does_not_duplicate_Purchaser_membership verifies the DISTINCT(unnest) guard.
  • go build ./... passes.
  • go vet ./internal/auth/... ./internal/database/... passes.
  • TypeScript typecheck passes.
  • On a manual DB with 000057 + 000059 applied (Purchaser absent), applying 000064 inserts the row and the group picker shows "Purchaser".

Summary by CodeRabbit

  • Bug Fixes
    • Relocated the Purchaser system group to a new canonical identifier; existing memberships are migrated and administrator accounts are backfilled so group access remains correct.
  • Tests
    • Added an integration test validating the relocation, membership backfill, and idempotent behavior of the migration.

…-backfill (closes #942)

Migration 000057 claimed UUID 00000000-0000-5000-8000-000000000005 for
"Standard Users". Migration 000059 tried to seed "Purchaser" at the same
UUID and ended with ON CONFLICT (id) DO NOTHING, silently no-op'ing on
every database that ran both migrations. Admins never received
execute:purchases / approve-any:purchases / retry-any:purchases.

Migration 000064 relocates Purchaser to 00000000-0000-5000-8000-000000000007
(the next free UUID in the seeded namespace). The DO block handles three
cases: relocate an existing Purchaser at a wrong UUID, insert a fresh row
when none exists (the common bug-case), and fall through if already at
the target UUID. The admin-backfill UPDATE re-runs idempotently so all
Administrators members also join Purchaser. A pre-run guard RAISE EXCEPTION
if the new UUID is claimed by a different group, preventing a repeat of
the same silent-collision bug class.

Updates DefaultPurchaserGroupID (internal/auth/types.go) and
PURCHASER_GROUP_ID (frontend/src/permissions.ts) to match.

Adds an integration test that covers: Purchaser existence at new UUID after
full migration run, admin-backfill fires on 000064, and idempotency.
@cristim cristim added triaged Item has been triaged priority/p1 Next up; this sprint severity/high Significant harm urgency/now Drop other things impact/all-users Affects every user effort/s Hours type/bug Defect labels Jun 4, 2026
@cristim

cristim commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fdae1505-6cb3-4c07-ad86-c7d24db0262c

📥 Commits

Reviewing files that changed from the base of the PR and between 765fd0b and dae3462.

📒 Files selected for processing (1)
  • frontend/src/permissions.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/src/permissions.ts

📝 Walkthrough

Walkthrough

Migration 000064 relocates the Purchaser system group from UUID ...000005 (colliding with Standard Users seeded in 000057) to a collision-free UUID ...000007, backfills Administrators with Purchaser membership, and updates backend and frontend constants accordingly to resolve issue #942.

Changes

Purchaser Group UUID Relocation and Backfill

Layer / File(s) Summary
Backend and frontend Purchaser UUID constants
internal/auth/types.go, frontend/src/permissions.ts
DefaultPurchaserGroupID and PURCHASER_GROUP_ID are updated from ...000005 to ...000007. The backend constant includes documentation linking to migration 000064 and the collision-resolution issue.
Database migration: group relocation and admin backfill
internal/database/postgres/migrations/000064_relocate_purchaser_group.up.sql, internal/database/postgres/migrations/000064_relocate_purchaser_group.down.sql
Up migration validates the new UUID is unclaimed, handles three relocation cases (existing at wrong UUID, missing entirely, already at canonical UUID), rewrites affected users' group_ids, and backfills Administrators members into Purchaser via idempotent array operations. Down migration is an intentional no-op with documentation explaining why safe rollback is not possible.
Integration test for migration relocation and backfill
internal/database/postgres/migrations/000064_relocate_purchaser_group_test.go
Test defines Purchaser and Administrators UUIDs, provides a helper to fetch user group_ids, and validates three scenarios: seeding at the new UUID, admin backfill when rolling back before 000064 and re-applying, and idempotent re-runs without duplicate membership.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

priority/p2, severity/medium, urgency/this-sprint, effort/m

Poem

🐰 A Purchaser lost in a UUID sea,
Collision stirred confusion—whee!
Migrations hop in, tidy the mess,
New UUID brings calm and progress,
Admins regain powers—hoppity, whee!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(auth/seed): relocate Purchaser group to free UUID + recover admin-backfill (closes #942)' directly and specifically summarizes the main change: relocating the Purchaser group UUID and restoring admin permissions.
Linked Issues check ✅ Passed All coding requirements from issue #942 are met: migration 000064 relocates Purchaser to UUID ...000007, prevents silent collisions with RAISE EXCEPTION, handles three database states, re-runs admin-backfill, UUID constants are updated, and integration tests are included.
Out of Scope Changes check ✅ Passed All changes are directly scoped to issue #942: migration files, UUID constant updates, and corresponding tests. No unrelated modifications were introduced.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/purchaser-uuid-collision

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
frontend/src/permissions.ts (1)

80-86: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update the Purchaser migration reference comment to match the relocation.

The constant value is correct, but the docblock still references the old seed migration path and not the 000064 relocation context used by this UUID.

Proposed doc fix
- * Well-known group UUID for the Purchaser group seeded by migration
- * 000058 (issue `#923`). The three money-spending verbs
+ * Well-known group UUID for the Purchaser group relocated by migration
+ * 000064 (issue `#942`; originally seeded for issue `#923`). The three money-spending verbs
🤖 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 `@frontend/src/permissions.ts` around lines 80 - 86, Update the docblock above
PURCHASER_GROUP_ID to reference the correct migration/relocation context
(000064) instead of the old seed migration path (000058); locate the doc comment
for the exported constant PURCHASER_GROUP_ID and change the migration reference
text to mention the 000064 relocation context so the comment matches the actual
relocation used for this UUID.
🤖 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.

Outside diff comments:
In `@frontend/src/permissions.ts`:
- Around line 80-86: Update the docblock above PURCHASER_GROUP_ID to reference
the correct migration/relocation context (000064) instead of the old seed
migration path (000058); locate the doc comment for the exported constant
PURCHASER_GROUP_ID and change the migration reference text to mention the 000064
relocation context so the comment matches the actual relocation used for this
UUID.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bc9749fd-55af-4ad5-9ce2-299b9bbf3e50

📥 Commits

Reviewing files that changed from the base of the PR and between f035285 and 765fd0b.

📒 Files selected for processing (5)
  • frontend/src/permissions.ts
  • internal/auth/types.go
  • internal/database/postgres/migrations/000064_relocate_purchaser_group.down.sql
  • internal/database/postgres/migrations/000064_relocate_purchaser_group.up.sql
  • internal/database/postgres/migrations/000064_relocate_purchaser_group_test.go

…gration 000064

Update the comment above PURCHASER_GROUP_ID to reference the
relocation migration 000064 (issue #942) instead of the original
seed migration path (000058), as the UUID was relocated by the
purchaser-uuid-collision fix.
@cristim

cristim commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim
cristim merged commit 2d3f032 into feat/multicloud-web-frontend Jun 4, 2026
5 checks passed
@cristim
cristim deleted the fix/purchaser-uuid-collision branch June 4, 2026 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/all-users Affects every user priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/bug Defect urgency/now Drop other things

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant