[IAmJSD]

Root cause

A group's allowed_accounts can hold an account's display name (the group duplicate modal writes acct.name). The stored name is not bound to any account. AccountScope.Allows matched an entry against the account ID or the name, so once the account a name entry was meant for was deleted or renamed, the entry granted whichever account was later created or renamed to that name. #1032 limited name matching to names exactly one registered account carries. That does not help in this case, because the newcomer is the only account with that name.

Fix (fail closed)

This takes the issue's first option: resolve names to account IDs at grant time and store the IDs. Existing name entries are migrated, and name matching on scope entries is removed.

1. Matching (internal/auth/account_scope.go): scope entries are compared with the account identifier the caller passes, never with a display name. WithRegisteredNames(names) becomes WithRegisteredAccounts([]RegisteredAccount{ID, Name}). It still lets a caller that knows an account only by name (legacy history and inventory rows keyed by external ID) identify it. The name resolves to the one registered account that carries it, and that account's ID must be in the scope. A shared name resolves to nothing (sec(auth): AccountScope.Allows grants every account that shares an allowed display name #433 semantics kept). Naming an account after an in-scope account's ID gains it nothing.

2. Grant-time binding (internal/api/handler_groups.go, bindGroupAccounts): on POST /api/groups and PUT /api/groups/{id}, each allowed_accounts entry must be one of:

   • "*";
   • a registered account ID;
   • a registered account's external ID (kept verbatim, as before);
   • a name that exactly one registered account carries and that the acting principal's own scope covers. The name is stored as that account's ID.

   Anything else is a 400 with one message for unknown, ambiguous and out-of-scope names, so the response does not reveal which accounts exist. Entries are never silently dropped, so a restricted list cannot shrink to [] (which means unrestricted). If the account list or the actor's scope cannot be loaded, the write is refused. Omitted (nil), [] and ["*"] pass through unchanged to the existing ceiling checks. Binding runs before CreateGroupAPI/UpdateGroupAPI, so the account ceiling compares IDs.

3. Migration 000135 (bind_group_account_names_to_ids) rewrites a stored name entry to an account ID only when all of these hold:

   • the entry is not * and not empty;
   • it is not already an account ID or external ID;
   • exactly one account carries the name (any provider, enabled or not);
   • that account's created_at <= groups.updated_at. An account registered after the grant was last written cannot be the account it was meant for, and that is the issue's delete-then-recreate case.

   Order is preserved and entries are only replaced, never dropped. update_groups_updated_at is disabled for the statement (same pattern as 000127), so the rewrite does not move updated_at. Otherwise a re-apply could bind an entry the first run left unresolved. Entries the migration cannot bind stay in place and match nothing. The migration header has a query that lists them. Down is a no-op: pre-fix(auth): name-based account scope entries match a later account that reuses a deleted or renamed name #1034 code matches entries by ID too, so rewritten entries keep working after a rollback.

4. Name-only helpers removed: auth.MatchesAccount and AuthContext.CanAccessAccount (tests only, no production callers), with their tests.

