fix(auth): lock the default Standard and Read-Only user groups

[cristim]

Summary

Closes #168.

The seeded Standard Users (00000000-0000-5000-8000-000000000005) and Read-Only Users (...0006) groups were not system_managed, so the #1737 guard did not protect them. An admin could strip or widen their permissions. The owner decided on 2026-10-16 to lock them.

Migration 000125 sets system_managed = TRUE for both ids. The down migration restores FALSE for them; neither group was locked before this migration, since 000059 only locked ...0001 to ...0004. Locking is by id, so a group an admin already renamed is locked with its name and permissions unchanged.

After this change, UpdateGroupAPI and DeleteGroup on either group return ErrSystemManagedGroup (HTTP 403), as they already do for Administrators and Purchaser. Group membership lives in users.group_ids and does not go through the guarded paths. Admins can still add or remove users, and new users still default to Standard Users.

Tests

000125_lock_default_user_groups_test.go runs against a real Postgres 16 via testcontainers (integration tag). It does the following:

1. Migrates to 124 and asserts both groups are unlocked.
2. Renames Standard Users.
3. Migrates to 125 and asserts both groups are locked, the rename and permissions are preserved, and Purchaser is still locked.
4. Migrates down and asserts both groups are unlocked and Purchaser is still locked.

Mutations of the migration, each of which made the test fail:

• An up migration that does nothing fails with ...0005 must be system-managed.
• Locking only ...0005 fails with ...0006 must be system-managed.
• A down migration that does nothing fails with down must restore ...0005.

The guard itself is already covered generically by internal/auth/group_system_managed_test.go (UpdateGroupAPI and DeleteGroup return ErrSystemManagedGroup).

Verification (local, macOS)

• go test -tags integration -race -count=1 ./internal/database/postgres/migrations/: the full package passes, so no other migration test is affected.
• go test -race -count=1 ./internal/auth/... ./internal/api/...: 3997 passed.
• golangci-lint v2.10.1 with --build-tags integration: 0 issues. go vet is clean with and without the tag.
• Runtime writers to groups outside migrations: only the guarded UpdateGroupAPI and DeleteGroup paths. There is no startup seeder, and the e2e fixtures do not write these groups.
• The migration was run only on a throwaway testcontainers database. No real environment was touched.

Migration ordering

origin/main is at 000124. No open PR uses 000125, which is claimed on the agent bus.

Review

An independent non-author ran two passes at 3fc16e2 and found nothing blocking. Non-blocking notes:

• The groups page still shows Edit and Delete for system-managed groups. Edit surfaces the 403 message; Delete shows a generic error. Both are tracked in fix(frontend): hide Edit and Delete on system-managed groups #919, which predates this PR for Administrators and Purchaser.
• Service.UpdateGroup has no guard of its own. It relies on its only caller, UpdateGroupAPI, to apply it. This is also noted in fix(frontend): hide Edit and Delete on system-managed groups #919.

🤖 Generated with Claude Code

[cristim]

@coderabbitai review

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 40 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 64 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: c07d4210-47e8-4535-b6a4-3aa19c5e8daf

📥 Commits

Reviewing files that changed from the base of the PR and between 2e7a535 and 3fc16e2.

📒 Files selected for processing: 3

• internal/database/postgres/migrations/000125_lock_default_user_groups.down.sql
• internal/database/postgres/migrations/000125_lock_default_user_groups.up.sql
• internal/database/postgres/migrations/000125_lock_default_user_groups_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

⚠️ Action not completed

Review rate limited.

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]

Independent review of 3fc16e2: no actionable findings

Findings: none actionable.

• nit (non-blocking): the lock is by id, so if 000057's ON CONFLICT DO NOTHING skipped the seed because a different-id group already owned the name, the UPDATE is a silent no-op for that id. The deployed-DB path is the normal case (ids exist), so I would not change it.

What I attacked

• Guard coverage: the only runtime writers to groups are PostgresStore.UpdateGroup (store_postgres.go:547) and DeleteGroup (:587) on origin/main. Their only service callers are UpdateGroupAPI (service_api.go:376, checks SystemManaged, then calls Service.UpdateGroup at :401) and Service.DeleteGroup (service_group.go:50). Renames and permission edits go through UpdateGroupAPI, deletes through DeleteGroup; both refuse with ErrSystemManagedGroup, mapped to 403 at handler_groups.go:73. Service.UpdateGroup (service_group.go:30) has no guard of its own, but has no other caller (already noted in the PR body and #919). No startup seeder writes these rows.
• Blocked legitimate work: membership lives in users.group_ids, outside the guarded group-row paths, so adding or removing users in Standard Users still works. Editing or deleting these two groups is blocked by design (owner decision per PR body).
• Tenancy: groups rows are global seeds, no tenant column. Ids ...0005 and ...0006 match 000057_drop_user_role_to_groups.up.sql:18 (Standard Users) and :35 (Read-Only Users). ...0007 is Purchaser (000064).
• Down migration: 000059_seed_purchaser_group.up.sql:14-21 locked only ...0001 to ...0004 and its down only drops the column, so before 000125 neither id was locked; restoring FALSE is the correct inverse. Test asserts Purchaser stays locked after down.
• Ordering: origin/main tops out at 000124; no open PR other than #920 touches migrations 0001xx; #905 has no migration files; bus claim claim-20261016-claude-cc-impl-migration-000125.txt is #920's own. #631 is not resolvable as a PR number.
• Test: ran go test -tags integration -race -count=1 -run TestMigration_LockDefaultUserGroups against real Postgres 16 (testcontainers): PASS. Mutation probe: replaced the up SQL with SELECT 1;, test failed with "after 000125 ...0005 must be system-managed"; reverted. That is the fail-before proof.

Gate status

• Independent review: this comment, at the SHA above.
• Findings resolved: none open. CodeRabbit check is "rate limited", no threads posted.
• CI on SHA: at review time several checks (Unit Tests, Lint Code, Integration Tests, E2E, Build Docker Image) were still pending; the remaining listed checks passed. Merge must wait for them to go green.
• Local real-path evidence: present in the PR body (testcontainers Postgres, auth/api suites, lint); I reproduced the migration test. No real environment was touched.
• Labels: priority/p2, severity/medium, urgency/this-sprint, impact/many, effort/s, type/security, triaged present; I did not diff them against #168's labels.

[cristim]

Gate record for 3fc16e2 (origin/main CLAUDE.md "Review gate"):

• Independent review of 3fc16e2: no actionable findings.
  • The #1737 guard covers every runtime writer to groups: UpdateGroupAPI and DeleteGroup, both mapped to 403.
  • Membership lives in users.group_ids, so it is unaffected.
  • The ids match 000057.
  • The down migration is the correct inverse of the state 000059 left.
  • 000125 is the next free number.
• CI on 3fc16e2: all checks pass, including CI Success and Run pre-commit hooks.
• Local verification (macOS, testcontainers Postgres 16, -tags integration -race):
  • The migration test passes.
  • With the up SQL replaced by a no-op, it fails with "after 000125 ...0005 must be system-managed", so it fails without the migration.
  • No real-account run.
• CR skipped: quota-limited; covered by the independent review plus local verification.
• Labels mirrored from sec(auth): Standard Users and Read-Only Users are not system_managed, so #1737's guard does not protect them #168.

Merging at 3fc16e2 with --match-head-commit.