refactor(auth): represent account scope as a type whose zero value denies

[cristim]

Refs #1748. Stacked on #1752 — merge that first; this branch contains its commits.

#1752 fixes the bug at the producers. This removes the trap that made it possible, so a future producer cannot reintroduce it.

Why producers alone were not enough

Two consumers read empty as "everything" on their own, independently of any producer:

handler_marketplace.go:435       if len(allowed) == 0 { return nil }
handler_purchases_revoke.go:398  if len(allowed) > 0 && !stringInSlice(...)

Neither could be reached by fixing producers. The point of a type change is that it makes them stop compiling, not merely stop being reachable. Both did, and both are fixed here.

The revoke one was worse than recorded: it called GetAllowedAccountsAPI directly, bypassing getAccountScope entirely, so it also skipped the admin-API-key and nil-auth branches. It now routes through the seam.

The type

AccountScope{Accounts []string; Unrestricted bool}. Its defining property, stated in the doc comment as a requirement rather than an observation:

The zero value MUST deny. AccountScope{} is restricted to nothing.

So a forgotten initialisation, a value returned on an error path, or a field a later change fails to populate all fail closed. The comment also names the Restricted bool inversion explicitly as the thing not to do — it reads marginally better and would make the zero value grant everything, rebuilding #1748 inside the new type with the compiler blessing it.

Mutation-verified rather than asserted: reintroducing "empty means all" inside Allows/AllowsAll kills 4 tests, including ZeroValueDeniesEverything and UninitialisedOnErrorPathDenies. Two tests survive that mutation and are reported as honest survivors, not counted — both use non-empty lists, so the mutation cannot reach them.

The compiler found a 19th site

filterReservationsByScopeIndex (handler_ri_exchange.go:420) took the legacy []string. It is a downstream helper, not a caller of getAllowedAccounts, so no call-site scan could have found it — the hand count said 18. That is exactly the property a type change buys and a grep never will.

ScopeFromLegacyList — borrowed safety, now stated accurately

It maps both spellings of empty to unrestricted, so it carries no safety of its own. Its doc comment previously said it was "safe for a successful resolution", which was true and underspecified. Under the pre-#1752 resolver — which refused only total failure — it would have converted a widened empty union into UnrestrictedScope(), carrying #1748 forward inside the new type.

Rewritten to name the guarantee it actually depends on: the resolver refuses any empty union that a skipped group could have caused, with all three refused inputs enumerated. Verified end to end: with the partial configuration driven through the new type, requireAccountAccess(acct-B) refuses at baseline and under partial failure.

ScopeForAccounts — the constructor for raw, unvetted input — deliberately does not inherit the empty-means-all rule, pinned by a test.

An operability gain that falls out of the type

String() renders "all accounts" versus "no accounts", so the two states are distinguishable in a log. They were not before: both printed as an empty []string, which is the same ambiguity that made #1748 invisible in the first place — a scope granting everything and a scope granting nothing looked identical in any diagnostic.

Migration safety

• Parity test: AllowsAll agrees with IsUnrestrictedAccess and Allows agrees with MatchesAccount across every legacy input shape, so who can reach what does not silently change.
• Both legitimate unrestricted principals set the flag positively: the admin API key returns UnrestrictedScope(); resolved lists convert through the vetted path.
• A scoped principal is not promoted, asserted directly.
• h.auth == nil returns AccountScope{} alongside its error, so a caller that ignores the error still gets a deny.

Gates

go build ./...                       exit 0
go vet ./...                         exit 0
go test ./...                        exit 0
gocyclo -over 10 -ignore "_test\.go" .   no output
golangci-lint v2.10.1                exit 0 with a non-empty findings block ("0 issues.")

The lint verdict asserts both exit 0 and a non-empty block, because v2 exits 3 with an empty block when a concurrent run holds the lock — which reads exactly like clean. It caught a gocritic importShadow here, now fixed.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 42 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a3d5635d-fed6-461d-89e6-58fdf067d01d