Migration number: main ends at 000134 (#1110, merged), so this PR takes 000135. It must merge before #1140 (000136); golang-migrate skips a lower version once a higher one is applied. 000134 touches only purchase_suppressions and 000135 only groups, so the two do not interact.

Behavior changes (please confirm)

• Name entries stop granting access. A group that scoped an account by name loses it until the migration binds the entry or an admin grants the ID. The migration binds every name that unambiguously identifies an account that predates the grant. Entries it skips are shared names, names no account carries, and names of accounts created after the group's last write. Those entries now fail closed.
• Group writes reject unbindable entries with 400. The edit modal never sends allowed_accounts, so it is unaffected. The duplicate modal sends ticked names, which now get bound to IDs. If nothing is ticked, it copies the source group's list, and a source group that still holds an unresolved legacy name now gets a 400 asking for an account ID.
• GET /api/groups shows account IDs where names used to be.
• A "" entry no longer matches a caller that passes "" as the account ID.
• One extra ListCloudAccounts per group write that carries non-* entries, plus the existing scope load for restricted actors.

Concurrency

• Group write vs. account rename or create: binding reads the account list and then stores IDs. If the account is renamed, or another account takes its name, between the read and the write, the grant stays on the account that carried the name when the admin submitted. IDs are server-generated UUIDs (createAccount sets uuid.New()) and are never reused, so a stored ID cannot later point at a different account. A concurrent delete leaves a dangling ID that matches nothing.
• Migration vs. a concurrent group update: UpdateGroupAPI reads the group and then writes the whole row. If it reads before the migration commits and writes after, it writes the old name entries back. With this PR those entries are inert (no access, fail closed), so I did not add a race test. Real-Postgres coverage of the migration itself is below.

Verification

The branch is merged with origin/main (a14dc8f) at 3a6f92d. The runs below were first done on macOS, then repeated on the merged tree 3a6f92d on Linux, including the full -tags integration suites of ./internal/api/, ./internal/auth/ and ./internal/database/postgres/migrations/ (details in the PR comment for 3a6f92d).

All runs were local with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's default AWS profile is invalid. No cloud APIs were called. Evidence is unit and mock tests through HandleRequest and the handlers, plus a real Postgres container (testcontainers) for the migration. There is no real-account run.

• go build ./..., go vet ./..., go vet -tags integration ./internal/...: pass
• go test -count=1 ./internal/auth/ ./internal/api/ ./internal/mocks/ ./internal/server/ ./internal/database/postgres/migrations/: pass. go test -race ./internal/auth/: pass.
• go test -tags integration ./internal/database/postgres/migrations/ -run 'TestMigration_BindGroupAccountNamesToIDs|TestMigrations_DownAllRoundTrip|TestMigrations_FullStackIdempotent': pass
• Scope-related integration tests on real Postgres: go test -tags integration ./internal/api/ -run 'TestMarketplacePermissionConstraintsHTTP|TestMarketplaceListCancelFailure|TestAdminUpdateUserHTTPKeepsConcurrentMFA' and ./internal/server/ -run TestConfigPermissionConstraintsHTTP: pass
• golangci-lint v2.10.1 --allow-parallel-runners on ./internal/auth/... ./internal/api/..., and with --build-tags integration on the migrations package: 0 issues
• gocyclo -over 10 on the changed non-test files: clean. scripts/gofmt-hook.sh and scripts/gosec-hook.sh on the changed packages: clean.
• git-secrets on the changed files in a fresh git init repo with an isolated HOME: clean. bash scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.

Fail before, pass after. I reverted account_scope.go, types.go, group_ceiling.go, handler.go and handler_groups.go to origin/main and kept the new tests. These fail on main and pass here:

• TestHandleRequest_AccountScope_NameReusedAfterDelete: the issue's scenario. A name entry whose account was deleted reaches the account later registered under that name (200 on main, 404 here).
• TestGroupGrantByName_SurvivesRenameAndNameReuse: create a group by name, rename the account, register a newcomer under the old name. The grant stays with the renamed account.
• TestGrantScoped_NameEntryDoesNotMatch, TestGetAccount_NameEntry_Returns404, TestGetRIUtilization_ScopeByAccountNameReachesNothing, the name-entry cases of TestHandler_getRIExchangeHistory_AccountScope and TestAuthorizeSessionRevoke_RevokeAny_AccountScope, and TestHandleRequest_AccountScope_SharedDisplayName/a_unique_name_entry_grants_nothing_either.
• All the write-path tests: TestCreateGroup_BindsAccountNamesToIDs, ..._RefusesUnbindableAccountEntries (unknown, shared by three accounts, empty, outside the actor's scope), ..._ScopedActorBindsInScopeName, ..._AccountListFailureRefusesWrite, ..._NoAccountStoreRefusesWrite, TestUpdateGroup_BindsAccountNamesToIDs, ..._RefusesUnbindableAccountEntry.

Controls pass on both: IDGrantSurvivesRename, the pass-through cases for omitted, [] and *, and OmittedAccountsStayNil.

Mutation probes, each killed by at least one test:

• Matching:
  • name entries match again;
  • a resolved name grants without the ID check;
  • shared names treated as unique (c >= 1);
  • getAccountScope never registers accounts.
• Binding:
  • ignore the actor's scope;
  • a third duplicate makes a name unique again;
  • store unknown entries verbatim;
  • create stores unbound entries;
  • a list failure passes entries through.
• Migration:
  • drop the created_at guard;
  • drop the * exclusion (an account named * would narrow wildcard groups);
  • leave the updated_at trigger on;
  • count >= 1;
  • ignore external IDs.

Existing tests updated: fixtures that scoped by display name (execution scope matrix, history, inventory, analytics, revoke, dashboard, accounts, RI utilization, RI exchange history, and the #1161 analytics provider-scope tests brought in by merging main, unit and integration) now scope by account ID and keep their original assertions. Tests that pinned name matching (TestGrantScoped_MatchesByAccountName, TestGetAccount_InScope_ByName, the #433 shared-name suite, and the by-name cases of #1169's RI exchange history unit and integration tests brought in by merging main) are inverted or reworked. The TestNoUnfailableMockAssertions guard (go test ./internal/mocks/) passes. markdownlint is not installed here, so the CHANGELOG entry was checked by eye against the existing entries.

Out of scope / notes

• Frontend: the duplicate modal still sends names (cb.value = acct.name). The backend now binds them. Sending IDs would make the modal immune to a same-name race, and it should not copy a source group's unresolved entries. I left this for UI work because this PR does not touch frontend/.
• External-ID entries keep their literal semantics. Provider external IDs are issued by the provider rather than chosen as labels, so they were not rebound.
• The migration's created_at guard cannot detect an existing account that was renamed into a dangling name after the grant, or a group edited after the reusing account appeared. In both cases the name is bound to the account that carries it at migration time. Groups that relied on the pre-fix behavior were already exposed to this.
• Pre-existing and unchanged: resolveAccountNamesByID keys names by internal ID and external ID in one map. An account whose external ID equals another account's internal UUID could shadow that account's name in the lookup. This is contrived, but worth a separate look.

Please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, urgency/this-quarter, impact/few, effort/m, type/security, triaged.

Closes #1034

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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

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

[cristim]

Independent review 2 (claude-fleet, cold): REQUEST_CHANGES 5d34f77

The code and the migration look correct. The only reason for changes is that the branch is stale against origin/main: it does not merge cleanly, and after resolving the conflicts, two of main's tests fail.

Migration order: this PR adds 000135 and is link 2 of a chain that must merge strictly in this order:

#1110 (134) -> #1116 (135) -> #1140 (136) -> #1170 (137) -> #1197 (138) -> #1201 (139)

golang-migrate skips a lower version once a higher one is applied.

• origin/main (b6a0649) is at 000133. The PR body's "main is at 000130" is stale.
• The PR is based on main, not stacked on fix(purchase): scope purchase suppressions to the AZ, platform and tenancy bought #1110. Both share merge-base 8caed39, and 000134 exists only on fix(purchase): scope purchase suppressions to the AZ, platform and tenancy bought #1110.
• Applying 134 then 135 in that order works (see below). 134 touches only purchase_suppressions and 135 only groups, so the two migrations do not interact.

Findings:

• [blocking] Merge conflicts. git merge-tree --write-tree origin/main 5d34f77 reports content conflicts in two test files, both from fix(api): scope a scoped user's raw analytics account id to its provider #1161 (cb25d99) on main:
  • internal/api/handler_analytics_scope_provider_test.go
  • internal/api/handler_per_account_perms_test.go
• [blocking] After resolving those conflicts (keeping main's sec(api): analytics queries with a scoped user's raw external id are provider-unconstrained and can return another provider's unregistered rows #624 provider-keyed result shape plus this PR's ID scoping), two of main's new fix(api): scope a scoped user's raw analytics account id to its provider #1161 tests still scope by display name, so they fail under ID-only matching:
  • TestHandleRequest_Analytics_RawExternalIDProviderScopedOnEveryEndpoint, with scope "data" (:217, :224 on the merged tree);
  • TestHandleRequest_Analytics_ProviderlessAccountFailsClosed, with scope "x" (:248, :252).
  • This is the intended behaviour change, so the fix is a rebase that switches those fixtures to account IDs.
  • Everything else in ./internal/api and ./internal/auth passes on the merged tree.
• [non-blocking] Down is a documented no-op (SELECT 1). That is acceptable for a one-way data rewrite with no schema change, since pre-fix(auth): name-based account scope entries match a later account that reuses a deleted or renamed name #1034 code still matches by ID. Up is idempotent on re-apply.
• Safety on a populated table:
  • NULL entries, empty arrays and a NULL updated_at fail closed.
  • No trigger other than update_groups_updated_at exists on groups (checked migrations 000001 to 000135), and it is re-enabled in the same transaction (tgenabled = 'O' checked).
  • 1,000 groups and 2,000 accounts took 1.15 s.

Attacked:

• Callers of Allows (20 sites): every name passed in comes from the registry, so resolving it leads back to the same account and creates no new path.
• Removed functions: MatchesAccount, CanAccessAccount and WithRegisteredNames have no callers left on head or on the merged tree.
• Other checks:
  • There is no SQL-side name matching on the scope.
  • The seeded system group is rewritten correctly.
  • The migration and the Go binder agree: exact-case matching, ID and external-ID entries kept verbatim, any provider, enabled or not.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp. All 5 were caught:
  • M1, name entries match again: NameEntryNeverMatches, GrantScoped_NameEntryDoesNotMatch and GetAccount_NameEntry_Returns404 FAIL.
  • M2, the binder ignores the actor's scope: TestCreateGroup_RefusesUnbindableAccountEntries FAILS.
  • M3, the created_at guard dropped: the 135 test FAILS.
  • M4, the updated_at trigger left on: the 135 test FAILS.
  • M5, the * exclusion dropped: the 135 test FAILS.
• Fail-before: the new tests do not compile on base because of the API rename. M1 restores base's name matching and fails them.

Verified locally, with no cloud credentials and testcontainers postgres:16-alpine, under the heavy lock:

• Head:
  • gofmt: clean.
  • go build ./...: pass. go vet on auth and api, and with -tags integration on migrations: pass.
  • go test -count=1 on auth, api, mocks, server and migrations: pass.
  • -tags integration -run 'TestMigration_BindGroupAccountNamesToIDs|TestMigrations_DownAllRoundTrip|TestMigrations_FullStackIdempotent': pass.
• Chain tree (origin/main, this PR with conflicts resolved, and fix(purchase): scope purchase suppressions to the AZ, platform and tenancy bought #1110's 000134):
  • TestMigration_PurchaseSuppressionsVariant, the 135 test, DownAllRoundTrip and FullStackIdempotent: pass.
  • A scratch chain test: pass.
    • It migrates 133 to 134 to 135, checking the version and dirty flag at each step.
    • It checks the bindings: names are bound; IDs, external IDs, unknown names and * are kept; updated_at is unchanged.
    • It rolls back 2 steps to 133 with the data intact, re-applies to 135 with an identical result, and confirms the trigger fires again.
  • Unit tests: api FAILS on the 2 tests above. auth, mocks, server and migrations pass.

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

[IAmJSD]

Re: Independent review 2 (5d34f77). New head: d980764. I merged origin/main (c8bdd46) rather than rebasing. Migration 000135 is unchanged.

[blocking] Merge conflicts (#1161, cb25d99). Resolved in merge commit d980764. Both sides survive:

• internal/api/handler_analytics_scope_provider_test.go:125-132: kept main's loop and its {"aws": {"111"}} provider-keyed assertion (sec(api): analytics queries with a scoped user's raw external id are provider-unconstrained and can return another provider's unregistered rows #624). The account entry is the ID "u-in", not the name "in", and the follow-up 404 call also scopes by "u-in".
• internal/api/handler_per_account_perms_test.go:337-340: kept main's resolveAnalyticsAccountFilter name and this PR's AccountScope.Allows wording, since MatchesAccount is removed here.

[blocking] #1161 tests that scope by display name. They now scope by account ID:

• TestHandleRequest_Analytics_RawExternalIDProviderScopedOnEveryEndpoint: "data" changed to "u-gcp" at handler_analytics_scope_provider_test.go:217, :224, :233.
• TestHandleRequest_Analytics_ProviderlessAccountFailsClosed: "x" changed to "u-x" at :248, :252.
• Also fixed, not in the review: fix(api): scope a scoped user's raw analytics account id to its provider #1161's integration tests in internal/api/handler_analytics_scope_provider_integration_test.go:198, :206, :226 scoped by the name "dev". They now scope by db.awsUUID.
  • With main's version on the merged tree, TestAnalyticsScopedRawExternalIDIsProviderScoped_DB fails in 2 subtests (scope_dev, scoped_UUID_request).
  • TestAnalyticsScopedRawExternalIDConcurrentRegistration_DB passed only vacuously, because every request was refused. Its assertions don't require ok > 0. I left that as is because it is main's test and the count is timing-dependent.

[non-blocking] Down is a no-op. No change. Agreed it is acceptable for this one-way data rewrite.

PR body. The stale "main is at 000130" line now says: main is at 000133, #1110 holds 000134, and this PR must merge after #1110 and before #1140. I also noted the #1161 fixture updates and the re-run on the merged tree. Closes #1034 is unchanged.

Note, unchanged: for a raw external ID, main's scopedAnalyticsAccountFilter calls allowed.Allows(accountID, match.Name). An ID-scoped user therefore reaches the account through name resolution. If the account's display name is shared, the request fails closed (404) even though the external ID is unique. This is not a leak. Passing match.ID would avoid it, but that changes code from #1161, so I left it for a follow-up.

Verification on d980764, local, with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null. No cloud APIs were called.

• go build ./... passes. go vet on api and auth, and with -tags integration on api and migrations, passes.
• go test -count=1 passes, one package at a time, for auth, api, mocks, server and migrations.
• go test -tags integration ./internal/api/ (full suite, testcontainers Postgres) passes in 242s.
• With -tags integration, these pass:
  • migrations: TestMigration_BindGroupAccountNamesToIDs|TestMigrations_DownAllRoundTrip|TestMigrations_FullStackIdempotent
  • server: TestConfigPermissionConstraintsHTTP
• golangci-lint v2.10.1 --allow-parallel-runners on api and auth, and --build-tags integration on api and migrations: 0 issues. gofmt -l is clean. gocyclo -over 10 on the changed non-test files is clean.
• git-secrets (--register-aws plus the repo's .gitallowed) on the 28 changed files, in a fresh git init repo with an isolated HOME: clean.

The PR stays a draft.

🤖 Generated with Claude Code

[IAmJSD]

Merged origin/main (a14dc8f) to resolve conflicts. New head: 3a6f92d. This is a normal merge commit, with no rebase or force-push. No cristim review has been posted since review 2 (5d34f77), which was addressed in d980764, so this push only brings the branch up to date. Migration 000135 is unchanged and main ends at 000134 (#1110 merged).

Conflicts. Both came from #1169 (ef96ae7), which moved the RI exchange history scope filter into the store query (GetRIExchangeHistoryForAccounts):

• internal/api/handler_ri_exchange.go: kept main's loadRIExchangeHistory / exchangeAccountIDsInScope. This PR's only change in that block was the comment "UUID or name" -> "UUID", so I carried that wording into main's relocated doc comment (:2274). No logic change on either side.
• internal/api/router_handlers_test.go:1023-1029 (TestHandler_getRIExchangeHistory_AccountScope): kept main's wantIDs column (the ID set pushed into the store query) with this PR's ID-only expectations.
  • The name entry "prod" now pushes only ["prod"] and sees 0 rows (renamed "a name entry matches nothing").
  • "dev" pushes only ["dev"] instead of ["444455556666", "dev"].
  • The other rows are main's.

Not a textual conflict, also fixed. #1169's TestRIExchangeHistory_ScopedUserSeesOlderOwnRows_Integration had a "scoped to B by name" case expecting B's 3 rows, which fails under ID-only matching. It is now "a name entry for B matches nothing" and asserts an empty history (handler_ri_exchange_history_scope_integration_test.go:63, :79). The uuid and STS-ID cases and main's limit-after-filter assertions are unchanged.

PR body. Updated the migration-order line (#1110 has merged) and the verification base. Added #1169's by-name cases to the list of inverted tests. Closes #1034 is unchanged.

Verification on 3a6f92d, run locally on Linux with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null. No cloud APIs were called.

• go build ./..., go vet ./..., go vet -tags integration ./internal/...: pass.
• Unit tests, go test -count=1 one package at a time: auth, api, mocks, server, migrations and config pass.
• Integration tests, go test -v -tags integration -count=1, one package at a time, testcontainers postgres:16-alpine:
  • ./internal/database/postgres/migrations/, full package: 146 PASS. Includes TestMigration_BindGroupAccountNamesToIDs (000135), TestMigration_PurchaseSuppressionsVariant (000134), TestMigrations_DownAllRoundTrip and TestMigrations_FullStackIdempotent.
  • ./internal/api/, full package: 4412 PASS. Includes the edited fix(api): apply the RI exchange history limit after the account filter #1169 test and fix(api): scope a scoped user's raw analytics account id to its provider #1161's TestAnalyticsScopedRawExternalID*_DB.
  • ./internal/auth/: 1080 PASS.
  • Some tests in each package failed only at container startup (testcontainers reaper unexpected container status "removing" or a wait-for-ready deadline). This VM's shared Docker daemon was heavily loaded. There were no assertion failures. Each of those tests passed when rerun alone by name:
    • migrations: ExecutionsAccountFKRestrict_Rollback, DropUserRoleToGroups, DropUserRoleToGroups_Down, GrantViewConfigToNonAdminGroups, AutoPurchaseSwitchStamps, SplitSavingsPlans
    • api: TestLadderTimelineAmendmentRoutedPersistence, TestDeleteManagedPlanByExecutionStatus, TestUpdatePlan_CapPresencePostgres
    • auth: TestIntegration_ResetConfirmRejectsStaleRead
  • Because of the load I used -timeout=60m for the full packages; CI uses 20m. Two earlier full runs hit the timeout, and every test they completed passed apart from the startup failures above.
• golangci-lint v2.10.1 --allow-parallel-runners on api and auth, and with --build-tags integration on api, auth and migrations: 0 issues. gofmt -l: clean. gocyclo -over 10 on the changed non-test files: clean.
• git-secrets (--register-aws plus the repo's .gitallowed) on the 28 changed files in a fresh git init repo with an isolated HOME: clean. A planted fake key was flagged, so the scan was live. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
• Not run: the gosec hook, because it installs gosec, which I can't do on this VM. The merge changes no non-test Go logic, so CI's gosec step covers it.

The PR stays a draft.

🤖 Generated with Claude Code