[IAmJSD]

#1392 hid Edit and Delete on system-managed group cards, but the cards never changed in production because the API never sent system_managed: auth.APIGroup had no such field and groupToAPIGroup did not copy it. Separately, Service.UpdateGroup had no guard of its own. It relied on its only caller, UpdateGroupAPI, to check SystemManaged first, which is what the issue flags.

Closes #919

Root cause and fix

• internal/auth/service_api.go: APIGroup gains SystemManaged bool `json:"system_managed,omitempty"` and groupToAPIGroup copies it. The field name matches what the frontend reads (frontend/src/api/types.ts APIGroup.system_managed, and isSystemManagedGroup in frontend/src/groups/systemGroups.ts, which checks === true). omitempty keeps every custom group's JSON byte-identical to before, so the chore(api): replace any with concrete request types in AuthServiceInterface #169 wire-format pins don't move.
• internal/auth/service_group.go: Service.UpdateGroup now loads the stored row and refuses a system-managed group with ErrSystemManagedGroup, or a missing group with group not found, before writing. mapGroupAuthError already turns that error into a 403 at the handler. The check moved into a shared helper, refuseSystemManagedGroup, which DeleteGroup now uses too (its behavior is unchanged; it already had the same guard inline).
• The guard reads the stored row rather than trusting group.SystemManaged, because the caller builds the *Group itself and may leave the flag out. The new test passes such a struct.
• internal/api/openapi.yaml: documents system_managed on the Group schema.

How #919 is now covered

Item	Where
Hide or disable Edit and Delete when system_managed is true, and say why	#1392 (frontend badge and tooltip)
Jest test	#1392 (groups-system-managed-919.test.ts)
Maintainer comment: show the server's reason when Delete fails	#1392 (groupActions.ts)
Backend sends system_managed, so the #1392 badge actually appears	this PR
Guard on Service.UpdateGroup	this PR
Delete guard	already present since #1629. This PR only moves it into the shared helper.

Concurrency

The check reads the row and then writes it, so I looked at whether a concurrent request could slip in between. None can. No runtime path ever writes groups.system_managed: PostgresStore.CreateGroup doesn't insert the column and PostgresStore.UpdateGroup doesn't set it. Only migrations change it, so no concurrent request can flip the flag between the read and the write. If another request deletes the group in that gap, the store's RowsAffected() == 0 path still returns group not found.

Cost

UpdateGroupAPI already loads the group for its own guard, so an API update now reads the group twice. It's one extra primary-key lookup on a rare admin action. I kept UpdateGroupAPI calling the guarded Service.UpdateGroup so that removing either guard alone still leaves the other in place. Four existing mock tests that pinned a single GetGroup call now expect two (service_api_test.go, service_api_fields_test.go, server/adapter_test.go), and TestService_UpdateGroup stubs the new read.

Verification

Unit, mock and local-Postgres evidence on branch head c18b648e, which sits on main 3bfc2fe5. Run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.

New tests:

• internal/auth/group_system_managed_919_test.go, unit with a mock store:
  • TestServiceUpdateGroup_SystemManagedGuard: a seeded group is refused even when the caller's struct has SystemManaged: false; a missing group returns not-found without a write; an ordinary group still updates.
  • TestAPIGroup_CarriesSystemManaged: GetGroupAPI and ListGroupsAPI JSON carries "system_managed": true for a seeded group and omits the key for a custom group.
• internal/auth/group_system_managed_db_test.go (integration tag, real migrated Postgres):
  • TestIntegration_SystemManagedGroup_APIFlagAndUpdateGuard: ListGroupsAPI flags Administrators and Purchaser.
  • Service.UpdateGroup on the Purchaser row, given a flagless struct that renames it and wipes its permissions, returns ErrSystemManagedGroup, and the stored name, permissions and updated_at are unchanged.
  • Negative control: a custom group still updates.

Fail before, pass after:

