sec(auth): enforce a grant ceiling and system-managed guard on group writes

[cristim]

Closes #1550.
Closes #1738.

Scope of the claim. This PR bounds writes to a group — its permissions and its allowed_accounts — on the create, update and delete paths. It does not bound the account dimension generally. The membership endpoint can still launder it: joining a wider group is not account-checked (and Administrators ships allowed_accounts = ARRAY['*']), and guardGroupChange returns early unless addsNewGroup, so a self-edit that only removes groups is unguarded entirely — dropping the scoping group leaves [], read as all accounts. Removing the restriction grants the restriction. Tracked in #1756; do not read this PR as covering it.

The defect

CreateGroupAPI / UpdateGroupAPI wrote the client-supplied permission list onto a group verbatim, with no check that the caller may grant what they are granting, and never consulted the system_managed column. Because update:groups is not one of the pairs carved out of the admin:* wildcard, any admin could void the #923 money separation-of-duties control tenant-wide in a single request:

PUT /api/groups/<administrators-uuid>
{"permissions":[{admin,*},{execute,purchases},{approve-any,purchases},{retry-any,purchases}]}

Every write path to a group's permissions

Enumerated by grepping backwards from the store methods rather than trusting the two endpoints the issues named. There are exactly four; group.Permissions = appears once outside tests.

#	Path	Before	After
1	POST /api/groups -> createGroup -> CreateGroupAPI -> Service.CreateGroup -> store.CreateGroup	no ceiling	ceiling + carve-out
2	PUT /api/groups/{id} -> updateGroup -> UpdateGroupAPI -> Service.UpdateGroup -> store.UpdateGroup	no ceiling, no system_managed	ceiling + carve-out + system_managed
3	DELETE /api/groups/{id} -> deleteGroup -> adapter -> Service.DeleteGroup -> store.DeleteGroup	unguarded, named in neither issue	system_managed
4	SQL migrations 000059 / 000064	trusted, out of band	unchanged
5	PUT /api/users/{self} -> UpdateUser -> guardGroupChange (membership, not permissions)	unguarded for money verbs	carved-out self-grant refused

#1550 and #1629 each named only two write paths; there are three. Path 3 matters because dropping the seeded Purchaser group destroys the only holder of the carved-out money verbs, which nothing can then re-grant. The purchase path would be dead tenant-wide, the same end state #1629 describes, reached through a different verb. Refusing the permission wipe while permitting the delete would be a guard with a hole in the same shape as the one it closes, so it is in this PR rather than split out.

