[IAmJSD]

Closes #1409

Root cause

#1205 refuses a view-only plus write-capable group combination when a user is created or updated (checkGroupCombination, internal/auth/group_combination.go). UpdateGroupAPI did not apply the rule, so a group update could create the contradiction for every member without any user write. Two ways this happened: adding a write permission to a group whose members also belong to a view-only group, or making a group view-only while its members belong to a write-capable group.

Fix

• UpdateGroupAPI builds the updated group as a copy and calls the new checkGroupPermissionChange(prior, updated) before it saves. This runs after the grant and account ceiling checks, so a 403 still wins over the 400, matching the user-write order.
• Fast path: the rule only depends on a group's role (view-only, write-capable or neither). If the update keeps the role, the check returns without reading any user. That covers omitted, repeated and same-role permissions.
• If the role changes, the check lists users and compares each member's combination before and after the update. The update is refused if any member who was consistent before would become contradictory. The ErrContradictoryGroups error names the group and the number of affected members, e.g. group "Team" would grant write permissions, but 2 of its members also belong to a view-only group; remove them from one of the two groups first.
• A member who was already contradictory (assigned before fix(auth): refuse view-only + write-capable group combinations on user create/update #1205) does not block the update, the same way an unchanged membership does not block a user edit.
• Store errors fail closed.
• checkGroupCombination and the new check share the same classification through a group loader. The new check caches loaded groups for the length of one call.
• mapGroupAuthError maps ErrContradictoryGroups to 400 on PUT /api/groups/{id}, which otherwise would have been a 500. openapi.yaml documents the case.

I went with the issue's "refuse with a 400 listing how many members would be affected" option rather than "flag them". Like #1205, this is an accident guard, not a security boundary. There is a window where a concurrent user write between the member check and the group save is not seen.

Verification

• New internal/auth/group_permission_change_test.go, run through UpdateGroupAPI, covers:
  • a write permission on a group with two view-only members: refused, count 2;
  • a group made view-only with a mixed (write-capable) member: refused, count 1;
  • the first permission on an empty group: checked;
  • a role change with no conflicting members: saved;
  • an already-contradictory member: does not block;
  • the same-role fast path (omitted, unchanged, view to view, write to write): no ListUsers call;
  • ListUsers and group-lookup store errors: fail closed, not reported as a contradiction.
• The refusal tests fail on origin/main's UpdateGroupAPI (the un-stubbed UpdateGroup write is reached) and pass with the fix.
• New TestUpdateGroup_ContradictoryMembersMaps400 covers the handler mapping.
• Existing fixtures whose target group gains its first permission now stub ListUsers with no members.
• go build, go vet (default, integration, e2e), golangci-lint v2.10.1 on internal/auth and internal/api with and without --build-tags integration: 0 issues. gocyclo -over 10 on the changed files: clean.
• go test -race -short on internal/auth, internal/api, internal/mocks (including the vacuous-assertion guard) and internal/server: ok.
• go test -race -tags integration ./internal/auth/: ok. Two earlier runs timed out starting Postgres on the overloaded shared Docker host.
• The secrets scan and scripts/test-git-secrets-allowlist.sh pass.

🤖 Generated with Claude Code

[IAmJSD]

Opened at b0f8126: group updates that change a group's view-only/write-capable role are refused (400) when they would contradict a member's group combination. Local checks are listed in the PR body.

[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]

Label mirror: issue #1409 has no triage labels (priority/severity/urgency/impact/effort/type or triaged), so none were copied to this PR. The issue needs triage.

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• internal/auth/service_api.go:426-433: as the body says, a user write that lands between ListUsers and UpdateGroup isn't seen. The same is true in the other direction for the fix(auth): refuse view-only + write-capable group combinations on user create/update #1205 user-side check. That's acceptable for an accident guard, but if this rule ever becomes a security boundary, both checks need to run under one lock or transaction.

Attacked:

• Route coverage: UpdateGroupAPI is the only non-test caller of Service.UpdateGroup / store.UpdateGroup (cmd/ and internal/ searched). CreateGroup makes a memberless group, and deleting a group can't create a contradiction, so no other write path can make a member contradictory.
• Fast-path bypass: updated := *group is a shallow copy. applyUpdateGroupRequest only reassigns Permissions / AllowedAccounts and never mutates the shared slices in place, so prior keeps the stored role and the role comparison can't be fooled into skipping the check. The role is a function of Permissions alone. ErrEmptyPermissions blocks an update to zero permissions, so neutral can only be the starting state.
• Rule logic: walked view->write (members also in view-only groups count), write->view (members also in write-capable or mixed groups count), write->view where the member was already contradictory (doesn't count, correct), neutral->either, missing groups (nil, cached as nil, skipped), duplicate group IDs (deduped). Store errors on ListUsers and GetGroup fail closed and are returned as plain errors (500), not as 400 contradictions. The handler maps ErrContradictoryGroups to 400 after the 403 checks.
• Fail-before: put main's internal/auth/service_api.go back in the scratch tree and ran the new tests. TestUpdateGroupAPI_RefusesWritePermissionOnGroupWithViewOnlyMembers fails (the un-stubbed UpdateGroup is reached). Restored, it passes.
• Ran: go build ./...; go vet on auth and api, with and without -tags integration; go test -race -short -count=1 on auth, api, mocks and server (all ok); go test -race -tags integration -count=1 ./internal/auth/ (ok, 98s) and -run 'Group|OpenAPI' ./internal/api/ (ok); golangci-lint v2.10.1 on auth and api, with and without the integration tag (0 issues); gocyclo -over 10 on the changed files (clean). The issue asks for a 400 with a member count or a flag, plus tests for the add-write and unchanged-permission cases. All are present, so Closes fix(auth): apply the view-only group-combination rule when a group's permissions change #1409 fits.

Body-stale:

• none

_{Reviewer: rE2. 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 b0f812629fb28dcdbca9dca554593f92a7fcff91

Blocking:

• none
  Non-blocking:
• internal/auth/group_combination.go:176: the member check reads store.ListUsers, which is LIMIT 10000 ORDER BY created_at DESC (store_postgres.go:219). Past 10k users, the oldest members are silently skipped, so the guard fails open for them. It's an accident guard and the body says so, but a membership query (WHERE $1 = ANY(group_ids)) would be exact and cheaper than loading every user's row, password hash and MFA fields included.
• Deactivated users count as affected members. A role change can be refused because of an account nobody can log in as, and the 400 doesn't say so. Either skip !Active users or mention them in the message. This is a product call; the user-write path has no equivalent.
• The 400 tells an update:groups holder how many members of the group conflict, even when that holder lacks view:users. It's only a count, so negligible.
  Attacked: I checked that UpdateGroup in service_group.go has a single caller (UpdateGroupAPI), so no other write path skips the check. The 403 grant and account ceiling checks still run before the 400. updated := *group is safe because applyUpdateGroupRequest replaces slices rather than mutating them, so prior keeps the stored role. The role classification matches groupIsViewOnly/groupIsWriteCapable, so the fast path skips only true no-op role changes. Single-group members and already-contradictory members are handled, and a store error fails closed. Ran: go build ./..., go vet with and without -tags integration on auth/api, go test -race -short -count=1 on auth, api, mocks and server (ok), go test -race -tags integration -count=1 ./internal/auth/ (ok, 159s), golangci-lint run ./internal/auth/ (0 issues), and gocyclo -over 10 on the changed files (clean). Fail-before: with internal/auth/service_api.go reset to the merge-base, both Refuses* tests fail by reaching the un-stubbed UpdateGroup, and AlreadyContradictoryMemberDoesNotBlock fails. All pass at the head. GitHub reports MERGEABLE.
  Body-stale: none

_{Reviewer: rE3. 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]

Local verification at b0f812629fb28dcdbca9dca554593f92a7fcff91 merged onto main (990f4936), together with #1462, #1467, #1468 in that order (combined tree 7a76ef40, 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 2 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: 2 approving independent review(s) on b0f81262 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly b0f812629fb28dcdbca9dca554593f92a7fcff91.