• With service_group.go and service_api.go reverted to main:
  • go test -count=1 -run TestServiceUpdateGroup_SystemManagedGuard ./internal/auth/ fails: An error is expected but got nil, then an unexpected UpdateGroup mock call.
  • go test -count=1 -run TestAPIGroup_CarriesSystemManaged ./internal/auth/ fails on both subtests: system_managed is absent.
  • go vet -tags integration ./internal/auth/ fails to compile the integration test (APIGroup has no field SystemManaged).
• With only service_group.go reverted, go test -tags integration -count=1 -timeout 20m -run 'TestIntegration_SystemManagedGroup_APIFlagAndUpdateGuard' ./internal/auth/ fails: Expected error with "system-managed group cannot be modified" in chain but got nil.
• On the branch, all of the above pass. The integration test was confirmed run, not skipped, with -v: --- PASS (12.64s).

Checks on the branch:

• go build ./..., go vet ./... and go vet -tags integration ./... are clean.
• go test -count=1 passes for ./internal/auth/, ./internal/api/, ./internal/server/ and ./internal/mocks/, run one package at a time.
• golangci-lint v2.10.1 run --allow-parallel-runners ./internal/auth/ ./internal/server/ and --build-tags integration ./internal/auth/ both report 0 issues.
• gocyclo -over 10 -ignore "_test\.go" internal/auth/service_group.go internal/auth/service_api.go reports nothing.
• openapi.yaml parses, and the new property is present.

Combined-tree verification at local queue commit 2be43e1e = origin/main b9271e88 + fix/issue-363-next, fix/issue-177-next, fix/issue-919-next, fix/issue-313-next, fix/issue-532-next and fix/issue-316-next merged in that order. Passed: go build ./...; go vet with and without -tags integration; go test -race -short -count=1 ./...; golangci-lint v2.10.1 with and without the integration tag; gocyclo -over 10; go test -race -tags integration on internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase, internal/server and internal/testutil; the git-secrets scan and scripts/test-git-secrets-allowlist.sh.

Out of scope