📥 Commits

Reviewing files that changed from the base of the PR and between 6d5275c and d0c2dc1.

📒 Files selected for processing (16)

• internal/api/account_scope_fail_closed_test.go
• internal/api/grantscoped_test.go
• internal/api/handler.go
• internal/api/handler_accounts.go
• internal/api/handler_analytics.go
• internal/api/handler_dashboard.go
• internal/api/handler_history.go
• internal/api/handler_ladder.go
• internal/api/handler_marketplace.go
• internal/api/handler_purchases_revoke.go
• internal/api/handler_recommendations.go
• internal/api/handler_ri_exchange.go
• internal/api/scoping.go
• internal/auth/account_scope.go
• internal/auth/account_scope_test.go
• internal/auth/group_ceiling.go

---

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

[cristim]

Review at 090706074 — the type is sound and the widening does not survive into it. Six stale comments, one of them materially wrong.

Fresh worktree at 090706074. Probes throwaway, removed before the gates. No CodeRabbit ping, no merge. No blocking findings. One documentation defect I would want fixed before merge because it asserts the opposite of the invariant this PR exists to establish; the rest is minor.

---

1. The zero value denies, on every path ✅

Enumerated by construction, not by reading the doc. A Python scan over every .go file for composite literals, both constructors, var declarations, and the forbidden inversion found exactly five non-test construction sites, in two files:

site	value	verdict
account_scope.go:40 UnrestrictedScope()	{Unrestricted: true}	positive, by definition
account_scope.go:50 ScopeForAccounts()	{Accounts: accounts} — Unrestricted false	denies when the list is empty
handler.go:535	auth.UnrestrictedScope()	admin API key — positively established
handler.go:541	auth.AccountScope{} + error	nil auth
handler.go:545	auth.AccountScope{} + error	resolver error
handler.go:550	auth.ScopeFromLegacyList(resolved)	the one legacy conversion

Both zero-value returns are on error paths, which is exactly the claim: even a caller that ignored the error gets a value that denies. Measured directly:

AccountScope{} (zero)   AllowsAll=false  Allows("anything","any-name")=false  FilterIDs→0 entries  String="no accounts"

Restricted bool: 0 occurrences repo-wide. The forbidden inversion is not present anywhere.

2. ScopeFromLegacyList — the guarantee is named correctly, and it holds ✅

The rewritten doc (849916e73) names the real guarantee rather than "safe for a successful resolution", including the third condition — SOME group unresolvable AND the surviving union empty → error — which is the one an earlier resolver did not enforce and which would have carried #1748 forward inside the new type. That is the correct statement.

Driven end to end through the new type: real auth.Service over auth.MockStore → ResolveAllowedAccounts → ScopeFromLegacyList → AccountScope → three enforcement seams (requireAccountAccess, checkRevokeOwnAccountAccess, authorizeAllowedAccount).

A. survivor contributes NOTHING; restricting group carries [acct-A]   (the widening)
   baseline (both resolve)                scope=acct-A|unrestricted=false   reqAcct=REFUSED  revokeOwn=REFUSED  marketplace=REFUSED
   PARTIAL restricting group ErrNoRows    scope=REFUSED                     reqAcct=REFUSED  revokeOwn=REFUSED  marketplace=REFUSED
   PARTIAL restricting group (nil,nil)    scope=REFUSED                     reqAcct=REFUSED  revokeOwn=REFUSED  marketplace=REFUSED

B. survivor carries [acct-A]; LOST group carried [acct-C]              (narrowing, must be allowed)
   baseline                               scope=acct-A, acct-C              reqAcct=REFUSED (acct-B out of scope)
   stale membership ErrNoRows             scope=acct-A                      reqAcct=REFUSED

