[IAmJSD]

Refs #619 (backend half only; the issue stays open for the frontend change below).

Problem

Since #615, POST /api/recommendations/refresh returns 403 permission denied: refreshing recommendations covers every account and requires unrestricted account access for a caller whose allowed_accounts is a subset. The frontend's stale-data auto-refresh (triggerAutoRefreshIfStale, frontend/src/recommendations.ts) fires for any user with view:recommendations, so scoped users get "Refreshing recommendations…" followed by an error toast on Home and Opportunities. The frontend can't avoid this because nothing tells it the caller's scope: GET /api/auth/me/permissions returns only permissions and is_admin.

Re-checked on current main (c8bdd46, merged into this branch): the response has no scope information, and triggerAutoRefreshIfStale / startRecommendationsRefresh are unchanged. No merged PR addresses #619. An older commit, 6b407fb, mentions this issue number in its message, but it is a docs-coverage change from an older numbering, so it doesn't apply here.

Change (backend only)

GET /api/auth/me/permissions now also returns unrestricted_account_access (bool, always present). It is computed by running the refresh's own gate, requireUnrestrictedAccountScope, not a copy of it, so the two can't drift:

• bearer session: the session user's scope;
• user API key: the key owner's scope. The refresh evaluates a key through authorizeAPIKey, which builds a session with the owner's ID and the key's ID; the flag runs the gate on that same session shape, so the two stay in step even if getAccountScope ever starts reading the key ID;
• admin API key: true (getAccountScope returns UnrestrictedScope).

If the scope lookup fails, the error is logged and the flag is reported as false. The whole response does not fail. My reasoning: the flag is UX-only, like the permission entries. The refresh endpoint still enforces the scope (fail closed there is unchanged). false is the safe answer, because the UI then skips a global, paid collection. And failing /me/permissions would drop the frontend to its coarser group-based fallback gating (app.ts catches that error). If you'd rather fail the request, it's a two-line change.

The existing frontend parser (getUserPermissions in frontend/src/api/auth.ts) ignores unknown fields, so this is backward-compatible.