• No frontend change is needed. The UI already reads system_managed (fix(frontend): hide Edit and Delete on system-managed groups #1392) and will start showing the "System group" badge as soon as this deploys.
• The Edit modal itself is still not guarded on the client (fix(frontend): hide Edit and Delete on system-managed groups #1392 removed the button). The server now refuses at both UpdateGroupAPI and Service.UpdateGroup.
• PostgresStore.UpdateGroup and DeleteGroup themselves still don't filter on system_managed in SQL. The service is the guard layer, and no write path can set the flag, so I left the store unchanged.

Maintainer: please mirror the issue's triage labels onto this PR: triaged, priority/p3, severity/low, urgency/this-quarter, impact/few, effort/xs, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

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

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

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE c18b648e5e4a99f02f52e42f000ab3a1df993f2a

Blocking:

• none

Non-blocking:

• internal/auth/service_group.go refuseSystemManagedGroup: the not-found branch returns a plain fmt.Errorf("group not found: ...") rather than wrapping ErrGroupNotFound, so a direct Service.UpdateGroup/DeleteGroup caller can't errors.Is it and mapGroupAuthError won't turn it into a 404. This is unchanged from the old inline Delete guard, and UpdateGroupAPI checks for nil first, so nothing regresses. Worth tidying the next time this file is touched.
• An API update now reads the group twice (the PR body says so). That's fine on a rare admin action. I agree with keeping both guards.

Attacked:

• Callers: Service.UpdateGroup has one non-test caller (UpdateGroupAPI). No runtime write path sets groups.system_managed. The grep finds it only in the two store SELECTs and migrations, so the read-then-write guard has no flip window. omitempty keeps the JSON for custom groups unchanged. The frontend reads system_managed === true (frontend/src/groups/systemGroups.ts).
• Fail-before: I reverted service_group.go to main, and TestServiceUpdateGroup_SystemManagedGuard fails. I then restored it.
• Ran: go build ./...; go vet with and without -tags integration (auth, server); go test -race -short -count=1 on auth, server and api (all ok); go test -race -tags integration -count=1 ./internal/auth/ (ok, 189s; TestIntegration_SystemManagedGroup_APIFlagAndUpdateGuard ran and passed); golangci-lint v2.10.1 with and without the integration tag (0 issues).
• Closes fix(frontend): hide Edit and Delete on system-managed groups #919 is right: the frontend half landed in fix(frontend): hide Edit and Delete on system-managed groups #1392, and this PR adds the backend flag plus the Service.UpdateGroup guard the issue suggested.

Body-stale:

• none found

_{Reviewer: rF6. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE c18b648e5e4a99f02f52e42f000ab3a1df993f2a

Blocking:

• none

Non-blocking:

• internal/auth/service_group.go:63: the not-found branch returns a plain fmt.Errorf("group not found: %s"), not ErrGroupNotFound, so a group deleted between UpdateGroupAPI's read and the guard's read surfaces as a 500 instead of a 404. This matches what UpdateGroupAPI and the old DeleteGroup already did, so it isn't a regression. Wrapping ErrGroupNotFound here would let mapGroupAuthError map it later.
• PostgresStore.UpdateGroup and DeleteGroup still don't filter on system_managed in SQL, so a future second caller of the store could bypass the service guard. The PR body acknowledges this. Not needed today: the only Service.UpdateGroup caller is UpdateGroupAPI, and grep finds no other store caller outside tests and mocks.

Attacked:

• Callers: grepped every UpdateGroup/DeleteGroup reference. The only service caller is UpdateGroupAPI, and no startup or seed path updates a system-managed group through the service, so the new guard can't break a legitimate internal write.
• Wire format: omitempty keeps custom-group JSON unchanged. The field name matches frontend/src/api/types.ts and isSystemManagedGroup (=== true). Exposing the flag needs view:groups, so nothing leaks. fix(frontend): hide Edit and Delete on system-managed groups #1392 (the frontend half) is merged, so Closes #919 is justified.
• Fail-before: with service_group.go reverted to main, TestServiceUpdateGroup_SystemManagedGuard (unit) and TestIntegration_SystemManagedGroup_APIFlagAndUpdateGuard (real Postgres) fail with "error expected but got nil". With service_api.go reverted, TestAPIGroup_CarriesSystemManaged fails on both subtests. All restored and passing afterwards.
• Ran on c18b648: go build ./...; go vet with and without -tags integration on auth, server and api; go test -race -short -count=1 ./internal/auth/ ./internal/server/ ./internal/api/, ok; go test -race -tags integration -count=1 ./internal/auth/, ok (whole package); golangci-lint v2.10.1 on auth and server with and without the integration tag, 0 issues; gocyclo -over 10 on the two changed files, silent.

Body-stale:

• "Delete guard: already present since #1629": #1629 doesn't resolve to a PR or issue in this repo. git log -S shows the guard came in with commit 1f363bc (#1737), and the code comments use #1629 as an issue reference. This is cosmetic.

_{Reviewer: rF8. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[cristim]

VERDICT: APPROVE c18b648

PR #1513 (OPEN), head c18b648, one non-merge commit on origin/main. No migrations, no SQL changes.

Findings:

• nit internal/auth/service_group.go:68 - the missing-group branch returns a plain "group not found: " string, not ErrGroupNotFound, so a PUT/DELETE on a nonexistent UUID is not mapped to 404 by mapGroupAuthError/handleRequestError. This was already the case on main (UpdateGroupAPI's own "group not found", and DeleteGroup's inline guard), so it is not a regression. Fix it in a follow-up if needed.
• nit internal/auth/service_api.go:416 and service_group.go:71 - the same refusal message is now built in two places. The PR body says this is deliberate (two guards on purpose). Acceptable, but UpdateGroupAPI could call s.refuseSystemManagedGroup... it still needs the loaded row for the later ceiling checks, so the extra GetGroup cannot be dropped cleanly. Not blocking.

No correctness, security or completeness findings.

Attacked:

• All group write paths: grepped every caller of store.CreateGroup/UpdateGroup/DeleteGroup and every raw INSERT INTO|UPDATE|DELETE FROM groups in internal/ and cmd/. The only runtime writers are PostgresStore.{Create,Update,Delete}Group (store_postgres.go:471,522,562). They are reached only through Service.CreateGroup, Service.UpdateGroup and Service.DeleteGroup (service_group.go:15,38,53). Update and Delete are now both guarded in the service. Other SQL writers exist only in migration tests.
• Callers: UpdateGroupAPI (service_api.go:407, guards itself, then calls the guarded Service.UpdateGroup at :438), handler deleteGroup (handler_groups.go:151) and server authServiceAdapter.DeleteGroup (app.go:1246) both go through the guarded Service.DeleteGroup. Nothing calls the store directly.
• Rename and permission or allowed_accounts change: all go through the single PUT body (applyUpdateGroupRequest), so the guard covers them. Membership is stored on users (user.GroupIDs), not on the group row, so changing it is a user write, not a group mutation, and adding users to Administrators/Purchaser is intended behaviour. This PR has no membership endpoint to guard.
• Flipping the flag in the same request: APIUpdateGroupRequest and APICreateGroupRequest (service_api.go:131,151) have no system_managed field. encoding/json drops the unknown key. Nothing unmarshals a request into auth.Group. The store's INSERT and UPDATE never write the system_managed column. The guard reads the STORED row (refuseSystemManagedGroup -> store.GetGroup), so a struct with SystemManaged=false is still refused (this is what the unit test pins). A client cannot create a group with the flag or clear it, so the new field is read-only.
• TOCTOU: there is no runtime writer of system_managed, so nothing can flip the flag between the read and the write. A concurrent delete is caught by RowsAffected()==0.
• Leak: the routes are AuthAdmin (router.go:297-298), and requirePermission(update|delete, groups) runs before the service. The 403 body names the group ("Purchaser"), but only callers who can already GET /api/groups (AuthUser) reach it. An unauthenticated caller or a caller without the permission is refused first. validateUUID runs before any lookup.
• Wire contract: omitempty keeps custom groups byte-identical. The frontend reads system_managed === true (frontend/src/groups/systemGroups.ts:15, api/types.ts:560), which matches the JSON tag. The openapi Group schema documents the field.
• Seed data: migrations 000059/000064/000074/000096/000125 set system_managed, and the integration test confirms that Administrators and Purchaser are flagged on a migrated DB.
• Tests: go test -race -count=1 ./internal/auth/ ./internal/api/ ./internal/server/ passed, 6730 tests. go test -race -tags integration -run TestIntegration_SystemManagedGroup ./internal/auth/ (under heavy.lock) passed, 2.39s, and actually ran (-v).
• Mutation probes (scratch copy, each restored; both files cmp byte-identical to HEAD afterwards):
  1. Removed the guard call from Service.UpdateGroup: TestServiceUpdateGroup_SystemManagedGuard FAILS.
  2. Removed SystemManaged: g.SystemManaged from groupToAPIGroup: TestAPIGroup_CarriesSystemManaged FAILS.
  3. Removed the guard call from Service.DeleteGroup (the refactored helper path): the existing TestSystemManagedGroup_Immutable FAILS.

[IAmJSD]

Local verification at c18b648e5e4a99f02f52e42f000ab3a1df993f2a merged onto main (fe8b2136), together with #1513, #1514, #1516 in that order (combined tree f683b14a, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 3 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 3 approving independent review(s) on c18b648e (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly c18b648e5e4a99f02f52e42f000ab3a1df993f2a.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE c18b648

Findings:

• internal/auth/service_api.go:416 - the UpdateGroupAPI guard is now redundant with the new Service.UpdateGroup guard, and every update reads the group twice (service_api.go:409, then service_group.go:64). Deletion probe M5 (remove the API-level guard) leaves all auth/api/server tests green, so the order "403 system-managed before the ceiling, account and fix(auth): apply the view-only group-combination rule when a group's permissions change #1409 checks" is not pinned. Failure scenario: someone later drops either guard thinking the other covers it, and an actor below the ceiling gets ErrPermissionCeiling instead of ErrSystemManagedGroup. Either keep both and add a test with a below-ceiling actor, or drop one. [non-blocking]
• internal/api/openapi.yaml:6202 - system_managed is not marked readOnly: true. Group is only used as a response schema (lines 3741, 3764, 3785, 3810), so this is documentation only. [non-blocking, nit]
• internal/auth/group_system_managed_db_test.go:28 - on base it fails to compile (APIGroup.SystemManaged is undefined), not at an assertion. The unit tests do give an assertion-level fail-before. [non-blocking, note]

Attacked:

• Write paths to a group row: only store UpdateGroup (store_postgres.go:522), CreateGroup (:471) and DeleteGroup (:562). Their only non-test callers are Service.UpdateGroup (service_group.go:42, guarded), Service.DeleteGroup (:57, guarded) and Service.CreateGroup (:26, always a new UUID). UpdateGroup has one caller, UpdateGroupAPI (service_api.go:438), which is guarded at :416 and :438. The server adapter (app.go:1239-1248) exposes only the API methods. bindGroupAccounts (handler_groups.go:187) only rewrites the request before UpdateGroupAPI. No AddUserToGroup or SetGroupAccounts exists. Membership lives on users.group_ids and is meant to stay editable (types.ts:558-559). The repo has no SCIM or SSO group sync that writes groups.
• Clearing the flag: the UPDATE (store_postgres.go:532-538) and the INSERT (:489-492) never write system_managed. APIUpdateGroupRequest (service_api.go:151-156) has no such field, and json.Unmarshal (handler_groups.go:119) ignores unknown keys. Only migrations 059, 064, 096 and 125 set it, so a body with system_managed true or false is ignored.
• TOCTOU: the read and the write are not in one transaction. Since no runtime path writes the flag, the only window is a migration such as 000125 running during a deploy, which is the same as an edit landing just before the migration. Not material.
• Error mapping: ErrSystemManagedGroup maps to 403 (handler_groups.go:80-82). The new UpdateGroup guard runs only after UpdateGroupAPI's own not-found check, so it adds no existence oracle. Seeded group names are constants.
• DeleteGroup: the moved code is byte-identical to base apart from return nil, and not-found still returns "group not found: ".
• Frontend: isSystemManagedGroup uses === true (systemGroups.ts:15), so a missing field reads as false, which matches system_managed?: boolean.
• Mutation probes (head export, each restored with cp and checked with cmp): M1 drop the UpdateGroup guard: TestServiceUpdateGroup_SystemManagedGuard FAILS. M2 trust group.SystemManaged from the caller: FAILS. M3 drop the field copy: TestAPIGroup_CarriesSystemManaged FAILS. M4 drop omitempty: that test and api TestUserGroupWrites_WireFormat FAIL. M5 drop the UpdateGroupAPI guard: all pass (finding 1). M6 not-found returns nil: the guard test FAILS.

Verified locally (GOTOOLCHAIN=go1.26.10, no creds, heavy runs under the lock):

• go build ./... OK. go vet and go vet -tags integration on internal/auth, internal/api and internal/server: clean. gofmt -l: clean.
• go test -race -count=1 on the three packages: ok.
• go test -tags integration -run TestIntegration_SystemManagedGroup_APIFlagAndUpdateGuard ./internal/auth/ (testcontainers): PASS, not skipped.
• golangci-lint run (v2.11.4, repo config) on the three packages, plus --build-tags integration on auth: 0 issues.
• Fail-before on the base export: all 3 guard subtests and both API-flag subtests FAIL. The copied files were then removed.
• The PR diff matches git diff base head. After fetching, origin/main is ec78d85. merge-tree --write-tree gives a clean tree 6a0307d24684cc131c5ec5ccc8980a373487d2a6, which builds and passes the auth, api and server tests. Main has no group-related drift since base. The PR adds no migration.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.