[IAmJSD]

Root cause

The check that a view-only group must not be combined with a write-capable group lived only in the frontend (validateGroupCombination in frontend/src/users/utils.ts, #1405). The backend's guardGroupChange enforced >=1 group, last-admin and self-escalation (#907) and the #226 ceiling, but not this rule. So POST /api/users and PUT /api/users/{id} accepted a contradictory membership from any non-UI API consumer.

Closes #78

Fix

• New internal/auth/group_combination.go ports the frontend's classification. The frontend classifies by permissions, not by names, and so does this port. Each group's stored permissions are read with store.GetGroup:
  • view-only: at least one permission, and every action is "view"
  • write-capable: any action other than "view" (so admin, * and a mixed view+create group all count)
  • a group with no permissions is neither and takes no part; a missing group (pgx.ErrNoRows / nil) is skipped, the same way the frontend skips IDs it can't find
  • any other store error fails closed
• New sentinel auth.ErrContradictoryGroups, wrapped with %w and the same explanation the UI shows (contradictory group combination: "Viewers" is view-only but "Plan Authors" grants write permissions; assign a view-only group or write-capable groups, not both). mapAuthError maps it to 400, next to ErrNoGroups. It's the same ClientError shape as the other guardGroupChange refusals.
• Wired into every backend path that sets a user's groups:
  • create: CreateUser -> checkCreateUserGroups (renamed from checkCreateUserCeiling; it runs the sec(auth): user-membership writes have no grant ceiling, so update:users mints an admin #226 ceiling, then the combination check)
  • update: guardGroupChange now runs the existing checks (moved unchanged into guardGroupChangeAccess), then checkGroupMembershipChange
  • There is no separate group-membership endpoint. The frontend's inline toggles and bulk add both go through PUT /api/users/{id}, and SetupAdmin only ever assigns Administrators. The other store.UpdateUser writers (password, MFA) write back the user's existing group_ids unchanged.
• Ordering: the combination check runs after the access guards, so an escalation attempt that also happens to be contradictory is still reported as ErrSelfEscalation / ceiling (403). TestUpdateUser_SelfEscalationDenied (a viewer adding Administrators to themselves) pins this. The check runs for every caller, including trusted internal ones (actorUserID == ""), because it's about the membership, not the actor.
• Existing contradictory users aren't locked out. If the requested membership is the same set as the stored one (groups omitted, or sent back in any order or with duplicates), it isn't re-checked. Renames, deactivation and edit forms that send back the unchanged group list keep working. Any real change is checked against the full resulting set, as the frontend does: a change has to resolve the contradiction (drop either side) and can't keep it or add to it.

Existing tests whose fixtures changed

Five existing tests used a view-only "ordinary"/"subset" group next to an admin group and expected success. That is exactly the combination this PR refuses:

• TestSelfAccountScope_JoiningSubsetScopedGroupAllowed
• TestSelfCarvedOutGrant_OrdinaryGroupStillAllowed
• TestSelfCarvedOutGrant_AdminMayAddAnotherUserToOrdinaryGroup
• TestUpdateUser_AdminEditingSelfAllowed
• TestSelfAccountScopeDispatch_NonWideningChangeAllowed

None of them was testing the combination itself (they cover the carve-out, account-scope and self-edit rules), so their fixture group is now write-capable (create:plans), keeping each test's intent. Four more tests send several groups without caring about the combination, so they only got GetGroup stubs:

• TestSelfCarvedOutGrant_InternalCallerUnaffected
• TestAPIAdapters_CreateUser_AllFieldsReachStore
• the create user with group IDs subtest
• the update groupIDs successfully subtest

Concurrency

There's no DB change. The write still goes through UpdateUserAdminFields, which compares group_ids with what was read, so two concurrent membership PUTs can't merge into a set nobody validated: each PUT replaces the whole membership with the set it validated, and the losing writer gets a 409 (ErrUserChanged). The only remaining window is cross-row: a group's permissions could be edited (view-only -> write) between the check and the commit. The same kind of edit can also make existing memberships contradictory at any time, with no race needed (see out of scope). Per the issue this is an accident guard, not a security boundary, so I didn't add a DB trigger.

Verification

All of this is unit, mock-store and in-process dispatch evidence (real auth.Service and real router, mocked store). I didn't run anything against real Postgres or a real cloud.

Fail-before (guard written but not wired into CreateUser / guardGroupChange):

$ go test ./internal/auth/ -count=1 -run '^TestCreateUser_RejectsContradictoryGroups$'
--- FAIL: TestCreateUser_RejectsContradictoryGroups
    mock: I don't know what to return because the method call was unexpected.
        This method was unexpected: CreateUser(context.backgroundCtx,*auth.User)
$ go test ./internal/auth/ -count=1 -run '^TestUpdateUser_RejectsChangeToContradictoryGroups$'
--- FAIL: TestUpdateUser_RejectsChangeToContradictoryGroups/replace_with_a_contradictory_set
        This method was unexpected: UpdateUserAdminFields(context.backgroundCtx,*auth.User,string,[]string,bool)
$ go test ./internal/server/ -count=1 -run GroupCombinationDispatch
--- FAIL: TestGroupCombinationDispatch_UpdateRefusedWith400
--- FAIL: TestGroupCombinationDispatch_CreateRefusedWith400   (unexpected CreateUser)

The contradictory membership reached the store write on both paths. Pass-after: all green.

Mutation probes, each applied alone and then reverted (all caught by the tests):

#	Mutation	Result
M1	drop ErrContradictoryGroups from mapAuthError (500 instead of 400)	caught
M2	guardGroupChange skips the combination check	caught
M3	checkCreateUserGroups skips the combination check	caught
M4	remove the unchanged-membership skip (legacy user locked out)	caught
M5	run the combination check before the access guards	caught
M6	empty-permission group counted as view-only	caught (after adding the w1 + e1 case)
M7	admin action not counted as write	caught
M8	pgx.ErrNoRows not skipped	caught
M9	fail open on a store error	caught
M10	require 3 groups instead of 2	caught

New tests:

• internal/auth/group_combination_test.go
  • TestCheckGroupCombination_MirrorsFrontend: a case-for-case port of the jest suite validateGroupCombination (issue #1405), plus write + empty, two view-only, unknown next to view-only, and order-independence cases
  • _UsesPermissionsNotNames
  • _StoreErrorFailsClosed
  • create reject/allow
  • update reject (including a partial change on a legacy contradictory user)
  • TestUpdateUser_LegacyContradictoryUserKeepsUnrelatedEdits: rename without groups, rename sending back the same groups, reorder, duplicate, deactivate. It also asserts no GetGroup call is made.
  • TestUpdateUser_LegacyContradictoryUserCanBeResolved
• internal/server/group_combination_dispatch_test.go: real HTTP dispatch through the router. Contradictory PUT -> 400, contradictory POST -> 400, no store write in either case. A legacy user's rename -> 200, with groups omitted and with groups sent back unchanged.
• internal/api/handler_users_test.go: TestHandler_users_ContradictoryGroupsMapTo400 (create and update).

Commands run, on macOS, with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid:

go build ./...
go vet ./...
go test ./internal/auth/ -count=1        # ok
go test ./internal/api/ -count=1         # ok
go test ./internal/server/ -count=1      # ok
go test ./internal/mocks/ -count=1       # ok (repo-wide test guards)
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/auth/... ./internal/api/... ./internal/server/...   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/auth internal/api/handler_users.go                                 # silent
bash scripts/gofmt-hook.sh <changed files>; make tidy-check; bash scripts/gosec-hook.sh <changed non-test files>  # clean
bash scripts/test-git-secrets-allowlist.sh   # 42 PASS, 0 FAIL
git secrets --scan                           # clean (changed files + .gitallowed in a fresh git init repo, isolated HOME)

Not run locally: the repo-wide go test -short -race ./... pre-commit hook (skipped because of disk limits on this machine; the change adds no concurrency) and the integration-tagged suites (no integration tests were changed). The frontend wasn't touched.

Out of scope / notes

• Editing a group's permissions can still create contradictions. PUT /api/groups/{id} can turn a view-only group write-capable (or the reverse) while it has members, which silently makes existing memberships contradictory. The issue covers user membership writes only, and the frontend has the same gap. This PR doesn't lock such users out (see above).
• The message follows Go error-string style (lower-case sentinel prefix, ; instead of .), so it isn't byte-for-byte the frontend's string. The wording and singular/plural (is/are, grants/grant) match. Duplicate IDs are de-duplicated before naming groups. The frontend would name a duplicated group twice; the classification is the same either way.
• internal/auth/service_user.go was already over the 500-line guideline before this PR, so the new logic lives in its own file.

Frontend follow-up for the UI owner

No UI change is required: the frontend already blocks these combinations and shows the backend's 400 message if one gets through. Optional: userModals.ts validates selectedGroups before an email-only edit, so a legacy contradictory user can't be renamed from the edit modal even though the backend now allows it. If that should work, skip validateGroupCombination there when the selected set equals currentEditingUser.groups.

Labels

Could a maintainer please mirror the issue's triage labels onto this PR: type/chore, triaged, priority/p3, severity/low.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 13 billable files and costs up to $3.25.}

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: b6355a8
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 2348505e-10a4-4121-bd9b-3f4ddc77f8fe

Commits

Reviewing files that changed from the base of the PR and between b6355a8 and 8ca620f.

Files selected for processing: 13

• internal/api/handler_users.go
• internal/api/handler_users_test.go
• internal/auth/errors.go
• internal/auth/group_combination.go
• internal/auth/group_combination_test.go
• internal/auth/self_account_scope_test.go
• internal/auth/self_escalation_carveout_test.go
• internal/auth/service_api_fields_test.go
• internal/auth/service_group_only_authz_test.go
• internal/auth/service_user.go
• internal/auth/service_user_test.go
• internal/server/group_combination_dispatch_test.go
• internal/server/self_account_scope_dispatch_test.go

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

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

[cristim]

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

Findings (none blocking):

• [non-blocking] internal/auth/group_combination.go:130-138 (uniqueGroupIDs) - No test covers de-duplication. Removing it (probe P5) leaves everything green. The only effect is cosmetic: a duplicated ID is named twice in the 400 message, and the classification stays correct. Worth one test case.
• [non-blocking, out of scope per the body] The rule can still be broken from the group side. PUT /api/groups/{id} can turn a view-only group that has members into a write-capable one, or the reverse. The issue covers only membership writes, so this belongs in a follow-up issue.

Against #78: this fixes the root cause.

• The rule sits in the service layer, on the only two group-writing paths:
  • CreateUser (service_user.go:172-179, 206);
  • UpdateUser via guardGroupChange (service_user.go:343, 459-464).
• Every route to them goes through that layer: CreateUserAPI/UpdateUserAPI (service_api.go:255, 286), the server adapters (app.go:1176-1181) and the handlers (handler_users.go:70, 144).
• SetupAdmin (service_user.go:52) assigns only the admin group, and the password and MFA writers do not touch group_ids.

Attacked:

• Frontend parity: I compared validateGroupCombination (frontend/src/users/utils.ts:47-75) with group_combination.go:36-104 line by line.
  • They match on the view-only/write rules, on empty groups counting as neither, on skipping unknown IDs, and on is/are and grants/grant.
  • ActionView is "view" (types.go:381).
  • scanGroup returns a bare pgx.ErrNoRows (store_postgres.go:835-836), so skipping missing groups works with the real store.
• Concurrency: UpdateUserAdminFields does a compare-and-swap on group_ids (store_postgres_credentials.go:18-24, WHERE ... group_ids = $7) and returns ErrUserChanged on a lost race. Confirmed.
• Ordering:
  • The 403 access checks run before the 400 combination check, on both create and update.
  • sameGroupSet compares both directions, so a removal-only change on a user who already has a contradictory combination is still checked.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  • P1, check only when groups are added: TestUpdateUser_RejectsChangeToContradictoryGroups FAILS.
  • P2, skip the update check for internal callers: the same test FAILS.
  • P3, count only create/update/delete as write: _UsesPermissionsNotNames and Dispatch_Update FAIL.
  • P4, ErrContradictoryGroups dropped from mapAuthError: TestHandler_users_ContradictoryGroupsMapTo400 and both dispatch tests FAIL.
  • P5, no de-duplication: not caught (the gap above).
  • P6, stop skipping a nil group: TestAPIAdapters_CreateUser_AllFieldsReachStore FAILS (nil dereference).
  • P7, skip the create check for internal callers: TestCreateUser_RejectsContradictoryGroups FAILS.

Verified locally, with no cloud credentials and GOPROXY=off, under the heavy lock:

• Merge: git merge-tree --write-tree origin/main 8ca620f is clean. Main changed internal/auth, internal/api and internal/server since the base, so I tested the merged tree too.
• Head and the merged tree, with the same results:
  • gofmt -l on the 13 changed files: clean.
  • go build ./... and go vet on auth, api and server: ok.
  • go test -count=1 ./internal/auth/ ./internal/api/ ./internal/server/: ok.
  • -race on the new tests: ok.
• Fail-before:
  • The dispatch test on base FAILS (TestGroupCombinationDispatch_UpdateRefusedWith400).
  • Base with the guard added but not wired in FAILS TestCreateUser_RejectsContradictoryGroups.
• No migration, store or frontend change.

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

[cristim]

VERDICT: APPROVE 8ca620f

Findings:

• nit (process) - the single commit is 694 changed lines, over the 400 cap. Non-blocking.
• non-blocking internal/auth/group_combination.go:64-69 + service_group.go:30 - UpdateGroup (PUT /api/groups/{id}) can turn a group that members already hold view-only into write-capable, or the reverse. That makes memberships contradictory without any user write. The PR body names this as out of scope, and the issue only covers user membership writes. Suggest a follow-up issue.
• nit group_combination.go:64 - a legacy contradictory user whose group_ids holds a stale deleted ID cannot have just that stale ID removed. The set changes, the result is re-checked, and the edit is refused with 400 unless the contradiction is also resolved. The behavior is intended, but nothing documents it.
• nit - checkGroupCombination calls GetGroup once per ID. permissionsForGroups already loads the same groups on the self-edit path. The duplicate load is small and acceptable.

Attacked:

• Rule: group_combination.go:35-104 against frontend/src/users/utils.ts:47-75. The classification matches exactly. View-only means at least one permission and every action == "view". Write-capable means any action != "view". Empty-permission groups take no part, and missing groups are skipped. Store errors fail closed, and the refusal maps to 400 (handler_users.go:88).
• Write paths: store.CreateUser is called only at service_user.go:254 (CreateUser -> checkCreateUserGroups). UpdateUserAdminFields is called only at service_user.go:366, behind guardGroupChange when GroupIDs != nil. store.UpdateUser has no non-test callers. The HTTP POST/PUT routes (handler_users.go:70, app.go:1182/1186) and the APIAdapters (service_api.go:253/275) all go through those two functions. Invites are created by CreateUser. OIDC, federation and registration handlers never set group_ids. API keys carry their own scoped permissions, not group membership, so they are not a bypass. The stateless admin key (actorUserID "") is still checked.
• Existing users: a write that leaves the membership unchanged skips the check (sameGroupSet), so renames and deactivation keep working. Login and BuildAuthContext are untouched, so nothing locks a user out. Any real change must resolve the contradiction.
• Seeded groups: Administrators (admin) and Purchase Approvers, Plan Authors and Standard Users are write-capable; Read-Only Users (000057) is view-only. Migrations 000057 and 000056 assign groups only to users whose group_ids is empty, so no seeded user is contradictory. Admin + Read-Only Users is now refused on create and change. Removing Administrators still passes the last-admin guard first.
• Ordering: access guards (403) run before the contradiction check (400).
• Merge-tree against origin/main 6af022f: clean (tree a6a0d3fd). On the merged tree with Go 1.26.10: go build ./... ok, go vet on auth/api/server ok, and go test ./internal/auth ./internal/api ./internal/server ./internal/mocks: 5766 passed. No store, SQL or migration change, so integration runs were not needed.
• Mutation probes (each restored afterwards):
  • unwire the guardGroupChange check: auth and server tests FAIL;
  • unwire the create check: auth and server tests FAIL;
  • drop the unchanged-set skip: legacy-user tests FAIL in auth and server;
  • count an empty group as view-only: MirrorsFrontend FAILs;
  • drop the 400 mapping: api and server tests FAIL.
• CI: all pass, Snyk skipped. The checks ran on the old base b6355a8 (134 commits behind main). The merged-tree local run covers the gap.

[IAmJSD]

Local verification at 8ca620f574af5304f0301b0d93d8b1f90fcff685 merged onto current main (6a4a506e), together with #1140, #1170, #1197, #1201, #1202, #1204, #1205, #1208 and #1209 in that order (combined tree 8b907eb6, Linux, Go 1.26.10):

• go build ./..., go vet ./..., go vet -tags integration ./..., (cd tests/e2e && go vet -tags e2e ./...): clean.
• go test -race -short -count=1 ./...: 36 packages ok, no failures.
• go test -race -tags integration -count=1 on every package the nine PRs change (analytics, api, auth, config, database/postgres/migrations, mocks, server), against Docker Postgres: all ok.

CI on this head: CI Success and pre-commit green. cristim's independent review approved this head. Migrations in this batch (000136-000139) are applied in numeric order. Nothing was run against any cloud.

[IAmJSD]

Marked ready and merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), after cristim's independent review approving 8ca620f5 and the combined-tree verification above. Squash-merged at exactly 8ca620f574af5304f0301b0d93d8b1f90fcff685.