Remaining frontend work (not in this PR; I'm not touching frontend/ while UI work is in progress)

• getUserPermissions: parse unrestricted_account_access and keep it on the user state.
• triggerAutoRefreshIfStale: return early (no toast, no POST) when it is false. Explicit user refreshes, such as the lookback change, could stay as they are or be hidden the same way.
• Add a recommendations.test.ts case next to the existing triggerAutoRefreshIfStale tests (~1622-1680).

Once that lands, the issue is fully addressed.

Verification (unit/mock evidence; no real cloud or deployed-stack check)

New file internal/api/handler_auth_me_permissions_scope_test.go. Requests go through Router.Route / HandleRequest with the shared scoped-auth mocks (grantPermissionsScoped):

• scoped session -> false, with the permission set unchanged; unrestricted session -> true;
• the JSON wire key unrestricted_account_access is present as false for a scoped caller;
• user API key -> the owner's scope (scoped owner false, unrestricted owner true);
• scope lookup error -> 200, flag false, permissions intact;
• admin API key -> true, with no GetAllowedAccountsAPI call;
• parity: for six allowed_accounts shapes (one account, two accounts, nil, empty, ["*"], ["*", acct]), the same handler is asked for /me/permissions and then POST /api/recommendations/refresh. The flag is true exactly when the refresh passes the gate, and false exactly when it returns the scope 403 with nothing started;
• the same parity for a user API key (scoped, unrestricted and wildcard owner), with both calls made with the same key.

Existing /me/permissions mock tests now expect the extra GetAllowedAccountsAPI call (handler_auth_test.go, router_authuser_test.go, apikey_usage_booking_test.go), and the bearer, admin and admin-key cases assert the flag.

Fail-before / mutation checks, run locally:

• source reverted to main: the package fails to build (UnrestrictedAccountAccess undefined);
• field present but the handler never sets it: the unrestricted-session, API-key, admin-key, lookup-error and parity (unrestricted shapes) tests fail;
• helper forced to return true: the scoped-session, wire-field, scoped-owner API key, lookup-error and parity (scoped shapes) tests fail;
• API-key path resolving scope for the key ID instead of the owner: fails;
• getAccountScope made key-aware (scratch probe) while the flag passes an owner-only session: the user-API-key parity test fails (unrestricted and wildcard owner); with the key session as committed it passes.

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

go build ./...
go vet ./internal/api/ && go vet -tags integration ./internal/api/
go test ./internal/api/ -count=1                # ok
go test -short -race ./internal/api/ -count=1   # ok
go test ./internal/mocks/ -count=1              # ok (repo-wide test guards)
golangci-lint v2.10.1 run ./internal/api/       # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/api/handler_auth.go   # silent
scripts/gosec-hook.sh internal/api/handler_auth.go internal/api/types.go   # clean
git-secrets --scan on the changed files in a fresh repo with isolated HOME   # clean
scripts/test-git-secrets-allowlist.sh            # 42 PASS, 0 FAIL

There are no DB writes or schema changes, so concurrency isn't affected: the new code is one more read of the existing scope lookup.

Notes / out of scope

• For a scoped caller, getAccountScope also loads the cloud account list (withRegisteredAccountNames, sec(auth): AccountScope.Allows grants every account that shares an allowed display name #433). That makes one more read per /me/permissions call for scoped users only.
• internal/api/handler_auth.go was already over the 500-line guideline (732 lines). This adds about 30 more.
• Per-account refresh for scoped users, which needs a new CollectRecommendations contract with an account filter and a per-account marker, is not attempted. That is still the owner's call, as noted in fix(api): require unrestricted scope to refresh recommendations #615.

Maintainers: please mirror #619's triage labels on this PR: triaged, priority/p3, severity/low, urgency/this-quarter, impact/few, effort/s, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

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

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

[cristim]

VERDICT: REQUEST_CHANGES 372304d

Reviewed: git diff origin/main...372304de (1 commit, 6 files, +233/-7; largest commit 240 changed lines, under the 400 cap). Base 2d822f3 is stale; git merge-tree --write-tree origin/main 372304de (origin/main ca1e8a8, includes #1161) merges cleanly (tree 047a3a64). All tests below ran on that merged tree.

Findings

• blocking (PR metadata) PR body - the body is meant as "Refs fix(recommendations): scoped users see 'Recommendations refresh failed: permission denied' after the stale-data auto-refresh became scope-gated #619 (backend half only)", but it quotes the commit text "closes #619" in the Problem section, and GitHub parses that: gh pr view 1155 --json closingIssuesReferences returns [619]. Merging would auto-close fix(recommendations): scoped users see 'Recommendations refresh failed: permission denied' after the stale-data auto-refresh became scope-gated #619 while the user-visible bug stays (the frontend still fires the auto-refresh and shows the 403 toast). Reword that sentence so it has no closing keyword, then confirm closingIssuesReferences is [].
• medium (completeness) frontend/src/api/auth.ts:137-157 - nothing in frontend/src reads unrestricted_account_access (grep: zero hits), so this PR alone does not change behaviour for any user. That is declared, so it does not block, but fix(recommendations): scoped users see 'Recommendations refresh failed: permission denied' after the stale-data auto-refresh became scope-gated #619 needs a tracked follow-up (the frontend parse plus the triggerAutoRefreshIfStale early return), or the field ships dead.
• nit internal/api/openapi.yaml - /api/auth/me/permissions is undocumented in the spec (pre-existing, listed in the 2026-09-02 audit), so the new field has no spec entry either.
• nit internal/api/handler_auth.go:194-203 - every /me/permissions call now runs ResolveAllowedAccounts (and ListCloudAccounts for scoped users), and a persistent scope-lookup failure logs at error level on every page load. This is acceptable for a UX flag; noting the added load.

Correctness: the flag runs the refresh's own gate (requireUnrestrictedAccountScope -> getAccountScope), which ignores UserAPIKeyID, so the owner-scope choice for user API keys matches the refresh exactly. Admin bearer users are not short-circuited, in the flag or the gate: an admin whose group restricts allowed_accounts reports false and is refused by the refresh, which is consistent. The admin API key reports UnrestrictedScope without an auth call. The field is a bool about the caller's own scope, and no account list or other user's data is exposed.

Attacked:

• go vet ./internal/api/ and go vet -tags integration ./internal/api/: clean. go test ./internal/api/ -count=1: ok.
• jest on the merged tree: auth.test.ts, recommendations.test.ts, app.test.ts: 502/502 pass (frontend untouched).
• Mutation probes on handler_auth.go (scratch export, each reverted):
  • flag always true: 5 tests fail (scoped session, wire field, scoped-owner key, lookup error, refresh-gate parity).
  • key path uses keyID: an existing UserAPIKey test fails (strict mock).
  • lookup error fails open (true): the lookup-error test fails.
  • admin-key flag false: 2 tests fail.

[cristim]

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

Findings (none blocking):

• [non-blocking] internal/api/handler_auth.go:189-203 - unrestrictedAccountAccess passes &Session{UserID: userID} without UserAPIKeyID.
  • That matches today's gate, because getAccountScope (internal/api/handler.go:660) ignores the key ID.
  • If getAccountScope ever becomes key-aware, the two would drift, and the parity test (FlagMatchesRefreshGate) covers only sessions.
• [non-blocking, disclosed] handler.go:689 - For a scoped caller, every /me/permissions call now also runs ListCloudAccounts through withRegisteredAccountNames. That result can never flip AllowsAll, so the read is wasted. Cost only.
• [non-blocking] This is the backend half only. fix(recommendations): scoped users see 'Recommendations refresh failed: permission denied' after the stale-data auto-refresh became scope-gated #619's acceptance criteria (skipping the auto-refresh, and the recommendations.test.ts case) remain open. The PR correctly says "Refs fix(recommendations): scoped users see 'Recommendations refresh failed: permission denied' after the stale-data auto-refresh became scope-gated #619" rather than closing it.

Root cause: #619 is that the frontend cannot tell the caller's scope.

• The new field calls requireUnrestrictedAccountScope (handler_recommendations_refresh.go:118) itself, the same check postRefreshRecommendations runs at :71. It is not a re-implementation.
• User API keys:
  • The refresh's authorizeAPIKey (handler.go:348) puts the owner's ID on the session.
  • The flag uses keyOwnerID (handler_auth.go:145), so the two agree.
• The admin key gets UnrestrictedScope (handler.go:661) and reports true with no lookup.

Attacked:

• Error path:
  • A lookup error gives false plus a log line, and the response is still 200.
  • A non-403 client error is logged too.
  • The refresh endpoint still fails closed on its own.
• Blast radius:
  • UserPermissionsResponse is built only in handler_auth.go.
  • The frontend parser (frontend/src/api/auth.ts:137) destructures only permissions and is_admin, so the extra field is ignored.
  • openapi.yaml does not document /auth/me/permissions, so there is no spec drift.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp. All 6 were caught:
  • M1, the 403 branch returns true: 4 tests FAIL.
  • M2, the key path uses keyID instead of the owner: a test FAILS.
  • M3, the lookup-error branch returns true: ScopeLookupError FAILS.
  • M4, the admin flag forced false: 2 tests FAIL.
  • M5, the session path does not set the flag: 8 tests FAIL.
  • M6, the key path does not set the flag: 4 tests FAIL.
• Fail-before:
  • With only the tests copied into base, the build fails ("UnrestrictedAccountAccess undefined").
  • With the tests plus only the struct field, 15 tests FAIL, including every unrestricted parity subcase.
• Drift: main moved internal/api and internal/auth since the base, but merge-tree is clean, so I tested the merged tree too.

Verified locally, on git archive exports with no cloud credentials, under the heavy lock:

• Head:
  • go build ./...: OK.
  • go vet ./internal/api/, plain and with -tags integration: OK.
  • gofmt -l on the 6 changed files: clean.
  • go test -count=1 ./internal/api/: ok.
• Merged tree:
  • go build ./...: OK.
  • go vet on api and auth, plus -tags integration: OK.
  • go test -count=1 ./internal/api/ ./internal/auth/: ok.
• There is no migration or DB code, so the integration suites were not run.

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

[IAmJSD]

Addressing the VERDICT (372304d) and Independent review 2 (372304d). New head: bc07006b9b7fbed6a503f6f281377d7747f6f91d.

Merged origin/main (c8bdd46) into the branch as e908fa6. No conflicts.

VERDICT findings

• [blocking] PR body linked fix(recommendations): scoped users see 'Recommendations refresh failed: permission denied' after the stale-data auto-refresh became scope-gated #619 as closing: fixed. The Problem section no longer quotes the old commit's closing keyword, and "Once that lands, fix(recommendations): scoped users see 'Recommendations refresh failed: permission denied' after the stale-data auto-refresh became scope-gated #619 can close." is reworded too. gh pr view 1155 --json closingIssuesReferences now returns []. The body still has Refs #619 on its own line, and no commit on the branch carries a closing keyword.
• [medium] frontend does not read the field yet: left as declared. I did not touch frontend/. fix(recommendations): scoped users see 'Recommendations refresh failed: permission denied' after the stale-data auto-refresh became scope-gated #619 stays open because this PR is Refs only, and the PR body's "Remaining frontend work" section lists what is left: parse the field in getUserPermissions, return early in triggerAutoRefreshIfStale when it is false, and add a recommendations.test.ts case. If you want a separate tracking issue instead, say so and I'll open one.
• [nit] openapi.yaml has no /api/auth/me/permissions entry: left. Draft fix(api): document every router.go route in openapi.yaml and guard against drift #1100 adds this path and a UserPermissionsResponse schema (permissions, is_admin). Adding it here too would conflict with that PR. Whichever of the two merges second should add unrestricted_account_access (boolean, required) to that schema.
• [nit] extra scope lookup per call, and an error log on persistent failure: left as is, as you said is acceptable for a UX flag.

Independent review 2

• [non-blocking] the flag passed &Session{UserID: owner} without the key ID, and parity was tested only for sessions: fixed.
  • internal/api/handler_auth.go:145: the key path now passes &Session{UserID: keyOwnerID, UserAPIKeyID: keyID}, the same shape authorizeAPIKey builds.
  • unrestrictedAccountAccess (handler_auth.go:193) now takes a *Session.
  • New TestGetCurrentUserPermissions_UserAPIKey_FlagMatchesRefreshGate (internal/api/handler_auth_me_permissions_scope_test.go:182) calls /me/permissions and the refresh with the same key, for a scoped, an unrestricted and a wildcard owner.
  • Probe 1: I made getAccountScope key-aware on a scratch copy and put the old owner-only session back. The new test fails for the unrestricted and wildcard owners. With the committed code it passes.
  • Probe 2: forcing the flag to true fails the scoped-owner case.
  • Both probes were reverted.
• [non-blocking] the wasted ListCloudAccounts read for scoped callers: left. Avoiding it means changing getAccountScope, which is the shared gate, and that is out of scope here. It is cost only and is listed in the body.
• [non-blocking] the frontend half is still open: same as the medium item above.

Verification (merged tree, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null): go build ./... ok. go vet ./internal/api/, plain and with -tags integration, ok. go test ./internal/api/ -count=1 ok. go test -short -race ./internal/api/ -count=1 ok. go test ./internal/mocks/ -count=1 ok. golangci-lint v2.10.1 on ./internal/api/: 0 issues. gocyclo -over 10 on handler_auth.go: silent. gofmt clean. scripts/gosec-hook.sh internal/api/handler_auth.go clean. git-secrets --scan on the two changed files in a fresh repo with an isolated HOME is clean, and an AKIA positive control was caught. No frontend, Terraform or workflow changes.

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• internal/api/handler_auth.go unrestrictedAccountAccess: if the scope lookup fails, the flag reads false and the response is still 200. That fails closed for the UI: it skips a global, paid collection, and the refresh endpoint still enforces the scope itself. One side effect: during a scope-store outage an unrestricted admin's UI will also skip the stale-data auto-refresh, with only a server log line to show for it. I think that is acceptable. The PR body raises it for the owner and I agree with its default.
• For scoped callers, every /me/permissions call now also runs GetAllowedAccountsAPI plus ListCloudAccounts (through withRegisteredAccountNames). The body discloses this. It is fine unless the frontend polls that endpoint.
• Refs #619 is correct, not Closes: the issue's acceptance needs the frontend skip and a recommendations.test.ts case, and both are listed as remaining work.

Attacked:

• Scope and leak: the flag reports only the caller's own scope, and it is computed by calling requireUnrestrictedAccountScope, the same function the refresh uses, not a copy. For a user API key it uses the owner ID plus the key ID, the same session shape authorizeAPIKey builds. For the admin key, getAccountScope short-circuits to UnrestrictedScope. A nil h.auth gives an error, so the flag is false. No permission or authorization decision reads this flag, so it cannot widen access.
• Fail-before (mutations in the scratch checkout, restored afterwards): (a) the helper returns false on a passed gate, so the unrestricted-session, user-API-key, admin-key and both parity tests fail. (b) The helper returns true on the 403, so the scoped-session, wire-field, user-API-key and both parity tests fail. Together these show the tests pin both directions and the gate parity.
• Ran: go build ./..., go vet ./internal/api/ with and without -tags integration, go test -race -short -count=1 ./internal/api/ (ok, 190s), and golangci-lint --allow-parallel-runners with and without --build-tags integration (0 issues each). gocyclo -over 10 on handler_auth.go/types.go is silent, and the secrets-scan printed "clean; planted key flagged". The full -tags integration ./internal/api/ run hit a testcontainers reaper error (TestLadderReconcile_TailLockBlocksRealRetryLink, "container status removing") and the default 10m timeout in a marketplace test, both while another suite was loading the host. No integration test touches /me/permissions or the files this PR changes.

Body-stale:

• none

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

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• internal/api/handler_auth.go:192-203: a scope-lookup failure is folded into false with no signal to the client, so the frontend can't tell "scoped" from "unknown". That's acceptable because the flag only suppresses an automatic refresh and the refresh endpoint still enforces the scope itself. But the frontend follow-up should treat false as "skip the auto-refresh quietly", not as "the user is scoped" (for example, don't hide scoped-only UI on the strength of it).
• internal/api/handler_auth.go: the file was already over the 500-line guideline and this adds about 30 lines. The new helper would fit in handler_recommendations_refresh.go next to requireUnrestrictedAccountScope.
• For scoped callers, each /me/permissions call now also does GetAllowedAccountsAPI plus ListCloudAccounts (via withRegisteredAccountNames). That's fine at page-load frequency; worth remembering if the endpoint ever gets polled.

Attacked:

• Checked that the flag runs the refresh's own gate (requireUnrestrictedAccountScope -> getAccountScope) and doesn't copy it, on all three principal paths: bearer session, user API key (same {UserID: owner, UserAPIKeyID: key} session shape as the refresh path) and admin key (UnrestrictedScope, no DB call). A 403 ClientError maps to false; any other error is logged and also maps to false. Neither path fails open, and the refresh endpoint's enforcement is unchanged. The flag only reveals the caller's own scope, so no other tenant's information leaks.
• Fail-before: I reverted only internal/api/handler_auth.go to origin/main (keeping the new struct field). 15 tests fail, including the unrestricted, user-API-key, admin-key, lookup-error and both refresh-parity tests. The restored head passes.
• Ran go build ./..., go vet ./internal/api/, go vet -tags integration ./internal/api/ and go test -race -short -count=1 ./internal/api/: all ok.
• Against issue fix(recommendations): scoped users see 'Recommendations refresh failed: permission denied' after the stale-data auto-refresh became scope-gated #619: the backend half of option 1 (scope flag on /me/permissions) is done. The frontend skip and the jest test are still outstanding, so Refs #619 (not Closes) is correct.

Body-stale: none

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

[IAmJSD]

Local verification at 6adef39fc25547924e6f67ddb5772651e127d322 merged onto main (5aa3df5b), together with #1155, #1219, #1221, #1223, #1224, #1225, #1226, #1227, #1256, #1262, #1266, #1270, #1272, #1275, #1283, #1286, #1293, #1297, #1299, #1300, #1301, #1304, #1307, #1311, #1313, #1325 in that order (combined tree f3a3331f, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

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