From 5d34f77f055c7e37476d7c7090411540b66eefb9 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 19 Oct 2026 13:44:48 +0100 Subject: [PATCH 01/11] fix(auth): bind group account scope entries to account IDs A group's allowed_accounts could hold an account's display name. The name was not bound to any account, so once that account was deleted or renamed the entry matched whichever account was later created or renamed to the same name. #1032 limited name matching to unique names, which does not help here: the newcomer is the only account carrying the name. Scope entries now match by account identifier only: - AccountScope.Allows never compares an entry with a display name. A caller that knows an account only by name (legacy rows keyed by external ID) still resolves through WithRegisteredAccounts to the one account carrying that name, and that account's ID must be in the scope. - Group create/update bind allowed_accounts at grant time: "*", account IDs and external IDs are kept; a name exactly one registered account carries, inside the actor's own scope, is stored as that account's ID; anything else is a 400. A failed account or scope load refuses the write. - Migration 000135 rewrites existing name entries to IDs when exactly one account carries the name and it predates the group's last write. Other entries are left in place and no longer grant access. Down is a no-op. - The name-only MatchesAccount and AuthContext.CanAccessAccount helpers, which had no production callers, are removed. Closes #1034 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 + .../api/account_scope_shared_name_test.go | 71 +++-- internal/api/execution_scope_test.go | 24 +- internal/api/grantscoped_test.go | 21 +- internal/api/handler.go | 27 +- internal/api/handler_accounts_test.go | 36 ++- .../handler_analytics_scope_provider_test.go | 17 +- internal/api/handler_dashboard_test.go | 8 +- internal/api/handler_groups.go | 125 ++++++++ .../handler_groups_account_binding_test.go | 266 ++++++++++++++++++ .../handler_history_scope_provider_test.go | 32 +-- internal/api/handler_history_test.go | 2 +- .../handler_inventory_scope_provider_test.go | 10 +- .../api/handler_per_account_perms_test.go | 14 +- internal/api/handler_purchases_revoke_test.go | 20 +- internal/api/handler_ri_exchange.go | 2 +- .../api/handler_ri_utilization_scope_test.go | 8 +- internal/api/openapi.yaml | 17 ++ internal/api/router_handlers_test.go | 7 +- internal/auth/account_scope.go | 103 ++++--- internal/auth/account_scope_test.go | 134 ++++++--- internal/auth/group_ceiling.go | 2 +- internal/auth/service_group_test.go | 111 -------- internal/auth/types.go | 29 -- ...5_bind_group_account_names_to_ids.down.sql | 11 + ...135_bind_group_account_names_to_ids.up.sql | 69 +++++ ...35_bind_group_account_names_to_ids_test.go | 131 +++++++++ 27 files changed, 956 insertions(+), 347 deletions(-) create mode 100644 internal/api/handler_groups_account_binding_test.go create mode 100644 internal/database/postgres/migrations/000135_bind_group_account_names_to_ids.down.sql create mode 100644 internal/database/postgres/migrations/000135_bind_group_account_names_to_ids.up.sql create mode 100644 internal/database/postgres/migrations/000135_bind_group_account_names_to_ids_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 01744a46c..57a861d79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- Account scopes match `allowed_accounts` entries by account ID only. A + display-name entry used to grant whichever account was later created or + renamed to that name. Group writes now store the ID of the one account a + name identifies (and reject names that identify none or several), and + migration 000135 rewrites existing name entries whose account predates the + grant; entries it cannot bind stop granting access (#1034) - Group writes reject an explicit `max_amount: 0` spend cap with a 400 instead of storing it as no cap; omit the field for no limit. API-key creation over HTTP is not covered yet (#980) (#133) - Remove debug console.log from frontend recommendation handler - Align pre-commit gocyclo threshold (10) with CI pipeline diff --git a/internal/api/account_scope_shared_name_test.go b/internal/api/account_scope_shared_name_test.go index 2c88892bf..f1b778f34 100644 --- a/internal/api/account_scope_shared_name_test.go +++ b/internal/api/account_scope_shared_name_test.go @@ -17,9 +17,12 @@ import ( // Issue #433: display names are not unique, so a scope entry that names an // account must not also grant every other account with the same name (in -// another provider or tenant, or one registered later). A name entry matches -// only while exactly one registered account carries it; ID entries are -// unaffected. +// another provider or tenant, or one registered later). +// +// Issue #1034: a name entry is not bound to any account either, so once the +// account it was meant for is deleted or renamed it would grant whichever +// account takes the name next. Scope entries therefore match by account ID +// only; a name entry grants nothing, unique or not. var ( sharedNameAWS = config.CloudAccount{ID: "a1a1a1a1-0000-4000-8000-000000000001", Name: "prod", Provider: "aws", ExternalID: "111", Enabled: true} @@ -120,11 +123,11 @@ func TestHandleRequest_AccountScope_SharedDisplayName(t *testing.T) { }, }, { - name: "unique name still matches", + name: "a unique name entry grants nothing either", scope: []string{"solo"}, - wantList: []string{sharedNameSolo.ID}, + wantList: []string{}, reachable: map[string]bool{ - sharedNameAWS.ID: false, sharedNameAzure.ID: false, sharedNameSolo.ID: true, + sharedNameAWS.ID: false, sharedNameAzure.ID: false, sharedNameSolo.ID: false, }, }, } { @@ -144,34 +147,52 @@ func TestHandleRequest_AccountScope_SharedDisplayName(t *testing.T) { } } -// The issue's "including future ones" case: a name scope that was fine while -// the name was unique must not silently extend to a same-named account -// registered later. It fails closed for both instead. -func TestHandleRequest_AccountScope_LaterSameNamedAccount(t *testing.T) { +// Issue #1034: the account a name entry was meant for is deleted, and a new +// account is registered under the same name. The new account is the only one +// carrying the name, so a name match would grant it; it must not. +func TestHandleRequest_AccountScope_NameReusedAfterDelete(t *testing.T) { t.Parallel() - before := sharedNameHandler(t, []string{"prod"}, []config.CloudAccount{sharedNameAWS, sharedNameSolo}, nil) - assert.Equal(t, 200, sharedNameGET(t, before, "/api/accounts/"+sharedNameAWS.ID).StatusCode) - assert.Equal(t, []string{sharedNameAWS.ID}, listedIDs(t, before)) - - after := sharedNameHandler(t, []string{"prod"}, []config.CloudAccount{sharedNameAWS, sharedNameSolo, sharedNameAzure}, nil) - assert.Equal(t, 404, sharedNameGET(t, after, "/api/accounts/"+sharedNameAzure.ID).StatusCode, - "the newly registered same-named account must not be granted") - assert.Equal(t, 404, sharedNameGET(t, after, "/api/accounts/"+sharedNameAWS.ID).StatusCode, - "the name is ambiguous now, so it grants no account by name") - assert.Empty(t, listedIDs(t, after)) + reuser := sharedNameAzure // registered after sharedNameAWS ("prod") was deleted + h := sharedNameHandler(t, []string{"prod"}, []config.CloudAccount{reuser, sharedNameSolo}, nil) + + assert.Equal(t, 404, sharedNameGET(t, h, "/api/accounts/"+reuser.ID).StatusCode, + "an account that took a deleted account's name must not inherit its grant") + assert.Empty(t, listedIDs(t, h)) } -// A disabled row still carries the name, so it still makes the name ambiguous. +// Issue #1034: the granted account is renamed and another account takes its +// old name. A grant stored as the account's ID stays with the renamed account +// and does not move to the newcomer. +func TestHandleRequest_AccountScope_IDGrantSurvivesRename(t *testing.T) { + t.Parallel() + renamed := sharedNameAWS + renamed.Name = "prod-legacy" + newcomer := sharedNameAzure // now the only account called "prod" + h := sharedNameHandler(t, []string{sharedNameAWS.ID}, []config.CloudAccount{renamed, newcomer}, nil) + + assert.Equal(t, 200, sharedNameGET(t, h, "/api/accounts/"+renamed.ID).StatusCode) + assert.Equal(t, 404, sharedNameGET(t, h, "/api/accounts/"+newcomer.ID).StatusCode) + assert.Equal(t, []string{renamed.ID}, listedIDs(t, h)) +} + +// A row known only by external ID is identified through the name of the +// account carrying it. A disabled row still carries its name, so it still +// makes the name ambiguous (#433) and the row fails closed. func TestHandleRequest_AccountScope_DisabledDuplicateCounts(t *testing.T) { t.Parallel() disabled := sharedNameAzure disabled.Enabled = false - h := sharedNameHandler(t, []string{"prod"}, []config.CloudAccount{sharedNameAWS, disabled}, nil) - assert.Equal(t, 404, sharedNameGET(t, h, "/api/accounts/"+sharedNameAWS.ID).StatusCode) + legacy := []config.PurchaseHistoryRecord{{PurchaseID: "legacy", AccountID: sharedNameAWS.ExternalID, Status: "completed"}} + scope := []string{sharedNameAWS.ID} + + assert.Equal(t, []string{"legacy"}, historyScopeIDs(t, scope, []config.CloudAccount{sharedNameAWS}, legacy), + "control: a unique name identifies the in-scope account") + assert.Empty(t, historyScopeIDs(t, scope, []config.CloudAccount{sharedNameAWS, disabled}, legacy), + "a disabled same-named account makes the name ambiguous") } -// When the account list cannot be loaded, uniqueness cannot be established: -// name entries match nothing, ID entries keep working. +// When the account list cannot be loaded, names cannot be resolved. Name +// entries match nothing in any case, and ID entries keep working. func TestGetAccountScope_AccountListFailureIsIDOnly(t *testing.T) { t.Parallel() registered := []config.CloudAccount{sharedNameSolo} diff --git a/internal/api/execution_scope_test.go b/internal/api/execution_scope_test.go index 488429c2d..50607f08c 100644 --- a/internal/api/execution_scope_test.go +++ b/internal/api/execution_scope_test.go @@ -59,12 +59,12 @@ var scopeTestBothRecs = []config.RecommendationRecord{ var scopeTestKinds = []scopeTestKind{ // Plan-level parent: CloudAccountID nil, fans out to every plan account. - {name: "plan parent", planID: scopeTestPlanID, recs: scopeTestBothRecs, partialScope: []string{scopeTestProd.Name}}, + {name: "plan parent", planID: scopeTestPlanID, recs: scopeTestBothRecs, partialScope: []string{scopeTestProd.ID}}, // Fan-out child of the same plan, buying only in Staging. {name: "fan-out child", planID: scopeTestPlanID, cloudAccountID: &scopeTestStage.ID, - recs: []config.RecommendationRecord{scopeTestRec("rec-stage", &scopeTestStage.ID)}, partialScope: []string{scopeTestProd.Name}}, + recs: []config.RecommendationRecord{scopeTestRec("rec-stage", &scopeTestStage.ID)}, partialScope: []string{scopeTestProd.ID}}, // Ad-hoc web purchase: plan_id NULL, one recommendation per account. - {name: "ad-hoc", recs: scopeTestBothRecs, partialScope: []string{scopeTestProd.Name}}, + {name: "ad-hoc", recs: scopeTestBothRecs, partialScope: []string{scopeTestProd.ID}}, } type scopeTestScope struct { @@ -74,7 +74,7 @@ type scopeTestScope struct { } var scopeTestScopes = []scopeTestScope{ - {name: "all in scope", allow: true, scope: func(scopeTestKind) []string { return []string{scopeTestProd.Name, scopeTestStage.Name} }}, + {name: "all in scope", allow: true, scope: func(scopeTestKind) []string { return []string{scopeTestProd.ID, scopeTestStage.ID} }}, {name: "partial scope", scope: func(k scopeTestKind) []string { return k.partialScope }}, {name: "no scope", scope: func(scopeTestKind) []string { return []string{"Other"} }}, } @@ -199,8 +199,8 @@ func newScopeTestHandler(t *testing.T, exec *config.PurchaseExecution, scope []s } return nil, nil } - // The registered accounts, which getAccountScope loads so that the - // name-scoped sessions below can match by (unique) display name (#433). + // The registered accounts, which getAccountScope loads to resolve + // display names to account IDs (#433, #1034). store.ListCloudAccountsFn = func(context.Context, config.CloudAccountFilter) ([]config.CloudAccount, error) { return []config.CloudAccount{scopeTestProd, scopeTestStage}, nil } @@ -334,7 +334,7 @@ func TestRevoke_AuthorizedNonRevocableStatusIs409(t *testing.T) { scope []string token string }{ - {"session", []string{scopeTestStage.Name}, ""}, + {"session", []string{scopeTestStage.ID}, ""}, {"token", []string{"Other"}, "email-token"}, } { for _, status := range []string{"pending", "notified", "failed"} { @@ -367,7 +367,7 @@ func TestCancel_OutOfScopeSessionWithTokenUsesContactGate(t *testing.T) { scopeTestRec("rec-stage", &scopeTestStage.ID), }}, "pending") mockPurchase := new(MockPurchaseManager) - h := newScopeTestHandler(t, exec, []string{scopeTestProd.Name}, new(MockConfigStore), mockPurchase) + h := newScopeTestHandler(t, exec, []string{scopeTestProd.ID}, new(MockConfigStore), mockPurchase) res, err := h.cancelPurchase(context.Background(), scopeTestRequest(), scopeTestExecID, "email-token") @@ -388,7 +388,7 @@ func TestApprove_OutOfScopeSessionWithTokenUsesContactGate(t *testing.T) { }}, "pending") store := new(MockConfigStore) mockPurchase := new(MockPurchaseManager) - h := newScopeTestHandler(t, exec, []string{scopeTestProd.Name}, store, mockPurchase) + h := newScopeTestHandler(t, exec, []string{scopeTestProd.ID}, store, mockPurchase) mockPurchase.On("ApproveExecution", mock.Anything, scopeTestExecID, "email-token", "operator@example.com").Return("revocation-token", nil) store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil) @@ -410,7 +410,7 @@ func TestExecutionAccounts_AdHocUnattributedDenied(t *testing.T) { scopeTestRec("rec-prod", &scopeTestProd.ID), scopeTestRec("rec-none", missing), }}, "pending") - h := newScopeTestHandler(t, exec, []string{scopeTestProd.Name, scopeTestStage.Name}, new(MockConfigStore), new(MockPurchaseManager)) + h := newScopeTestHandler(t, exec, []string{scopeTestProd.ID, scopeTestStage.ID}, new(MockConfigStore), new(MockPurchaseManager)) err := h.requireExecutionAccess(context.Background(), &Session{UserID: "operator-1"}, scopeTestExecID) assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) } @@ -424,8 +424,8 @@ func TestPlannedPurchases_HidesPartiallyScopedParent(t *testing.T) { scope []string want int }{ - {[]string{scopeTestProd.Name}, 0}, - {[]string{scopeTestProd.Name, scopeTestStage.Name}, 1}, + {[]string{scopeTestProd.ID}, 0}, + {[]string{scopeTestProd.ID, scopeTestStage.ID}, 1}, } { exec := scopeTestExecution(scopeTestKinds[0], "pending") store := new(MockConfigStore) diff --git a/internal/api/grantscoped_test.go b/internal/api/grantscoped_test.go index 5edc80517..4ca763634 100644 --- a/internal/api/grantscoped_test.go +++ b/internal/api/grantscoped_test.go @@ -96,20 +96,21 @@ func TestGrantAdmin_UnrestrictedReachesAnyAccount(t *testing.T) { require.NotNil(t, got) } -// The allow-list matches on display name as well as ID (auth.MatchesAccount), -// so a scoped principal named by account NAME resolves too. Pinned because a -// regression here silently widens or narrows every scoped handler at once. -func TestGrantScoped_MatchesByAccountName(t *testing.T) { +// The allow-list matches by account ID only. A display-name entry is not bound +// to an account, so after a delete or rename it would grant whichever account +// took the name next (issue #1034). It must match nothing, even when the name +// is currently unique. Pinned because a regression here silently widens every +// scoped handler at once. +func TestGrantScoped_NameEntryDoesNotMatch(t *testing.T) { ctx := context.Background() h, mockStore := scopedHandler(t, "prod-account") - mine := &config.CloudAccount{ID: scopedInAccount, Name: "prod-account"} - mockStore.On("GetCloudAccount", ctx, scopedInAccount).Return(mine, nil) - // The name is unique among the registered accounts, so it may match (#433). - mockStore.On("ListCloudAccounts", ctx, config.CloudAccountFilter{}).Return([]config.CloudAccount{*mine}, nil) + later := &config.CloudAccount{ID: scopedInAccount, Name: "prod-account"} + mockStore.On("GetCloudAccount", ctx, scopedInAccount).Return(later, nil) + mockStore.On("ListCloudAccounts", ctx, config.CloudAccountFilter{}).Return([]config.CloudAccount{*later}, nil) got, err := h.requireAccountAccess(ctx, &Session{UserID: scopedUserID}, scopedInAccount) - require.NoError(t, err) - require.NotNil(t, got) + require.ErrorIs(t, err, errNotFound) + require.Nil(t, got) } diff --git a/internal/api/handler.go b/internal/api/handler.go index 5deb120ef..ea0b2dfe2 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -653,9 +653,10 @@ func (h *Handler) requireRIExchangeAutoModeGrant(ctx context.Context, req *event // alongside a zero AccountScope, so a caller that ignores the error still // denies. // -// A restricted scope matches an entry by display name only when exactly one -// registered account carries that name (withRegisteredAccountNames, issue -// #433). Every scoped handler gets its scope here, so this is the one place +// A restricted scope matches its entries by account ID only. A caller that +// knows an account only by display name is resolved to the one registered +// account carrying that name (withRegisteredAccountNames, issues #433 and +// #1034). Every scoped handler gets its scope here, so this is the one place // that has to load the account list. func (h *Handler) getAccountScope(ctx context.Context, session *Session) (auth.AccountScope, error) { if session.UserID == apiKeyAdminUserID { @@ -680,12 +681,14 @@ func (h *Handler) getAccountScope(ctx context.Context, session *Session) (auth.A return h.withRegisteredAccountNames(ctx, auth.ScopeFromLegacyList(resolved)), nil } -// withRegisteredAccountNames lets a restricted scope match entries by display -// name, limited to names that exactly one cloud account carries. Display names -// are not unique, so matching a shared name would grant every account that -// uses it, including ones registered later (issue #433). When the account list -// cannot be loaded the scope stays ID-only: that only narrows access, and ID -// entries keep working. +// withRegisteredAccountNames lets a restricted scope identify an account by +// its display name, limited to names that exactly one cloud account carries. +// The scope must still hold that account's ID: a name never matches a scope +// entry, because a stored name is not bound to an account and would grant +// whichever account takes the name later (issue #1034). Display names are not +// unique either, so a shared name identifies no account (issue #433). When the +// account list cannot be loaded the scope cannot resolve names at all: that +// only narrows access, and callers that pass the account ID are unaffected. func (h *Handler) withRegisteredAccountNames(ctx context.Context, scope auth.AccountScope) auth.AccountScope { if scope.AllowsAll() || len(scope.Accounts) == 0 || h.config == nil { return scope @@ -695,11 +698,11 @@ func (h *Handler) withRegisteredAccountNames(ctx context.Context, scope auth.Acc logging.Warnf("account scope: cannot load cloud accounts, matching scope entries by ID only: %v", err) return scope } - names := make([]string, len(registered)) + known := make([]auth.RegisteredAccount, len(registered)) for i := range registered { - names[i] = registered[i].Name + known[i] = auth.RegisteredAccount{ID: registered[i].ID, Name: registered[i].Name} } - return scope.WithRegisteredNames(names) + return scope.WithRegisteredAccounts(known) } // setSecurityHeaders adds comprehensive security headers to the response. diff --git a/internal/api/handler_accounts_test.go b/internal/api/handler_accounts_test.go index bf6dd62dd..d95b750fb 100644 --- a/internal/api/handler_accounts_test.go +++ b/internal/api/handler_accounts_test.go @@ -214,7 +214,7 @@ func TestListAccountsMinimal_NoViewRecommendations_403(t *testing.T) { func TestListAccountsMinimal_ScopesByAllowedAccounts(t *testing.T) { ctx := context.Background() mockAuth := new(MockAuthService) - setupStandardUserAuth(ctx, mockAuth, "view", "recommendations", true, []string{"Production"}) + setupStandardUserAuth(ctx, mockAuth, "view", "recommendations", true, []string{sampleAccount().ID}) prod := sampleAccount() prod.Name = "Production" @@ -1954,20 +1954,18 @@ func TestGetAccount_OutOfScope_Returns404(t *testing.T) { assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) } -func TestGetAccount_InScope_ByName(t *testing.T) { +func TestGetAccount_InScope_ByID(t *testing.T) { ctx := context.Background() mockAuth := new(MockAuthService) - // allowed_accounts = ["Production"] matches acct.Name - setupScopedAuth(ctx, mockAuth, "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", "view", "accounts", []string{"Production"}) + // allowed_accounts holds acct.ID; entries match by account ID only (#1034). + setupScopedAuth(ctx, mockAuth, "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", "view", "accounts", []string{"11111111-1111-1111-1111-111111111111"}) acct := sampleAccount() acct.Name = "Production" customStore := &mockConfigStoreAccounts{ MockConfigStore: setupAdminMock(ctx), getResult: &acct, - // "Production" is unique among the registered accounts, so it may - // match by name (#433). - listResult: []config.CloudAccount{acct}, + listResult: []config.CloudAccount{acct}, } handler := &Handler{auth: mockAuth, config: customStore} @@ -1977,6 +1975,30 @@ func TestGetAccount_InScope_ByName(t *testing.T) { assert.Equal(t, "Production", got.Name) } +// A display-name entry is not bound to an account, so it must not grant the +// account that currently carries that name, even when the name is unique +// (#1034): that account may have taken the name after the intended one was +// deleted or renamed. +func TestGetAccount_NameEntry_Returns404(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupScopedAuth(ctx, mockAuth, "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", "view", "accounts", []string{"Production"}) + + acct := sampleAccount() + acct.Name = "Production" + customStore := &mockConfigStoreAccounts{ + MockConfigStore: setupAdminMock(ctx), + getResult: &acct, + listResult: []config.CloudAccount{acct}, + } + + handler := &Handler{auth: mockAuth, config: customStore} + result, err := handler.getAccount(ctx, scopedRequest(""), "11111111-1111-1111-1111-111111111111") + assert.Nil(t, result) + require.Error(t, err) + assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) +} + func TestDeleteAccount_OutOfScope_Returns404_NoDelete(t *testing.T) { ctx := context.Background() mockAuth := new(MockAuthService) diff --git a/internal/api/handler_analytics_scope_provider_test.go b/internal/api/handler_analytics_scope_provider_test.go index 7655b752e..8a2c0bdca 100644 --- a/internal/api/handler_analytics_scope_provider_test.go +++ b/internal/api/handler_analytics_scope_provider_test.go @@ -65,7 +65,7 @@ func TestHandleRequest_Analytics_RawExternalIDSharedAcrossProviders(t *testing.T } for order, accounts := range bothOrders { - for _, scope := range [][]string{{"dev"}, {"prod"}, {"123"}} { + for _, scope := range [][]string{{"u-aws"}, {"u-az"}, {"123"}} { t.Run(scope[0]+" scope, "+order, func(t *testing.T) { t.Parallel() status, uuids, externals := analyticsScopeCall(t, scope, "123", accounts) @@ -77,33 +77,34 @@ func TestHandleRequest_Analytics_RawExternalIDSharedAcrossProviders(t *testing.T t.Run("account UUID resolves to its own provider, "+order, func(t *testing.T) { t.Parallel() - status, uuids, externals := analyticsScopeCall(t, []string{"dev"}, "u-aws", accounts) + status, uuids, externals := analyticsScopeCall(t, []string{"u-aws"}, "u-aws", accounts) assert.Equal(t, 200, status) assert.Equal(t, []string{"u-aws"}, uuids) assert.Equal(t, map[string][]string{"aws": {"123"}}, externals) - status, _, _ = analyticsScopeCall(t, []string{"dev"}, "u-az", accounts) + status, _, _ = analyticsScopeCall(t, []string{"u-aws"}, "u-az", accounts) assert.Equal(t, 404, status, "the other provider's account stays out of scope") }) } } // A raw external id that exactly one account owns still works for a scoped -// user, by name or by id, and an out-of-scope account stays hidden. +// user, by account ID or by a raw external-id entry, and an out-of-scope +// account stays hidden. func TestHandleRequest_Analytics_UniqueRawExternalID(t *testing.T) { t.Parallel() in := config.CloudAccount{ID: "u-in", Name: "in", Provider: "aws", ExternalID: "111"} out := config.CloudAccount{ID: "u-out", Name: "out", Provider: "aws", ExternalID: "222"} accounts := []config.CloudAccount{in, out} - status, _, externals := analyticsScopeCall(t, []string{"in"}, "111", accounts) + status, _, externals := analyticsScopeCall(t, []string{"u-in"}, "111", accounts) assert.Equal(t, 200, status) assert.Equal(t, map[string][]string{"": {"111"}}, externals, "legacy raw-id filter shape is unchanged") status, _, _ = analyticsScopeCall(t, []string{"111"}, "111", accounts) assert.Equal(t, 200, status) - status, _, _ = analyticsScopeCall(t, []string{"in"}, "222", accounts) + status, _, _ = analyticsScopeCall(t, []string{"u-in"}, "222", accounts) assert.Equal(t, 404, status) } @@ -113,7 +114,7 @@ func TestHandleRequest_Analytics_SameProviderAmbiguousRawExternalID(t *testing.T a := config.CloudAccount{ID: "u-a", Name: "dev", Provider: "aws", ExternalID: "123"} b := config.CloudAccount{ID: "u-b", Name: "prod", Provider: "aws", ExternalID: "123"} for _, accounts := range [][]config.CloudAccount{{a, b}, {b, a}} { - for _, scope := range []string{"dev", "prod"} { + for _, scope := range []string{"u-a", "u-b"} { status, _, externals := analyticsScopeCall(t, []string{scope}, "123", accounts) assert.Equal(t, 404, status, scope) assert.Nil(t, externals) @@ -142,7 +143,7 @@ func TestHandleRequest_Analytics_UnregisteredRawID(t *testing.T) { status, _, _ := analyticsScopeCall(t, []string{"999"}, "999", accounts) assert.Equal(t, 200, status) - status, _, _ = analyticsScopeCall(t, []string{"solo"}, "999", accounts) + status, _, _ = analyticsScopeCall(t, []string{"u-solo"}, "999", accounts) assert.Equal(t, 404, status) } diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index 58934c380..2c8b5fe00 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -629,8 +629,8 @@ func TestHandler_getUpcomingPurchases_ScopedUser(t *testing.T) { planB.ID: {{ID: "acc-stage", Name: "Staging"}}, }, } - // Registered accounts, loaded by getAccountScope so the unique name - // "Production" can match by name (#433). + // Registered accounts, loaded by getAccountScope to resolve display names + // to account IDs (#433, #1034). mockStore.ListCloudAccountsFn = func(context.Context, config.CloudAccountFilter) ([]config.CloudAccount, error) { return []config.CloudAccount{{ID: "acc-prod", Name: "Production"}, {ID: "acc-stage", Name: "Staging"}}, nil } @@ -639,7 +639,7 @@ func TestHandler_getUpcomingPurchases_ScopedUser(t *testing.T) { UserID: "viewer-1", }, nil) mockAuth.On("HasPermissionAPI", ctx, "viewer-1", "view", "purchases").Return(true, nil) - mockAuth.On("GetAllowedAccountsAPI", ctx, "viewer-1").Return([]string{"Production"}, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, "viewer-1").Return([]string{"acc-prod"}, nil) handler := &Handler{auth: mockAuth, config: store} req := &events.LambdaFunctionURLRequest{ @@ -1748,7 +1748,7 @@ func TestFilterDashboardRecommendations_FreshSlice(t *testing.T) { t.Cleanup(func() { mockAuth.AssertExpectations(t) }) handler := &Handler{auth: mockAuth, config: mockStore} - _ = listCalled // asserted implicitly: if not called the name map is empty and MatchesAccount uses only ID + _ = listCalled // asserted implicitly: if not called the name map is empty and AccountScope.Allows uses only ID scope, err := handler.getAccountScope(ctx, session) require.NoError(t, err) diff --git a/internal/api/handler_groups.go b/internal/api/handler_groups.go index 13d2c2ca7..9d5135a5c 100644 --- a/internal/api/handler_groups.go +++ b/internal/api/handler_groups.go @@ -5,9 +5,11 @@ import ( "context" "encoding/json" "errors" + "fmt" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/aws/aws-lambda-go/events" ) @@ -50,6 +52,11 @@ func (h *Handler) createGroup(ctx context.Context, req *events.LambdaFunctionURL return nil, NewClientError(400, "invalid request body") } + createReq.AllowedAccounts, err = h.bindGroupAccounts(ctx, session, createReq.AllowedAccounts) + if err != nil { + return nil, err + } + group, err := h.auth.CreateGroupAPI(ctx, session.UserID, createReq) if err != nil { return nil, mapGroupAuthError(err) @@ -112,6 +119,11 @@ func (h *Handler) updateGroup(ctx context.Context, req *events.LambdaFunctionURL return nil, NewClientError(400, "invalid request body") } + updateReq.AllowedAccounts, err = h.bindGroupAccounts(ctx, session, updateReq.AllowedAccounts) + if err != nil { + return nil, err + } + group, err := h.auth.UpdateGroupAPI(ctx, session.UserID, groupID, updateReq) if err != nil { return nil, mapGroupAuthError(err) @@ -137,3 +149,116 @@ func (h *Handler) deleteGroup(ctx context.Context, req *events.LambdaFunctionURL return map[string]string{"status": "group deleted"}, nil } + +// groupAccountEntryMsg is the refusal for an allowed_accounts entry that a +// group write cannot bind to an account. It reads the same whether the entry +// is unknown, ambiguous or names an account outside the actor's own scope, so +// the response does not reveal which accounts exist. +const groupAccountEntryMsg = "allowed_accounts entry %q does not identify exactly one cloud account you can access; use the account ID" + +// allAccountsEntry is the allowed_accounts wildcard (auth.IsUnrestrictedAccess). +const allAccountsEntry = "*" + +// bindGroupAccounts binds the allowed_accounts of a group write to stable +// account identifiers before anything is stored (issue #1034). +// +// AccountScope matches entries by account ID only, because a display name is +// not bound to an account: once the account is deleted or renamed, the name +// would grant whichever account takes it next. Clients still send names (the +// group duplicate modal does), so a name is resolved HERE, at grant time, to +// the ID of the one registered account that carries it, and that ID is what +// the group stores. Later renames and reuse of the name then change nothing. +// +// Each entry must be "*", a registered account ID, a registered account's +// external ID (kept verbatim, as before), or a name exactly one registered +// account carries and the acting principal can access. Anything else is +// refused with 400 rather than stored: an unresolvable entry must not be kept +// as a name, and dropping it could turn a restricted list into an empty, +// unrestricted one. nil ("not sent") and an empty list pass through unchanged +// for the ceiling checks in the auth service to judge. +// +// Fails closed: if the account list or the actor's scope cannot be loaded, +// the write is refused. +func (h *Handler) bindGroupAccounts(ctx context.Context, session *Session, entries []string) ([]string, error) { + if !needsAccountBinding(entries) { + return entries, nil + } + if h.config == nil { + return nil, fmt.Errorf("cannot bind allowed_accounts: account store not configured") + } + accounts, err := h.config.ListCloudAccounts(ctx, config.CloudAccountFilter{}) + if err != nil { + return nil, fmt.Errorf("cannot bind allowed_accounts: %w", err) + } + actor, err := h.getAccountScope(ctx, session) + if err != nil { + return nil, fmt.Errorf("cannot bind allowed_accounts: %w", err) + } + dir := newGroupAccountDirectory(accounts) + bound := make([]string, len(entries)) + for i, entry := range entries { + id, ok := dir.bind(entry, actor) + if !ok { + return nil, NewClientError(400, fmt.Sprintf(groupAccountEntryMsg, entry)) + } + bound[i] = id + } + return bound, nil +} + +// needsAccountBinding reports whether entries holds anything besides "*". +func needsAccountBinding(entries []string) bool { + for _, e := range entries { + if e != allAccountsEntry { + return true + } + } + return false +} + +// groupAccountDirectory indexes the registered accounts for bindGroupAccounts. +type groupAccountDirectory struct { + ids map[string]bool + externalIDs map[string]bool + uniqueByName map[string]config.CloudAccount +} + +func newGroupAccountDirectory(accounts []config.CloudAccount) groupAccountDirectory { + d := groupAccountDirectory{ + ids: make(map[string]bool, len(accounts)), + externalIDs: make(map[string]bool, len(accounts)), + uniqueByName: make(map[string]config.CloudAccount, len(accounts)), + } + shared := make(map[string]bool) + for i := range accounts { + a := accounts[i] + d.ids[a.ID] = true + if a.ExternalID != "" { + d.externalIDs[a.ExternalID] = true + } + if a.Name == "" { + continue + } + if _, seen := d.uniqueByName[a.Name]; seen || shared[a.Name] { + shared[a.Name] = true + delete(d.uniqueByName, a.Name) + continue + } + d.uniqueByName[a.Name] = a + } + return d +} + +// bind returns the identifier to store for entry, or false when entry does +// not identify one account the actor can access. Names are compared exactly, +// as AccountScope compares IDs. +func (d groupAccountDirectory) bind(entry string, actor auth.AccountScope) (string, bool) { + if entry == allAccountsEntry || d.ids[entry] || d.externalIDs[entry] { + return entry, true + } + acct, ok := d.uniqueByName[entry] + if !ok || !actor.Allows(acct.ID, acct.Name) { + return "", false + } + return acct.ID, true +} diff --git a/internal/api/handler_groups_account_binding_test.go b/internal/api/handler_groups_account_binding_test.go new file mode 100644 index 000000000..4ecf2db2f --- /dev/null +++ b/internal/api/handler_groups_account_binding_test.go @@ -0,0 +1,266 @@ +package api + +import ( + "context" + "errors" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Issue #1034: group writes bind allowed_accounts to account IDs at grant +// time. AccountScope matches entries by ID only, so a name the client sends +// is resolved here to the one account carrying it, and the ID is stored. A +// later rename or reuse of the name then cannot move the grant. + +const bindingGroupID = "11111111-1111-1111-1111-111111111111" + +var ( + bindProd = config.CloudAccount{ID: "b1b1b1b1-0000-4000-8000-000000000001", Name: "prod", Provider: "aws", ExternalID: "123456789012"} + bindDupA = config.CloudAccount{ID: "b2b2b2b2-0000-4000-8000-000000000002", Name: "shared", Provider: "aws", ExternalID: "210987654321"} + bindDupB = config.CloudAccount{ID: "b3b3b3b3-0000-4000-8000-000000000003", Name: "shared", Provider: "azure", ExternalID: "sub-b"} + bindOther = config.CloudAccount{ID: "b4b4b4b4-0000-4000-8000-000000000004", Name: "other", Provider: "gcp", ExternalID: "proj-other"} + bindDupC = config.CloudAccount{ID: "b5b5b5b5-0000-4000-8000-000000000005", Name: "shared", Provider: "gcp", ExternalID: "proj-c"} +) + +// bindingHandler wires an admin session (restricted to scope when scope is +// non-nil) against a store holding accounts. listErr makes ListCloudAccounts +// fail. listCalls counts ListCloudAccounts calls. +func bindingHandler(t *testing.T, scope []string, accounts []config.CloudAccount, listErr error) (*Handler, *MockAuthService, *int) { + t.Helper() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "admin-token"). + Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"}, nil) + if scope == nil { + mockAuth.grantAdmin() + } else { + mockAuth.grantScoped(scope...) + } + listCalls := 0 + store := new(MockConfigStore) + store.ListCloudAccountsFn = func(context.Context, config.CloudAccountFilter) ([]config.CloudAccount, error) { + listCalls++ + if listErr != nil { + return nil, listErr + } + return append([]config.CloudAccount(nil), accounts...), nil + } + return &Handler{auth: mockAuth, config: store}, mockAuth, &listCalls +} + +func bindingRequest(body string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: body, + } +} + +// expectCreate records the allowed_accounts CreateGroupAPI receives. +func expectCreate(m *MockAuthService, got *[]string) { + m.On("CreateGroupAPI", mock.Anything, mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + *got = args.Get(2).(auth.APICreateGroupRequest).AllowedAccounts + }). + Return(&auth.APIGroup{ID: bindingGroupID}, nil).Once() +} + +// expectUpdate records the allowed_accounts UpdateGroupAPI receives. +func expectUpdate(m *MockAuthService, got *[]string) { + m.On("UpdateGroupAPI", mock.Anything, mock.Anything, bindingGroupID, mock.Anything). + Run(func(args mock.Arguments) { + *got = args.Get(3).(auth.APIUpdateGroupRequest).AllowedAccounts + }). + Return(&auth.APIGroup{ID: bindingGroupID}, nil).Once() +} + +func TestCreateGroup_BindsAccountNamesToIDs(t *testing.T) { + t.Parallel() + all := []config.CloudAccount{bindProd, bindDupA, bindDupB, bindOther} + for _, tc := range []struct { + name string + body string + want []string + }{ + {"a unique name is stored as its account's ID", `{"name":"g","allowed_accounts":["prod"]}`, []string{bindProd.ID}}, + {"an ID is kept", `{"name":"g","allowed_accounts":["` + bindDupA.ID + `"]}`, []string{bindDupA.ID}}, + {"an external ID is kept verbatim", `{"name":"g","allowed_accounts":["123456789012"]}`, []string{"123456789012"}}, + {"order and mixed forms are preserved", `{"name":"g","allowed_accounts":["` + bindOther.ID + `","prod"]}`, []string{bindOther.ID, bindProd.ID}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + h, mockAuth, _ := bindingHandler(t, nil, all, nil) + var got []string + expectCreate(mockAuth, &got) + + _, err := h.createGroup(context.Background(), bindingRequest(tc.body)) + + require.NoError(t, err) + assert.Equal(t, tc.want, got) + mockAuth.AssertExpectations(t) + }) + } +} + +// An entry that does not identify exactly one account is refused, not stored +// as a name: stored, it would grant whichever account takes the name later. +func TestCreateGroup_RefusesUnbindableAccountEntries(t *testing.T) { + t.Parallel() + // Three accounts share "shared", so a third copy cannot make it unique again. + all := []config.CloudAccount{bindProd, bindDupA, bindDupB, bindOther, bindDupC} + for _, tc := range []struct { + name string + scope []string + entry string + }{ + {"unknown name", nil, "deleted-account"}, + {"name shared by two accounts", nil, "shared"}, + {"empty entry", nil, ""}, + {"name of an account outside the actor's scope", []string{bindProd.ID}, "other"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + h, mockAuth, _ := bindingHandler(t, tc.scope, all, nil) + + _, err := h.createGroup(context.Background(), + bindingRequest(`{"name":"g","allowed_accounts":["`+bindProd.ID+`","`+tc.entry+`"]}`)) + + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "want a 400 client error, got %T: %v", err, err) + assert.Equal(t, 400, ce.code) + assert.Contains(t, err.Error(), "does not identify exactly one cloud account you can access") + mockAuth.AssertNumberOfCalls(t, "CreateGroupAPI", 0) + }) + } +} + +// A scoped actor can bind a name only to an account inside their own scope. +func TestCreateGroup_ScopedActorBindsInScopeName(t *testing.T) { + t.Parallel() + h, mockAuth, _ := bindingHandler(t, []string{bindProd.ID}, []config.CloudAccount{bindProd, bindOther}, nil) + var got []string + expectCreate(mockAuth, &got) + + _, err := h.createGroup(context.Background(), bindingRequest(`{"name":"g","allowed_accounts":["prod"]}`)) + + require.NoError(t, err) + assert.Equal(t, []string{bindProd.ID}, got) +} + +// The account list is needed to bind; without it the write fails closed. +func TestCreateGroup_AccountListFailureRefusesWrite(t *testing.T) { + t.Parallel() + h, mockAuth, _ := bindingHandler(t, nil, nil, errors.New("db down")) + + _, err := h.createGroup(context.Background(), bindingRequest(`{"name":"g","allowed_accounts":["prod"]}`)) + + require.Error(t, err) + _, isClient := IsClientError(err) + assert.False(t, isClient, "a store failure is a server error") + mockAuth.AssertNumberOfCalls(t, "CreateGroupAPI", 0) +} + +func TestCreateGroup_NoAccountStoreRefusesWrite(t *testing.T) { + t.Parallel() + h, mockAuth, _ := bindingHandler(t, nil, nil, nil) + h.config = nil + + _, err := h.createGroup(context.Background(), bindingRequest(`{"name":"g","allowed_accounts":["prod"]}`)) + + require.Error(t, err) + mockAuth.AssertNumberOfCalls(t, "CreateGroupAPI", 0) +} + +// Nothing to bind: omitted, empty and wildcard-only lists pass through +// untouched (the auth service's ceiling judges them) without loading accounts. +func TestCreateGroup_NothingToBindPassesThrough(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + body string + want []string + }{ + {"omitted", `{"name":"g"}`, nil}, + {"empty", `{"name":"g","allowed_accounts":[]}`, []string{}}, + {"wildcard", `{"name":"g","allowed_accounts":["*"]}`, []string{"*"}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + h, mockAuth, listCalls := bindingHandler(t, nil, nil, errors.New("must not be called")) + var got []string + expectCreate(mockAuth, &got) + + _, err := h.createGroup(context.Background(), bindingRequest(tc.body)) + + require.NoError(t, err) + assert.Equal(t, tc.want, got) + assert.Zero(t, *listCalls) + }) + } +} + +func TestUpdateGroup_BindsAccountNamesToIDs(t *testing.T) { + t.Parallel() + h, mockAuth, _ := bindingHandler(t, nil, []config.CloudAccount{bindProd, bindOther}, nil) + var got []string + expectUpdate(mockAuth, &got) + + _, err := h.updateGroup(context.Background(), bindingRequest(`{"allowed_accounts":["prod","`+bindOther.ID+`"]}`), bindingGroupID) + + require.NoError(t, err) + assert.Equal(t, []string{bindProd.ID, bindOther.ID}, got) +} + +func TestUpdateGroup_RefusesUnbindableAccountEntry(t *testing.T) { + t.Parallel() + h, mockAuth, _ := bindingHandler(t, nil, []config.CloudAccount{bindDupA, bindDupB}, nil) + + _, err := h.updateGroup(context.Background(), bindingRequest(`{"allowed_accounts":["shared"]}`), bindingGroupID) + + ce, ok := IsClientError(err) + require.True(t, ok, "want a 400 client error, got %T: %v", err, err) + assert.Equal(t, 400, ce.code) + mockAuth.AssertNumberOfCalls(t, "UpdateGroupAPI", 0) +} + +// "not sent" stays nil, so the update leaves the group's scope alone. +func TestUpdateGroup_OmittedAccountsStayNil(t *testing.T) { + t.Parallel() + h, mockAuth, listCalls := bindingHandler(t, nil, nil, errors.New("must not be called")) + got := []string{"sentinel"} + expectUpdate(mockAuth, &got) + + _, err := h.updateGroup(context.Background(), bindingRequest(`{"description":"d"}`), bindingGroupID) + + require.NoError(t, err) + assert.Nil(t, got) + assert.Zero(t, *listCalls) +} + +// End to end for the issue: a group created by naming "prod" keeps reaching +// that account after it is renamed, and does not reach the account that +// later takes the name "prod". +func TestGroupGrantByName_SurvivesRenameAndNameReuse(t *testing.T) { + t.Parallel() + h, mockAuth, _ := bindingHandler(t, nil, []config.CloudAccount{bindProd, bindOther}, nil) + var stored []string + expectCreate(mockAuth, &stored) + _, err := h.createGroup(context.Background(), bindingRequest(`{"name":"g","allowed_accounts":["prod"]}`)) + require.NoError(t, err) + + renamed := bindProd + renamed.Name = "prod-legacy" + newcomer := bindOther + newcomer.Name = "prod" + scoped := sharedNameHandler(t, stored, []config.CloudAccount{renamed, newcomer}, nil) + + assert.Equal(t, 200, sharedNameGET(t, scoped, "/api/accounts/"+renamed.ID).StatusCode) + assert.Equal(t, 404, sharedNameGET(t, scoped, "/api/accounts/"+newcomer.ID).StatusCode, + "the account that took the name must not inherit the grant") +} diff --git a/internal/api/handler_history_scope_provider_test.go b/internal/api/handler_history_scope_provider_test.go index 8ed8d6681..569dc0546 100644 --- a/internal/api/handler_history_scope_provider_test.go +++ b/internal/api/handler_history_scope_provider_test.go @@ -52,7 +52,7 @@ func historyScopeIDs(t *testing.T, scope []string, accounts []config.CloudAccoun } // Issue #550: external ids are unique per (provider, external_id) only. A -// name-scoped History user must see rows of the account in their scope and +// History user scoped to an account must see rows of the account in their scope and // none of the same-external-id account under another provider, whatever the // order the accounts are listed in. func TestHandleRequest_History_SameExternalIDAcrossProviders(t *testing.T) { @@ -70,31 +70,31 @@ func TestHandleRequest_History_SameExternalIDAcrossProviders(t *testing.T) { for order, accounts := range bothOrders { t.Run("scope names azure account, "+order, func(t *testing.T) { t.Parallel() - assert.Equal(t, []string{"az-row"}, historyScopeIDs(t, []string{"prod"}, accounts, rows)) + assert.Equal(t, []string{"az-row"}, historyScopeIDs(t, []string{"u-az"}, accounts, rows)) }) t.Run("scope names aws account, "+order, func(t *testing.T) { t.Parallel() - assert.Equal(t, []string{"aws-row"}, historyScopeIDs(t, []string{"dev"}, accounts, rows)) + assert.Equal(t, []string{"aws-row"}, historyScopeIDs(t, []string{"u-aws"}, accounts, rows)) }) } t.Run("legacy row without provider on an ambiguous external id is not name-matched", func(t *testing.T) { t.Parallel() legacy := []config.PurchaseHistoryRecord{{PurchaseID: "legacy", AccountID: "123", Status: "completed"}} - assert.Empty(t, historyScopeIDs(t, []string{"prod"}, []config.CloudAccount{awsDev, azProd}, legacy)) + assert.Empty(t, historyScopeIDs(t, []string{"u-az"}, []config.CloudAccount{awsDev, azProd}, legacy)) }) t.Run("a set cloud account id stays authoritative", func(t *testing.T) { t.Parallel() uuid := "u-az" row := []config.PurchaseHistoryRecord{{PurchaseID: "by-uuid", Provider: "aws", AccountID: "123", CloudAccountID: &uuid, Status: "completed"}} - assert.Equal(t, []string{"by-uuid"}, historyScopeIDs(t, []string{"prod"}, []config.CloudAccount{awsDev, azProd}, row)) - assert.Empty(t, historyScopeIDs(t, []string{"dev"}, []config.CloudAccount{awsDev, azProd}, row)) + assert.Equal(t, []string{"by-uuid"}, historyScopeIDs(t, []string{"u-az"}, []config.CloudAccount{awsDev, azProd}, row)) + assert.Empty(t, historyScopeIDs(t, []string{"u-aws"}, []config.CloudAccount{awsDev, azProd}, row)) }) } -// Unambiguous accounts keep matching by name or id, with and without a -// provider on the row. +// Unambiguous accounts keep matching by account ID or by a raw external-id +// entry, with and without a provider on the row. func TestHandleRequest_History_UniqueExternalIDStillMatches(t *testing.T) { t.Parallel() in := config.CloudAccount{ID: "u-in", Name: "in", Provider: "aws", ExternalID: "111"} @@ -104,14 +104,14 @@ func TestHandleRequest_History_UniqueExternalIDStillMatches(t *testing.T) { {PurchaseID: "in-legacy", AccountID: "111", Status: "completed"}, {PurchaseID: "out", Provider: "aws", AccountID: "222", Status: "completed"}, } - got := historyScopeIDs(t, []string{"in"}, []config.CloudAccount{in, out}, rows) + got := historyScopeIDs(t, []string{"u-in"}, []config.CloudAccount{in, out}, rows) assert.ElementsMatch(t, []string{"in-prov", "in-legacy"}, got) got = historyScopeIDs(t, []string{"111"}, []config.CloudAccount{in, out}, rows) assert.ElementsMatch(t, []string{"in-prov", "in-legacy"}, got) } // In-flight history rows carry the account UUID in AccountID with no -// CloudAccountID, so a name-scoped user must still see their own rows. +// CloudAccountID, so a scoped user must still see their own rows. func TestHandleRequest_History_UUIDInAccountID(t *testing.T) { t.Parallel() dev := config.CloudAccount{ID: "u-aws", Name: "dev", Provider: "aws", ExternalID: "123"} @@ -121,25 +121,25 @@ func TestHandleRequest_History_UUIDInAccountID(t *testing.T) { t.Run("pending row with provider", func(t *testing.T) { t.Parallel() rows := []config.PurchaseHistoryRecord{{PurchaseID: "p1", Provider: "aws", AccountID: "u-aws", Status: "pending"}} - assert.Equal(t, []string{"p1"}, historyScopeIDs(t, []string{"dev"}, accounts, rows)) - assert.Empty(t, historyScopeIDs(t, []string{"prod"}, accounts, rows)) + assert.Equal(t, []string{"p1"}, historyScopeIDs(t, []string{"u-aws"}, accounts, rows)) + assert.Empty(t, historyScopeIDs(t, []string{"u-az"}, accounts, rows)) }) t.Run("pending row without provider", func(t *testing.T) { t.Parallel() rows := []config.PurchaseHistoryRecord{{PurchaseID: "p2", AccountID: "u-aws", Status: "pending"}} - assert.Equal(t, []string{"p2"}, historyScopeIDs(t, []string{"dev"}, accounts, rows)) - assert.Empty(t, historyScopeIDs(t, []string{"prod"}, accounts, rows)) + assert.Equal(t, []string{"p2"}, historyScopeIDs(t, []string{"u-aws"}, accounts, rows)) + assert.Empty(t, historyScopeIDs(t, []string{"u-az"}, accounts, rows)) }) } // Two accounts of one provider sharing an external id are ambiguous: the row -// is hidden from both name scopes. +// is hidden from both account scopes. func TestHandleRequest_History_SameProviderAmbiguousExternalID(t *testing.T) { t.Parallel() a := config.CloudAccount{ID: "u-a", Name: "dev", Provider: "aws", ExternalID: "123"} b := config.CloudAccount{ID: "u-b", Name: "prod", Provider: "aws", ExternalID: "123"} rows := []config.PurchaseHistoryRecord{{PurchaseID: "amb", Provider: "aws", AccountID: "123", Status: "completed"}} - for _, scope := range []string{"dev", "prod"} { + for _, scope := range []string{"u-a", "u-b"} { assert.Empty(t, historyScopeIDs(t, []string{scope}, []config.CloudAccount{a, b}, rows), scope) assert.Empty(t, historyScopeIDs(t, []string{scope}, []config.CloudAccount{b, a}, rows), scope) } diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index 617477408..ae9c228ef 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -509,7 +509,7 @@ func TestHandler_getHistory_ScopedUserSeesEmptyAccountRows(t *testing.T) { mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{ambientExec}, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) // resolveAccountNamesByID must not error — return an empty accounts list - // so MatchesAccount falls back to ID-only matching (safe for our assertions). + // so AccountScope.Allows falls back to ID-only matching (safe for our assertions). mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return nil, nil } diff --git a/internal/api/handler_inventory_scope_provider_test.go b/internal/api/handler_inventory_scope_provider_test.go index cb005663d..02bc9c0b0 100644 --- a/internal/api/handler_inventory_scope_provider_test.go +++ b/internal/api/handler_inventory_scope_provider_test.go @@ -66,7 +66,7 @@ func activeRow(purchaseID, provider, accountID string) config.PurchaseHistoryRec // Issue #520: external ids are unique per (provider, external_id) only. The // inventory account name was keyed by external id alone, so with two accounts -// sharing an id the name on a row (and the name-scope decision made with it) +// sharing an id the name on a row (and the scope decision made with it) // depended on the order the accounts were listed in. func TestHandleRequest_Inventory_SameExternalIDAcrossProviders(t *testing.T) { t.Parallel() @@ -81,12 +81,12 @@ func TestHandleRequest_Inventory_SameExternalIDAcrossProviders(t *testing.T) { for order, accounts := range bothOrders { t.Run("scope names aws account, "+order, func(t *testing.T) { t.Parallel() - got := inventoryNames(t, []string{"dev"}, accounts, rows) + got := inventoryNames(t, []string{"u-aws"}, accounts, rows) assert.Equal(t, map[string]string{"123:aws-row": "dev"}, got) }) t.Run("scope names azure account, "+order, func(t *testing.T) { t.Parallel() - got := inventoryNames(t, []string{"prod"}, accounts, rows) + got := inventoryNames(t, []string{"u-az"}, accounts, rows) assert.Equal(t, map[string]string{"123:az-row": "prod"}, got) }) t.Run("unrestricted sees each row under its own account name, "+order, func(t *testing.T) { @@ -123,7 +123,7 @@ func TestHandleRequest_Inventory_ResolutionOrder(t *testing.T) { } // Two accounts of one provider sharing an external id are ambiguous: the row -// is attributed to neither, so a name-scoped user does not see it and an +// is attributed to neither, so a scoped user does not see it and an // unrestricted user sees it without a name. func TestHandleRequest_Inventory_SameProviderAmbiguousExternalID(t *testing.T) { t.Parallel() @@ -131,7 +131,7 @@ func TestHandleRequest_Inventory_SameProviderAmbiguousExternalID(t *testing.T) { b := config.CloudAccount{ID: "u-b", Name: "prod", Provider: "aws", ExternalID: "123"} rows := []config.PurchaseHistoryRecord{activeRow("amb", "aws", "123")} for _, accounts := range [][]config.CloudAccount{{a, b}, {b, a}} { - for _, scope := range []string{"dev", "prod"} { + for _, scope := range []string{"u-a", "u-b"} { assert.Empty(t, inventoryNames(t, []string{scope}, accounts, rows), scope) } assert.Equal(t, map[string]string{"123:amb": ""}, inventoryNames(t, nil, accounts, rows)) diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index 9979d639d..7b6d07afe 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -93,7 +93,7 @@ func scopedReq() *events.LambdaFunctionURLRequest { // permsAccountList returns a ListCloudAccountsFn-compatible slice containing // both test accounts — used so resolveAccountNamesByID can map UUID→name for -// the account-name branch of MatchesAccount. +// the display-name lookup of AccountScope.Allows. func permsAccountList() []config.CloudAccount { return []config.CloudAccount{ {ID: permsAccA, Name: permsAccAName, Provider: "aws"}, @@ -304,7 +304,7 @@ func TestPerAccountPerms_History_ListFilter(t *testing.T) { Return([]config.PurchaseHistoryRecord{rowA, rowB}, nil) mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything). Return([]config.PurchaseExecution{}, nil) - // resolveAccountNamesByID needs ListCloudAccounts so MatchesAccount can + // resolveAccountNamesByID needs ListCloudAccounts so AccountScope.Allows can // resolve display names when the AccountID differs from the UUID. mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return permsAccountList(), nil @@ -334,10 +334,10 @@ func TestPerAccountPerms_History_ListFilter(t *testing.T) { // TestPerAccountPerms_HistoryAnalytics_CrossAccountRejected verifies that a // scoped user (allowed_accounts: [permsAccA]) gets 404 when they request -// analytics for account B. validateAnalyticsAccountScope calls MatchesAccount +// analytics for account B. validateAnalyticsAccountScope calls AccountScope.Allows // and returns errNotFound on mismatch. // -// Regression: removing validateAnalyticsAccountScope (or its MatchesAccount +// Regression: removing validateAnalyticsAccountScope (or its AccountScope.Allows // call) allows the analytics query to proceed and returns 200 to the caller. func TestPerAccountPerms_HistoryAnalytics_CrossAccountRejected(t *testing.T) { ctx := context.Background() @@ -593,7 +593,7 @@ func TestPerAccountPerms_DashboardSummary_CommitmentMetricsExcludeOtherAccounts( // recommendation tagged to account B gets 403 from validatePurchaseRecommendationScope. // // Regression: removing validatePurchaseRecommendationScope from executePurchase -// (or its inner MatchesAccount call) allows the purchase to proceed and the +// (or its inner AccountScope.Allows call) allows the purchase to proceed and the // assertions below fail. func TestPerAccountPerms_ExecutePurchase_CrossAccountRejected403(t *testing.T) { ctx := context.Background() @@ -1114,7 +1114,7 @@ func TestPerAccountPerms_PlannedPurchase_AllowedAccountPlanSucceeds(t *testing.T // permsAccB (outside their scope). // // The handler resolves the deployment CloudAccount ID via reshapeAccountResolver -// and checks it against the session's allowed_accounts with auth.MatchesAccount. +// and checks it against the session's allowed_accounts with auth.AccountScope.Allows. // When they don't match, no AWS SDK call is made and an empty list is returned. // // Regression: removing the getAllowedAccounts guard from listConvertibleRIs @@ -1124,7 +1124,7 @@ func TestPerAccountPerms_ListConvertibleRIs_ScopedOutsideAllowedReturnsEmpty(t * ctx := context.Background() mockStore := new(MockConfigStore) - // resolveAccountNamesByID calls ListCloudAccounts so MatchesAccount can + // resolveAccountNamesByID calls ListCloudAccounts so AccountScope.Allows can // resolve display names. Return both accounts so the name lookup is realistic. mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return permsAccountList(), nil diff --git a/internal/api/handler_purchases_revoke_test.go b/internal/api/handler_purchases_revoke_test.go index a88254cfe..7532e406c 100644 --- a/internal/api/handler_purchases_revoke_test.go +++ b/internal/api/handler_purchases_revoke_test.go @@ -487,9 +487,11 @@ func TestAuthorizeSessionRevoke_RevokeAny_AccountScope(t *testing.T) { {name: "empty account", scope: []string{inScope}, accountID: &empty, wantErr: "cannot verify ownership"}, // Issue #534: History shows these rows, so revoke must accept them. {name: "external id only, scoped by external id", scope: []string{"111122223333"}, external: "111122223333"}, - {name: "external id only, scoped by name", scope: []string{"prod"}, external: "111122223333"}, - {name: "uuid row, scoped by name", scope: []string{"prod"}, accountID: &inScope, external: "111122223333"}, - {name: "external id only, other account", scope: []string{"dev"}, external: "999999999999", wantErr: "account you do not have access to"}, + {name: "external id only, scoped by account id", scope: []string{inScope}, external: "111122223333"}, + // Issue #1034: a display-name entry is not bound to an account. + {name: "external id only, name entry", scope: []string{"prod"}, external: "111122223333", wantErr: "account you do not have access to"}, + {name: "uuid row, name entry", scope: []string{"prod"}, accountID: &inScope, external: "111122223333", wantErr: "account you do not have access to"}, + {name: "external id only, other account", scope: []string{outOfScope}, external: "999999999999", wantErr: "account you do not have access to"}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -544,19 +546,19 @@ func TestAuthorizeSessionRevoke_RevokeAny_ProviderAndUUIDAuthority(t *testing.T) rec config.PurchaseHistoryRecord wantErr string }{ - {name: "same external id, scope names other provider account, aws first", scope: []string{"prod"}, + {name: "same external id, scope holds other provider account, aws first", scope: []string{"u-az"}, accounts: []config.CloudAccount{awsDev, azProd}, rec: config.PurchaseHistoryRecord{Provider: "aws", AccountID: "123"}, wantErr: denied}, - {name: "same external id, scope names other provider account, azure first", scope: []string{"prod"}, + {name: "same external id, scope holds other provider account, azure first", scope: []string{"u-az"}, accounts: []config.CloudAccount{azProd, awsDev}, rec: config.PurchaseHistoryRecord{Provider: "aws", AccountID: "123"}, wantErr: denied}, - {name: "same external id, scope names own provider account, aws first", scope: []string{"dev"}, + {name: "same external id, scope holds own provider account, aws first", scope: []string{"u-aws"}, accounts: []config.CloudAccount{awsDev, azProd}, rec: config.PurchaseHistoryRecord{Provider: "aws", AccountID: "123"}}, - {name: "same external id, scope names own provider account, azure first", scope: []string{"dev"}, + {name: "same external id, scope holds own provider account, azure first", scope: []string{"u-aws"}, accounts: []config.CloudAccount{azProd, awsDev}, rec: config.PurchaseHistoryRecord{Provider: "aws", AccountID: "123"}}, - {name: "uuid out of scope, in-scope external id", scope: []string{"in"}, + {name: "uuid out of scope, in-scope external id", scope: []string{"u-in"}, accounts: []config.CloudAccount{in, out}, rec: config.PurchaseHistoryRecord{CloudAccountID: str("u-out"), AccountID: "111"}, wantErr: denied}, {name: "uuid in scope, out-of-scope external id", scope: []string{"u-in"}, accounts: []config.CloudAccount{in, out}, rec: config.PurchaseHistoryRecord{CloudAccountID: str("u-in"), AccountID: "222"}}, - {name: "resolver error, name scope", scope: []string{"prod"}, listErr: errors.New("db down"), + {name: "resolver error", scope: []string{"u-az"}, listErr: errors.New("db down"), rec: config.PurchaseHistoryRecord{AccountID: "123"}, wantErr: denied}, } for _, tc := range cases { diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 89a961297..49bb57873 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2224,7 +2224,7 @@ func (h *Handler) getRIExchangeHistory(ctx context.Context, req *events.LambdaFu // RI exchange records carry the 12-digit AWS account id from STS, so // resolve it to its single registered AWS account and match the scope by - // that account's UUID or name. An unregistered or ambiguous id matches + // that account's UUID. An unregistered or ambiguous id matches // only a literal entry in the allow-list. A lookup failure errors out. allowed, err := h.getAccountScope(ctx, session) if err != nil { diff --git a/internal/api/handler_ri_utilization_scope_test.go b/internal/api/handler_ri_utilization_scope_test.go index b0a858c9d..19abd7731 100644 --- a/internal/api/handler_ri_utilization_scope_test.go +++ b/internal/api/handler_ri_utilization_scope_test.go @@ -119,9 +119,9 @@ func TestGetRIUtilization_ScopedUserDoesNotSeeLinkedMemberRIs(t *testing.T) { assert.Equal(t, 1, ec2.calls, "the deployment account's own RIs are listed exactly once") } -// The scope may name the account instead of listing its UUID; the filter -// applies the same way. -func TestGetRIUtilization_ScopeByAccountNameStillFiltersMembers(t *testing.T) { +// A scope entry that names the account instead of listing its UUID is not +// bound to that account, so it reaches none of its rows (#1034). +func TestGetRIUtilization_ScopeByAccountNameReachesNothing(t *testing.T) { ctx := context.Background() auth := new(MockAuthService) auth.On("ValidateSession", ctx, permsScopedToken).Return(&Session{UserID: permsScopedUserID}, nil) @@ -134,7 +134,7 @@ func TestGetRIUtilization_ScopeByAccountNameStillFiltersMembers(t *testing.T) { resp, err := h.getRIUtilization(ctx, scopedReq()) require.NoError(t, err) - assert.ElementsMatch(t, []string{utilOwnRI, utilStaleOwn}, utilIDs(t, resp)) + assert.Empty(t, utilIDs(t, resp)) } // Unrestricted users keep every row and trigger no EC2 listing. diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 5ad33ce0a..004613b6c 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2711,6 +2711,11 @@ components: $ref: '#/components/schemas/Permission' allowed_accounts: type: array + description: > + Cloud accounts the group may reach: account IDs (or provider + external IDs), or "*" for all accounts. Entries match by ID only; + a display name stored before #1034 that no longer identifies its + account matches nothing. items: type: string created_at: @@ -2734,6 +2739,12 @@ components: $ref: '#/components/schemas/Permission' allowed_accounts: type: array + description: > + Account IDs, provider external IDs, or "*". A display name is + accepted only when exactly one registered account that the caller + can access carries it, and is stored as that account's ID, so the + grant stays with the account after a rename. Any other entry is + rejected with 400. items: type: string @@ -2750,6 +2761,12 @@ components: $ref: '#/components/schemas/Permission' allowed_accounts: type: array + description: > + Account IDs, provider external IDs, or "*". A display name is + accepted only when exactly one registered account that the caller + can access carries it, and is stored as that account's ID, so the + grant stays with the account after a rename. Any other entry is + rejected with 400. items: type: string diff --git a/internal/api/router_handlers_test.go b/internal/api/router_handlers_test.go index 243ea4539..620252488 100644 --- a/internal/api/router_handlers_test.go +++ b/internal/api/router_handlers_test.go @@ -997,8 +997,9 @@ func TestRouter_rejectRIExchangeHandler_RateLimited(t *testing.T) { } // RI exchange records carry the 12-digit STS account id. A scoped user must -// see a deployment's own exchanges whether the allow-list names the account by -// UUID, by name or by that id, and never another account's (issue #541). +// see a deployment's own exchanges whether the allow-list holds the account's +// UUID or that id, and never another account's (issue #541). A display-name +// entry is not bound to an account and matches nothing (issue #1034). func TestHandler_getRIExchangeHistory_AccountScope(t *testing.T) { t.Parallel() const stsAccount = "111122223333" @@ -1014,7 +1015,7 @@ func TestHandler_getRIExchangeHistory_AccountScope(t *testing.T) { }{ {name: "unrestricted sees all", scope: nil, want: 1}, {name: "scoped by account uuid", scope: []string{"u-prod"}, want: 1}, - {name: "scoped by account name", scope: []string{"prod"}, want: 1}, + {name: "a name entry matches nothing", scope: []string{"prod"}, want: 0}, {name: "scoped by sts account id", scope: []string{stsAccount}, want: 1}, {name: "scoped to a different account", scope: []string{"u-dev"}, want: 0}, {name: "scoped to a different account by name", scope: []string{"dev"}, want: 0}, diff --git a/internal/auth/account_scope.go b/internal/auth/account_scope.go index f3fb2a4c7..1f30d728d 100644 --- a/internal/auth/account_scope.go +++ b/internal/auth/account_scope.go @@ -24,24 +24,33 @@ import "strings" // admin API key, or a group carrying the "*" wildcard. type AccountScope struct { // Accounts is the explicit allow-list, meaningful only when - // Unrestricted is false. Entries match by cloud account ID, or by - // display name when that name belongs to exactly one registered account - // (see WithRegisteredNames). + // Unrestricted is false. Entries are account identifiers (the cloud + // account ID, or a provider external ID) and are compared with the + // identifier the caller passes, never with a display name (issue #1034). Accounts []string - // uniqueNames holds the display names carried by exactly one registered - // cloud account. Allows matches an entry by name only when the name is in - // this set, so a name shared by two accounts grants neither of them by - // name (issue #433). nil (the zero value, and every scope built without - // WithRegisteredNames) matches by ID only: name matching is opt-in, so + // idByUniqueName maps each display name carried by exactly one registered + // cloud account to that account's ID. It lets a caller that knows an + // account only by name (for example a legacy row keyed by external ID) + // identify it: Allows resolves the name to the account carrying it and + // then checks that account's ID against Accounts. A name shared by two + // accounts resolves to neither (issue #433). nil (the zero value, and every + // scope built without WithRegisteredAccounts) disables the lookup, so // forgetting to populate it fails closed. - uniqueNames map[string]struct{} + idByUniqueName map[string]string // Unrestricted grants access to every account. Set it only where // unlimited access has been positively established. Unrestricted bool } +// RegisteredAccount is the identity of one registered cloud account, as +// WithRegisteredAccounts needs it. +type RegisteredAccount struct { + ID string + Name string +} + // UnrestrictedScope returns a scope granting every account. Use it only at a // site that has positively established unlimited access; never as a fallback // or a default. @@ -58,7 +67,7 @@ func UnrestrictedScope() AccountScope { // - An empty list grants NOTHING here; under the legacy convention empty // meant "all accounts". // - "*" is a literal identifier here, NOT a wildcard. ScopeForAccounts(["*"]) -// matches an account whose ID or name is the single character "*" and +// matches an account whose ID is the single character "*" and // denies everything else, where ScopeFromLegacyList(["*"]) is unrestricted. // // The second matters for the obvious future call. Passing a group's stored @@ -105,40 +114,49 @@ func ScopeFromLegacyList(accounts []string) AccountScope { return ScopeForAccounts(accounts) } -// WithRegisteredNames returns a copy of the scope that may match entries by -// display name. names must hold the display name of EVERY registered cloud -// account (all providers, enabled or not), one element per account, so that -// duplicates are visible. Only names that occur exactly once become matchable. +// WithRegisteredAccounts returns a copy of the scope that can identify an +// account by its display name. accounts must hold EVERY registered cloud +// account (all providers, enabled or not), so that a shared name is visible +// as shared. Only names that exactly one account carries are kept. // -// Display names are not unique. Without this check, a group scoped to the -// name "prod" was granted every account called "prod", including one added -// later under another provider or tenant (issue #433). With it, a shared name -// matches no account by name; the accounts stay reachable through ID entries. -// Pass a partial list and a name could look unique when it is not, so callers -// that cannot load the full list must leave the scope ID-only. -func (s AccountScope) WithRegisteredNames(names []string) AccountScope { +// This never makes a scope entry match by name. Entries are account IDs, and +// a stored name could otherwise outlive its account and grant whichever +// account later takes that name (issue #1034). The name only tells Allows +// which account the caller means; the scope must still hold that account's +// ID. Pass a partial list and a name could look unique when it is not, so +// callers that cannot load the full list must leave the scope without it. +func (s AccountScope) WithRegisteredAccounts(accounts []RegisteredAccount) AccountScope { if s.Unrestricted { return s } - counts := make(map[string]int, len(names)) - for _, n := range names { - if n != "" { - counts[n]++ + counts := make(map[string]int, len(accounts)) + ids := make(map[string]string, len(accounts)) + for _, a := range accounts { + if a.Name != "" { + counts[a.Name]++ + ids[a.Name] = a.ID } } - s.uniqueNames = make(map[string]struct{}, len(counts)) + s.idByUniqueName = make(map[string]string, len(counts)) for n, c := range counts { - if c == 1 { - s.uniqueNames[n] = struct{}{} + if c == 1 && ids[n] != "" { + s.idByUniqueName[n] = ids[n] } } return s } -// Allows reports whether the scope permits the given cloud account, matched by -// internal ID, or by display name when the scope was built with -// WithRegisteredNames and no other registered account has that name. Pass "" -// for the name when unavailable. +// Allows reports whether the scope permits the given cloud account. The +// account matches when accountID is in the allow-list, or when the scope was +// built with WithRegisteredAccounts, exactly one registered account carries +// accountName, and THAT account's ID is in the allow-list. Pass "" for the +// name when unavailable. +// +// A display name in the allow-list matches nothing: it is not bound to any +// account, so it would grant whichever account carries the name today +// (issue #1034). Group writes store IDs (see the group handlers), and +// migration 000135 rewrote the name entries that still identified one +// account. // // A restricted scope with no accounts allows nothing, which is what makes the // zero value deny. @@ -146,12 +164,23 @@ func (s AccountScope) Allows(accountID, accountName string) bool { if s.Unrestricted { return true } - _, nameMatchable := s.uniqueNames[accountName] + if s.contains(accountID) { + return true + } + if accountName == "" { + return false + } + namedID, ok := s.idByUniqueName[accountName] + return ok && s.contains(namedID) +} + +// contains reports whether id is a non-empty entry of the allow-list. +func (s AccountScope) contains(id string) bool { + if id == "" { + return false + } for _, a := range s.Accounts { - if a == accountID { - return true - } - if nameMatchable && accountName != "" && a == accountName { + if a == id { return true } } diff --git a/internal/auth/account_scope_test.go b/internal/auth/account_scope_test.go index 42708411d..1b762d5db 100644 --- a/internal/auth/account_scope_test.go +++ b/internal/auth/account_scope_test.go @@ -63,10 +63,12 @@ func TestAccountScope_EmptyExplicitListGrantsNothing(t *testing.T) { } func TestAccountScope_RestrictedAllowsOnlyItsOwn(t *testing.T) { - s := ScopeForAccounts([]string{"acct-A", "prod-name"}).WithRegisteredNames([]string{"prod-name", "other-name"}) + s := ScopeForAccounts([]string{"acct-A", "uuid-prod"}).WithRegisteredAccounts([]RegisteredAccount{ + {ID: "uuid-prod", Name: "prod-name"}, {ID: "uuid-other", Name: "other-name"}, + }) assert.True(t, s.Allows("acct-A", ""), "matches by ID") - assert.True(t, s.Allows("some-uuid", "prod-name"), "matches by a unique display name") + assert.True(t, s.Allows("ext-123", "prod-name"), "a unique name identifies an account whose ID is in scope") assert.False(t, s.Allows("acct-B", ""), "an account outside the list is denied") assert.False(t, s.Allows("acct-B", "other-name")) assert.False(t, s.AllowsAll()) @@ -113,23 +115,15 @@ func TestAccountScope_ScopedPrincipalStaysLimited(t *testing.T) { assert.Equal(t, []string{"acct-A"}, s.FilterIDs([]string{"acct-A", "acct-C"})) } -// Behavioral parity with the helpers it replaces, so the migration cannot -// silently change who can reach what. -func TestAccountScope_ParityWithLegacyHelpers(t *testing.T) { +// AllowsAll agrees with the legacy IsUnrestrictedAccess convention for every +// resolved list, so the type cannot silently change who is unrestricted. +func TestAccountScope_AllowsAllParityWithLegacyConvention(t *testing.T) { for _, legacy := range [][]string{ - nil, {}, {"*"}, {"acct-A"}, {"acct-A", "acct-B"}, {"prod-name"}, + nil, {}, {"*"}, {"acct-A"}, {"acct-A", "acct-B"}, {"prod-name"}, {"acct-A", "*"}, } { - // Every probe name is unique among the registered accounts, which is - // the only case where the name match is unchanged (issue #433). - s := ScopeFromLegacyList(legacy).WithRegisteredNames([]string{"prod-name", "nope"}) + s := ScopeFromLegacyList(legacy) assert.Equal(t, IsUnrestrictedAccess(legacy), s.AllowsAll(), "AllowsAll must agree with IsUnrestrictedAccess for %v", legacy) - for _, probe := range []struct{ id, name string }{ - {"acct-A", ""}, {"acct-B", ""}, {"some-uuid", "prod-name"}, {"acct-C", "nope"}, - } { - assert.Equal(t, MatchesAccount(legacy, probe.id, probe.name), s.Allows(probe.id, probe.name), - "Allows must agree with MatchesAccount for legacy=%v probe=%v", legacy, probe) - } } } @@ -153,54 +147,104 @@ func TestAccountScope_LegacyConversionIsBorrowedSafety(t *testing.T) { assert.False(t, ScopeForAccounts(nil).Allows("acct-A", "")) } -// Issue #433: display names are not unique. A name entry matches only when -// exactly one registered account carries that name. -func TestAccountScope_SharedNameMatchesNoAccountByName(t *testing.T) { - registered := []string{"prod", "prod", "solo"} // aws "prod", azure "prod", gcp "solo" - s := ScopeForAccounts([]string{"prod", "solo"}).WithRegisteredNames(registered) +// Issue #1034: a scope entry is bound to an account ID. A name entry is not +// bound to anything, so once the account it was meant for is deleted or +// renamed it would grant whichever account takes the name next. It must +// therefore match no account, even when the name is unique. +func TestAccountScope_NameEntryNeverMatches(t *testing.T) { + // The account the grant was meant for is gone; a new account took its name. + later := []RegisteredAccount{{ID: "uuid-new", Name: "prod"}} + s := ScopeForAccounts([]string{"prod"}).WithRegisteredAccounts(later) + + assert.False(t, s.Allows("uuid-new", "prod"), "a later account reusing the name must not be granted") + assert.False(t, s.Allows("uuid-new", "")) + assert.False(t, s.Allows("ext-new", "prod"), "nor when the caller knows it by external ID and name") + assert.Empty(t, s.FilterIDs([]string{"uuid-new"})) +} - assert.False(t, s.Allows("uuid-aws-prod", "prod"), "a shared name must not grant the first account") - assert.False(t, s.Allows("uuid-az-prod", "prod"), "nor the other account with the same name") - assert.True(t, s.Allows("uuid-solo", "solo"), "a unique name still matches") - assert.False(t, s.Allows("uuid-other", "other")) +// The same grant made by ID survives the rename: the renamed account keeps +// access, the account that takes the old name does not. +func TestAccountScope_IDEntrySurvivesRenameAndReuse(t *testing.T) { + afterRename := []RegisteredAccount{ + {ID: "uuid-orig", Name: "prod-old"}, // renamed away from "prod" + {ID: "uuid-new", Name: "prod"}, // took the old name + } + s := ScopeForAccounts([]string{"uuid-orig"}).WithRegisteredAccounts(afterRename) + + assert.True(t, s.Allows("uuid-orig", "prod-old")) + assert.True(t, s.Allows("ext-orig", "prod-old"), "the renamed account is still identified by its new name") + assert.False(t, s.Allows("uuid-new", "prod"), "the account that took the old name is not granted") + assert.False(t, s.Allows("ext-new", "prod")) } -func TestAccountScope_IDEntriesIgnoreNameAmbiguity(t *testing.T) { - s := ScopeForAccounts([]string{"uuid-aws-prod"}).WithRegisteredNames([]string{"prod", "prod"}) +// Issue #433: display names are not unique. A shared name identifies no +// account, so it cannot be used to reach either of them. +func TestAccountScope_SharedNameIdentifiesNoAccount(t *testing.T) { + registered := []RegisteredAccount{ + {ID: "uuid-aws-prod", Name: "prod"}, {ID: "uuid-az-prod", Name: "prod"}, {ID: "uuid-solo", Name: "solo"}, + } + s := ScopeForAccounts([]string{"uuid-aws-prod", "uuid-solo"}).WithRegisteredAccounts(registered) assert.True(t, s.Allows("uuid-aws-prod", "prod"), "an ID entry grants its account whatever the name") - assert.True(t, s.Allows("uuid-aws-prod", "")) - assert.False(t, s.Allows("uuid-az-prod", "prod")) + assert.False(t, s.Allows("ext-az", "prod"), "a shared name must not resolve to the in-scope account") + assert.False(t, s.Allows("uuid-az-prod", "prod"), "nor grant the other account with that name") + assert.True(t, s.Allows("ext-solo", "solo"), "a unique name still identifies its account") + assert.False(t, s.Allows("uuid-other", "other")) assert.Equal(t, []string{"uuid-aws-prod"}, s.FilterIDs([]string{"uuid-aws-prod", "uuid-az-prod"})) + + // Whichever copy is in scope, the shared name identifies neither. + for _, id := range []string{"uuid-aws-prod", "uuid-az-prod"} { + assert.False(t, ScopeForAccounts([]string{id}).WithRegisteredAccounts(registered).Allows("ext", "prod"), + "shared name must not resolve to %s", id) + } +} + +// A name only identifies the account carrying it. Naming an account after an +// in-scope account's ID gains that account nothing. +func TestAccountScope_NameResolvesToTheAccountCarryingIt(t *testing.T) { + registered := []RegisteredAccount{ + {ID: "uuid-in", Name: "in-scope"}, + {ID: "uuid-attacker", Name: "uuid-in"}, // display name set to the in-scope ID + } + s := ScopeForAccounts([]string{"uuid-in"}).WithRegisteredAccounts(registered) + + assert.False(t, s.Allows("uuid-attacker", "uuid-in"), + "a display name equal to an in-scope ID must not grant the account carrying it") + assert.True(t, s.Allows("uuid-in", "in-scope")) } -// Name matching is opt-in: a scope that never learned the registered names +// Name lookup is opt-in: a scope that never learned the registered accounts // (or learned an empty list) is ID-only, so forgetting the step fails closed. -func TestAccountScope_NameMatchRequiresRegisteredNames(t *testing.T) { - s := ScopeForAccounts([]string{"acct-A", "prod"}) - assert.False(t, s.Allows("uuid-prod", "prod"), "no registered names: ID only") +func TestAccountScope_NameLookupRequiresRegisteredAccounts(t *testing.T) { + s := ScopeForAccounts([]string{"acct-A", "uuid-prod"}) + assert.False(t, s.Allows("ext-prod", "prod"), "no registered accounts: ID only") assert.True(t, s.Allows("acct-A", "")) - assert.False(t, s.WithRegisteredNames(nil).Allows("uuid-prod", "prod")) - assert.False(t, s.WithRegisteredNames([]string{"other"}).Allows("uuid-prod", "prod"), - "a name no registered account carries does not match") + assert.False(t, s.WithRegisteredAccounts(nil).Allows("ext-prod", "prod")) + assert.False(t, s.WithRegisteredAccounts([]RegisteredAccount{{ID: "uuid-x", Name: "other"}}).Allows("ext-prod", "prod"), + "a name no registered account carries identifies nothing") var zero AccountScope - assert.False(t, zero.WithRegisteredNames([]string{"prod"}).Allows("uuid-prod", "prod"), + assert.False(t, zero.WithRegisteredAccounts([]RegisteredAccount{{ID: "uuid-prod", Name: "prod"}}).Allows("ext-prod", "prod"), "the zero value still denies") } -func TestAccountScope_WithRegisteredNamesEdgeCases(t *testing.T) { - assert.True(t, UnrestrictedScope().WithRegisteredNames([]string{"a", "a"}).Allows("x", "a"), +func TestAccountScope_WithRegisteredAccountsEdgeCases(t *testing.T) { + dup := []RegisteredAccount{{ID: "u1", Name: "a"}, {ID: "u2", Name: "a"}} + assert.True(t, UnrestrictedScope().WithRegisteredAccounts(dup).Allows("x", "a"), "unrestricted stays unrestricted") - s := ScopeForAccounts([]string{""}).WithRegisteredNames([]string{""}) - assert.False(t, s.Allows("uuid", ""), "an empty name never matches") + s := ScopeForAccounts([]string{""}).WithRegisteredAccounts([]RegisteredAccount{{ID: "", Name: ""}}) + assert.False(t, s.Allows("", ""), "an empty entry never matches") + assert.False(t, s.Allows("uuid", "")) - base := ScopeForAccounts([]string{"prod"}) - _ = base.WithRegisteredNames([]string{"prod"}) - assert.False(t, base.Allows("uuid", "prod"), "WithRegisteredNames returns a copy") + base := ScopeForAccounts([]string{"uuid-prod"}) + _ = base.WithRegisteredAccounts([]RegisteredAccount{{ID: "uuid-prod", Name: "prod"}}) + assert.False(t, base.Allows("ext-prod", "prod"), "WithRegisteredAccounts returns a copy") - assert.True(t, ScopeForAccounts([]string{"Prod"}).WithRegisteredNames([]string{"Prod", "prod"}).Allows("uuid", "Prod"), - "names are compared exactly, as entries are") + exact := ScopeForAccounts([]string{"uuid-P"}).WithRegisteredAccounts([]RegisteredAccount{ + {ID: "uuid-P", Name: "Prod"}, {ID: "uuid-p", Name: "prod"}, + }) + assert.True(t, exact.Allows("ext", "Prod"), "names are compared exactly") + assert.False(t, exact.Allows("ext", "prod")) } diff --git a/internal/auth/group_ceiling.go b/internal/auth/group_ceiling.go index 665078897..1dc587150 100644 --- a/internal/auth/group_ceiling.go +++ b/internal/auth/group_ceiling.go @@ -376,7 +376,7 @@ func (s *Service) checkAccountGrant(ctx context.Context, actorUserID string, req // against a restricted holder is the widening this exists to catch. // // Comparison is EXACT -- no trimming, no case folding -- because that is what -// MatchesAccount does at enforcement (`a == accountID`). Normalising here +// AccountScope.Allows does at enforcement (`a == accountID`). Normalising here // would make the ceiling MORE permissive than enforcement: an actor whose // stored scope is " acct-A" matches nothing when access is actually checked, // yet a trimming ceiling would treat them as holding "acct-A" and let them diff --git a/internal/auth/service_group_test.go b/internal/auth/service_group_test.go index 7ef508d4f..ca711ec75 100644 --- a/internal/auth/service_group_test.go +++ b/internal/auth/service_group_test.go @@ -806,88 +806,6 @@ func TestAuthContext_HasPermission(t *testing.T) { }) } -func TestAuthContext_CanAccessAccount(t *testing.T) { - t.Run("admin can access any account", func(t *testing.T) { - authCtx := &AuthContext{ - User: &User{}, - AllowedAccounts: []string{}, - } - assert.True(t, authCtx.CanAccessAccount("111111111111", "")) - assert.True(t, authCtx.CanAccessAccount("999999999999", "")) - }) - - t.Run("empty allowed accounts means all access", func(t *testing.T) { - authCtx := &AuthContext{ - User: &User{}, - AllowedAccounts: []string{}, - } - assert.True(t, authCtx.CanAccessAccount("111111111111", "")) - assert.True(t, authCtx.CanAccessAccount("999999999999", "")) - }) - - t.Run("wildcard in allowed accounts", func(t *testing.T) { - authCtx := &AuthContext{ - User: &User{}, - AllowedAccounts: []string{"*"}, - } - assert.True(t, authCtx.CanAccessAccount("111111111111", "")) - assert.True(t, authCtx.CanAccessAccount("999999999999", "")) - }) - - t.Run("specific accounts only", func(t *testing.T) { - authCtx := &AuthContext{ - User: &User{}, - AllowedAccounts: []string{"111111111111", "222222222222"}, - } - assert.True(t, authCtx.CanAccessAccount("111111111111", "")) - assert.True(t, authCtx.CanAccessAccount("222222222222", "")) - assert.False(t, authCtx.CanAccessAccount("333333333333", "")) - assert.False(t, authCtx.CanAccessAccount("999999999999", "")) - }) - - t.Run("readonly user with account restrictions", func(t *testing.T) { - authCtx := &AuthContext{ - User: &User{}, - AllowedAccounts: []string{"111111111111"}, - } - assert.True(t, authCtx.CanAccessAccount("111111111111", "")) - assert.False(t, authCtx.CanAccessAccount("222222222222", "")) - }) - - t.Run("match by account name", func(t *testing.T) { - authCtx := &AuthContext{ - User: &User{}, - AllowedAccounts: []string{"Production", "Staging"}, - } - // UUID doesn't match, name does - assert.True(t, authCtx.CanAccessAccount("uuid-1", "Production")) - assert.True(t, authCtx.CanAccessAccount("uuid-2", "Staging")) - // Neither matches - assert.False(t, authCtx.CanAccessAccount("uuid-3", "Development")) - // Name empty — falls back to ID-only, no match - assert.False(t, authCtx.CanAccessAccount("uuid-4", "")) - }) - - t.Run("mixed UUID and name entries", func(t *testing.T) { - authCtx := &AuthContext{ - User: &User{}, - AllowedAccounts: []string{"uuid-prod", "Staging"}, - } - assert.True(t, authCtx.CanAccessAccount("uuid-prod", "Production")) - assert.True(t, authCtx.CanAccessAccount("uuid-stg", "Staging")) - assert.False(t, authCtx.CanAccessAccount("uuid-dev", "Development")) - }) - - t.Run("wildcard combined with specific names", func(t *testing.T) { - authCtx := &AuthContext{ - User: &User{}, - AllowedAccounts: []string{"*", "Production"}, - } - // Wildcard wins — everything matches - assert.True(t, authCtx.CanAccessAccount("any-uuid", "Anything")) - }) -} - func TestIsUnrestrictedAccess(t *testing.T) { assert.True(t, IsUnrestrictedAccess(nil), "nil slice is unrestricted") assert.True(t, IsUnrestrictedAccess([]string{}), "empty slice is unrestricted") @@ -897,35 +815,6 @@ func TestIsUnrestrictedAccess(t *testing.T) { assert.False(t, IsUnrestrictedAccess([]string{"foo", "bar"}), "multiple specific entries are restricted") } -func TestMatchesAccount(t *testing.T) { - t.Run("unrestricted matches anything", func(t *testing.T) { - assert.True(t, MatchesAccount(nil, "any", "any")) - assert.True(t, MatchesAccount([]string{}, "any", "any")) - assert.True(t, MatchesAccount([]string{"*"}, "any", "any")) - }) - - t.Run("match by ID", func(t *testing.T) { - assert.True(t, MatchesAccount([]string{"uuid-1"}, "uuid-1", "SomeName")) - assert.False(t, MatchesAccount([]string{"uuid-1"}, "uuid-2", "SomeName")) - }) - - t.Run("match by name", func(t *testing.T) { - assert.True(t, MatchesAccount([]string{"Production"}, "uuid-1", "Production")) - assert.False(t, MatchesAccount([]string{"Production"}, "uuid-1", "Staging")) - }) - - t.Run("empty name falls back to ID-only", func(t *testing.T) { - assert.True(t, MatchesAccount([]string{"uuid-1"}, "uuid-1", "")) - assert.False(t, MatchesAccount([]string{"Production"}, "uuid-1", "")) - }) - - t.Run("empty name does not match empty entry", func(t *testing.T) { - // Edge case: allowed entry "" should not match an account with no name - // (otherwise any accountless entry would grant access). - assert.False(t, MatchesAccount([]string{""}, "uuid-1", "")) - }) -} - func TestService_HasPermission_Constraints(t *testing.T) { ctx := context.Background() diff --git a/internal/auth/types.go b/internal/auth/types.go index 898465075..e3add2f47 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -220,35 +220,6 @@ func IsUnrestrictedAccess(allowed []string) bool { return false } -// MatchesAccount returns true if the allowed list matches an account by its -// internal ID or display name. Exact string match against either field. The -// name is optional — pass "" when unavailable; the match then falls back to -// ID-only. Empty allowed list or a "*" entry matches any account. -func MatchesAccount(allowed []string, accountID, accountName string) bool { - if IsUnrestrictedAccess(allowed) { - return true - } - for _, a := range allowed { - if a == accountID { - return true - } - if accountName != "" && a == accountName { - return true - } - } - return false -} - -// CanAccessAccount checks if the user can access a specific account by its -// ID or display name. Access is derived from the union of the user's groups' -// AllowedAccounts via MatchesAccount. Administrators-group members carry the -// "*" wildcard (seeded with allowed_accounts=['*']) and so match any account; -// a user with no groups has an empty AllowedAccounts and, combined with the -// permission check at the call site, is denied (fail closed). -func (ctx *AuthContext) CanAccessAccount(accountID, accountName string) bool { - return MatchesAccount(ctx.AllowedAccounts, accountID, accountName) -} - // Session represents an active user session. type Session struct { Token string `json:"token" dynamodbav:"PK"` diff --git a/internal/database/postgres/migrations/000135_bind_group_account_names_to_ids.down.sql b/internal/database/postgres/migrations/000135_bind_group_account_names_to_ids.down.sql new file mode 100644 index 000000000..4ccd77fd3 --- /dev/null +++ b/internal/database/postgres/migrations/000135_bind_group_account_names_to_ids.down.sql @@ -0,0 +1,11 @@ +-- 000135 down: intentional no-op. +-- +-- The up migration replaced display-name entries in groups.allowed_accounts +-- with the IDs of the accounts they named. Writing the names back would +-- restore the unbound entries issue #1034 removes, and the original spelling +-- of a rewritten entry is not recorded anyway. +-- +-- Leaving the IDs in place is safe for a rollback: pre-#1034 code matches an +-- allowed_accounts entry against the account ID as well as the name, so every +-- rewritten entry keeps granting the same account under the old code. +SELECT 1; -- no-op: rewritten entries keep working by ID diff --git a/internal/database/postgres/migrations/000135_bind_group_account_names_to_ids.up.sql b/internal/database/postgres/migrations/000135_bind_group_account_names_to_ids.up.sql new file mode 100644 index 000000000..d364e3782 --- /dev/null +++ b/internal/database/postgres/migrations/000135_bind_group_account_names_to_ids.up.sql @@ -0,0 +1,69 @@ +-- 000135: bind display-name entries in groups.allowed_accounts to account IDs +-- (issue #1034). +-- +-- Account scopes now match allowed_accounts entries by cloud account ID only +-- (auth.AccountScope.Allows). A display-name entry is not bound to any +-- account: once the account it was meant for is deleted or renamed, it would +-- grant whichever account is later created or renamed to that name. Group +-- writes now resolve names to IDs at grant time; this migration does the same +-- for the entries already stored. +-- +-- An entry is rewritten to an account's ID only when ALL of these hold: +-- * it is not "*" (the wildcard) and not empty; +-- * it is not already an account ID or an account's external ID (those +-- are kept verbatim, as the write path keeps them); +-- * exactly one cloud account, of any provider, enabled or not, carries +-- it as its display name (a shared name cannot say which one was meant); +-- * that account already existed when the group was last written +-- (created_at <= groups.updated_at). An account registered after the +-- grant cannot be the account the grant was made for, which is exactly +-- the delete-then-recreate case of the issue. +-- +-- Every other entry is left as it is. It no longer matches any account, so +-- the group loses that access (fail closed) until an administrator grants the +-- intended account by ID. Such entries can be listed with: +-- +-- SELECT g.name, e.entry +-- FROM groups g, unnest(g.allowed_accounts) AS e(entry) +-- WHERE e.entry <> '*' +-- AND NOT EXISTS (SELECT 1 FROM cloud_accounts ca +-- WHERE ca.id::text = e.entry OR ca.external_id = e.entry); +-- +-- Order is preserved. Groups with a NULL or empty list are untouched, and an +-- entry is only ever replaced, never dropped, so a restricted list can never +-- become empty (which would mean unrestricted). +-- +-- update_groups_updated_at would stamp every rewritten group with NOW(). That +-- is not a grant decision, and it would weaken the created_at guard above if +-- this migration is ever re-applied (down is a no-op), letting an entry left +-- unresolved the first time bind to an account registered after the real +-- grant. So the trigger is disabled for this one statement. DISABLE TRIGGER +-- names only that trigger and needs no superuser; the migration runs in one +-- transaction, so it is re-enabled before commit and a failure rolls both back. +ALTER TABLE groups DISABLE TRIGGER update_groups_updated_at; + +UPDATE groups g +SET allowed_accounts = bound.entries +FROM ( + SELECT src.id, + array_agg(COALESCE(match.account_id, e.entry) ORDER BY e.ord) AS entries + FROM groups src + CROSS JOIN LATERAL unnest(src.allowed_accounts) WITH ORDINALITY AS e(entry, ord) + LEFT JOIN LATERAL ( + SELECT min(ca.id::text) AS account_id + FROM cloud_accounts ca + WHERE ca.name = e.entry + HAVING count(*) = 1 + AND bool_and(ca.created_at <= src.updated_at) + ) match ON e.entry <> '*' + AND e.entry <> '' + AND NOT EXISTS ( + SELECT 1 FROM cloud_accounts known + WHERE known.id::text = e.entry OR known.external_id = e.entry + ) + GROUP BY src.id +) bound +WHERE g.id = bound.id + AND g.allowed_accounts IS DISTINCT FROM bound.entries; + +ALTER TABLE groups ENABLE TRIGGER update_groups_updated_at; diff --git a/internal/database/postgres/migrations/000135_bind_group_account_names_to_ids_test.go b/internal/database/postgres/migrations/000135_bind_group_account_names_to_ids_test.go new file mode 100644 index 000000000..e41a39293 --- /dev/null +++ b/internal/database/postgres/migrations/000135_bind_group_account_names_to_ids_test.go @@ -0,0 +1,131 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" +) + +const ( + m135AcctProd = "13500000-0000-4000-8000-000000000001" + m135AcctSharedA = "13500000-0000-4000-8000-000000000002" + m135AcctSharedB = "13500000-0000-4000-8000-000000000003" + m135AcctReuser = "13500000-0000-4000-8000-000000000004" + m135AcctStar = "13500000-0000-4000-8000-000000000005" + m135AcctExtTwin = "13500000-0000-4000-8000-000000000006" + m135AcctDisabled = "13500000-0000-4000-8000-000000000007" + m135AcctLater = "13500000-0000-4000-8000-000000000008" + + // The group was last written at this time; accounts created after it + // cannot be the account a name entry was granted for. + m135GroupWritten = "2026-06-01T00:00:00Z" + m135Before = "2026-01-01T00:00:00Z" + m135After = "2026-09-01T00:00:00Z" +) + +func m135InsertAccount(ctx context.Context, t *testing.T, pool *pgxpool.Pool, id, name, provider, externalID, createdAt string, enabled bool) { + t.Helper() + _, err := pool.Exec(ctx, ` + INSERT INTO cloud_accounts (id, name, provider, external_id, enabled, created_at) + VALUES ($1, $2, $3, $4, $5, $6::timestamptz)`, id, name, provider, externalID, enabled, createdAt) + require.NoError(t, err) +} + +func m135InsertGroup(ctx context.Context, t *testing.T, pool *pgxpool.Pool, name string, allowed []string) { + t.Helper() + _, err := pool.Exec(ctx, ` + INSERT INTO groups (name, permissions, allowed_accounts, created_at, updated_at) + VALUES ($1, '[]', $2, $3::timestamptz, $3::timestamptz)`, name, allowed, m135GroupWritten) + require.NoError(t, err) +} + +func m135Allowed(ctx context.Context, t *testing.T, pool *pgxpool.Pool, name string) []string { + t.Helper() + var allowed []string + require.NoError(t, pool.QueryRow(ctx, `SELECT allowed_accounts FROM groups WHERE name = $1`, name).Scan(&allowed)) + return allowed +} + +// TestMigration_BindGroupAccountNamesToIDs covers 000135 (issue #1034): name +// entries that identify exactly one account that predates the grant become +// that account's ID; everything else is left untouched. +func TestMigration_BindGroupAccountNamesToIDs(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) + pool, path := container.DB.Pool(), getMigrationsPath() + + const version = 135 + prev := previousMigrationVersion(t, version) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + + m135InsertAccount(ctx, t, pool, m135AcctProd, "prod", "aws", "111111111111", m135Before, true) + m135InsertAccount(ctx, t, pool, m135AcctSharedA, "shared", "aws", "222222222222", m135Before, true) + m135InsertAccount(ctx, t, pool, m135AcctSharedB, "shared", "azure", "sub-shared-b", m135Before, false) + // Registered after the grant under the name of an account that is gone. + m135InsertAccount(ctx, t, pool, m135AcctReuser, "deleted-acct", "gcp", "proj-reuser", m135After, true) + // An account whose display name is the wildcard must not narrow "*". + m135InsertAccount(ctx, t, pool, m135AcctStar, "*", "gcp", "proj-star", m135Before, true) + // Display name equal to another account's external ID. + m135InsertAccount(ctx, t, pool, m135AcctExtTwin, "111111111111", "gcp", "proj-twin", m135Before, true) + m135InsertAccount(ctx, t, pool, m135AcctDisabled, "dormant", "aws", "333333333333", m135Before, false) + m135InsertAccount(ctx, t, pool, m135AcctLater, "later", "aws", "444444444444", m135After, true) + + m135InsertGroup(ctx, t, pool, "unique-name", []string{"prod"}) + m135InsertGroup(ctx, t, pool, "mixed", []string{m135AcctSharedA, "prod", "dormant", "unknown"}) + m135InsertGroup(ctx, t, pool, "shared-name", []string{"shared"}) + m135InsertGroup(ctx, t, pool, "reused-name", []string{"deleted-acct"}) + m135InsertGroup(ctx, t, pool, "created-later", []string{"later"}) + m135InsertGroup(ctx, t, pool, "wildcard", []string{"*"}) + m135InsertGroup(ctx, t, pool, "external-id", []string{"111111111111"}) + m135InsertGroup(ctx, t, pool, "blank-entry", []string{"", "prod"}) + m135InsertGroup(ctx, t, pool, "empty", []string{}) + _, err := pool.Exec(ctx, ` + INSERT INTO groups (name, permissions, allowed_accounts, updated_at) + VALUES ('null-list', '[]', NULL, $1::timestamptz)`, m135GroupWritten) + require.NoError(t, err) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) + + want := map[string][]string{ + "unique-name": {m135AcctProd}, + // Order kept; a disabled account still binds; an unknown name stays. + "mixed": {m135AcctSharedA, m135AcctProd, m135AcctDisabled, "unknown"}, + "shared-name": {"shared"}, + "reused-name": {"deleted-acct"}, + "created-later": {"later"}, + "wildcard": {"*"}, + "external-id": {"111111111111"}, + "blank-entry": {"", m135AcctProd}, + "empty": {}, + } + for group, entries := range want { + assert.Equal(t, entries, m135Allowed(ctx, t, pool, group), group) + } + assert.Nil(t, m135Allowed(ctx, t, pool, "null-list"), "a NULL list stays NULL") + + var updatedAt string + require.NoError(t, pool.QueryRow(ctx, + `SELECT to_char(updated_at AT TIME ZONE 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS"Z"') FROM groups WHERE name = 'unique-name'`). + Scan(&updatedAt)) + assert.Equal(t, m135GroupWritten, updatedAt, + "the rewrite must not move updated_at, which the created_at guard reads") + + // Down is a no-op: the IDs stay, and old code matches them by ID. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + assert.Equal(t, []string{m135AcctProd}, m135Allowed(ctx, t, pool, "unique-name")) + + // Re-applying is idempotent. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) + for group, entries := range want { + assert.Equal(t, entries, m135Allowed(ctx, t, pool, group), "after re-up: %s", group) + } +} From ecb84b29c8fc30a35e26370acf41cd0e3b633dfa Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sun, 25 Oct 2026 16:03:33 +0000 Subject: [PATCH 02/11] test(api): scope the history-summary fixture by account ID The history-summary integration tests from #1200 scoped their user with the tenant's display name. This branch binds allowed_accounts entries to account IDs and makes a bare name entry match nothing (#1034), so the scoped user saw no tenant A rows and the totals and listed-rows checks failed. Grant the scope by tenant A's account ID, the form group writes now store. The assertions are unchanged. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_history_summary_integration_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/api/handler_history_summary_integration_test.go b/internal/api/handler_history_summary_integration_test.go index af830bb6c..40b8ed591 100644 --- a/internal/api/handler_history_summary_integration_test.go +++ b/internal/api/handler_history_summary_integration_test.go @@ -236,7 +236,7 @@ func TestIntegration_HistorySummary_TotalsCoverAllInScopeRows(t *testing.T) { scopedAuth := new(MockAuthService) scopedAuth.On("ValidateSession", mock.Anything, scopedToken).Return(&Session{UserID: scopedUserID}, nil).Maybe() scopedAuth.On("GetUser", mock.Anything, mock.Anything).Return(&User{Email: "creator@example.com"}, nil).Maybe() - scopedAuth.grantScoped(env.tenantA.Name) + scopedAuth.grantScoped(env.tenantA.ID) adminAuth := new(MockAuthService) adminAuth.On("GetUser", mock.Anything, mock.Anything).Return(&User{Email: "creator@example.com"}, nil).Maybe() adminAuth.On("ValidateSession", mock.Anything, adminToken).Return(&Session{UserID: adminUserID}, nil).Maybe() @@ -304,7 +304,7 @@ func TestIntegration_HistorySummary_MatchesListedRowsUnderTheCap(t *testing.T) { scopedAuth := new(MockAuthService) scopedAuth.On("ValidateSession", mock.Anything, scopedToken).Return(&Session{UserID: scopedUserID}, nil).Maybe() scopedAuth.On("GetUser", mock.Anything, mock.Anything).Return(&User{Email: "creator@example.com"}, nil).Maybe() - scopedAuth.grantScoped(env.tenantA.Name) + scopedAuth.grantScoped(env.tenantA.ID) adminAuth := new(MockAuthService) adminAuth.On("GetUser", mock.Anything, mock.Anything).Return(&User{Email: "creator@example.com"}, nil).Maybe() adminAuth.On("ValidateSession", mock.Anything, "admin-token").Return(&Session{UserID: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"}, nil).Maybe() From d05012f26badbe9a2a5a3f4a10b24324ae4b8c50 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sun, 25 Oct 2026 16:06:27 +0000 Subject: [PATCH 03/11] chore(db): renumber the group scope binding migration to 000149 main now ships migrations up to 000139, so 000135 would sit below versions a deployed database has already applied and golang-migrate would never run it. Open PRs hold 000140-000148, so this takes 000149 and must merge after that chain. The SQL is unchanged; only the file names, header comments, test version and test fixture IDs move. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- internal/auth/account_scope.go | 2 +- ..._bind_group_account_names_to_ids.down.sql} | 2 +- ...49_bind_group_account_names_to_ids.up.sql} | 2 +- ...9_bind_group_account_names_to_ids_test.go} | 88 +++++++++---------- 5 files changed, 48 insertions(+), 48 deletions(-) rename internal/database/postgres/migrations/{000135_bind_group_account_names_to_ids.down.sql => 000149_bind_group_account_names_to_ids.down.sql} (94%) rename internal/database/postgres/migrations/{000135_bind_group_account_names_to_ids.up.sql => 000149_bind_group_account_names_to_ids.up.sql} (98%) rename internal/database/postgres/migrations/{000135_bind_group_account_names_to_ids_test.go => 000149_bind_group_account_names_to_ids_test.go} (54%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f7c492ae..a35a44130 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,7 +36,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). display-name entry used to grant whichever account was later created or renamed to that name. Group writes now store the ID of the one account a name identifies (and reject names that identify none or several), and - migration 000135 rewrites existing name entries whose account predates the + migration 000149 rewrites existing name entries whose account predates the grant; entries it cannot bind stop granting access (#1034) - API-key creation over HTTP rejects an explicit `max_amount: 0` spend cap with a 400 instead of minting the key with no cap, matching group writes (#980) - Group writes reject an explicit `max_amount: 0` spend cap with a 400 instead of storing it as no cap; omit the field for no limit (#133) diff --git a/internal/auth/account_scope.go b/internal/auth/account_scope.go index 1f30d728d..c8905dc42 100644 --- a/internal/auth/account_scope.go +++ b/internal/auth/account_scope.go @@ -155,7 +155,7 @@ func (s AccountScope) WithRegisteredAccounts(accounts []RegisteredAccount) Accou // A display name in the allow-list matches nothing: it is not bound to any // account, so it would grant whichever account carries the name today // (issue #1034). Group writes store IDs (see the group handlers), and -// migration 000135 rewrote the name entries that still identified one +// migration 000149 rewrote the name entries that still identified one // account. // // A restricted scope with no accounts allows nothing, which is what makes the diff --git a/internal/database/postgres/migrations/000135_bind_group_account_names_to_ids.down.sql b/internal/database/postgres/migrations/000149_bind_group_account_names_to_ids.down.sql similarity index 94% rename from internal/database/postgres/migrations/000135_bind_group_account_names_to_ids.down.sql rename to internal/database/postgres/migrations/000149_bind_group_account_names_to_ids.down.sql index 4ccd77fd3..e9d139714 100644 --- a/internal/database/postgres/migrations/000135_bind_group_account_names_to_ids.down.sql +++ b/internal/database/postgres/migrations/000149_bind_group_account_names_to_ids.down.sql @@ -1,4 +1,4 @@ --- 000135 down: intentional no-op. +-- 000149 down: intentional no-op. -- -- The up migration replaced display-name entries in groups.allowed_accounts -- with the IDs of the accounts they named. Writing the names back would diff --git a/internal/database/postgres/migrations/000135_bind_group_account_names_to_ids.up.sql b/internal/database/postgres/migrations/000149_bind_group_account_names_to_ids.up.sql similarity index 98% rename from internal/database/postgres/migrations/000135_bind_group_account_names_to_ids.up.sql rename to internal/database/postgres/migrations/000149_bind_group_account_names_to_ids.up.sql index d364e3782..d0e50ef0d 100644 --- a/internal/database/postgres/migrations/000135_bind_group_account_names_to_ids.up.sql +++ b/internal/database/postgres/migrations/000149_bind_group_account_names_to_ids.up.sql @@ -1,4 +1,4 @@ --- 000135: bind display-name entries in groups.allowed_accounts to account IDs +-- 000149: bind display-name entries in groups.allowed_accounts to account IDs -- (issue #1034). -- -- Account scopes now match allowed_accounts entries by cloud account ID only diff --git a/internal/database/postgres/migrations/000135_bind_group_account_names_to_ids_test.go b/internal/database/postgres/migrations/000149_bind_group_account_names_to_ids_test.go similarity index 54% rename from internal/database/postgres/migrations/000135_bind_group_account_names_to_ids_test.go rename to internal/database/postgres/migrations/000149_bind_group_account_names_to_ids_test.go index e41a39293..65875ef4f 100644 --- a/internal/database/postgres/migrations/000135_bind_group_account_names_to_ids_test.go +++ b/internal/database/postgres/migrations/000149_bind_group_account_names_to_ids_test.go @@ -15,23 +15,23 @@ import ( ) const ( - m135AcctProd = "13500000-0000-4000-8000-000000000001" - m135AcctSharedA = "13500000-0000-4000-8000-000000000002" - m135AcctSharedB = "13500000-0000-4000-8000-000000000003" - m135AcctReuser = "13500000-0000-4000-8000-000000000004" - m135AcctStar = "13500000-0000-4000-8000-000000000005" - m135AcctExtTwin = "13500000-0000-4000-8000-000000000006" - m135AcctDisabled = "13500000-0000-4000-8000-000000000007" - m135AcctLater = "13500000-0000-4000-8000-000000000008" + m149AcctProd = "14900000-0000-4000-8000-000000000001" + m149AcctSharedA = "14900000-0000-4000-8000-000000000002" + m149AcctSharedB = "14900000-0000-4000-8000-000000000003" + m149AcctReuser = "14900000-0000-4000-8000-000000000004" + m149AcctStar = "14900000-0000-4000-8000-000000000005" + m149AcctExtTwin = "14900000-0000-4000-8000-000000000006" + m149AcctDisabled = "14900000-0000-4000-8000-000000000007" + m149AcctLater = "14900000-0000-4000-8000-000000000008" // The group was last written at this time; accounts created after it // cannot be the account a name entry was granted for. - m135GroupWritten = "2026-06-01T00:00:00Z" - m135Before = "2026-01-01T00:00:00Z" - m135After = "2026-09-01T00:00:00Z" + m149GroupWritten = "2026-06-01T00:00:00Z" + m149Before = "2026-01-01T00:00:00Z" + m149After = "2026-09-01T00:00:00Z" ) -func m135InsertAccount(ctx context.Context, t *testing.T, pool *pgxpool.Pool, id, name, provider, externalID, createdAt string, enabled bool) { +func m149InsertAccount(ctx context.Context, t *testing.T, pool *pgxpool.Pool, id, name, provider, externalID, createdAt string, enabled bool) { t.Helper() _, err := pool.Exec(ctx, ` INSERT INTO cloud_accounts (id, name, provider, external_id, enabled, created_at) @@ -39,22 +39,22 @@ func m135InsertAccount(ctx context.Context, t *testing.T, pool *pgxpool.Pool, id require.NoError(t, err) } -func m135InsertGroup(ctx context.Context, t *testing.T, pool *pgxpool.Pool, name string, allowed []string) { +func m149InsertGroup(ctx context.Context, t *testing.T, pool *pgxpool.Pool, name string, allowed []string) { t.Helper() _, err := pool.Exec(ctx, ` INSERT INTO groups (name, permissions, allowed_accounts, created_at, updated_at) - VALUES ($1, '[]', $2, $3::timestamptz, $3::timestamptz)`, name, allowed, m135GroupWritten) + VALUES ($1, '[]', $2, $3::timestamptz, $3::timestamptz)`, name, allowed, m149GroupWritten) require.NoError(t, err) } -func m135Allowed(ctx context.Context, t *testing.T, pool *pgxpool.Pool, name string) []string { +func m149Allowed(ctx context.Context, t *testing.T, pool *pgxpool.Pool, name string) []string { t.Helper() var allowed []string require.NoError(t, pool.QueryRow(ctx, `SELECT allowed_accounts FROM groups WHERE name = $1`, name).Scan(&allowed)) return allowed } -// TestMigration_BindGroupAccountNamesToIDs covers 000135 (issue #1034): name +// TestMigration_BindGroupAccountNamesToIDs covers 000149 (issue #1034): name // entries that identify exactly one account that predates the grant become // that account's ID; everything else is left untouched. func TestMigration_BindGroupAccountNamesToIDs(t *testing.T) { @@ -63,69 +63,69 @@ func TestMigration_BindGroupAccountNamesToIDs(t *testing.T) { t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) pool, path := container.DB.Pool(), getMigrationsPath() - const version = 135 + const version = 149 prev := previousMigrationVersion(t, version) require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) - m135InsertAccount(ctx, t, pool, m135AcctProd, "prod", "aws", "111111111111", m135Before, true) - m135InsertAccount(ctx, t, pool, m135AcctSharedA, "shared", "aws", "222222222222", m135Before, true) - m135InsertAccount(ctx, t, pool, m135AcctSharedB, "shared", "azure", "sub-shared-b", m135Before, false) + m149InsertAccount(ctx, t, pool, m149AcctProd, "prod", "aws", "111111111111", m149Before, true) + m149InsertAccount(ctx, t, pool, m149AcctSharedA, "shared", "aws", "222222222222", m149Before, true) + m149InsertAccount(ctx, t, pool, m149AcctSharedB, "shared", "azure", "sub-shared-b", m149Before, false) // Registered after the grant under the name of an account that is gone. - m135InsertAccount(ctx, t, pool, m135AcctReuser, "deleted-acct", "gcp", "proj-reuser", m135After, true) + m149InsertAccount(ctx, t, pool, m149AcctReuser, "deleted-acct", "gcp", "proj-reuser", m149After, true) // An account whose display name is the wildcard must not narrow "*". - m135InsertAccount(ctx, t, pool, m135AcctStar, "*", "gcp", "proj-star", m135Before, true) + m149InsertAccount(ctx, t, pool, m149AcctStar, "*", "gcp", "proj-star", m149Before, true) // Display name equal to another account's external ID. - m135InsertAccount(ctx, t, pool, m135AcctExtTwin, "111111111111", "gcp", "proj-twin", m135Before, true) - m135InsertAccount(ctx, t, pool, m135AcctDisabled, "dormant", "aws", "333333333333", m135Before, false) - m135InsertAccount(ctx, t, pool, m135AcctLater, "later", "aws", "444444444444", m135After, true) - - m135InsertGroup(ctx, t, pool, "unique-name", []string{"prod"}) - m135InsertGroup(ctx, t, pool, "mixed", []string{m135AcctSharedA, "prod", "dormant", "unknown"}) - m135InsertGroup(ctx, t, pool, "shared-name", []string{"shared"}) - m135InsertGroup(ctx, t, pool, "reused-name", []string{"deleted-acct"}) - m135InsertGroup(ctx, t, pool, "created-later", []string{"later"}) - m135InsertGroup(ctx, t, pool, "wildcard", []string{"*"}) - m135InsertGroup(ctx, t, pool, "external-id", []string{"111111111111"}) - m135InsertGroup(ctx, t, pool, "blank-entry", []string{"", "prod"}) - m135InsertGroup(ctx, t, pool, "empty", []string{}) + m149InsertAccount(ctx, t, pool, m149AcctExtTwin, "111111111111", "gcp", "proj-twin", m149Before, true) + m149InsertAccount(ctx, t, pool, m149AcctDisabled, "dormant", "aws", "333333333333", m149Before, false) + m149InsertAccount(ctx, t, pool, m149AcctLater, "later", "aws", "444444444444", m149After, true) + + m149InsertGroup(ctx, t, pool, "unique-name", []string{"prod"}) + m149InsertGroup(ctx, t, pool, "mixed", []string{m149AcctSharedA, "prod", "dormant", "unknown"}) + m149InsertGroup(ctx, t, pool, "shared-name", []string{"shared"}) + m149InsertGroup(ctx, t, pool, "reused-name", []string{"deleted-acct"}) + m149InsertGroup(ctx, t, pool, "created-later", []string{"later"}) + m149InsertGroup(ctx, t, pool, "wildcard", []string{"*"}) + m149InsertGroup(ctx, t, pool, "external-id", []string{"111111111111"}) + m149InsertGroup(ctx, t, pool, "blank-entry", []string{"", "prod"}) + m149InsertGroup(ctx, t, pool, "empty", []string{}) _, err := pool.Exec(ctx, ` INSERT INTO groups (name, permissions, allowed_accounts, updated_at) - VALUES ('null-list', '[]', NULL, $1::timestamptz)`, m135GroupWritten) + VALUES ('null-list', '[]', NULL, $1::timestamptz)`, m149GroupWritten) require.NoError(t, err) require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) want := map[string][]string{ - "unique-name": {m135AcctProd}, + "unique-name": {m149AcctProd}, // Order kept; a disabled account still binds; an unknown name stays. - "mixed": {m135AcctSharedA, m135AcctProd, m135AcctDisabled, "unknown"}, + "mixed": {m149AcctSharedA, m149AcctProd, m149AcctDisabled, "unknown"}, "shared-name": {"shared"}, "reused-name": {"deleted-acct"}, "created-later": {"later"}, "wildcard": {"*"}, "external-id": {"111111111111"}, - "blank-entry": {"", m135AcctProd}, + "blank-entry": {"", m149AcctProd}, "empty": {}, } for group, entries := range want { - assert.Equal(t, entries, m135Allowed(ctx, t, pool, group), group) + assert.Equal(t, entries, m149Allowed(ctx, t, pool, group), group) } - assert.Nil(t, m135Allowed(ctx, t, pool, "null-list"), "a NULL list stays NULL") + assert.Nil(t, m149Allowed(ctx, t, pool, "null-list"), "a NULL list stays NULL") var updatedAt string require.NoError(t, pool.QueryRow(ctx, `SELECT to_char(updated_at AT TIME ZONE 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS"Z"') FROM groups WHERE name = 'unique-name'`). Scan(&updatedAt)) - assert.Equal(t, m135GroupWritten, updatedAt, + assert.Equal(t, m149GroupWritten, updatedAt, "the rewrite must not move updated_at, which the created_at guard reads") // Down is a no-op: the IDs stay, and old code matches them by ID. require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) - assert.Equal(t, []string{m135AcctProd}, m135Allowed(ctx, t, pool, "unique-name")) + assert.Equal(t, []string{m149AcctProd}, m149Allowed(ctx, t, pool, "unique-name")) // Re-applying is idempotent. require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) for group, entries := range want { - assert.Equal(t, entries, m135Allowed(ctx, t, pool, group), "after re-up: %s", group) + assert.Equal(t, entries, m149Allowed(ctx, t, pool, group), "after re-up: %s", group) } } From b78c8c4941ac9fe8f00c265a964557c640abcf22 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 08:47:11 +0000 Subject: [PATCH 04/11] chore(db): renumber bind_group_account_names_to_ids migration 000149 -> 000145 golang-migrate only applies versions above the database's current one, so PRs must merge in migration-number order. 000140-000144 had been taken by PRs that are now held on owner decisions (#1239, #1241, #1253). The ready PRs move to 000140-000145 so they can merge ahead of them, and the held ones will take the next free numbers when decided. Rename only: the SQL statements are unchanged, and the test version constants and the comments naming the migration follow the new number. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- internal/auth/account_scope.go | 2 +- ...wn.sql => 000145_bind_group_account_names_to_ids.down.sql} | 2 +- ...s.up.sql => 000145_bind_group_account_names_to_ids.up.sql} | 2 +- ...test.go => 000145_bind_group_account_names_to_ids_test.go} | 4 ++-- 5 files changed, 6 insertions(+), 6 deletions(-) rename internal/database/postgres/migrations/{000149_bind_group_account_names_to_ids.down.sql => 000145_bind_group_account_names_to_ids.down.sql} (94%) rename internal/database/postgres/migrations/{000149_bind_group_account_names_to_ids.up.sql => 000145_bind_group_account_names_to_ids.up.sql} (98%) rename internal/database/postgres/migrations/{000149_bind_group_account_names_to_ids_test.go => 000145_bind_group_account_names_to_ids_test.go} (98%) diff --git a/CHANGELOG.md b/CHANGELOG.md index a35a44130..2e75833e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,7 +36,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). display-name entry used to grant whichever account was later created or renamed to that name. Group writes now store the ID of the one account a name identifies (and reject names that identify none or several), and - migration 000149 rewrites existing name entries whose account predates the + migration 000145 rewrites existing name entries whose account predates the grant; entries it cannot bind stop granting access (#1034) - API-key creation over HTTP rejects an explicit `max_amount: 0` spend cap with a 400 instead of minting the key with no cap, matching group writes (#980) - Group writes reject an explicit `max_amount: 0` spend cap with a 400 instead of storing it as no cap; omit the field for no limit (#133) diff --git a/internal/auth/account_scope.go b/internal/auth/account_scope.go index c8905dc42..3c44dbb37 100644 --- a/internal/auth/account_scope.go +++ b/internal/auth/account_scope.go @@ -155,7 +155,7 @@ func (s AccountScope) WithRegisteredAccounts(accounts []RegisteredAccount) Accou // A display name in the allow-list matches nothing: it is not bound to any // account, so it would grant whichever account carries the name today // (issue #1034). Group writes store IDs (see the group handlers), and -// migration 000149 rewrote the name entries that still identified one +// migration 000145 rewrote the name entries that still identified one // account. // // A restricted scope with no accounts allows nothing, which is what makes the diff --git a/internal/database/postgres/migrations/000149_bind_group_account_names_to_ids.down.sql b/internal/database/postgres/migrations/000145_bind_group_account_names_to_ids.down.sql similarity index 94% rename from internal/database/postgres/migrations/000149_bind_group_account_names_to_ids.down.sql rename to internal/database/postgres/migrations/000145_bind_group_account_names_to_ids.down.sql index e9d139714..95be2a56b 100644 --- a/internal/database/postgres/migrations/000149_bind_group_account_names_to_ids.down.sql +++ b/internal/database/postgres/migrations/000145_bind_group_account_names_to_ids.down.sql @@ -1,4 +1,4 @@ --- 000149 down: intentional no-op. +-- 000145 down: intentional no-op. -- -- The up migration replaced display-name entries in groups.allowed_accounts -- with the IDs of the accounts they named. Writing the names back would diff --git a/internal/database/postgres/migrations/000149_bind_group_account_names_to_ids.up.sql b/internal/database/postgres/migrations/000145_bind_group_account_names_to_ids.up.sql similarity index 98% rename from internal/database/postgres/migrations/000149_bind_group_account_names_to_ids.up.sql rename to internal/database/postgres/migrations/000145_bind_group_account_names_to_ids.up.sql index d0e50ef0d..c2e8679e9 100644 --- a/internal/database/postgres/migrations/000149_bind_group_account_names_to_ids.up.sql +++ b/internal/database/postgres/migrations/000145_bind_group_account_names_to_ids.up.sql @@ -1,4 +1,4 @@ --- 000149: bind display-name entries in groups.allowed_accounts to account IDs +-- 000145: bind display-name entries in groups.allowed_accounts to account IDs -- (issue #1034). -- -- Account scopes now match allowed_accounts entries by cloud account ID only diff --git a/internal/database/postgres/migrations/000149_bind_group_account_names_to_ids_test.go b/internal/database/postgres/migrations/000145_bind_group_account_names_to_ids_test.go similarity index 98% rename from internal/database/postgres/migrations/000149_bind_group_account_names_to_ids_test.go rename to internal/database/postgres/migrations/000145_bind_group_account_names_to_ids_test.go index 65875ef4f..b00f62881 100644 --- a/internal/database/postgres/migrations/000149_bind_group_account_names_to_ids_test.go +++ b/internal/database/postgres/migrations/000145_bind_group_account_names_to_ids_test.go @@ -54,7 +54,7 @@ func m149Allowed(ctx context.Context, t *testing.T, pool *pgxpool.Pool, name str return allowed } -// TestMigration_BindGroupAccountNamesToIDs covers 000149 (issue #1034): name +// TestMigration_BindGroupAccountNamesToIDs covers 000145 (issue #1034): name // entries that identify exactly one account that predates the grant become // that account's ID; everything else is left untouched. func TestMigration_BindGroupAccountNamesToIDs(t *testing.T) { @@ -63,7 +63,7 @@ func TestMigration_BindGroupAccountNamesToIDs(t *testing.T) { t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) pool, path := container.DB.Pool(), getMigrationsPath() - const version = 149 + const version = 145 prev := previousMigrationVersion(t, version) require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) From d7eba142fa4c4b01d7873c360a360d879aee91a4 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 09:00:31 +0000 Subject: [PATCH 05/11] test(api): register the wire-format group account for grant-time binding TestUserGroupWrites_WireFormat came in from main (#1261) and builds a Handler with no config store. Since #1034, group writes bind every allowed_accounts entry to a registered account and refuse the write when the account list cannot be loaded, so POST /api/groups answered 500. The test now registers its account ID in a store mock, as the binding tests do; the wire format it pins is unchanged. Co-Authored-By: Claude Opus 5.5 --- internal/api/user_group_wire_format_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/internal/api/user_group_wire_format_test.go b/internal/api/user_group_wire_format_test.go index 32050adc1..f94e89c01 100644 --- a/internal/api/user_group_wire_format_test.go +++ b/internal/api/user_group_wire_format_test.go @@ -17,6 +17,7 @@ import ( "testing" "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -198,7 +199,13 @@ func TestUserGroupWrites_WireFormat(t *testing.T) { Run(func(args mock.Arguments) { gotReq = args.Get(reqIdx) }). Return(tc.response, nil).Once() - h := &Handler{auth: mockAuth} + // Group writes bind allowed_accounts entries to registered + // accounts (issue #1034), so the wire account must exist. + store := new(MockConfigStore) + store.ListCloudAccountsFn = func(context.Context, config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{{ID: wireAccountID, Name: "Wire"}}, nil + } + h := &Handler{auth: mockAuth, config: store} resp, err := h.HandleRequest(context.Background(), &events.LambdaFunctionURLRequest{ Headers: map[string]string{ "Authorization": "Bearer " + wireToken, From 6415e6ac42364da7b02c2f3528c830d62e129b76 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 09:28:52 +0000 Subject: [PATCH 06/11] test(api): scope the History paging user by account ID, not name TestIntegration_HistoryPaging_ScopedUserNeverPagesIntoAnotherTenant came in from main (#1277) and grants the scoped user tenant A by its display name. Since #1034 an allowed_accounts entry matches by cloud account ID only, so that user saw none of tenant A's rows and the walk returned only its own unscoped execution. The grant now names tenant A's ID, as the other scope tests on this branch do; what the test asserts is unchanged. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_history_page_integration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/api/handler_history_page_integration_test.go b/internal/api/handler_history_page_integration_test.go index 62b4d5e0f..3d34cf2e9 100644 --- a/internal/api/handler_history_page_integration_test.go +++ b/internal/api/handler_history_page_integration_test.go @@ -214,7 +214,7 @@ func historyPagingHandlers(env *historyPagingEnv) (admin, scoped *Handler) { scopedAuth := new(MockAuthService) scopedAuth.On("ValidateSession", mock.Anything, scopedToken).Return(&Session{UserID: scopedUserID}, nil).Maybe() scopedAuth.On("GetUser", mock.Anything, mock.Anything).Return(&User{Email: "creator@example.com"}, nil).Maybe() - scopedAuth.grantScoped(env.tenantA.Name) + scopedAuth.grantScoped(env.tenantA.ID) adminAuth := new(MockAuthService) adminAuth.On("ValidateSession", mock.Anything, "admin-token").Return(&Session{UserID: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"}, nil).Maybe() adminAuth.On("GetUser", mock.Anything, mock.Anything).Return(&User{Email: "creator@example.com"}, nil).Maybe() From d065a62402f57f08a2fea376b4735cd5b7d52f92 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 12:24:24 +0000 Subject: [PATCH 07/11] test(analytics): scope account_ids fixtures by account ID The account_ids analytics tests that arrived with the main merge (#1448) grant scope by account name ("a", "b", "c"). This branch binds group scope entries to account IDs and no longer matches an entry against a display name (#1034), so those name entries authorize nothing and the scoped cases got 404. List the account IDs in the scope instead, as the other analytics scope tests on this branch already do. The assertions are unchanged. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_analytics_account_ids_test.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/internal/api/handler_analytics_account_ids_test.go b/internal/api/handler_analytics_account_ids_test.go index c255f3372..68936cc0c 100644 --- a/internal/api/handler_analytics_account_ids_test.go +++ b/internal/api/handler_analytics_account_ids_test.go @@ -10,7 +10,8 @@ import ( ) // Issue #1429: the analytics routes read the plural account_ids as well as the -// legacy account_id, with the same per-id scope check. +// legacy account_id, with the same per-id scope check. Scope entries are +// account IDs (#1034), so the scoped fixtures list IDs, not names. const ( idsAcctA = "11111111-1111-4111-8111-111111111111" idsAcctB = "22222222-2222-4222-8222-222222222222" @@ -28,7 +29,7 @@ func idsAccounts() []config.CloudAccount { func TestAnalytics_AccountIDs_AllReachFilter(t *testing.T) { t.Parallel() for _, path := range analyticsPaths { - for name, scope := range map[string][]string{"unrestricted": nil, "scoped": {"a", "b", "c"}} { + for name, scope := range map[string][]string{"unrestricted": nil, "scoped": {idsAcctA, idsAcctB, idsAcctC}} { t.Run(path+" "+name, func(t *testing.T) { t.Parallel() params := map[string]string{"account_ids": idsAcctA + "," + idsAcctB + "," + idsAcctC} @@ -46,7 +47,7 @@ func TestAnalytics_AccountIDs_AllReachFilter(t *testing.T) { func TestAnalytics_AccountIDs_ForeignIDRefusedLikeSingle(t *testing.T) { t.Parallel() - scope := []string{"a"} // C and B are out of scope + scope := []string{idsAcctA} // C and B are out of scope for _, path := range analyticsPaths { single, _, _ := analyticsParamsCall(t, path, scope, map[string]string{"account_id": idsAcctC}, idsAccounts()) for name, ids := range map[string]string{ @@ -68,7 +69,7 @@ func TestAnalytics_AccountIDs_ForeignIDRefusedLikeSingle(t *testing.T) { func TestAnalytics_AccountIDs_ScopedAllowedSubset(t *testing.T) { t.Parallel() - status, uuids, externals := analyticsParamsCall(t, "/api/history/analytics", []string{"a", "b"}, + status, uuids, externals := analyticsParamsCall(t, "/api/history/analytics", []string{idsAcctA, idsAcctB}, map[string]string{"account_ids": idsAcctA + "," + idsAcctB}, idsAccounts()) assert.Equal(t, 200, status) assert.Equal(t, []string{idsAcctA, idsAcctB}, uuids) @@ -98,7 +99,7 @@ func TestAnalytics_AccountIDs_AbsentUnchanged(t *testing.T) { assert.Nil(t, uuids, path) assert.Nil(t, externals, path) - status, _, _ = analyticsParamsCall(t, path, []string{"a"}, map[string]string{}, idsAccounts()) + status, _, _ = analyticsParamsCall(t, path, []string{idsAcctA}, map[string]string{}, idsAccounts()) assert.Equal(t, 400, status, "%s: scoped user must still name an account", path) status, uuids, externals = analyticsParamsCall(t, path, nil, map[string]string{"account_id": idsAcctA}, idsAccounts()) @@ -113,7 +114,7 @@ func TestAnalytics_AccountIDs_AbsentUnchanged(t *testing.T) { // scope cannot widen or break an otherwise in-scope plural request. func TestAnalytics_AccountIDs_PluralWinsOverSingular(t *testing.T) { t.Parallel() - status, uuids, _ := analyticsParamsCall(t, "/api/history/analytics", []string{"a", "b"}, + status, uuids, _ := analyticsParamsCall(t, "/api/history/analytics", []string{idsAcctA, idsAcctB}, map[string]string{"account_ids": idsAcctA + "," + idsAcctB, "account_id": idsAcctC}, idsAccounts()) assert.Equal(t, 200, status) assert.Equal(t, []string{idsAcctA, idsAcctB}, uuids, "singular account_id is ignored when account_ids is present") From 555b3ba39427354517e8acf450a40164e0d7a45d Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 12:25:01 +0000 Subject: [PATCH 08/11] test(analytics): scope the account_ids DB test by account ID TestAnalyticsAccountIDs_SumsSelectedAccountsOnly_DB (from #1448) grants the scoped user ["a", "b"], which are account names. Scope entries are account IDs on this branch (#1034), so the name entries allowed nothing and the in-scope query returned 404. Use the created accounts' IDs. The foreign-id refusal assertion is unchanged. Co-Authored-By: Claude Opus 5.5 --- internal/api/analytics_account_ids_integration_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/api/analytics_account_ids_integration_test.go b/internal/api/analytics_account_ids_integration_test.go index 1066b137b..d8a446341 100644 --- a/internal/api/analytics_account_ids_integration_test.go +++ b/internal/api/analytics_account_ids_integration_test.go @@ -86,9 +86,10 @@ func TestAnalyticsAccountIDs_SumsSelectedAccountsOnly_DB(t *testing.T) { assert.InDelta(t, tc.want, got.Summary.TotalMonthlySavings, 1e-9, tc.ids) } - // A scoped user sees the sum of their accounts; a foreign id in the list - // refuses the whole request (404), never a partial or all-account total. - scoped := newHandler([]string{"a", "b"}) + // A scoped user (scope entries are account IDs, #1034) sees the sum of + // their accounts; a foreign id in the list refuses the whole request + // (404), never a partial or all-account total. + scoped := newHandler([]string{ids["a"], ids["b"]}) got, err := query(scoped, ids["a"]+","+ids["b"]) require.NoError(t, err) assert.InDelta(t, 30, got.Summary.TotalMonthlySavings, 1e-9) From 4707a33a943f401804c6f0c8bfb58a1b5367aa0c Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 12:51:53 +0000 Subject: [PATCH 09/11] test(history): scope the plan_id filter DB test by account ID TestIntegration_HistoryPlanFilter_CrossTenant (from #1445) grants the scoped user tenant A by account name. Scope entries are account IDs on this branch (#1034), so the name allowed nothing and tenant A's own plan came back empty. Grant tenant A's ID, as the other history integration tests on this branch do. The cross-tenant assertions are unchanged. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_history_plan_filter_integration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/api/handler_history_plan_filter_integration_test.go b/internal/api/handler_history_plan_filter_integration_test.go index 1af99fabf..b6044f054 100644 --- a/internal/api/handler_history_plan_filter_integration_test.go +++ b/internal/api/handler_history_plan_filter_integration_test.go @@ -82,7 +82,7 @@ func newPlanFilterEnv(ctx context.Context, t *testing.T) *planFilterEnv { scopedAuth := new(MockAuthService) scopedAuth.On("ValidateSession", mock.Anything, scopedToken).Return(&Session{UserID: scopedUserID}, nil).Maybe() scopedAuth.On("GetUser", mock.Anything, mock.Anything).Return(&User{Email: "creator@example.com"}, nil).Maybe() - scopedAuth.grantScoped(e.tenantA.Name) + scopedAuth.grantScoped(e.tenantA.ID) adminAuth := new(MockAuthService) adminAuth.On("GetUser", mock.Anything, mock.Anything).Return(&User{Email: "creator@example.com"}, nil).Maybe() adminAuth.On("ValidateSession", mock.Anything, "admin-token").Return(&Session{UserID: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"}, nil).Maybe() From 0ccf687ab609d4971ad0a3c41f5ec240ac163085 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 14:56:11 +0000 Subject: [PATCH 10/11] test(api): scope #1464's actionable History tests by account ID #1464 landed on main with tests that scope the user by the display name "dev" and with an executionScope case built on WithRegisteredNames. This branch (#1034) makes name entries grant nothing and replaces that constructor, so after merging main those tests no longer compiled or no longer exercised the scoped path. Scope them by the account's ID, which is what a group stores now, and pin that a name entry contributes only its literal value to the SQL scope. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_history_actionable_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/api/handler_history_actionable_test.go b/internal/api/handler_history_actionable_test.go index 84d08b708..20763ed3c 100644 --- a/internal/api/handler_history_actionable_test.go +++ b/internal/api/handler_history_actionable_test.go @@ -93,7 +93,7 @@ func TestHandleRequest_History_ScopedPendingNotCrowdedOutByOtherAccounts(t *test })).Return([]config.PurchaseExecution{mine}, nil) store.On("GetExecutionsByStatusesFiltered", mock.Anything, mock.MatchedBy(isStaleSweepQuery)).Return([]config.PurchaseExecution{}, nil) - body := actionableHistoryRequest(t, store, []string{"dev"}, map[string]string{}) + body := actionableHistoryRequest(t, store, []string{actionableDevUUID}, map[string]string{}) ids := make([]string, 0, len(body.Purchases)) for _, p := range body.Purchases { @@ -140,7 +140,7 @@ func TestHandleRequest_History_StaleSweepIgnoresFiltersAndScope(t *testing.T) { Return([]config.PurchaseExecution{}, nil).Once() before := time.Now() - actionableHistoryRequest(t, store, []string{"dev"}, map[string]string{"provider": "azure"}) + actionableHistoryRequest(t, store, []string{actionableDevUUID}, map[string]string{"provider": "azure"}) store.AssertExpectations(t) assert.Nil(t, sweep.Scope) @@ -243,8 +243,9 @@ func TestHistoryScope_ExecutionScope(t *testing.T) { {"empty denies attributed rows", auth.ScopeForAccounts(nil), &config.ExecutionScope{UserID: "u-1"}}, {"by id", auth.ScopeForAccounts([]string{actionableDevUUID}), &config.ExecutionScope{UserID: "u-1", AccountIDs: []string{actionableDevUUID, "111111111111"}}}, - {"by unique name", auth.ScopeForAccounts([]string{"dev"}).WithRegisteredNames([]string{"dev", "other"}), - &config.ExecutionScope{UserID: "u-1", AccountIDs: []string{"dev", actionableDevUUID, "111111111111"}}}, + {"name entry grants no account (#1034)", auth.ScopeForAccounts([]string{"dev"}).WithRegisteredAccounts([]auth.RegisteredAccount{ + {ID: actionableDevUUID, Name: "dev", ExternalID: "111111111111"}, {ID: actionableOtherUUID, Name: "other", ExternalID: "222222222222"}, + }), &config.ExecutionScope{UserID: "u-1", AccountIDs: []string{"dev"}}}, {"literal external number", auth.ScopeForAccounts([]string{"222222222222"}), &config.ExecutionScope{UserID: "u-1", AccountIDs: []string{"222222222222"}}}, } From cdd9eeaee2bf6e28517d835f7270f754872e8b49 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 14:56:11 +0000 Subject: [PATCH 11/11] fix(auth,api): bind account names to the identifier in scope checks Review (rE7) found that an ID-scoped user could pass the purchase gate for an account outside their scope. resolveAccountNamesByID keyed names by both account ID and the free-text external ID in one map, so an editor of in-scope account Z who set Z's external ID to victim Y's ID got Z's name back for Y. AccountScope.Allows then resolved that name to Z, which is in scope, and allowed a purchase in Y. The same map fed the dashboard, ladder and RI reshape filters. Decide those gates on the record's cloud_account_id itself, the ID the purchase resolves its credentials from, and delete the dual-keyed map. Bind the name path in Allows as well, so a name only identifies an account through that account's own external ID and never through a value that is some registered account's ID. Callers that pass a name for an external ID already resolve it within the record's provider or require exactly one owner. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 +- .../api/account_scope_shared_name_test.go | 77 ++++++++++++++++- internal/api/handler.go | 2 +- internal/api/handler_dashboard.go | 7 +- internal/api/handler_dashboard_test.go | 6 +- internal/api/handler_history_test.go | 4 +- internal/api/handler_ladder.go | 3 +- .../api/handler_per_account_perms_test.go | 8 +- internal/api/handler_ri_exchange.go | 3 +- internal/api/scoping.go | 43 +++------- internal/auth/account_scope.go | 85 ++++++++++++++----- internal/auth/account_scope_test.go | 51 +++++++++-- 12 files changed, 213 insertions(+), 82 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea0a2fdd1..592c1cc9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,7 +50,11 @@ are the full record until the next release. renamed to that name. Group writes now store the ID of the one account a name identifies (and reject names that identify none or several), and migration 000145 rewrites existing name entries whose account predates the - grant; entries it cannot bind stop granting access (#1034) + grant; entries it cannot bind no longer match any account by name. The + purchase, dashboard, ladder and RI reshape scope checks decide on the + record's cloud account ID rather than on a name looked up for it, and a name + only identifies an account through that account's own, unambiguous external + ID (#1034) - API-key creation over HTTP rejects an explicit `max_amount: 0` spend cap with a 400 instead of minting the key with no cap, matching group writes (#980) - Group writes reject an explicit `max_amount: 0` spend cap with a 400 instead of storing it as no cap; omit the field for no limit (#133) - Remove debug console.log from frontend recommendation handler diff --git a/internal/api/account_scope_shared_name_test.go b/internal/api/account_scope_shared_name_test.go index f1b778f34..57316626d 100644 --- a/internal/api/account_scope_shared_name_test.go +++ b/internal/api/account_scope_shared_name_test.go @@ -207,7 +207,7 @@ func TestGetAccountScope_AccountListFailureIsIDOnly(t *testing.T) { // The other gate shapes take the same scope from getAccountScope: plan // accounts (requirePlanAccess / requirePlanMutationAccess) and the -// resolveAccountNamesByID map (validatePurchaseRecommendationScope). +// recommendation's cloud_account_id (validatePurchaseRecommendationScope). func TestAccountScope_SharedDisplayName_OtherGates(t *testing.T) { t.Parallel() ctx := context.Background() @@ -243,3 +243,78 @@ func TestAccountScope_SharedDisplayName_OtherGates(t *testing.T) { }) } } + +// rE7 on #1116: external IDs are free text, so an editor of in-scope account Z +// can set Z's external ID to victim Y's internal ID (and, with a different +// provider, to Y's provider account number). Every gate that decides on a +// record's cloud_account_id must decide on that ID, so neither change reaches +// Y. Before the fix the purchase gate looked the name up in a map keyed by +// both IDs and external IDs, got Z's name for Y's ID, and resolved it back to +// Z, which is in scope. +var ( + shadowVictim = config.CloudAccount{ID: "b1b1b1b1-0000-4000-8000-000000000001", Name: "prod-payments", Provider: "aws", ExternalID: "123456789012", Enabled: true} + shadowEditor = config.CloudAccount{ID: "b2b2b2b2-0000-4000-8000-000000000002", Name: "zz-team", Provider: "azure", ExternalID: "b1b1b1b1-0000-4000-8000-000000000001", Enabled: true} +) + +func TestAccountScope_EditorControlledExternalIDGrantsNothing(t *testing.T) { + t.Parallel() + ctx := context.Background() + session := &Session{UserID: "u-1"} + sameNumber := shadowEditor + sameNumber.ExternalID = shadowVictim.ExternalID // ambiguous: two accounts carry it + + for _, tc := range []struct { + name string + editor config.CloudAccount + targetID string + }{ + {"editor's external ID is the victim's ID", shadowEditor, shadowVictim.ID}, + {"ambiguous external ID", sameNumber, shadowVictim.ExternalID}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + // Listed in name order, as ListCloudAccounts returns them. + h := sharedNameHandler(t, []string{tc.editor.ID}, []config.CloudAccount{shadowVictim, tc.editor}, nil) + target, own := tc.targetID, tc.editor.ID + + err := h.validatePurchaseRecommendationScope(ctx, session, []config.RecommendationRecord{{CloudAccountID: &target}}) + var ce *clientError + require.ErrorAs(t, err, &ce, "a purchase in %s must be refused", target) + assert.Equal(t, 403, ce.code) + assert.NoError(t, h.validatePurchaseRecommendationScope(ctx, session, []config.RecommendationRecord{{CloudAccountID: &own}}), + "control: the editor's own account is still purchasable") + + scope, err := h.getAccountScope(ctx, session) + require.NoError(t, err) + recs := filterDashboardRecommendations(scope, []config.RecommendationRecord{{ID: "victim", CloudAccountID: &target}, {ID: "own", CloudAccountID: &own}}) + require.Len(t, recs, 1) + assert.Equal(t, "own", recs[0].ID, "dashboard") + + ladders, err := h.filterLadderConfigsByAllowedAccounts(ctx, session, []config.LadderConfigDB{{CloudAccountID: target}, {CloudAccountID: own}}) + require.NoError(t, err) + assert.Equal(t, []config.LadderConfigDB{{CloudAccountID: own}}, ladders, "ladder") + + for id, want := range map[string]bool{target: false, own: true} { + h.reshapeAccountResolver = func(context.Context) (string, error) { return id, nil } + inScope, err := h.reshapeCloudAccountInScope(ctx, session) + require.NoError(t, err) + assert.Equal(t, want, inScope, "RI reshape on %s", id) + } + }) + } +} + +// A legacy history row known only by an external ID that two registered +// accounts carry cannot be attributed, so it stays hidden from both. +func TestHandleRequest_AccountScope_AmbiguousExternalIDHistoryRow(t *testing.T) { + t.Parallel() + sameNumber := shadowEditor + sameNumber.ExternalID = shadowVictim.ExternalID + rows := []config.PurchaseHistoryRecord{{PurchaseID: "legacy", AccountID: shadowVictim.ExternalID, Status: "completed"}} + + assert.Equal(t, []string{"legacy"}, historyScopeIDs(t, []string{shadowVictim.ID}, []config.CloudAccount{shadowVictim}, rows), + "control: an unambiguous external ID identifies the in-scope account") + for _, scope := range [][]string{{shadowVictim.ID}, {sameNumber.ID}} { + assert.Empty(t, historyScopeIDs(t, scope, []config.CloudAccount{shadowVictim, sameNumber}, rows), "scope %v", scope) + } +} diff --git a/internal/api/handler.go b/internal/api/handler.go index 0c4e7ba13..f2dc5fd06 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -709,7 +709,7 @@ func (h *Handler) withRegisteredAccountNames(ctx context.Context, scope auth.Acc } known := make([]auth.RegisteredAccount, len(registered)) for i := range registered { - known[i] = auth.RegisteredAccount{ID: registered[i].ID, Name: registered[i].Name} + known[i] = auth.RegisteredAccount{ID: registered[i].ID, Name: registered[i].Name, ExternalID: registered[i].ExternalID} } return scope.WithRegisteredAccounts(known) } diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index 29d1b9b3c..4aeca1a17 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -46,7 +46,7 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun return nil, fmt.Errorf("failed to get recommendations: %w", err) } - recommendations = h.filterDashboardRecommendations(ctx, scope, recommendations) + recommendations = filterDashboardRecommendations(scope, recommendations) // Issue #196 — cap the headline "potential savings" by the per-account // coverage target. Without this scaling the dashboard always reports @@ -179,19 +179,18 @@ func (h *Handler) resolveAllowedAccountScope(ctx context.Context, session *Sessi // (already resolved into allowed) to the recommendations list before // aggregation so scoped users don't see cross-account totals. // Admin/unrestricted sessions pass through unchanged. -func (h *Handler) filterDashboardRecommendations(ctx context.Context, allowed auth.AccountScope, recs []config.RecommendationRecord) []config.RecommendationRecord { +func filterDashboardRecommendations(allowed auth.AccountScope, recs []config.RecommendationRecord) []config.RecommendationRecord { if allowed.AllowsAll() { return recs } - nameByID := h.resolveAccountNamesByID(ctx) filtered := make([]config.RecommendationRecord, 0, len(recs)) for _rvc := range recs { if recs[_rvc].CloudAccountID == nil { continue } id := *recs[_rvc].CloudAccountID - if allowed.Allows(id, nameByID[id]) { + if allowed.Allows(id, "") { filtered = append(filtered, recs[_rvc]) } } diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index 300507a4c..a2ca21846 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -1826,7 +1826,7 @@ func TestFilterDashboardRecommendations_FreshSlice(t *testing.T) { originalFirst := input[0] mockStore := new(MockConfigStore) - // resolveAccountNamesByID calls ListCloudAccounts via the Fn override path. + // getAccountScope calls ListCloudAccounts via the Fn override path. listCalled := false mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { listCalled = true @@ -1843,11 +1843,11 @@ func TestFilterDashboardRecommendations_FreshSlice(t *testing.T) { t.Cleanup(func() { mockAuth.AssertExpectations(t) }) handler := &Handler{auth: mockAuth, config: mockStore} - _ = listCalled // asserted implicitly: if not called the name map is empty and AccountScope.Allows uses only ID + _ = listCalled // asserted implicitly: if not called the scope has no registered accounts and AccountScope.Allows uses only ID scope, err := handler.getAccountScope(ctx, session) require.NoError(t, err) - filtered := handler.filterDashboardRecommendations(ctx, scope, input) + filtered := filterDashboardRecommendations(scope, input) // Only account A's rec must pass through. require.Len(t, filtered, 1) diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index 7ea0bb92b..53db575ed 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -510,8 +510,8 @@ func TestHandler_getHistory_ScopedUserSeesEmptyAccountRows(t *testing.T) { mockStore.On("ListPurchaseHistoryPage", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{ambientExec}, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) - // resolveAccountNamesByID must not error — return an empty accounts list - // so AccountScope.Allows falls back to ID-only matching (safe for our assertions). + // The account scope lists the registered accounts; an empty list leaves + // AccountScope.Allows matching by ID only (safe for our assertions). mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return nil, nil } diff --git a/internal/api/handler_ladder.go b/internal/api/handler_ladder.go index 20247759a..fc5a1be4a 100644 --- a/internal/api/handler_ladder.go +++ b/internal/api/handler_ladder.go @@ -54,11 +54,10 @@ func (h *Handler) filterLadderConfigsByAllowedAccounts(ctx context.Context, sess if allowed.AllowsAll() { return configs, nil } - nameByID := h.resolveAccountNamesByID(ctx) filtered := make([]config.LadderConfigDB, 0, len(configs)) for _rvc := range configs { c := configs[_rvc] - if allowed.Allows(c.CloudAccountID, nameByID[c.CloudAccountID]) { + if allowed.Allows(c.CloudAccountID, "") { filtered = append(filtered, c) } } diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index 7fa3be9b9..98090341e 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -92,8 +92,8 @@ func scopedReq() *events.LambdaFunctionURLRequest { } // permsAccountList returns a ListCloudAccountsFn-compatible slice containing -// both test accounts — used so resolveAccountNamesByID can map UUID→name for -// the display-name lookup of AccountScope.Allows. +// both test accounts — used so getAccountScope can register them for the +// display-name lookup of AccountScope.Allows. func permsAccountList() []config.CloudAccount { return []config.CloudAccount{ {ID: permsAccA, Name: permsAccAName, Provider: "aws"}, @@ -304,7 +304,7 @@ func TestPerAccountPerms_History_ListFilter(t *testing.T) { Return([]config.PurchaseHistoryRecord{rowA, rowB}, nil) mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything). Return([]config.PurchaseExecution{}, nil) - // resolveAccountNamesByID needs ListCloudAccounts so AccountScope.Allows can + // getAccountScope needs ListCloudAccounts so AccountScope.Allows can // resolve display names when the AccountID differs from the UUID. mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return permsAccountList(), nil @@ -1124,7 +1124,7 @@ func TestPerAccountPerms_ListConvertibleRIs_ScopedOutsideAllowedReturnsEmpty(t * ctx := context.Background() mockStore := new(MockConfigStore) - // resolveAccountNamesByID calls ListCloudAccounts so AccountScope.Allows can + // getAccountScope calls ListCloudAccounts so AccountScope.Allows can // resolve display names. Return both accounts so the name lookup is realistic. mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return permsAccountList(), nil diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 14e68379a..085c2e63a 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -1409,8 +1409,7 @@ func (h *Handler) reshapeCloudAccountInScope(ctx context.Context, session *Sessi if aErr != nil { return false, fmt.Errorf("failed to resolve cloud account scope: %w", aErr) } - nameByID := h.resolveAccountNamesByID(ctx) - return allowed.Allows(cloudAccountID, nameByID[cloudAccountID]), nil + return allowed.Allows(cloudAccountID, ""), nil } // requireReshapeAccountScope enforces the session's allowed_accounts scope for diff --git a/internal/api/scoping.go b/internal/api/scoping.go index 55c236ef5..9cd82cdef 100644 --- a/internal/api/scoping.go +++ b/internal/api/scoping.go @@ -170,6 +170,12 @@ func (h *Handler) requireAccountsAccess(ctx context.Context, session *Session, a // Admin / unrestricted sessions pass through. Recommendations with a nil // CloudAccountID are rejected when the session is scoped — we can't // attribute them, and silently letting them through would bypass the filter. +// +// The decision is on CloudAccountID itself, the internal account ID the +// purchase resolves its credentials from (GetCloudAccount), and never on a +// display name looked up for it. External IDs are free text an account editor +// sets, so a name keyed by identifier can belong to a different account than +// the one the purchase acts on. func (h *Handler) validatePurchaseRecommendationScope(ctx context.Context, session *Session, recs []config.RecommendationRecord) error { allowed, err := h.getAccountScope(ctx, session) if err != nil { @@ -178,14 +184,13 @@ func (h *Handler) validatePurchaseRecommendationScope(ctx context.Context, sessi if allowed.AllowsAll() { return nil } - nameByID := h.resolveAccountNamesByID(ctx) for i := range recs { rec := recs[i] if rec.CloudAccountID == nil { return NewClientError(400, fmt.Sprintf("recommendation %d has no cloud_account_id; scoped users cannot execute unattributed recommendations", i)) } id := *rec.CloudAccountID - if !allowed.Allows(id, nameByID[id]) { + if !allowed.Allows(id, "") { return NewClientError(403, fmt.Sprintf("recommendation %d targets account %s which is outside your allowed_accounts", i, id)) } } @@ -321,9 +326,9 @@ func (h *Handler) lookupCloudAccount(ctx context.Context, id string) (config.Clo // filter. purchase_history rows carry either the UUID FK (cloud_account_id) or // the cloud-provider external number (account_id) and frequently only one of // them, so a UUID-only WHERE silently drops the external-only rows. This -// resolver loads cloud_accounts once (reusing ListCloudAccounts, the same source -// as resolveAccountNamesByID), then returns the UUIDs unchanged plus the matched -// accounts' external ids grouped by provider. +// resolver loads cloud_accounts once (reusing ListCloudAccounts), then returns +// the UUIDs unchanged plus the matched accounts' external ids grouped by +// provider. // // The external ids are grouped by provider so the downstream predicate compares // each external number only against rows of its own provider @@ -539,31 +544,3 @@ func intersectDualColumnScope( } return uuids, externalIDsByProvider } - -// a map from account identifier → display name. The map is keyed by BOTH -// the internal UUID (CloudAccount.ID) and the cloud-provider external ID -// (CloudAccount.ExternalID, e.g. an AWS account number or Azure subscription -// ID). Dual-keying is necessary because different record types use different -// identifiers: recommendation records carry the UUID (cloud_account_id FK) -// while legacy purchase_history rows carry the external ID (account_id -// VARCHAR). Without both keys, the name lookup always misses for one path. -// -// Returns an empty map on error; callers fall through to ID-only matching, -// which is still safe (AccountScope.Allows falls back to ID comparison when -// the name is empty). -func (h *Handler) resolveAccountNamesByID(ctx context.Context) map[string]string { - accounts, err := h.config.ListCloudAccounts(ctx, config.CloudAccountFilter{}) - if err != nil { - return map[string]string{} - } - // Allocate 2x capacity since each account contributes up to two keys. - nameByID := make(map[string]string, len(accounts)*2) - for _rvc := range accounts { - a := accounts[_rvc] - nameByID[a.ID] = a.Name - if a.ExternalID != "" { - nameByID[a.ExternalID] = a.Name - } - } - return nameByID -} diff --git a/internal/auth/account_scope.go b/internal/auth/account_scope.go index 3c44dbb37..9d7ab864e 100644 --- a/internal/auth/account_scope.go +++ b/internal/auth/account_scope.go @@ -29,15 +29,21 @@ type AccountScope struct { // identifier the caller passes, never with a display name (issue #1034). Accounts []string - // idByUniqueName maps each display name carried by exactly one registered - // cloud account to that account's ID. It lets a caller that knows an - // account only by name (for example a legacy row keyed by external ID) - // identify it: Allows resolves the name to the account carrying it and - // then checks that account's ID against Accounts. A name shared by two - // accounts resolves to neither (issue #433). nil (the zero value, and every - // scope built without WithRegisteredAccounts) disables the lookup, so - // forgetting to populate it fails closed. - idByUniqueName map[string]string + // byUniqueName maps each display name carried by exactly one registered + // cloud account to that account. It lets a caller that knows an account by + // its external ID (for example a legacy row) identify it: Allows resolves + // the name to the account carrying it, checks that the identifier the + // caller passed really is that account's external ID, and then checks the + // account's ID against Accounts. A name shared by two accounts resolves to + // neither (issue #433). nil (the zero value, and every scope built without + // WithRegisteredAccounts) disables the lookup, so forgetting to populate + // it fails closed. + byUniqueName map[string]RegisteredAccount + + // internalIDs holds the ID of every registered account. External IDs are + // free text an account editor sets, so one can equal another account's + // ID; the name lookup never treats an internal ID as an external ID. + internalIDs map[string]struct{} // Unrestricted grants access to every account. Set it only where // unlimited access has been positively established. @@ -47,8 +53,9 @@ type AccountScope struct { // RegisteredAccount is the identity of one registered cloud account, as // WithRegisteredAccounts needs it. type RegisteredAccount struct { - ID string - Name string + ID string + Name string + ExternalID string } // UnrestrictedScope returns a scope granting every account. Use it only at a @@ -116,8 +123,10 @@ func ScopeFromLegacyList(accounts []string) AccountScope { // WithRegisteredAccounts returns a copy of the scope that can identify an // account by its display name. accounts must hold EVERY registered cloud -// account (all providers, enabled or not), so that a shared name is visible -// as shared. Only names that exactly one account carries are kept. +// account (all providers, enabled or not), with its ID, name and external ID, +// so that a shared name is visible as shared and no account's ID can pass for +// another account's external ID. Only names that exactly one account carries +// are kept. // // This never makes a scope entry match by name. Entries are account IDs, and // a stored name could otherwise outlive its account and grant whichever @@ -130,17 +139,19 @@ func (s AccountScope) WithRegisteredAccounts(accounts []RegisteredAccount) Accou return s } counts := make(map[string]int, len(accounts)) - ids := make(map[string]string, len(accounts)) + byName := make(map[string]RegisteredAccount, len(accounts)) + s.internalIDs = make(map[string]struct{}, len(accounts)) for _, a := range accounts { if a.Name != "" { counts[a.Name]++ - ids[a.Name] = a.ID + byName[a.Name] = a } + s.internalIDs[a.ID] = struct{}{} } - s.idByUniqueName = make(map[string]string, len(counts)) + s.byUniqueName = make(map[string]RegisteredAccount, len(counts)) for n, c := range counts { - if c == 1 && ids[n] != "" { - s.idByUniqueName[n] = ids[n] + if c == 1 && byName[n].ID != "" { + s.byUniqueName[n] = byName[n] } } return s @@ -148,9 +159,25 @@ func (s AccountScope) WithRegisteredAccounts(accounts []RegisteredAccount) Accou // Allows reports whether the scope permits the given cloud account. The // account matches when accountID is in the allow-list, or when the scope was -// built with WithRegisteredAccounts, exactly one registered account carries -// accountName, and THAT account's ID is in the allow-list. Pass "" for the -// name when unavailable. +// built with WithRegisteredAccounts and the name identifies the same account +// as accountID: exactly one registered account carries accountName, accountID +// is THAT account's external ID and no registered account's ID, and the +// account's ID is in the allow-list. Pass "" for the name when unavailable. +// +// The name is bound to the identifier because the two can come from different +// accounts. External IDs are free text, so an editor of an in-scope account +// can set its external ID to another account's ID; a caller that then looks +// the name up by that identifier gets the in-scope account's name for the +// other account. Without the binding the scope would grant the other account. +// The decision is therefore always on the ID of the account accountID +// denotes, never on the name alone. +// +// External IDs are unique per provider only, so the scope cannot tell which +// provider's account an external ID means. A caller that passes a name for an +// external ID must have resolved it unambiguously: within the record's +// provider, or across all providers only when exactly one account carries it +// (see historyRowAccountName and uniqueAccountByExternalID in package api). +// A caller that can resolve the account itself should pass its ID instead. // // A display name in the allow-list matches nothing: it is not bound to any // account, so it would grant whichever account carries the name today @@ -170,8 +197,20 @@ func (s AccountScope) Allows(accountID, accountName string) bool { if accountName == "" { return false } - namedID, ok := s.idByUniqueName[accountName] - return ok && s.contains(namedID) + named, ok := s.byUniqueName[accountName] + return ok && s.identifies(named, accountID) && s.contains(named.ID) +} + +// identifies reports whether id denotes the registered account a through a's +// external ID: it equals that external ID and is not the ID of any registered +// account. a's own ID needs no check here, because Allows has already matched +// the identifier itself against the allow-list. +func (s AccountScope) identifies(a RegisteredAccount, id string) bool { + if id == "" || id != a.ExternalID { + return false + } + _, isInternalID := s.internalIDs[id] + return !isInternalID } // contains reports whether id is a non-empty entry of the allow-list. diff --git a/internal/auth/account_scope_test.go b/internal/auth/account_scope_test.go index 1b762d5db..c712b2afd 100644 --- a/internal/auth/account_scope_test.go +++ b/internal/auth/account_scope_test.go @@ -64,11 +64,12 @@ func TestAccountScope_EmptyExplicitListGrantsNothing(t *testing.T) { func TestAccountScope_RestrictedAllowsOnlyItsOwn(t *testing.T) { s := ScopeForAccounts([]string{"acct-A", "uuid-prod"}).WithRegisteredAccounts([]RegisteredAccount{ - {ID: "uuid-prod", Name: "prod-name"}, {ID: "uuid-other", Name: "other-name"}, + {ID: "uuid-prod", Name: "prod-name", ExternalID: "ext-123"}, {ID: "uuid-other", Name: "other-name", ExternalID: "ext-456"}, }) assert.True(t, s.Allows("acct-A", ""), "matches by ID") assert.True(t, s.Allows("ext-123", "prod-name"), "a unique name identifies an account whose ID is in scope") + assert.False(t, s.Allows("ext-456", "prod-name"), "but only for that account's own external ID") assert.False(t, s.Allows("acct-B", ""), "an account outside the list is denied") assert.False(t, s.Allows("acct-B", "other-name")) assert.False(t, s.AllowsAll()) @@ -166,8 +167,8 @@ func TestAccountScope_NameEntryNeverMatches(t *testing.T) { // access, the account that takes the old name does not. func TestAccountScope_IDEntrySurvivesRenameAndReuse(t *testing.T) { afterRename := []RegisteredAccount{ - {ID: "uuid-orig", Name: "prod-old"}, // renamed away from "prod" - {ID: "uuid-new", Name: "prod"}, // took the old name + {ID: "uuid-orig", Name: "prod-old", ExternalID: "ext-orig"}, // renamed away from "prod" + {ID: "uuid-new", Name: "prod", ExternalID: "ext-new"}, // took the old name } s := ScopeForAccounts([]string{"uuid-orig"}).WithRegisteredAccounts(afterRename) @@ -181,7 +182,9 @@ func TestAccountScope_IDEntrySurvivesRenameAndReuse(t *testing.T) { // account, so it cannot be used to reach either of them. func TestAccountScope_SharedNameIdentifiesNoAccount(t *testing.T) { registered := []RegisteredAccount{ - {ID: "uuid-aws-prod", Name: "prod"}, {ID: "uuid-az-prod", Name: "prod"}, {ID: "uuid-solo", Name: "solo"}, + {ID: "uuid-aws-prod", Name: "prod", ExternalID: "ext-aws"}, + {ID: "uuid-az-prod", Name: "prod", ExternalID: "ext-az"}, + {ID: "uuid-solo", Name: "solo", ExternalID: "ext-solo"}, } s := ScopeForAccounts([]string{"uuid-aws-prod", "uuid-solo"}).WithRegisteredAccounts(registered) @@ -194,7 +197,7 @@ func TestAccountScope_SharedNameIdentifiesNoAccount(t *testing.T) { // Whichever copy is in scope, the shared name identifies neither. for _, id := range []string{"uuid-aws-prod", "uuid-az-prod"} { - assert.False(t, ScopeForAccounts([]string{id}).WithRegisteredAccounts(registered).Allows("ext", "prod"), + assert.False(t, ScopeForAccounts([]string{id}).WithRegisteredAccounts(registered).Allows("ext-aws", "prod"), "shared name must not resolve to %s", id) } } @@ -243,8 +246,44 @@ func TestAccountScope_WithRegisteredAccountsEdgeCases(t *testing.T) { assert.False(t, base.Allows("ext-prod", "prod"), "WithRegisteredAccounts returns a copy") exact := ScopeForAccounts([]string{"uuid-P"}).WithRegisteredAccounts([]RegisteredAccount{ - {ID: "uuid-P", Name: "Prod"}, {ID: "uuid-p", Name: "prod"}, + {ID: "uuid-P", Name: "Prod", ExternalID: "ext"}, {ID: "uuid-p", Name: "prod", ExternalID: "ext-lower"}, }) assert.True(t, exact.Allows("ext", "Prod"), "names are compared exactly") assert.False(t, exact.Allows("ext", "prod")) } + +// rE7 on #1116: the name and the identifier a caller passes can come from +// different accounts. External IDs are free text, so an editor of in-scope +// account Z can set Z's external ID to victim Y's internal ID; a lookup keyed +// by identifier then hands back Z's name for Y. The name must only ever +// identify the account the identifier denotes, so this is refused. +func TestAccountScope_NameMustIdentifyTheAccountOfTheIdentifier(t *testing.T) { + registered := []RegisteredAccount{ + {ID: "uuid-Y", Name: "prod-payments", ExternalID: "123456789012"}, + {ID: "uuid-Z", Name: "zz-team", ExternalID: "uuid-Y"}, // Z's external ID set to Y's ID + } + s := ScopeForAccounts([]string{"uuid-Z"}).WithRegisteredAccounts(registered) + + assert.False(t, s.Allows("uuid-Y", "zz-team"), "Z's name must not grant Y through Z's external ID") + assert.False(t, s.Allows("uuid-Y", "prod-payments")) + assert.False(t, s.Allows("123456789012", "zz-team"), "a name of another account never binds") + assert.True(t, s.Allows("uuid-Z", "zz-team"), "control: Z itself is still granted") +} + +// External IDs are unique per provider only, so two accounts can share one. +// The caller resolves which account a record means (within the record's +// provider) and passes that account's name; the name then binds only to its +// own account, never to the other account carrying the same external ID. +func TestAccountScope_SharedExternalIDBindsOnlyTheNamedAccount(t *testing.T) { + registered := []RegisteredAccount{ + {ID: "uuid-Y", Name: "prod-payments", ExternalID: "123456789012"}, + {ID: "uuid-Z", Name: "zz-team", ExternalID: "123456789012"}, // another provider + } + inY := ScopeForAccounts([]string{"uuid-Y"}).WithRegisteredAccounts(registered) + assert.True(t, inY.Allows("123456789012", "prod-payments"), "Y's own record") + assert.False(t, inY.Allows("123456789012", "zz-team"), "Z's record is not Y's") + + inZ := ScopeForAccounts([]string{"uuid-Z"}).WithRegisteredAccounts(registered) + assert.False(t, inZ.Allows("123456789012", "prod-payments"), "Y's record is not Z's") + assert.False(t, inZ.Allows("123456789012", ""), "without a name the shared number identifies no account") +}