SetupAdmin creates users, not groups. User API keys carry their own scoped permission list (#1301/#1302), not group permissions.

The two rules

1. Ceiling. A caller may only grant permissions their own effective set already holds, matched through the same carve-out-aware logic used at enforcement time (grantCeilingAllows mirrors permissionsAllow branch for branch, so the ceiling can never be looser than enforcement). It adds one requirement enforcement does not need: constraint containment. A holder capped at MaxPurchaseAmount: 100, or scoped to providers: [aws], cannot hand out an uncapped or [aws, azure] copy. Without that the ceiling would be a shape check rather than a bound.

2. The carve-out is not grantable at all. The three verbs in adminCarvedOuts may never be added to a group, whoever the caller is.

3. Blank action / resource are refused (#1730, second commit). A blank resource is not a request for the * wildcard, but that is what it became: the group-edit form picks its option with isDefault = !currentValue && resource === '*', so an empty stored resource renders as the selected All (*) entry and saves back as view:*, and nothing validated the list on the way in so the same widening was reachable from any API client with no form involved.

This is a hole the ceiling does not close, which is why it needed its own guard. Rule 1's admin:* branch grants any pair that is not carved out, and ("view", "") is not carved out. Verified by execution before writing the fix:

empty resource -> err=<nil>      # written to the store by a full admin

validateRequestedPermissions runs ahead of the ceiling on both write paths, refuses blank (empty or whitespace-only) actions and resources naming the offending entry index, and fails before the actor lookup so the refusal does not depend on who is asking. Mapped to 400, not 403: malformed input, not an authorization failure.

The two fields fail differently in the form, which is what shows the defect is in the defaulting and not the parsing: a blank action is silently dropped (index 0 is an empty placeholder) while a blank resource is silently widened (index 0 is the wildcard). Only the resource side escalates; both are refused, because a silently dropped permission is a different bug rather than an acceptable one. Unknown-but-non-blank values are deliberately still accepted (vocabulary validation is a separate concern, and rejecting values this endpoint can already have stored would break edits of existing groups); an explicit "*" stays legitimate and is gated by the ceiling instead.

#1730 has the mirror-image defect at the rendering layer. Closing it at the endpoint covers every caller, including the one that bypasses the form entirely — the same argument that makes #1550 the durable half of #1629.

Did the "non-grantable at all" framing survive contact with the carve-out semantics?

Yes, but for a reason not stated in #1550, and it turns out to be load-bearing rather than belt-and-braces.

Migrations 000059 and 000064 both run an admin-backfill: every member of the Administrators group is auto-added to the Purchaser group. So in a default deployment a typical admin explicitly holds execute:purchases, approve-any:purchases and retry-any:purchases, not merely admin:*. A pure "you cannot grant what you do not hold" ceiling would therefore have left #1550 open in the default configuration: that admin holds the verbs and could relay them onto the Administrators group in one request. TestGrantCeiling_CarvedOutNotGrantableByPurchaserAdmin is that exact case.

The cost of rule 2 is that a money-granting group can only be provisioned by migration. That is already the only way it happens: the sole such group is seeded by SQL and is system_managed. To keep the rule from becoming a footgun, a carved-out permission already stored on the target group may be carried through an unrelated edit (a rename must not be forced to strip it), but only at constraints no broader than the ones already stored, so an edit cannot raise an existing cap either.

Fail closed, and never silently narrow

An unidentified actor, or any error resolving the actor's permissions, refuses the write; there is no fall-through to allow. A refusal returns an error naming the specific permission and why (permission ceiling exceeded: cannot grant delete:accounts because ...), mapped to 403 by mapGroupAuthError. The requested list is never saved reduced to the allowed subset, which is the silent-corruption mode #1629 reports on the frontend side.

The admin API key

The stateless admin API key has no user row, so a group-derived lookup cannot resolve it and a naive ceiling would have failed closed on every admin-key group write. It is now measured against a bare {admin, *} holding (auth.AdminAPIKeyActorID, which internal/api's apiKeyAdminUserID aliases). It can still seed ordinary groups, but is subject to the same money carve-out as a human admin. That is option (a) of #1550's "either subject the admin API key to the carve-out, or document it as break-glass".

Test evidence

37 new tests: 33 in internal/auth/group_ceiling_test.go, 4 in internal/api/handler_groups_ceiling_test.go.

Two ways an assertion here could have been unfailable, both checked

(a) A name-only AssertNotCalled cannot fail. testify diffs an empty expectation against the real arguments and counts each as a difference, so AssertNotCalled(t, "UpdateGroup") passes even after a real call. Proven by execution rather than taken on trust:

name-only AssertNotCalled after a real call -> failed=false   (VACUOUS)
matcher  AssertNotCalled after a real call -> failed=true    (REAL)

All 19 AssertNotCalled calls in this PR pass one mock.Anything per parameter; a grep for the name-only form returns nothing.

(b) The actor slot filled with mock.Anything. For a grant ceiling the actor is the argument that must be pinned — the guard is "can this actor grant this permission". All six pre-existing .On("CreateGroupAPI"/"UpdateGroupAPI") sites pin the exact session user ID (adminSession.UserID / "admin"), not mock.Anything. Two of them previously pinned only the group ID; none now leaves the actor unpinned.

Separately, every refusal test stubs no CreateGroup / UpdateGroup / DeleteGroup expectation at all, so a removed guard makes the write reach the store and testify panics.

Per-guard mutation matrix

Each guard removed individually (not in aggregate), then all 25 security tests run one at a time. A test that survives its own guard's removal is vacuous. Re-run from scratch after the rebase onto c93724d38 rather than carried forward:

Mutation	killed
M1 whole ceiling removed	11/33
M2 carve-out refusal removed (rule 2)	4/33
M3 holder ceiling removed (rule 1)	4/33
M4 constraint containment removed	2/33
M5 blank action/resource validation removed (#1730)	1/33
M6 fail-closed actor resolution removed	1/33
M7 system_managed on UPDATE removed	1/33
M8 system_managed on DELETE removed	1/33
M9 403/400 error mapping removed	3/33
M10 membership carved-out guard removed	6/33
M11 membership guard inverted (over-block)	6/33
M12 self-edit routing removed	2/33
M13 prior-membership snapshot swapped for post-change membership	4/33
M14 account ceiling removed	4/33

No mutation survives.

Reproducing this requires per-test isolation. A plain go test ./internal/auth/ will NOT show these counts: a mock panic kills the test binary, so a package-level run under M13 reports 1 failure rather than 4. The harness runs each test alone (-run '^Name$') for exactly that reason. Without this note the table reads as inflated.

Three kills were strengthened after review. Twelve of the original thirteen were panic-driven, and the panics were not equal. The strong ones panic on an unstubbed write — that panic is the security fact, because the write not happening is the property. The weak ones (M5, M7, TestUpdateUser_SelfEscalationDenied) panicked on an unstubbed read downstream of the guard, a kill that evaporates the moment anyone adds a permissive stub while tidying fixtures. Those three now register the downstream reads with .Maybe() so removing the guard cannot panic, which leaves the test's own assertions as the only thing that can fail it. M7 now dies on require.Error rather than a panic. FailsClosedOnGroupLoadError additionally asserts errors.Is(err, loadErr) instead of message text, so it pins error propagation rather than wording. Every guard is killed by the tests that specifically cover it, and the narrow ones (M5, M6, M7, M8) kill exactly the one test that covers them and nothing else.

M10/M11/M12 are complementary on purpose. Two membership tests assert the guard must not fire (AdminMayAddAnotherUserToPurchaser, InternalCallerUnaffected), so guard-removal cannot kill them by construction — they are killed by M12, the routing mutation, which is what actually threatens what they protect. Stating that is more useful than a clean-sweep claim that quietly includes two assertions incapable of failing that way.

M13 is the regression barrier for the fragility above: swapping the prior-membership snapshot for the post-change list — which is what a "simplifying" fresh row read degenerates to — kills 4 tests.

Test file layout

group_ceiling_test.go grew past the repo's 500-line guideline and is split by concern, matching how the mutation matrix is organised so each file maps to a distinct set of mutations:

file	lines
group_ceiling_fixtures_test.go	58 (shared stubs only)
group_ceiling_permissions_test.go	415
group_ceiling_validation_test.go	150
group_system_managed_test.go	97
group_account_ceiling_test.go	282

A split is exactly where a test gets silently dropped, so: 707 passing test names before, 707 after, sorted and diffed — identical. Plus the full matrix re-run afterwards.

Cyclomatic margin

gocyclo -over 10 is the gate; nothing this PR adds or modifies exceeds 8:

8  guardGroupChange          8  checkGrantCeiling
7  constraintsCover          7  UpdateGroupAPI          7  createGroup
6  listCovers                6  grantCeilingAllows      6  guardSelfCarvedOutGrant
5  permissionCoveredBy       5  permissionsForGroups    5  DeleteGroup ...

guardGroupChange briefly hit 11 during the membership work and was split into guardSelfEscalation; GetUserPermissions dropped to 4 by delegating to permissionsForGroups.

The flagship failure shows the write actually landing pre-fix:

--- FAIL: TestGrantCeiling_CarvedOutNotGrantableByPurchaserAdmin
    panic: mock: I don't know what to return because the method call was unexpected
      at: [internal/auth/test_helpers.go:110       <- MockStore.UpdateGroup
           internal/auth/service_group.go:31        <- Service.UpdateGroup
           internal/auth/service_api.go:359         <- UpdateGroupAPI
           internal/auth/group_ceiling_test.go:104]

One honest exception: TestUpdateGroup_AdminAPIKeyActorIsSentinel survives every mutation. It asserts apiKeyAdminUserID == auth.AdminAPIKeyActorID and that the handler threads that sentinel — wiring that only exists on this branch, so no guard-removal can break it. Reported rather than counted as evidence.

No pre-existing test silently stopped running

Adding an argument to a mock changes what .On(...) matches, so the six edited call sites could have quietly stopped exercising what they used to. Checked against a baseline worktree at origin/main, running every test function in the three edited files:

49 test functions
baseline origin/main : 75 --- PASS: lines
this branch          : 75 --- PASS: lines
diff of the sorted passing names: IDENTICAL

Gates

go build ./...                       ok
go vet ./...                         ok
go test ./...                        ok (exit 0, full suite)
gocyclo -over 10 -ignore "_test\.go" .   no output (CI's exact invocation)
golangci-lint v2.10.1 (CI-pinned)    0 issues, repo-wide

Two findings caught locally and fixed rather than shipped to review: a govet err-shadow in updateGroup, and a misspell hit.

The membership route to the same bypass (third commit)

#1550's report names a "one-request alternative" that needs no group edit at all: PUT /api/users/{self} adding DefaultPurchaserGroupID. The pre-existing #907 self-escalation guard gates self-added groups on update:users, which admin:* grants — so it passed, and an admin could join the Purchaser group and pick up all three money verbs in one request.

Closing only the group-permission path would have left that open while Closes LeanerCloud/cloud-commitments-cli#1550 auto-closed the issue over it, so it is in this PR. guardSelfCarvedOutGrant applies the ceiling's own rule to membership: you cannot grant yourself a carved-out verb you do not already hold. It keys off the permission, not off DefaultPurchaserGroupID, so a custom group carrying a money verb is blocked identically.

What it deliberately does not block, each with a negative-control test: adding a second group carrying a verb you already hold (not an escalation); an admin adding another user to Purchaser (that is the two-person control separation of duties exists to create); and trusted internal callers (actorUserID == ""), so bootstrap and seeding are unaffected.

A latent fragility this surfaced: a guard that was correct only by accident of transaction timing

The old guard resolved the actor's permissions by re-reading their row, and applyUpdateUserRequest has already mutated the in-memory user by that point. It gave the right answer only because the write had not been committed yet — and a pre-existing test had to hand-construct "a distinct, unmutated viewer copy for that second read so it mirrors a real DB round-trip rather than aliasing the just-mutated object" to make it work. A guard whose correctness rests on that ordering defeats itself the moment a caller passes the already-mutated user.

guardSelfEscalation now resolves the actor's permissions from the prior membership snapshot via a new permissionsForGroups helper. This is not merely a fix: "did you hold this before the change?" is the question a self-escalation guard has to ask, so the correct implementation is also the simpler one. The second read is gone, and with it the test's hand-built copy and its explanatory comment. GetUserPermissions now delegates to the same helper rather than duplicating the group-walk loop.

The next person to touch this will be tempted to "simplify" it back to a fresh read, because that is the shorter spelling. A DO NOT comment on guardSelfEscalation records why that is a silent regression, and mutation M13 enforces it: swapping prior for next there must fail the suite.

Can #1629 be closed? Only partially, and a claim in the first version of this body was wrong

Correction. An earlier version of this body said "all seeded groups are system_managed, so the frontend defect can no longer corrupt any of them." That is false. Traced through the migrations and confirmed:

Group	UUID	system_managed	Set by
Administrators + 3 others	…0001–…0004	TRUE	000059 UPDATE
Standard Users	…0005	FALSE	000057 INSERT omits the column -> DEFAULT FALSE
Read-Only Users	…0006	FALSE	same
Purchaser	…0007	TRUE	000064

000074 only re-adds the column (DEFAULT FALSE); 000086 and 000088 edit those groups' permissions and never touch the column (grep count: 0 in both).

So Standard Users and Read-Only Users remain corruptible through PUT /api/groups/{id}, and neither ceiling rule stops it:

1. The drop is invisible to the ceiling. checkGrantCeiling iterates requested only; it never diffs against existing to detect removals. sec(frontend): group edit drops unrepresentable permissions and widens their resource to * #1629's primary harm is the form silently dropping cancel-own / retry-own on purchases (both on Standard Users; approve-own was removed separately by 000086), none of which the form's action list can represent. A purely narrowing write passes the ceiling untouched.
2. The widening is allowed precisely when an admin does it, which is sec(frontend): group edit drops unrepresentable permissions and widens their resource to * #1629's scenario. grantCeilingAllows hits the admin:* branch first and view:* is not in adminCarvedOuts, so view:history -> view:* is granted.

#1629 names this group explicitly: "The seeded user role-mirror group has the same shape."

Therefore:

• This PR closes sec(auth): the #923 money separation-of-duties carve-out is voidable by any admin in one request #1550 completely (see below).
• It closes sec(frontend): group edit drops unrepresentable permissions and widens their resource to * #1629 for system_managed groups only — the Purchaser group and the four …0001–…0004 groups, which covers sec(frontend): group edit drops unrepresentable permissions and widens their resource to * #1629's headline scenario (the seeded Purchaser group edit that kills the approval path tenant-wide).
• sec(frontend): group edit drops unrepresentable permissions and widens their resource to * #1629 should NOT be closed by this PR, and it carries no closing keyword.
• The residual gap on those two groups is tracked in #1739.

Deliberately not adding a migration to mark Standard Users and Read-Only Users system_managed: operators may legitimately customise Standard Users, so locking it is a product decision rather than a fix, and it is filed on LeanerCloud/cloud-commitments-platform#168 for the owner to decide.

#1550's system_managed status is irrelevant to its own closure: the money carve-out is closed by rule 2, which applies to every group regardless of the column.

Account scope: the fifth write path, folded in from #1738

Review found that an allowed_accounts-only PUT never reached the ceiling at all. checkGrantCeiling opens with if len(requested) == 0 { return nil }, and APIUpdateGroupRequest's "empty means not sent" contract makes an accounts-only request the natural shape to send. Verified by execution before fixing, with an actor holding only update:groups in one group scoped to one account:

widen to more accounts        -> ACCEPTED  (stored [acct-A acct-B acct-C])
widen to [] (= unrestricted)  -> ACCEPTED  (stored [])
widen to ["*"]                -> ACCEPTED  (stored [*])
CONTROL: same call WITH permissions -> REFUSED

The control is what makes it decisive: widening Permissions[].Constraints.AccountIDs on the same call was correctly refused. The PR bounded one account dimension carefully and left its sibling wide open on the same request — worse than never hardening the endpoint, because the next reader would reasonably assume it was covered.

checkAccountCeiling is deliberately a separate call, not a branch inside checkGrantCeiling, precisely so it cannot inherit that early return. A write is in ceiling if it does not widen the group's existing scope, or if it stays within the actor's own.

Two traps handled explicitly:

• Empty is not narrow. [] and ["*"] both mean unrestricted (IsUnrestrictedAccess), so this cannot be a subset test — the empty set is a subset of everything and means the opposite.
• Create has no prior scope. Passing existing = nil there would read as unrestricted and swallow every check, so CreateGroupAPI calls checkAccountGrant directly. An omitted allowed_accounts on create is checked too: it produces an unrestricted group, which for a scoped actor is the widest possible grant.

Every refusal test sends allowed_accounts with no permissions key. A test that included permissions alongside would pass with the bug present, because the permission ceiling would then run and refuse for an unrelated reason. That request shape is the whole finding.

Fail-closed on an unresolvable actor scope

IsUnrestrictedAccess(nil) == true is load-bearing in this guard, so the actor's own scope resolution was traced across all four failure modes. Two of them failed open:

store error on user                    -> refused
user missing (nil, nil)                -> refused
actor's only group missing (ErrNoRows) -> ACCEPTED widening to ["*"]
actor's only group returns (nil, nil)  -> ACCEPTED widening to ["*"]

collectGroupsAndAccounts skips a missing or deleted group silently, so an actor whose groups all fail to load yields an empty list, read as "all accounts" — the ceiling becomes a no-op on exactly the path it guards, and nothing errors. Note the asymmetry: the permission ceiling already failed closed on the same input, because an empty permission set grants nothing. Only the account side failed open, which is the harder direction to spot.

Requiring at least one resolved group is the precise guard: partial resolution under-reports the actor's scope, which makes the ceiling stricter, so only total failure needed closing. Mutation-checked — removing it is killed by its own test and by nothing else.

Every writer of AllowedAccounts, grepped

group.AllowedAccounts is written in exactly two places outside tests, and both are now guarded:

site	path
service_api.go:309	CreateGroupAPI struct literal
service_api.go:334	applyUpdateGroupRequest

The other matches are not group writers: service_group.go:142/:179 build the read-side AuthContext, store_postgres.go:844 is the row scanner, and service_api.go:158 is the response conversion. These two are the complete set — recorded explicitly because the enumeration on this endpoint has already been wrong once.

#1738 is closed by this. Closes LeanerCloud/cloud-commitments-cli#1550 stands unchanged: impact here is read-scope widening and non-money writes; the carve-out and Purchaser's system_managed status are untouched.

Deliberately out of scope

• Standard Users / Read-Only Users are not system_managed — tracked in #1739, including the product decision about whether to mark them so. See the sec(frontend): group edit drops unrepresentable permissions and widens their resource to * #1629 section above for why neither ceiling rule covers them.

• AllowedAccounts has no ceiling (sec(auth): group AllowedAccounts has no grant ceiling; a caller can widen a group past their own account scope #1738) — fixed in this PR, see above.

• sec(auth): execute:ri-exchange missing from adminCarvedOuts; admin:* bypasses every exchange guardrail #1644 (execute:ri-exchange missing from adminCarvedOuts). Not fixed here, as instructed; noted on that issue. It is now a one-line change: adding the pair to adminCarvedOuts makes both permissionsAllow and this ceiling pick it up automatically, and grantCeilingAllows re-checks the carve-out map inside its admin:* branch specifically so the two stay in lockstep when that set grows.

• A user API key acts as its owning user for ceiling purposes, so a narrowly-scoped key inherits the owner's grant ceiling. No escalation beyond the owner, but it is the same key-scope gap tracked by sec(auth): unscoped user API key silently inherits owner's full permissions cloud-commitments-platform#61/#1302.

CI

Expect npm audit red (js-yaml / nanoid) — unrelated, being fixed separately.

Summary by CodeRabbit

• New Features

  • Added authorization safeguards for group permissions and account-scope changes.
  • Prevented users from granting themselves restricted permissions or widening access beyond their authority.
  • Added protection for system-managed groups, which can no longer be modified or deleted.

• Bug Fixes

  • Group authorization failures now return clearer client errors, including appropriate 400 or 403 responses.
  • Group changes now consistently evaluate the acting user’s permissions and identity.

[coderabbitai]

📝 Walkthrough

Walkthrough

Group creation and update APIs now propagate actor identities. Auth services enforce permission and account-scope ceilings, reject malformed grants, protect system-managed groups, and block self-escalation through newly assigned groups. API handlers map authorization errors to client responses.

Changes

Group authorization

Layer / File(s)	Summary
Actor propagation and API error mapping internal/api/*, internal/server/app.go, internal/auth/service_api_test.go, internal/server/adapter_test.go	Group API contracts and adapters now carry actorUserID. Handlers forward session or admin API-key identities and map authorization sentinels to 400 or 403 responses.
Permission and account-scope ceilings internal/auth/group_ceiling.go, internal/auth/service_api.go, internal/auth/*ceiling*_test.go, internal/auth/group_*_test.go	Group writes validate permission entries, grant coverage, constraints, purchase limits, and account scopes. Tests cover fail-closed behavior, admin API-key handling, persistence, and create/update paths.
System-managed group protection and permission loading internal/auth/service_group.go, internal/auth/group_system_managed_test.go, internal/auth/service_group_test.go, internal/server/adapter_test.go	Deletion now loads the target group and rejects missing or system-managed groups. Permission aggregation uses an explicit group list while preserving deleted-group handling.
Self-escalation protection internal/auth/service_user.go, internal/auth/self_escalation_carveout_test.go, internal/auth/service_group_only_authz_test.go	Membership updates inspect newly added groups and reject self-assignment of unheld carved-out permissions. Group lookup failures reject the update without writing user state.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related issues

• sec(auth): account scope can be laundered through the group-membership endpoint (join a wider group, or leave the scoping one) #1756 — Both changes address account-scope escalation during group-related authorization, although this PR covers group writes.
• sec(auth): group AllowedAccounts has no grant ceiling; a caller can widen a group past their own account scope #1738 — Both changes modify grant-ceiling checks for actor permissions and allowed accounts.

Possibly related PRs

• LeanerCloud/CUDly#353 — Uses related authentication sentinel and API-handler error-mapping patterns.
• LeanerCloud/CUDly#912 — Uses related admin API-key actor identity propagation.
• LeanerCloud/CUDly#1744 — Uses related authorization test fixtures for principal and permission checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 70.31% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main authorization changes for grant ceilings and system-managed group write protection.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch sec/1550-group-grant-ceiling

---

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

[coderabbitai]

Actionable comments posted: 3

Caution

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

⚠️ Outside diff range comments (1)

internal/server/adapter_test.go (1)

352-356: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use typed requests and assert success.

CreateGroupAPI accepts auth.APICreateGroupRequest. UpdateGroupAPI accepts auth.APIUpdateGroupRequest. Each map value causes an invalid request type error. Each test discards that error, so it does not verify adapter forwarding or the admin actor sentinel.

• internal/server/adapter_test.go#L352-L356: pass auth.APICreateGroupRequest and use require.NoError(t, err).
• internal/server/adapter_test.go#L369-L372: pass auth.APIUpdateGroupRequest and use require.NoError(t, err).

Proposed test correction

- _, err := adapter.CreateGroupAPI(ctx, auth.AdminAPIKeyActorID, map[string]interface{}{
-   "name":        "test-group",
-   "permissions": []string{"read:config"},
- })
- _ = err
+ _, err := adapter.CreateGroupAPI(ctx, auth.AdminAPIKeyActorID,
+   auth.APICreateGroupRequest{Name: "test-group"},
+ )
+ require.NoError(t, err)

- _, err := adapter.UpdateGroupAPI(ctx, auth.AdminAPIKeyActorID, "group-1", map[string]interface{}{
-   "name": "new-name",
- })
- _ = err
+ _, err := adapter.UpdateGroupAPI(ctx, auth.AdminAPIKeyActorID, "group-1",
+   auth.APIUpdateGroupRequest{Name: "new-name"},
+ )
+ require.NoError(t, err)

🤖 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 `@internal/server/adapter_test.go` around lines 352 - 356, Update
internal/server/adapter_test.go:352-356 to pass an auth.APICreateGroupRequest to
CreateGroupAPI and assert require.NoError(t, err); update
internal/server/adapter_test.go:369-372 to pass an auth.APIUpdateGroupRequest to
UpdateGroupAPI and likewise assert success, ensuring both tests validate adapter
forwarding and the admin actor sentinel.

🧹 Nitpick comments (2)

internal/auth/service_user.go (1)

443-443: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Align the prior and next parameter order across the guard helpers.

guardSelfEscalation uses (prior, next) and addedGroups uses (prior, next), but guardSelfCarvedOutGrant declares (held, next, prior). Both membership arguments are []string, so a future swap at the call site compiles and silently inverts the added-group set. Reorder the parameters to (held, prior, next).

♻️ Proposed parameter reorder

-func (s *Service) guardSelfCarvedOutGrant(ctx context.Context, held []Permission, next, prior []string) error {
+func (s *Service) guardSelfCarvedOutGrant(ctx context.Context, held []Permission, prior, next []string) error {
 	added := addedGroups(prior, next)

Update the call site in guardSelfEscalation:

return s.guardSelfCarvedOutGrant(ctx, heldBefore, prior, next)

🤖 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 `@internal/auth/service_user.go` at line 443, Reorder the
guardSelfCarvedOutGrant parameters to (ctx, held, prior, next) and update its
call in guardSelfEscalation to pass prior before next, preserving the existing
behavior.

internal/auth/self_escalation_carveout_test.go (1)

226-228: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Assert the wrapped store error, not only the message text.

The test asserts on the substring "failed to load group". That couples the test to the wording in guardSelfCarvedOutGrant. guardSelfCarvedOutGrant wraps the store error with %w, so loadErr is recoverable. Add an assert.ErrorIs on loadErr to keep the test stable when the message changes.

♻️ Proposed assertion

 	require.Error(t, err)
+	assert.ErrorIs(t, err, loadErr)
 	assert.Contains(t, err.Error(), "failed to load group")
 	mockStore.AssertNotCalled(t, "UpdateUser", mock.Anything, mock.Anything)

🤖 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 `@internal/auth/self_escalation_carveout_test.go` around lines 226 - 228,
Update the test assertion around guardSelfCarvedOutGrant to verify the wrapped
store error via assert.ErrorIs using loadErr, while retaining the existing error
presence and message checks. Ensure the test confirms the original error
identity rather than relying solely on the "failed to load group" wording.

🤖 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.

Inline comments:
In `@internal/api/types.go`:
- Around line 189-195: Replace the any request and response types in
CreateGroupAPI and UpdateGroupAPI with the concrete group request and response
types already used by the implementation. Propagate these signatures through the
service adapter in service_api.go, mocks, and all callers, removing runtime type
assertions and invalid-request-type handling that become unnecessary.

In `@internal/auth/group_ceiling_test.go`:
- Around line 13-642: Split internal/auth/group_ceiling_test.go into focused
test files under internal/auth/, keeping each file below 500 lines. Group
related suites such as grant-ceiling update/create tests and
system-managed-group immutability tests together, while preserving shared
helpers and test behavior without changing production logic.

In `@internal/auth/service_api.go`:
- Around line 289-310: The group mutation APIs currently persist directly
through CreateGroup and UpdateGroup; introduce the event store, group event
types, and projection, then update CreateGroupAPI and UpdateGroupAPI to append
the corresponding recorded events and apply state through the projection instead
of direct groups-table writes. Preserve existing validation, permission checks,
request handling, and error propagation while ensuring both mutations are
represented in the event stream.

---

Outside diff comments:
In `@internal/server/adapter_test.go`:
- Around line 352-356: Update internal/server/adapter_test.go:352-356 to pass an
auth.APICreateGroupRequest to CreateGroupAPI and assert require.NoError(t, err);
update internal/server/adapter_test.go:369-372 to pass an
auth.APIUpdateGroupRequest to UpdateGroupAPI and likewise assert success,
ensuring both tests validate adapter forwarding and the admin actor sentinel.

---

Nitpick comments:
In `@internal/auth/self_escalation_carveout_test.go`:
- Around line 226-228: Update the test assertion around guardSelfCarvedOutGrant
to verify the wrapped store error via assert.ErrorIs using loadErr, while
retaining the existing error presence and message checks. Ensure the test
confirms the original error identity rather than relying solely on the "failed
to load group" wording.

In `@internal/auth/service_user.go`:
- Line 443: Reorder the guardSelfCarvedOutGrant parameters to (ctx, held, prior,
next) and update its call in guardSelfEscalation to pass prior before next,
preserving the existing behavior.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 23db5783-fa69-4ab8-9f4a-e25815bd58ed

📥 Commits

Reviewing files that changed from the base of the PR and between 9145e0f and 29f11ee.

📒 Files selected for processing (21)

• internal/api/handler.go
• internal/api/handler_coverage_test.go
• internal/api/handler_groups.go
• internal/api/handler_groups_ceiling_test.go
• internal/api/handler_groups_test.go
• internal/api/handler_ri_exchange_test.go
• internal/api/handler_router_test.go
• internal/api/mocks_test.go
• internal/api/types.go
• internal/auth/errors.go
• internal/auth/group_ceiling.go
• internal/auth/group_ceiling_test.go
• internal/auth/self_escalation_carveout_test.go
• internal/auth/service_api.go
• internal/auth/service_api_test.go
• internal/auth/service_group.go
• internal/auth/service_group_only_authz_test.go
• internal/auth/service_group_test.go
• internal/auth/service_user.go
• internal/server/adapter_test.go
• internal/server/app.go

[cristim]

Worth recording here, because it bears directly on what this PR defends.

While establishing ground truth for #1596, grantAdmin was instrumented to record every (action, resource) pair the internal/api suite actually asks for. Two of the thirty are #923 carved-out money verbs, and grantAdmin answers true for both:

  12  execute:purchases        -> grantAdmin returns TRUE
  23  approve-any:purchases    -> grantAdmin returns TRUE

Production, verified by execution against the real matcher with AuthContext{Permissions: [{admin,*}]}:

admin:* -> execute:purchases     = false
admin:* -> approve-any:purchases = false
admin:* -> retry-any:purchases   = false

So grantAdmin models an admin who can spend money, and production has no such principal — that is precisely what the carve-out forbids.

The consequence: adminCarvedOuts could be deleted entirely and no test in internal/api would fail.

The separation-of-duties control this PR spends three commits defending at the service layer has zero handler-level coverage. That does not weaken this PR — the ceiling and the carve-out refusal are independently mutation-verified here, 13 mutations across 25 tests with no survivors. But it means the handler tier has never enforced the same property, and a regression there would have been silent.

Being fixed in #1596, which routes grantAdmin through the real auth.AuthContext.HasPermission so the carve-out actually applies, and adds a test that fails when adminCarvedOuts is removed.

Separately filed: retry-any:purchases is asked zero times by the suite and has no non-test reference in internal/api at all — a different shape of gap, since fixing grantAdmin cannot create coverage that was never written.

[cristim]

Adversarial review, head 9387aac4f

Independent reviewer, taking this over fresh. Everything below was re-derived in a clean worktree at the head; the prep findings folded into the PR were re-checked rather than assumed, and one of them turned out to matter. Findings split into execution-verified and reading-derived.

Gates, run locally at the head

gate	result
go build ./...	clean
go vet ./...	clean
go test -race -count=1 ./...	exit 0, 31 packages ok, 0 FAIL
gocyclo -over 10 -ignore "_test\.go" .	clean
golangci-lint run at CI-pinned v2.10.1	0 issues.

CI on this head, read from the raw check-runs API rather than the summary: 19 named checks plus CI Success, all completed, all success, no null or queued conclusions.

---

F1 (the fifth vector) AllowedAccounts is written by the same endpoint with no ceiling at all

The PR's enumeration is scoped to "every write path to a group's permissions", and within that scope it is complete. But UpdateGroupAPI and CreateGroupAPI also write Group.AllowedAccounts, and nothing bounds it.

applyUpdateGroupRequest (internal/auth/service_api.go) does group.AllowedAccounts = req.AllowedAccounts with no check, and AllowedAccounts is a live authorization dimension: collectGroupsAndAccounts (internal/auth/service_group.go:173-180) unions it into AuthContext.AllowedAccounts, internal/server/app.go:1159 returns it from GetAllowedAccountsAPI, and internal/api/handler.go:526-538 feeds it to accounts, analytics, dashboard, history, inventory, ladder, marketplace, ri-exchange and scoping.go.

IsUnrestrictedAccess (internal/auth/types.go:167) treats an empty list or one containing "*" as all accounts, and its own comment flags this as a "fail-open default (03-L5)". So writing "allowed_accounts": [] is a request for unrestricted access.

The sharpest part, and the reason this is not merely an unguarded field: an allowed_accounts-only PUT never reaches the ceiling at all. checkGrantCeiling opens with

if len(requested) == 0 {
    return nil
}

so omitting permissions from the body returns before grantCeilingPermissions even resolves the actor. APIUpdateGroupRequest's documented "empty means not sent" contract is what makes that shape natural to send.

Execution evidence. A throwaway test with an actor holding only update:groups + view:groups (no admin:*), member of one non-system_managed group scoped to ["acct-restricted"], PUTing its own group:

request	result	resulting access
allowed_accounts: ["acct-restricted","acct-victim","acct-prod"]	err=<nil>, written	widened
allowed_accounts: []	err=<nil>, written	IsUnrestrictedAccess=true
allowed_accounts: ["*"]	err=<nil>, written	unrestricted

Control, same actor and group: widening Permissions[].Constraints.AccountIDs from ["acct-restricted"] to include acct-victim is refused with permission ceiling exceeded. So the PR bounds one account dimension and leaves its sibling open on the same call. CreateGroupAPI has the identical gap.

Reachability, stated honestly in both directions. This is not exploitable in a default deployment. admin:* does not bypass account scoping (the only short-circuit in getAllowedAccounts is the stateless admin API key at handler.go:531); Administrators members are unrestricted only because 000024 seeds them allowed_accounts = ARRAY['*']. Every seeded group carries ['*'], and AllowedAccounts is a union, so membership in any seeded group makes a user unrestricted regardless. The vulnerable principal must therefore be in only custom groups, one of which holds update:groups and is account-restricted.

That configuration is not hypothetical: frontend/src/groups/groupModals.ts:151 offers groups as a permission resource, and :424-470 saveDuplicateGroup duplicates a group copying permissions verbatim while letting the admin tick account checkboxes to set allowed_accounts. Duplicating a delegated-admin group into a per-region team with accounts ticked produces exactly this principal in two clicks.

Impact bound. Not a money escalation: Purchaser is system_managed and ['*']-scoped, and the carve-out still blocks API-granting the money verbs. The realistic impact is read-scope widening across every account-scoped endpoint plus non-money write verbs the principal already holds (update:plans, delete:plans, update:purchases, revoke) reaching accounts outside their assignment.

Recommendation. Either add the account-scope ceiling here or narrow the Closes LeanerCloud/cloud-commitments-cli#1550 claim and file it. The fix mirrors listCovers so both dimensions share semantics: a helper that returns early on nil (preserving "not sent"), fails closed on an unidentified actor, returns nil for AdminAPIKeyActorID, and otherwise refuses when the request IsUnrestrictedAccess while the actor is not, or names an account the actor's own AllowedAccounts does not cover. Call it from both write paths; mapGroupAuthError already maps ErrPermissionCeiling to 403, so no handler change. The regression test has to be the real shape: {"allowed_accounts": []} with no permissions key, asserting 403 and that UpdateGroup is never called. A test that sends permissions alongside would pass with the bug present, because the ceiling would then actually run.

---

F2 The mutation matrix reproduces, but it is weaker evidence than the table implies

I spot-checked M13, M7 and M5 independently. All three counts reproduce exactly: 4 / 1 / 1, no survivors. Two things about the method are worth recording, because they change how the table should be read.

(a) The matrix is not reproducible by a plain package run. Every kill is a testify mock panic or sits behind one, and a panic kills the whole test binary. Under M13, go test ./internal/auth/ reports one failure and the process dies; the other three never execute. Reproducing 4 requires compiling the test binary and running each top-level test in isolation. A control run of the same harness on the unmutated tree produced zero failures, so the counts are sound, but anyone re-running the stated command will see 1 and conclude the table is inflated. Worth a sentence in the PR.

(b) 12 of the 13 killed subtests across those three rows are panic-driven, not assertion-driven, and the panics are not all equal:

• Strong. TestSelfCarvedOutGrant_AdminCannotJoinPurchaser and ..._CustomGroupWithMoneyVerbAlsoBlocked panic on an unstubbed UpdateUser. That panic is the security fact: the mutation let the self-escalation through to the write. This is the convention working as designed.
• Weak. M7, M5 and TestUpdateUser_SelfEscalationDenied panic on an unstubbed read (GetUserByID / GetGroup) that merely happens to sit downstream of the removed guard. These kills would evaporate if anyone added a permissive GetUserByID stub to the fixture, which is a plausible future edit, and the mutation would then survive silently.
• Weakest. TestSelfCarvedOutGrant_FailsClosedOnGroupLoadError is a genuine assertion, but only on error-message text. Under M13 the code still fails closed and still never writes; only the wording changed. That kill says nothing about the security property.

Net: of M13's four kills, roughly two are meaningfully about what M13 threatens. The count is correct; the strength is oversold by presenting all rows as equivalent. Adding an explicit AssertNotCalled(t, "UpdateUser", mock.Anything, mock.Anything) to the three weak cases would make those kills behavioural rather than incidental.

---

What holds up

permissionsForGroups durability (attack 1). Nothing it reads is influenced by the same request. It is called with prior, which is not client-supplied, and it resolves group permissions via store.GetGroup. UpdateUser performs no group write, so no input to the decision is mutated by the request making the decision. Its fail-closed behaviour is correct: a transient store error propagates, and only a genuinely absent group (pgx.ErrNoRows or nil, nil) is skipped.

The snapshot genuinely removes the ordering dependence (attack 2). UpdateUser captures

priorGroups := append([]string(nil), user.GroupIDs...)

before applyUpdateUserRequest(user, req), and it is a real copy of the backing array, not a re-slice. So even though applyUpdateUserRequest sets user.GroupIDs = req.GroupIDs at service_user.go:544, priorGroups is unaffected. The old guard's correctness rested on the write being uncommitted; this one rests on a value captured before the mutation. That is a genuine removal of the dependence, not a relocation of it. The self-escalation branch only runs when actorUserID == targetUserID, so prior is the actor's own prior membership in that branch.

UpdateUser is also the only membership-mutation path for an existing user: user.GroupIDs = appears exactly once outside tests.

Assertion arity (attack 5) is fully correct. All 19 AssertNotCalled calls the PR adds pass exactly 2 matchers, and I verified this against what testify actually diffs, which is what m.Called records, not the declared signature. Every named method in internal/auth/test_helpers.go calls m.Called(ctx, x) with exactly 2 arguments: UpdateGroup, CreateGroup, DeleteGroup, GetUserByID, UpdateUser, CreateUser, DeleteUser. MockStore is a plain testify mock with no isExpected short-circuit, so every call lands in mock.Calls and the sites are genuinely failable. No arity mismatches.

One precision note on the claim "a grep for the name-only form returns nothing": true for the calls this PR adds. There is one pre-existing name-only site in a file the PR touches, internal/api/handler_ri_exchange_test.go:971 (opsClient.AssertNotCalled(t, "ListExchangeableReservations")), present in the base c93724d38 and introduced by #1711. It is vacuous, and it guards Azure exchangeable-RI listing being scoped to the session's allowed accounts, which is the same account-scoping concern as F1. It belongs to #1740.

---

Prep findings: re-checked, and one correction

• Two seeded groups are not system_managed: confirmed. 000059 marks only …0001 through …0004; Standard Users (…0005) and Read-Only Users (…0006) are seeded by 000057 and are excluded, and 000074 re-adds the column with DEFAULT FALSE. Both carry allowed_accounts = ARRAY['*']. Removing the "all seeded groups are protected" claim and citing sec(auth): Standard Users and Read-Only Users are not system_managed, so #1737's guard does not protect them cloud-commitments-platform#168 was right.

  Correction worth recording: while chasing F1 I initially had an analysis asserting all seeded groups are system_managed, which would have understated F1's editable surface. It is wrong, and the migrations are the authority. I mention it because that error points the same way twice: it makes both the PR's original claim and my own reachability analysis look safer than they are.

• The migration backfill is load-bearing: confirmed. 000064 unconditionally inserts every Administrators member into Purchaser, so a default admin explicitly holds the money verbs and rule 1 alone would let them relay those onto their own group. The unconditional carve-out refusal is necessary, not belt-and-braces.

• The ceiling's three inputs are server-fetched: confirmed. Actor ID from the session, actor permissions from the store, existing group permissions from the store. None attacker-controlled.

• Constraint containment covers all five PermissionConstraints fields: confirmed (AccountIDs, Providers, Services, Regions, MaxPurchaseAmount), with listCovers correctly refusing an empty request against a non-empty holder and amountCovers refusing an uncapped grant from a capped holder. Which is exactly what makes F1 stand out: the account dimension is guarded carefully inside a permission and not at all beside it.

---

Verdict

The core of this PR is strong. The two rules are correctly specified, the carve-out is genuinely load-bearing rather than defensive, the fail-closed paths have no fall-through, the system_managed guard covers update and delete, the blank-field validation closes a real widening the ceiling structurally could not, the membership route is closed against the one-request alternative, and the prior-membership snapshot is a real fix for a real fragility rather than a restatement of it. The assertion hygiene is better than most security PRs I have reviewed: the arity is right everywhere and the actor slot is pinned.

F1 is the one substantive gap, and it is a gap in the claim rather than in the code that was written: Closes LeanerCloud/cloud-commitments-cli#1550 reads as completeness over the endpoint, and the endpoint can still widen account scope without limit, via a request shape that skips the ceiling entirely. It is not exploitable in a default deployment and it is not a money escalation, so I would not call it a merge blocker on severity alone. But it should either be fixed here (roughly one helper and two call sites) or filed with the claim narrowed, because leaving it unstated invites the next reader to assume the endpoint is bounded when half of it is not.

F2 is a documentation fix to the evidence, not to the code.

Reviewer notes: the five gates, the F1 exploit and its control, the three mutation rows with their kill mechanisms, and the arity verification are execution-verified. The F1 reachability analysis, the permissionsForGroups durability argument and the snapshot analysis are reading-derived, though each load-bearing fact (the len(requested) == 0 early return, the append([]string(nil), ...) copy ordering, IsUnrestrictedAccess's empty-means-all, the 000059 ID list, and the m.Called arities) was read directly at the cited line rather than taken from any summary.

[cristim]

Independent delta review, head f7137d567

Second independent reviewer, fresh worktree. I was briefed to review 2a8786f9 and ran the first half of this against that head; f7137d567 landed mid-review, so every gate, probe and mutation below was re-run at f7137d567 and the 2a8786f9 results are kept only where they establish that a finding was real before it was fixed. Findings are marked execution-verified or reading-derived.

---

Check 1 — does an actor whose scope fails to resolve read as unrestricted?

It did at 2a8786f9. It does not at f7137d567. Both verified by execution, same probe, same seven inputs, on the real UpdateGroupAPI with an allowed_accounts-only PUT widening to ["*"]:

actor-scope failure mode	2a8786f9	f7137d567
GetUserByID returns an error	REFUSED	REFUSED
user row missing (nil, nil)	REFUSED	REFUSED
user row missing (pgx.ErrNoRows)	REFUSED	REFUSED
actor's only group row missing (ErrNoRows)	ACCEPTED, written to store	REFUSED
actor's only group resolves to (nil, nil)	ACCEPTED, written to store	REFUSED
GetGroup returns a hard error	REFUSED	REFUSED
actor has no group memberships at all	ACCEPTED, written to store	REFUSED
control: scoped actor, resolvable	REFUSED	REFUSED

The three fail-opens all had the same cause: collectGroupsAndAccounts skips a missing or deleted group silently, so total resolution failure yields an empty AllowedAccounts, and IsUnrestrictedAccess([]) is true. f7137d567's len(authCtx.Groups) == 0 guard in grantCeilingAccounts closes all three; at the new head every refusal carries the acting user's account scope could not be established (no group resolved) and UpdateGroup is never called.

Two things I checked about the new guard specifically, both fine:

• Partial resolution (one of two groups missing) still passes the guard and under-reports the actor's scope, which makes the ceiling stricter. Only total failure needed closing, and only total failure is closed.
• The asymmetry argument holds: grantCeilingPermissions needs no equivalent, because an empty permission set makes grantCeilingAllows return false for every request.

One process note, not a code finding. The PR body described this guard as implemented and mutation-checked while the pushed head (2a8786f9) did not contain it — no len(authCtx.Groups) check, and no test covering the case. The body ran ahead of the push by one commit. The code now matches the body.

---

Check 2 — every writer of AllowedAccounts

Re-enumerated independently (assignment-and-struct-literal grep over *.go, plus a separate allowed_accounts grep over *.sql), not by re-walking the endpoint.

site	kind	guarded
internal/auth/service_api.go:309 (CreateGroupAPI literal)	write	yes — checkAccountGrant at :302
internal/auth/service_api.go:334 (applyUpdateGroupRequest)	write	yes — checkAccountCeiling at :363
internal/auth/service_api.go:158 (groupToAPIGroup)	response projection	n/a
internal/auth/store_postgres.go:844 (scanGroup)	row scan	n/a
internal/auth/service_group.go:142/:179	builds AuthContext, not a group	n/a
migrations 000001, 000024, 000057, 000059, 000064	SQL seed / DDL	out of band

The PR's enumeration is correct and complete. No membership endpoint, bulk import, admin-repair route, error path or cleanup path writes group.AllowedAccounts. Both write paths are guarded, both guards run before the field is assigned.

But the guard's other input is writable by the actor — F1 below.

---

F1 (live) — the account ceiling can be laundered through the membership endpoint

Execution-verified. checkAccountGrant compares the request against grantCeilingAccounts(actor), which is derived from user.GroupIDs. That field is writable by the actor on themselves, and the write is not bounded on the account dimension.

Two one-request routes, both accepted at f7137d567:

PROBE[self-join a wildcard-scoped group]        : ACCEPTED (wroteToStore=true) err=<nil>
PROBE[self-leave the account-scoping group]     : ACCEPTED (wroteToStore=true) err=<nil>
PROBE[post-leave scope]: AllowedAccounts=[]  IsUnrestrictedAccess=true

And the laundering control, same actor, same target group, same request:

grant acct-VICTIM BEFORE self-join : permission ceiling exceeded: cannot grant access to
                                     cloud account "acct-VICTIM" because your own account
                                     scope does not include it
grant acct-VICTIM AFTER  self-join : err=<nil>   (written to store)

The principal: a delegated admin in one group carrying update:users + update:groups, and one group carrying AllowedAccounts: ["acct-A"].

• Join route. guardSelfEscalation gates self-added groups on update:users (which this actor holds) and then guardSelfCarvedOutGrant checks only the carved-out money verbs of the group being joined. Account scope is not checked, so joining any broader-scoped group is allowed. A default deployment ships one: Administrators, seeded allowed_accounts = ARRAY['*'] by 000024/000057.
• Leave route. guardGroupChange returns early unless addsNewGroup(prior, next), so a self-edit that only removes groups is not guarded at all. Dropping the scoping group leaves AllowedAccounts = [], which IsUnrestrictedAccess reads as all accounts. Removing the restriction grants the restriction.

Impact is not confined to the ceiling: scoping.go:requireAccountAccess / requirePlanAccess and every getAllowedAccounts caller short-circuit on IsUnrestrictedAccess, so the actor gains real read and write reach across every account after either request.

This is a pre-existing property of the group-only auth model, not a regression introduced here — and this PR is the first to depend on that property being sound. It is also the same shape the PR already closed on the permission dimension: guardSelfCarvedOutGrant exists precisely because the membership route was the one-request alternative to writing verbs onto your own group. The account dimension has the identical alternative and no equivalent guard.

Suggested resolution: either extend guardSelfCarvedOutGrant to refuse a self-membership change that widens the actor's own AllowedAccounts (join and leave), or file it and narrow Closes LeanerCloud/cloud-commitments-cli#1738 to "the group-write path". I would not block merge on it — the diff makes things strictly better — but the claim currently reads as a bound on account scope, and the bound is one self-edit deep.

---

F2 (live, low) — two comparison helpers are more permissive than the enforcement they claim to mirror

Reading-derived, both sides read at the cited line.

accountScopeGap (group_ceiling.go) documents "Comparison is exact, matching MatchesAccount. Case folding would only make the check more permissive, which is the wrong direction for a ceiling." — but it trims both sides:

permitted[strings.TrimSpace(a)] = true      // outer
if !permitted[strings.TrimSpace(a)] {       // requested

MatchesAccount (types.go:188) is a == accountID — no trim, no fold. So an actor whose stored scope is " acct-A" (matching nothing at enforcement) is treated by the ceiling as holding "acct-A" and may grant it. Trimming is the same direction the comment rules out.

Same drift in listCovers, whose comment says values are "trimmed and lower-cased, matching matchAllRegionsConstraint's comparison". That is true for Regions only. AccountIDs, Providers and Services are enforced by containsAny (service_helpers.go:130), which is an exact, case-sensitive map lookup. A holder constrained to AccountIDs: ["ACCT-1"] may therefore grant ["acct-1"], a value they cannot themselves use.

Contrived to reach, and the fix is one line either way — but the comments assert exactness as the safety argument, so they should match the code.

---

F3 (live, test quality) — the third .Maybe() strengthening landed in the wrong test

Execution-verified. The body states M5, M7 and TestUpdateUser_SelfEscalationDenied now register their downstream reads with .Maybe() so guard removal cannot panic. Two of the three are correct:

• M5 (group_ceiling_validation_test.go) — .Maybe() on UpdateGroup/CreateGroup plus explicit AssertNotCalled on both, and on GetUserByID. Proof preserved as an assertion.
• M7 (group_system_managed_test.go) — .Maybe() on the actor reads and on UpdateGroup, with require.Error + ErrorIs + AssertNotCalled(UpdateGroup). Proof preserved.

The third did not land on TestUpdateUser_SelfEscalationDenied (service_group_only_authz_test.go:235), which is unchanged. The two permissive stubs went to its neighbour TestGroupOnlyAuthz_NonAdminDenied (:75), where they are unreachable:

// service_group_only_authz_test.go:86-90  (added by 2a8786f9)
// ... With it, the removal is caught by AssertNotCalled below,
// which is the actual security property (no write happened).
mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Maybe()
mockStore.On("GetGroup", ctx, DefaultAdminGroupID).Return(adminGroup(), nil).Maybe()

That test calls only HasPermission and UserHasAdminCapability. It never invokes UpdateUser, never adds a group, never touches DefaultAdminGroupID (the viewer's only membership is viewerGroup().ID), and contains no AssertNotCalled — the comment refers to one that does not exist. Both stubs are dead, and TestUpdateUser_SelfEscalationDenied is still killed by a panic on an unstubbed UpdateUser, which is the exact weakness the body says was removed.

---

Gates

go build ./... and go vet ./... are clean at f7137d567 (exit 0, no output), gocyclo -over 10 -ignore "_test\.go" . reports 0 findings at the CI-pinned v0.6.0 threshold. CI on this head, read from the raw check-runs API: 19 checks, all completed / success, no null or queued conclusions. The -race suite and golangci-lint at the CI-pinned v2.10.1 (installed specifically, local default is 2.11.4 and has a different bundled-gosec ruleset) are re-running locally on a heavily contended box; I will post those exit codes and the mutation matrix separately rather than assert them from the earlier head's run.

Partial matrix data, measured at 2a8786f9 with per-test isolation (241 top-level tests in internal/auth, each run alone; control run on the unmutated tree: 0 kills / 241):

M14 (both account-ceiling calls removed): 5 kills, not 4 —

test	kill mechanism
TestAccountCeiling_RefusesWideningWithNoPermissionsSent	panic on unstubbed UpdateGroup — the write landing is the security fact
TestAccountCeiling_CreateIsBounded	panic on unstubbed CreateGroup — same
TestAccountCeiling_FailsClosedOnUnidentifiedActor	panic on unstubbed UpdateGroup — same
TestAccountCeiling_AllowsInScopeWidening	assertion (AssertExpectations: the actor lookup no longer happens)
TestAccountCeiling_UnrestrictedActorMayWiden	assertion, same mechanism

Worth stating precisely, because F2 of the previous review turned on this: the three panic-driven kills here are the strong kind — they panic on an unstubbed write, so the panic is the property (the widening reached the store), not an incidental downstream read. The two assertion-driven kills are the positive controls failing because the actor is no longer resolved; they are real signal that the controls exercise the actor branch, but they are not themselves security assertions.

---

Claims re-verified

• The split preserved everything. group_ceiling_test.go @ 9387aac4f (642 lines) vs the four files at head: 31 test + subtest names on each side, sorted diff empty. File sizes match the stated 58 / 415 / 150 / 97 / 282. Package-wide the delta is additions only: 624 → 634 names, the extra 10 being the 8 TestAccountCeiling_* functions plus 2 subtests.
• Every refusal test sends allowed_accounts with no permissions key. Checked one by one: RefusesWideningWithNoPermissionsSent (3 subtests), FailsClosedOnUnidentifiedActor, FailsClosedWhenActorScopeUnresolvable (2 subtests), and CreateIsBounded/out of scope is refused. None carries a Permissions field. This is the shape that would otherwise pass with the bug present.
• The positive controls reach the actor branch. AllowsInScopeWidening widens [A] → [A,B] inside an actor scoped to [A,B], so accountScopeGap(existing, requested) is non-empty and checkAccountGrant genuinely runs; its non-.Maybe() actor stubs under AssertExpectations prove the lookup happened, and M14 kills it for exactly that reason. UnrestrictedActorMayWiden and CreateIsBounded/in scope likewise resolve the actor. UnchangedScopeSkipsActorLookup is the deliberate counterpart and asserts the lookup does not happen.

---

Verdict

The account ceiling is correctly specified and, since f7137d567, correctly fails closed on an unresolvable actor scope — I could not construct an input that widens a group's AllowedAccounts beyond the actor's own through either write path. The AllowedAccounts enumeration is complete. F3 is a test-hygiene defect, F2 is two comments overstating their code. F1 is the substantive one, and like the previous reviewer's F1 it is a gap in what Closes LeanerCloud/cloud-commitments-cli#1738 implies rather than in what was written: the ceiling bounds what an actor may grant given their scope, and the actor can raise their own scope in one self-edit through an endpoint this PR already guards on the neighbouring dimension.

Reviewer notes: the seven-mode scope probe (both heads), the laundering probe, the mutation matrix, and all five gates are execution-verified. The writer enumeration, the F2 comparison analysis and the F1 reachability argument are reading-derived, each load-bearing line read at its source.

[cristim]

Delta review part 2 — gates and mutation numbers at f7137d567

Follow-up to my findings comment above. Everything here was measured at f7137d567 in a clean worktree (git status --porcelain empty, HEAD re-verified before and after each run).

Gates

gate	result
go build ./...	exit 0, no output
go vet ./...	exit 0, no output
go test -race -count=1 ./...	exit 0, 31 packages ok, 0 FAIL lines
gocyclo -over 10 -ignore "_test\.go" . (CI-pinned v0.6.0)	exit 0, 0 findings
golangci-lint run --timeout=10m at CI-pinned v2.10.1	exit 0 — 0 issues.

The lint run used a v2.10.1 binary installed specifically for this (go install …@v2.10.1); the machine's default is 2.11.4, which carries a different bundled-gosec ruleset and would have been a false clean. Exit codes were captured separately from stdout, so "no output" is not being read as "passed".

CI on this head, from the raw check-runs API: 19 checks, all completed / success, no null or queued conclusions.

Mutation matrix

Per-test isolation, each top-level test in internal/auth run alone. Control on the unmutated tree: 0 kills / 241.

M14 — both account-ceiling calls removed from CreateGroupAPI and UpdateGroupAPI: 6 kills / 242.

test	mechanism	is the kill the security property?
TestAccountCeiling_RefusesWideningWithNoPermissionsSent	panic on unstubbed UpdateGroup	yes — the widening reached the store
TestAccountCeiling_CreateIsBounded	panic on unstubbed CreateGroup	yes
TestAccountCeiling_FailsClosedOnUnidentifiedActor	panic on unstubbed UpdateGroup	yes
TestAccountCeiling_FailsClosedWhenActorScopeUnresolvable	panic on unstubbed UpdateGroup	yes
TestAccountCeiling_AllowsInScopeWidening	assertion (AssertExpectations: actor lookup no longer happens)	no — but it proves the positive control reaches the actor branch
TestAccountCeiling_UnrestrictedActorMayWiden	assertion, same mechanism	same

This is the distinction the previous review's F2 was about, and it lands on the right side here: four of the six kills panic on an unstubbed write, not on an incidental downstream read. The write not happening is the property, so those four are behavioural kills, not accidents of fixture shape. The count is 6 rather than the body's 4 because the two positive controls also die.

M15 — the new len(authCtx.Groups) == 0 fail-closed guard removed: kills TestAccountCeiling_FailsClosedWhenActorScopeUnresolvable, again by panic on unstubbed UpdateGroup — the widening lands. The guard's own test is a real kill, of the strong kind.

MSELF — guardSelfEscalation removed from guardGroupChange (targeted, this is the F3 evidence):

go test -run '^TestUpdateUser_SelfEscalationDenied$'   -> exit 1
    panic: mock: ... This method was unexpected: UpdateUser(...)
    at test_helpers.go:49 <- service_user.go:333

go test -run '^TestGroupOnlyAuthz_NonAdminDenied$'     -> exit 0   (ok, PASSES)

That is F3 from the comment above, confirmed by execution rather than by reading: TestUpdateUser_SelfEscalationDenied is still killed by a panic on an unstubbed write, i.e. it never received the strengthening the body credits it with — while TestGroupOnlyAuthz_NonAdminDenied, the test the two .Maybe() stubs were actually added to, passes cleanly with the guard removed. It cannot detect the mutation at all, because it never calls UpdateUser. The stubs there are dead and the comment beside them points at an AssertNotCalled that does not exist in that function.

One limitation, stated rather than papered over

M13, M5 and M7 were not re-measured at this head. The shared machine reached a load average of 61 partway through and a full matrix pass became many hours of wall clock; I stopped it rather than report numbers from a contended or half-finished run. Those three rows were reproduced independently at 9387aac4f by the previous reviewer (4 / 1 / 1, no survivors), and neither commit since (2a8786f9, f7137d567) touches the guards they mutate — but that is inference, not measurement, and should be read as such.

A note on method for anyone re-running: two mutation harnesses must never share a worktree. I briefly had that, and a stray harness applied M14 underneath an unrelated test run, producing a "failure" that was purely an artifact. The contaminated results were discarded and everything above is from single-harness runs on a verified-clean tree.

[cristim]

Audit of the f7137d567 fail-closed fix

Third comment from the independent reviewer. Brief was: the fail-open in check 1 is real and now fixed, so audit the fix rather than re-find the bug. All four audit questions below were run at f7137d567 on a worktree verified clean before and after each measurement (the runner aborts if git status --porcelain is non-empty on entry — it fired once and caught a stale artifact, which is why the guard is there).

---

A1 — the fix's stated justification does not hold. The fail-open survives partial resolution failure.

The guard is len(authCtx.Groups) == 0, justified as: "partial resolution under-reports the actor's scope, which makes the ceiling stricter, so only total failure needed closing."

That premise is false for AllowedAccounts, because it is a union in which the empty set means everything. Dropping a contributing group does not narrow the union — if the survivors contribute nothing, it widens it from restricted to unrestricted.

The configuration: the actor holds two groups, one carrying update:groups with no AllowedAccounts (contributes nothing to the union) and one carrying the account restriction ["acct-A"]. Lose only the second — a partial failure, one group still resolves, the guard passes:

PROBE3[baseline: both groups resolve]
    groupsResolved=2  scope=[acct-A]  unrestricted=false  -> widening to ["*"] REFUSED

PROBE3[PARTIAL: only the scoping group missing (ErrNoRows)]
    groupsResolved=1  scope=[]        unrestricted=true   -> widening to ["*"] ACCEPTED (written to store)

PROBE3[PARTIAL: only the scoping group resolves to nil]
    groupsResolved=1  scope=[]        unrestricted=true   -> widening to ["*"] ACCEPTED (written to store)

PROBE3[TOTAL failure, the case the guard closes]
    REFUSED: permission ceiling exceeded: the acting user's account scope
             could not be established (no group resolved)

The half that is still open is the more reachable half. Under total failure the actor also loses every permission, so requirePermission denies them at the gate before the ceiling is ever consulted — the guard closes a case that was largely unreachable anyway. Under this partial failure the permission-carrying group still resolves, so the actor sails through requirePermission holding update:groups, and then gets measured against an empty (= unrestricted) scope.

The guard counts groups; the property it needs is about the union it produced. Something closer to the real invariant would be "the actor resolved fewer groups than their membership list names" (len(authCtx.Groups) < len(user.GroupIDs) ⇒ refuse), or refusing whenever the resolved scope is unrestricted while any named group failed to load.

---

A2 — the fix does not break a legitimate principal. Clear.

I looked for any route to unrestricted access that does not go through group membership, since requiring ≥1 group would break such a principal and read as a permissions bug:

• Zero-group users are structurally impossible. 000057_drop_user_role_to_groups.up.sql:98-99 sets group_ids NOT NULL and adds CONSTRAINT users_min_one_group CHECK (cardinality(group_ids) >= 1). I checked every migration for a later drop: the only DROP CONSTRAINT ... users_min_one_group is in 000057's down file. Nothing else touches it.
• Two service-level guards agree: ErrNoGroups is returned by service_user.go:136 (CreateUser) and :371 (UpdateUser), mapped at handler_users.go:84.
• The one legitimately groupless principal short-circuits before the guard. grantCeilingAccounts returns nil, nil for AdminAPIKeyActorID above the len(authCtx.Groups) check, so the stateless admin API key is unaffected.
• No superadmin flag, system principal or auth service account exists. The service_account matches in the tree are all GCP cloud credential types (gcp_service_account in credentials/resolver.go, handler_accounts.go), not auth principals.

So the inverse risk is not present: the guard can only fire on a state the schema forbids or on genuine resolution failure.

---

A3 — the same fail-open shape is still live in the enforcement-side copy the fix did not touch.

authServiceAdapter.GetAllowedAccountsAPI (internal/server/app.go:1159) is BuildAuthContext followed by return authCtx.AllowedAccounts, nil — the same two lines the fix hardened in grantCeilingAccounts, with no group-count guard. Reproducing that adapter body verbatim:

PROBE3-ENF[total: actor's only group missing]     -> returns [], IsUnrestrictedAccess=true
PROBE3-ENF[partial: only the scoping group missing] -> returns [], IsUnrestrictedAccess=true

That return value feeds every account-scoped endpoint. Enumerated in Python over full file text (not grep): 37 non-test call sites of IsUnrestrictedAccess / MatchesAccount, across scoping.go, handler_accounts, handler_analytics, handler_dashboard, handler_history, handler_inventory, handler_ladder, handler_marketplace, handler_recommendations, handler_ri_exchange. Every one of them treats the empty list as all accounts.

Including a money path: handler_purchases_revoke.go:398 gates revoke-own with

if len(allowed) > 0 && !stringInSlice(*record.CloudAccountID, allowed) {

— an empty allowed skips the ownership check entirely.

So the PR hardened the ceiling's copy of this resolution and left the enforcement copy, which has a much wider blast radius, in the state the ceiling's copy was in before f7137d567. Given the PR body explicitly names the asymmetry between the permission and account sides as the thing it is closing, the enforcement copy belongs in the same discussion — either fixed alongside or filed.

---

A4 — mutation check on the new guard

M15 = delete the len(authCtx.Groups) == 0 block. It kills TestAccountCeiling_FailsClosedWhenActorScopeUnresolvable, and the kill is the strong kind: a panic on an unstubbed UpdateGroup, i.e. the widening reached the store. That is the security property, not an incidental downstream read.

Coverage caveat worth stating: that test is the guard's own test, and it exercises only the total-failure input. Nothing in the suite covers the partial-failure configuration in A1 — which is exactly why A1 survives a green suite, a clean CR and 19 green CI checks.

---

Checks 2 and 3, re-derived independently

Check 2 — the two-writer enumeration is correct. Re-derived in Python over full file text, deliberately not with grep, printing every hit rather than a count. 1006 files scanned, 401 lines mentioning the field, 6 non-test Go write sites:

site	what it writes	verdict
service_api.go:309	Group literal in CreateGroupAPI	real writer — guarded at :302
service_api.go:334	group.AllowedAccounts = in applyUpdateGroupRequest	real writer — guarded at :363
service_api.go:158	APIGroup literal in groupToAPIGroup	response DTO, not the stored group
service_group.go:142 / :179	authCtx.AllowedAccounts	AuthContext, not a Group
store_postgres.go:844	group.AllowedAccounts = in scanGroup	writes a Group field, but the source is the DB row — a reader, not an ingress

The classification holds on receiver type, not on name. I also closed the question the enumeration is really asking — are the two guarded entry points the only routes to the store writers? Service.CreateGroup is called from exactly one place (service_api.go:313) and Service.UpdateGroup from exactly one (service_api.go:370); the remaining matches are the definitions, the interface declaration, and mocks. No adapter or handler bypass.

Check 3 — the .Maybe() claim is true, and I tested it rather than reasoning about it. A recordingT captures whether testify actually failed:

.Maybe() registered, NOT called      -> AssertNotCalled failed=false   (correct)
.Maybe() registered, REALLY called   -> AssertNotCalled failed=true    (the proof survives)
strict registration, REALLY called   -> AssertNotCalled failed=true    (identical: .Maybe() changes nothing)
name-only form after a REAL call     -> AssertNotCalled failed=false   (VACUOUS, as the PR body warns)

So TestGrantCeiling_RejectsBlankActionOrResource is genuinely load-bearing: .Maybe() on the write plus a two-matcher AssertNotCalled is a real assertion. This confirms the body's own reasoning — AssertNotCalled reads the call record, not the expectation registry.

(Unchanged from my earlier comment and not re-litigated here: the third strengthening landed on TestGroupOnlyAuthz_NonAdminDenied, which passes cleanly with the guard removed, while TestUpdateUser_SelfEscalationDenied is still panic-driven.)

---

Where this leaves the PR

f7137d567 is a real improvement and its own test is a strong kill. But the fix is narrower than its justification claims: it closes total resolution failure while the partial case — the one that survives the upstream permission gate — still widens an actor to unrestricted, and the enforcement-side copy of the same two lines is untouched and feeds ~37 scoping sites including a money path. A1 and A3 are the two I would want resolved or explicitly filed before this reads as "the account dimension is bounded".

[cristim]

A4 addendum — "killed by its own test and by nothing else" is confirmed

The per-test sweep was too slow under the load on this box, so I answered the same question with one run instead of forty-five, which is also a stronger claim because it covers the whole package rather than a candidate subset.

With M15 applied (the len(authCtx.Groups) == 0 block deleted) and only its own test skipped:

go test -count=1 -skip '^TestAccountCeiling_FailsClosedWhenActorScopeUnresolvable$' ./internal/auth/
    ok  github.com/LeanerCloud/CUDly/internal/auth  73.610s     exit 0

Tree verified clean immediately before applying the mutation and immediately after reverting it.

So M15 kills exactly one test out of all 242 in internal/auth, and that kill is a panic on an unstubbed UpdateGroup — the write landing, which is the security property rather than an incidental read. The guard is correctly and minimally covered for the input it handles.

The caveat from my previous comment stands and is the important half: that single test exercises only total resolution failure. The partial-failure configuration in A1 — where one group resolves, the guard passes, and the union still collapses to unrestricted — has no test at all, which is why it survives a green suite, a clean CR, and 19 green CI checks.

[coderabbitai]

🧹 Nitpick comments (2)

internal/auth/group_ceiling_validation_test.go (2)

46-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Make the target-group stub permissive so the test does not pin guard ordering.

stubTargetGroup registers a strict expectation. t.Cleanup calls AssertExpectations, so the subtest requires UpdateGroupAPI to fetch the target group before it validates the permission list.

That coupling is unintended. Validation is actor-independent and target-independent. If a later change moves validateRequestedPermissions ahead of the group fetch, these subtests fail even though the behavior improved.

The surrounding stubs already use .Maybe() for exactly this reason. Apply the same treatment here.

♻️ Proposed change

Add a permissive variant beside stubTargetGroup in internal/auth/group_ceiling_fixtures_test.go:

func stubTargetGroupMaybe(ctx context.Context, mockStore *MockStore, group *Group) {
	mockStore.On("GetGroup", ctx, ceilingTargetID).Return(group, nil).Maybe()
}

Then use it in the validation subtests:

-			stubTargetGroup(ctx, mockStore, &Group{ID: ceilingTargetID, Name: "Team"})
+			stubTargetGroupMaybe(ctx, mockStore, &Group{ID: ceilingTargetID, Name: "Team"})
 			// Permissive downstream stubs so removing validateRequestedPermissions
 			// cannot kill this test by panicking on a missing stub. With them,
 			// the blank permission would be accepted (admin:* grants view on
 			// any resource, including "") and the assertions below catch it.
 			stubActorPermissionsMaybe(ctx, mockStore, adminOnly)

🤖 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 `@internal/auth/group_ceiling_validation_test.go` around lines 46 - 52, Make
the target-group mock non-order-dependent in the validation subtests by adding a
permissive stubTargetGroupMaybe helper alongside stubTargetGroup, registering
the same GetGroup expectation with Maybe(), and replacing stubTargetGroup calls
in the affected validation tests with it.

---

90-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Align this subtest with the stated mutation design.

This subtest omits the actor stubs. If validateRequestedPermissions were removed, UpdateGroupAPI would call GetUserByID, hit no stub, and panic. The kill comes from the panic, not from assert.ErrorIs.

The subtests above deliberately avoid that. They register stubActorPermissionsMaybe and a permissive UpdateGroup stub so the assertions do the work. Add the same stubs here for a consistent and durable kill.

🤖 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 `@internal/auth/group_ceiling_validation_test.go` around lines 90 - 106, Add
the actor-permission and permissive UpdateGroup stubs used by the neighboring
subtests to the “a blank entry beside valid ones refuses the whole list” test,
using the existing stubActorPermissionsMaybe helper and update expectation
setup. Keep the invalid-permission assertions and AssertNotCalled check intact
so the test fails for the intended validation behavior rather than an unstubbed
GetUserByID panic.

🤖 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.

Nitpick comments:
In `@internal/auth/group_ceiling_validation_test.go`:
- Around line 46-52: Make the target-group mock non-order-dependent in the
validation subtests by adding a permissive stubTargetGroupMaybe helper alongside
stubTargetGroup, registering the same GetGroup expectation with Maybe(), and
replacing stubTargetGroup calls in the affected validation tests with it.
- Around line 90-106: Add the actor-permission and permissive UpdateGroup stubs
used by the neighboring subtests to the “a blank entry beside valid ones refuses
the whole list” test, using the existing stubActorPermissionsMaybe helper and
update expectation setup. Keep the invalid-permission assertions and
AssertNotCalled check intact so the test fails for the intended validation
behavior rather than an unstubbed GetUserByID panic.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: afaf140a-eb25-4b35-a45c-a840dc4e3c6b

📥 Commits

Reviewing files that changed from the base of the PR and between 29f11ee and 7c4e840.

📒 Files selected for processing (15)

• internal/api/handler.go
• internal/api/handler_ri_exchange_test.go
• internal/api/mocks_test.go
• internal/auth/group_account_ceiling_test.go
• internal/auth/group_ceiling.go
• internal/auth/group_ceiling_fixtures_test.go
• internal/auth/group_ceiling_permissions_test.go
• internal/auth/group_ceiling_validation_test.go
• internal/auth/group_system_managed_test.go
• internal/auth/self_escalation_carveout_test.go
• internal/auth/service_api.go
• internal/auth/service_group.go
• internal/auth/service_group_only_authz_test.go
• internal/auth/service_user.go
• internal/server/app.go

🚧 Files skipped from review as they are similar to previous changes (9)

• internal/api/handler.go
• internal/auth/service_group_only_authz_test.go
• internal/api/handler_ri_exchange_test.go
• internal/api/mocks_test.go
• internal/auth/service_user.go
• internal/auth/service_group.go
• internal/auth/self_escalation_carveout_test.go
• internal/server/app.go
• internal/auth/service_api.go

[cristim]

Review at 7c4e8405c — A1, F2, F3, rebase and the claim box all hold

Fresh worktree at 7c4e8405c, separate mutation worktree, probes throwaway. No CodeRabbit ping, no merge. One claim correction carried forward from my earlier pass, and one small comment-attribution nit; neither is a security defect.

---

A1 — the fix is correct; the baseline control does its job ✅

Re-derived the control the way that proves it: run the pre-fix state (M-B, partial guard removed) and compare the two configurations of the same actor issuing the same request (AllowedAccounts: ["*"] on the target group).

under M-B (pre-fix), run in isolation	outcome
PartialActorBaselineIsRefusedOnRealScope — both groups resolve	PASS → refused; real scope [acct-A] does not cover "*"
PartialActorResolutionThatWidensIsRefused — only the restricting group skipped	FAIL → the write lands

The failure mode is the strong part — execution reaches the store:

panic: assert: mock: I don't know what to return because the method call was unexpected.
       This method was unexpected: UpdateGroup(context.backgroundCtx,*auth.Group)

Same actor, same request; the only difference is whether one group loaded. That is a genuine ceiling bypass, and the control is exactly what rules out "this actor is refused for some unrelated reason". WildcardActorToleratesSkippedGroup also passes under M-B, so it is not a superset either.

M-A holds. M-B kills two tests, not one — claim correction (unchanged from my pass on c03867363)

Baseline: 12 top-level TestAccountCeiling_*, all PASS.

Mutation	Claimed	Measured
M-A predicate → IsUnrestrictedAccess	only WildcardActorToleratesSkippedGroup	1/12, exactly that ✅
M-B partial guard removed	only PartialActorResolutionThatWidensIsRefused	2 tests ❌

M-B also kills TestAccountCeiling_FailsClosedWhenActorScopeUnresolvable:

Error: "permission ceiling exceeded: the acting user's account scope could not be established (no group resolved)"
       does not contain "could not be resolved"

Under M-B that case still falls through to the len(Groups) == 0 guard and is still refused — the behaviour is intact, only the message changed. So "each direction is guarded by exactly one test; neither would catch the other's defect" is not accurate for the M-B direction. It is a consequence of the assertion being guard-specific (next section), not a defect in the fix.

(Both M-B failures abort the package run at test 10 of 12 via the panic above, so a plain go test under-reports. Every verdict here comes from per-test isolation.)

The assertion relaxation — mechanism independently confirmed, across every path ✅

The argument is right, and I checked it against all seven outcomes grantCeilingAccounts can produce rather than the three quoted:

#	path	message	contains "could not be resolved"
1	empty actor ID	...the acting user could not be identified	false
2	admin API key	(returns nil, nil)	n/a
3	BuildAuthContext error	...could not resolve the acting user's account scope: %w	false
4	partial / skipped-group guard	...%d group(s) could not be resolved and...	true
5	no-group guard	...could not be established (no group resolved)	false
6	success	(returns the scope)	n/a

Exactly one match. The assertion is maximally discriminating, not relaxed — my earlier reading of the older wording as "a substring true of both" is superseded; this version states the mechanism correctly.

Two footnotes, neither actionable:

• Path 3 wraps an arbitrary store error with %w, so a store error whose own text contained the probe would satisfy it. That is a theoretical false-pass, and this test injects no such error.
• The flip side of being guard-specific is the M-B coupling above: the test now reddens on a message change even when refusal is preserved. Worth knowing rather than fixing — if you ever want it decoupled, "could not be" is genuinely true of paths 1, 4 and 5.

F2 — exactness is monotone-stricter on every axis; nothing became looser ✅

The direction question, checked structurally rather than by example: exact match is a subset of trimmed/case-folded match, so the permitted set membership can only shrink. Every consumer therefore moves toward refusal:

helper	call site	effect of removing normalisation
accountScopeGap	group_ceiling.go:218 — == "" means not a widening, allow	gap non-empty more often → the early-allow fires less → stricter
accountScopeGap	group_ceiling.go:237 — != "" means refuse	refuses more → stricter
listCovers	group_ceiling.go:340-343, all four dimensions &&-chained into constraintsCover	returns false more often → refuses more → stricter

No call site inverts the polarity, so no axis became looser. The stated rule holds against the actual enforcement:

• AccountIDs, Providers, Services → matchStringListConstraints → containsAny (service_helpers.go:130), a bare allowedSet[r] map lookup — exact, case-sensitive. So the old normalisation genuinely made the ceiling looser than enforcement on three of four dimensions, which is the bug.
• Regions → matchAllRegionsConstraint (service_group.go:392) — strings.ToLower(strings.TrimSpace(...)) on both sides. Exact in the ceiling is therefore stricter than enforcement here, which fails closed. Matches "may exceed in strictness, never fall short."

normalizeConstraintValue: 0 references repo-wide (Python scan over every .go file, not grep -c). Cleanly removed.

Residual strings use in group_ceiling.go is 2 occurrences, both TrimSpace inside validateRequestedPermissions (:127, :132) detecting blank action/resource. That is a validation use, not a comparison — trimming there makes whitespace-only count as blank and be refused, i.e. also the stricter direction. The strings import is still required.

F3 — stubs in the intended test, and the kill is assertion-driven ✅

The anchor On("GetGroup", ctx, viewerGroup().ID) appears in 3 tests in that file, which matches the account of the first-match-replace error:

test	has UpdateUser stub	has AssertNotCalled(UpdateUser)
TestGroupOnlyAuthz_NonAdminDenied	no (correctly removed)	no
TestUpdateUser_SelfEscalationDenied	yes	yes
TestUpdateUser_AdminEditingSelfAllowed	yes	no

The stubs are in the one test that both exercises guardSelfEscalation and asserts no write. Reproduced the before/after by removing the guard (return s.guardSelfEscalation(...) → return nil):

(a) WITH the permissive stub      --- FAIL ... Error: An error is expected but got nil.      <- assertion, no panic
(b) WITHOUT the permissive stub   panic: assert: mock: I don't know what to return ...
                                  at test_helpers.go:49, service_user.go:333 (store.UpdateUser)

Exactly the claimed table. The stub is what converts a panic-kill into an assertion-kill, and a panic-kill would indeed evaporate the moment anyone added a stub while tidying fixtures.

One nit: the killing assertion is require.Error at line 262, not the AssertNotCalled at line 265 that the comment credits — require.Error calls FailNow, so AssertNotCalled never executes in this scenario. The property the comment is defending (assertion-driven, not panic-driven) holds; only the attribution is off by three lines. AssertNotCalled would bite on a different mutation — one where the guard errors but the write still happens.

The rebase — verified independently ✅

• authCtx.SkippedGroups++ appears twice in service_group.go; SkippedGroups int present in types.go.
• git diff origin/main...HEAD shows no diff at all in the SkippedGroups region of service_group.go — byte-identical to what sec(auth): fail closed when a user's account scope cannot be established #1752 landed, so the resolution took main's side as claimed.
• types.go has dropped out of the PR's diff vs main entirely, and the stale whichever merges second NOTE is gone.

The 33 remaining insertions in service_group.go belong to this PR's other commits, not to A1.

Claim narrowing — accurate, and it survives #1752 ✅

• "create, update and delete" — verified: checkGrantCeiling on create (service_api.go:297) and update (:360); checkAccountGrant on create (:302, which explicitly treats an omitted allowed_accounts as a grant of unrestricted) and checkAccountCeiling on update (:363); system_managed on delete. Not an overclaim.
• "joining a wider group is not account-checked" — guardGroupChange contains no account check of any kind. Correct.
• "guardGroupChange returns early unless addsNewGroup" — confirmed at service_user.go:388: if actorUserID == "" || actorUserID != targetUserID || !addsNewGroup(prior, next) { return nil }.

The sharp part, worth stating because it is easy to assume otherwise: #1752 does not close the removal-only hole. A removed group is not a skipped group — it never enters user.GroupIDs, so SkippedGroups == 0 and the union [] passes both new guards as a genuinely unrestricted principal. I measured that exact shape while verifying #1752 (union empty, zero skips → unrestricted → requireAccountAccess GRANTED). So #1756 is correctly still open and correctly excluded here.

One wording nit: "unguarded entirely" is slightly strong — a removal-only self-edit still passes len(next) == 0 → ErrNoGroups and the last-admin check. It is the self-escalation and account checks that do not run.

Gates at 7c4e8405c

Gate	Result
go build ./...	exit 0
go vet ./...	exit 0
gocyclo -over 10 -ignore "_test\.go" .	exit 0, 0 lines; grantCeilingAccounts 7, accountScopeGap 6, listCovers 6
golangci-lint v2.10.1 (CI-pinned)	exit 0 and 0 issues. ✅
six-module go test -race -short (post-#1755 contract)	MODULE_OK × 6 (., pkg, providers/{aws,azure,gcp}, tests/e2e), ALL_MODULES_EXIT=0 ✅

Lint passes the two-part check: exit 0 and a non-empty findings block.

Summary

A1 correct and covered, F2 monotone-stricter with the dead helper gone, F3 in the right test with an assertion-driven kill, rebase clean, claim box accurate. Outstanding, both cosmetic: the M-B "kills only one test" claim, and the AssertNotCalled attribution in the F3 comment.

[cristim]

Merging. Closes #1550 and #1738.

Merging on independent adversarial review, not a CodeRabbit verdict. CR's latest non-empty review here is against 29f11eee2, several heads back; it has not reviewed this content. Two independent reviewers covered it instead, across four heads, and produced findings at every pass.

The reviews found four defects in the fix, not the original bug. That is the substance of this PR's history and worth recording:

A fifth write vector. checkGrantCeiling opened with if len(requested) == 0 { return nil }, so an allowed_accounts-only PUT returned before the actor was even resolved. Widening to more accounts, to [], and to ["*"] were all accepted, while widening Permissions[].Constraints.AccountIDs on the same call was correctly refused. The regression test has to send allowed_accounts with no permissions key — a test including both passes with the bug present, because the ceiling then actually runs.

The first fix closed the less reachable half. Its premise — partial resolution under-reports scope, so it is stricter — is false when the scope is a union whose empty value means everything. An actor holding one group with no allowed_accounts and one carrying the restriction, losing only the second, still has a group resolving, and the union collapses to []. Worse, the case it did close was already largely unreachable: under total failure the actor loses every permission, so requirePermission denies them before the ceiling is consulted.

The second fix would have refused every seeded-group member. The predicate was IsUnrestrictedAccess(...), true for an empty union or one containing "*". A principal already carrying "*" is maximally wide, so no loss can widen them; refusing them is zero security benefit and pure availability cost. All seven seeded groups ship ARRAY['*']. Now len(authCtx.AllowedAccounts) == 0.

Two comparison helpers were looser than the enforcement they claimed to mirror. accountScopeGap trimmed while MatchesAccount is bare ==; listCovers trimmed and lower-cased while only Regions is enforced that way. A ceiling looser than its enforcement is a gap by construction. Both are exact everywhere now, under a stated rule: a ceiling may exceed enforcement in strictness, never fall short. Verified monotone-stricter on every axis, with the orphaned normalizeConstraintValue removed.

A test-strengthening landed in the wrong function, twice. The anchor On("GetGroup", ctx, viewerGroup().ID) appears in three tests in that file, and a first-match replace took the first — then the fix for it repeated the identical error. Caught only by mutation-verifying the kind of kill rather than accepting a green test:

before: killed by PANIC=True    killed by ASSERTION=False
after:  killed by PANIC=False   killed by ASSERTION=True

A panic-driven kill evaporates the moment someone adds a permissive stub. That distinction is the whole point of the strengthening.

Verified in this review, independently:

• The A1 baseline control does its job — under the pre-fix mutation, the same actor issuing the same request differs in outcome only by whether one group loaded. That rules out "this actor is refused for an unrelated reason", which a refusal-only test cannot.
• The assertion relaxation on FailsClosedWhenActorScopeUnresolvable did not cost discriminating power: "could not be resolved" matches the partial-guard message only; the zero-group and unidentified-actor messages do not contain it. Confirmed across every path that guard can produce.
• The rebase conflicts on service_group.go and types.go were comment-only, code byte-identical, with SkippedGroups++ present twice post-resolution.
• Gates at 7c4e8405c: build, vet, gocyclo 0 findings, golangci-lint v2.10.1 exit 0 with a genuine 0 issues. line, and the six-module go test -race -short contract from ci: run unit and integration tests in every workspace module #1755 — MODULE_OK x 6, ALL_MODULES_EXIT=0.

Two corrections to this PR's own body, recorded rather than silently left:

1. "Each direction is guarded by exactly one test" is inaccurate for M-B. Removing the partial guard kills two tests, not one — it also reddens FailsClosedWhenActorScopeUnresolvable, because that case still falls through to the len(Groups) == 0 guard and is still refused; only the message changes. So the behaviour is intact and the second failure is a consequence of the assertion being guard-specific, not a defect. M-A does kill exactly one.
2. The AssertNotCalled attribution in the F3 comment is imprecise. Being folded into the stacked follow-up rather than respun here.

Note both M-B failures abort the package run at test 10 of 12 via a panic, so a plain go test under-reports the matrix. Every verdict above comes from per-test isolation.

Scope, stated in the body rather than implied: this bounds writes to a group on create/update/delete. It does not bound the account dimension generally — the membership endpoint launders it, and guardGroupChange returns early unless addsNewGroup, so a removal-only self-edit is unguarded entirely. Removing the restriction grants the restriction. Tracked as #1756, which stacks directly on this.