C. survivor carries ['*'] (all seven seeded groups do); one stale membership
   baseline                               scope=all accounts|unrestricted=true   all three seams GRANTED
   stale membership ErrNoRows             scope=all accounts|unrestricted=true   all three seams GRANTED

D. both resolve, neither carries accounts (legacy default, 0 skips)
   union empty, 0 skips                   scope=all accounts|unrestricted=true   all three seams GRANTED

E. h.auth == nil                          scope=REFUSED                     all three seams REFUSED

The widening is refused before the conversion is ever reached, so ScopeFromLegacyList never sees a widened empty list. The three legitimate unrestricted principals still resolve to Unrestricted: true, and String() renders "all accounts" vs "no accounts" — the two states are now distinguishable in a log, which they were not before.

3. The 19th call site, and the other downstream helpers ✅

filterReservationsByScopeIndex (handler_ri_exchange.go:420) now takes allowed auth.AccountScope and calls allowed.Allows(account.ID, account.Name) — both id and name, faithfully replacing auth.MatchesAccount(allowed, account.ID, account.Name). Correct, not merely compiling.

It is the only other function in internal/api taking an AccountScope parameter (signature scan), so there is no second downstream helper still carrying a legacy []string. Confirmed the other direction too: zero code references to IsUnrestrictedAccess or MatchesAccount remain anywhere in internal/api — the migration is complete, not partial. Every conversion follows the same two mechanical rewrites, and both preserve semantics exactly:

• auth.IsUnrestrictedAccess(x) → x.AllowsAll()
• auth.MatchesAccount(x, id, name) → x.Allows(id, name)

AllowsAll() reads only the flag where the old helper also returned true for empty-or-"*", but ScopeFromLegacyList folds both of those into Unrestricted: true first, so the composition is equivalent at every site. Verified: ScopeFromLegacyList(["*"]), (["acct-A","*"]) and (nil) all yield AllowsAll=true.

4. The two hand-rolled consumers ✅ — and F4 is half fixed, which changed revoke semantics

Both now route through getAccountScope:

• handler_marketplace.go:430 — the hand-rolled len(allowed) == 0 is gone, replaced by scope.Allows(cloudAccountID, "").
• handler_purchases_revoke.go:398 — no longer calls GetAllowedAccountsAPI directly, so it now picks up the admin-API-key and nil-auth branches it was skipping.

On my F4 from #1752 (stringInSlice honoured neither "*" nor account-name entries, unlike MatchesAccount) — this PR fixes the wildcard half and preserves the name half, and the wildcard half is a real behavioural change on the revoke path, so flagging it explicitly:

principal	before	after
scope ["*"], revoke-own on any account	len>0 && !stringInSlice("acct-B",["*"]) → 403	Unrestricted → permitted
scope ["Production"] (a name entry), revoke-own in that account	denied	still denied — measured: ScopeFromLegacyList(["Production"]).Allows("u-1","") = false, vs .Allows("u-1","Production") = true

The wildcard change is a fix: it aligns revoke with every other scoping site and with the documented model in which "*" genuinely means all accounts. But it is a loosening relative to main, so it belongs in the PR description rather than being discovered later. The name half survives because checkRevokeOwnAccountAccess passes "" for the name — the PurchaseHistoryRecord carries only CloudAccountID. Fail-closed direction, unchanged, still one for #950/the follow-up.

5. The rebase ✅

Verified rather than assumed:

• merge-base is 87a853e27 (sec(auth): enforce a grant ceiling and system-managed guard on group writes #1737's merge, which already contains sec(auth): fail closed when a user's account scope cannot be established #1752), and the branch is exactly 3 commits ahead — 9203891f7, 849916e73, 090706074. sec(auth): fail closed when a user's account scope cannot be established #1752's four commits are not replayed.
• No branch commit subject appears anywhere in the last 60 commits of origin/main (Python comparison), so nothing was duplicated.
• 1 commit behind (6d5275c85, a test fix on main). Nothing dropped.

---

Findings

D1 (would fix before merge) — the seam function's doc comment asserts the convention this PR abolishes. internal/api/handler.go:526-529:

// getAllowedAccounts returns the list of account IDs the user is allowed to
// access. Empty slice means all access (Administrators-group members carry the
// "*" wildcard, which GetAllowedAccountsAPI surfaces as unrestricted). The
// stateless admin API key has no user row, so it short-circuits to all access.
func (h *Handler) getAccountScope(ctx context.Context, session *Session) (auth.AccountScope, error) {

Three ways wrong, on the one function the whole PR pivots on: it names the old function, it says the return is a list of account IDs (it is an AccountScope), and it states "Empty slice means all access" — which is exactly the hazard AccountScope removes and is now false of the returned value, whose empty form denies. A reader trusting this comment would draw precisely the wrong conclusion. The body's inline comments are all correct; it is only the godoc block that was not updated.

D2 (minor) — five more comments still describe the removed representation, all in files this PR touches:

location	stale text
handler.go:224	"See requirePermission / requireAdmin / getAllowedAccounts."
handler_dashboard.go:131	"keeps those the session matches via auth.MatchesAccount"
handler_ri_exchange.go:417	"the allowed list covers (auth.MatchesAccount)"
scoping.go:12	"allowed_accounts list grants access via auth.MatchesAccount"
scoping.go:249	"MatchesAccount falls back to ID comparison"

Also internal/auth/group_ceiling.go:146 (already on main) points at "getAllowedAccounts in internal/api", a name that no longer exists after this PR. Not this PR's file, but this PR is what breaks the reference.

For a change whose stated purpose is to make the representation self-documenting, six comments describing the abolished one is worth one cleanup pass.

D3 (minor, forward-looking) — ScopeForAccounts does not warn that "*" is not a wildcard. Measured:

ScopeForAccounts(["*"])        AllowsAll=false  Allows("acct-A")=false  Allows("*")=true
ScopeFromLegacyList(["*"])     AllowsAll=true   Allows("acct-A")=true

ScopeForAccounts treats "*" as a literal account identifier. Not reachable today — production constructs scopes only via ScopeFromLegacyList and UnrestrictedScope, and the former routes "*" to unrestricted. But the obvious future call, ScopeForAccounts(group.AllowedAccounts), would silently produce a scope that denies everything for a wildcard-carrying group. The doc currently warns only about the empty list. Given this type exists to stop exactly this class of representation confusion, one sentence naming the "*" asymmetry would close it.

---

Gates at 090706074

Gate	Result
go build ./...	exit 0
go vet ./...	exit 0
gocyclo -over 10 -ignore "_test\.go" .	exit 0, 0 lines
golangci-lint v2.10.1 (CI-pinned)	exit 0 and 0 issues. ✅
six-module go test -race -short	pkg, providers/{aws,azure,gcp}, tests/e2e all OK; root module FAIL — see below

The root module has exactly one failing test across all six modules: TestGrantCeiling_ConstraintContainment. Attributed independently rather than taken on trust — checked out origin/main in its own worktree and ran that test alone:

group_ceiling_permissions_test.go:290
Error: Target error should be in err chain: expected "permission ceiling exceeded"
[FAIL] TestGrantCeiling_ConstraintContainment

It fails on main by itself, at the same line with the same error. This PR adds two new files to internal/auth and touches nothing in group_ceiling*, so the failure is inherited from the #1737/#1758 sequencing, not caused here. No other test fails in any module.

Summary

Zero value denies on every construction path, the forbidden inversion is absent, the legacy conversion's guarantee is correctly named and holds under the multi-group partial-resolution configuration end to end, the 19th call site and every other conversion are semantically faithful, both hand-rolled consumers are gone, and the rebase replayed exactly three commits with nothing duplicated or dropped. D1 is the one I would fix first — it is a comment, but it is the comment on the seam, and it currently teaches the bug.

[cristim]

Merging. Follow-up to #1752, closing the representation hazard behind #1748 rather than another instance of it.

Merging on independent adversarial review; CodeRabbit has produced no verdict at any head here.

What it does: replaces the []string-where-empty-means-unrestricted representation with AccountScope{Accounts []string; Unrestricted bool}, whose zero value denies. Absence and unrestricted are no longer the same value, which is what made #1748 possible: three producers could emit an empty slice without erroring, and every consumer read it as all accounts.

The compiler found a call site no scan could have. filterReservationsByScopeIndex (handler_ri_exchange.go:420) is a downstream helper, not a caller of getAllowedAccounts, so it was invisible to every call-site enumeration — 18 became 19 only because it stopped compiling. That is the property a type change buys and a grep never will.

Both hand-rolled emptiness checks were forced out, and one was worse than recorded: handler_purchases_revoke.go called GetAllowedAccountsAPI directly, bypassing the admin-API-key and nil-auth branches entirely.

Verified in review, independently:

• The zero value denies on every construction path, checked by enumerating every site that creates an AccountScope rather than by reading the doc comment. The forbidden Restricted bool inversion — which would make the zero value grant everything and rebuild sec(auth): account scoping fails OPEN when a user's groups cannot be resolved (unrestricted access to all accounts) #1748 inside the new type with the compiler blessing it — is absent.
• ScopeFromLegacyLists guarantee holds. It returns UnrestrictedScope() for an empty list, which on its face carries the bug forward. Driven end to end through the new type — real auth.Service over MockStore -> ResolveAllowedAccounts -> ScopeFromLegacyList -> three enforcement seams — under the multi-group partial-resolution configuration that broke sec(auth): fail closed when a user's account scope cannot be established #1752. The widening is refused before the conversion is reached, so it never sees a widened empty list.
• The rebase replayed exactly three commits, nothing duplicated or dropped. The naive rebase conflicted only because it tried to replay #1752s four commits already upstream; --onto with the three type-change commits applied with zero conflicts.

Three documentation findings, all fixed here — and D1 was not cosmetic. getAccountScopes godoc named a removed function, described the wrong return type, and asserted "Empty slice means all access" — not merely stale but inverted, since the returned values empty form now denies. On the one seam this PR pivots on, a reader trusting it would have drawn exactly the opposite conclusion.

D2 cleaned five further stale comments plus internal/auth/group_ceiling.go, which this PR is what breaks. Remaining mentions in test files were left deliberately — they are accurate past-tense history of the defect ("getAllowedAccounts returned (nil, nil) when h.auth was nil"), and rewriting them would falsify the record. The one exception, grantscoped_test.go:22, described the live seam by its old name and was fixed.

D3 is the one worth carrying forward. ScopeForAccounts treats "*" as a literal account identifier:

ScopeForAccounts(["*"])      AllowsAll=false  Allows("acct-A")=false  Allows("*")=true
ScopeFromLegacyList(["*"])   AllowsAll=true   Allows("acct-A")=true

Unreachable today — production builds scopes only via ScopeFromLegacyList and UnrestrictedScope. But the obvious future call, ScopeForAccounts(group.AllowedAccounts), would silently produce a scope that denies everything for a wildcard-carrying group, and all seven seeded groups carry the wildcard. A fresh instance of representation confusion inside the type built to abolish it. Now documented explicitly, naming the asymmetry and which constructor to use.

An operability gain that falls out of the type: String() renders "all accounts" versus "no accounts", where both previously printed as an empty []string — the same ambiguity that made #1748 invisible in the first place.

Gates at d0c2dc19e: build, vet, gocyclo 0 findings, golangci-lint v2.10.1 exit 0 with a genuine 0 issues. line. The internal/auth failure present during review was the inherited red-main breakage, verified against main alone before being attributed; #1772 fixed it and the re-run at 08:32 is green.