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 1bfcb9d99..d7f2233cc 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -662,9 +662,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 { @@ -689,12 +690,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 @@ -704,11 +707,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_integration_test.go b/internal/api/handler_analytics_scope_provider_integration_test.go index 3ce59616c..0c9157c50 100644 --- a/internal/api/handler_analytics_scope_provider_integration_test.go +++ b/internal/api/handler_analytics_scope_provider_integration_test.go @@ -192,9 +192,10 @@ func TestAnalyticsScopedRawExternalIDIsProviderScoped_DB(t *testing.T) { db := setupAnalyticsScopeDB(t) ctx := t.Context() - // A scoped user reaches the registered account by name or by its raw - // external id; either way only the aws row may be aggregated. - for _, scope := range []string{scopeAWSAccount, scopeSharedID} { + // A scoped user reaches the registered account by its account ID or by its + // raw external id; either way only the aws row may be aggregated. Scope + // entries match account IDs only, never display names (#1034). + for _, scope := range []string{db.awsUUID, scopeSharedID} { t.Run("scope "+scope+", raw id request", func(t *testing.T) { got := db.seen(ctx, t, db.session(t, []string{scope}), scopeSharedID) assertSeen(t, got, []string{"aws"}, []string{scopeSvcAWS}) @@ -202,7 +203,7 @@ func TestAnalyticsScopedRawExternalIDIsProviderScoped_DB(t *testing.T) { } t.Run("scoped UUID request is unchanged", func(t *testing.T) { - got := db.seen(ctx, t, db.session(t, []string{scopeAWSAccount}), db.awsUUID) + got := db.seen(ctx, t, db.session(t, []string{db.awsUUID}), db.awsUUID) assertSeen(t, got, []string{"aws"}, []string{scopeSvcAWS}) }) @@ -222,7 +223,7 @@ func TestAnalyticsScopedRawExternalIDIsProviderScoped_DB(t *testing.T) { func TestAnalyticsScopedRawExternalIDConcurrentRegistration_DB(t *testing.T) { db := setupAnalyticsScopeDB(t) ctx := t.Context() - token := db.session(t, []string{scopeAWSAccount}) + token := db.session(t, []string{db.awsUUID}) stop := make(chan struct{}) var wg sync.WaitGroup diff --git a/internal/api/handler_analytics_scope_provider_test.go b/internal/api/handler_analytics_scope_provider_test.go index 80b8cf96a..ce2362140 100644 --- a/internal/api/handler_analytics_scope_provider_test.go +++ b/internal/api/handler_analytics_scope_provider_test.go @@ -90,7 +90,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) @@ -102,34 +102,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} - for _, scope := range []string{"in", "111"} { + for _, scope := range []string{"u-in", "111"} { status, uuids, externals := analyticsScopeCall(t, []string{scope}, "111", accounts) assert.Equal(t, 200, status, scope) assert.Nil(t, uuids, scope) assert.Equal(t, map[string][]string{"aws": {"111"}}, externals, "%s: raw id is scoped to its account's provider (#624)", scope) } - status, _, _ := analyticsScopeCall(t, []string{"in"}, "222", accounts) - + status, _, _ := analyticsScopeCall(t, []string{"u-in"}, "222", accounts) assert.Equal(t, 404, status) } @@ -139,7 +139,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) @@ -172,7 +172,7 @@ func TestHandleRequest_Analytics_UnregisteredRawID(t *testing.T) { // id, so the filter keeps the provider-unconstrained "" bucket. assert.Equal(t, map[string][]string{"": {"999"}}, externals) - status, _, _ = analyticsScopeCall(t, []string{"solo"}, "999", accounts) + status, _, _ = analyticsScopeCall(t, []string{"u-solo"}, "999", accounts) assert.Equal(t, 404, status) } @@ -214,14 +214,14 @@ func TestHandleRequest_Analytics_RawExternalIDProviderScopedOnEveryEndpoint(t *t for _, path := range analyticsPaths { t.Run(path, func(t *testing.T) { t.Parallel() - for _, scope := range []string{"data", "424242"} { + for _, scope := range []string{"u-gcp", "424242"} { status, uuids, externals := analyticsScopeCallPath(t, path, []string{scope}, "424242", accounts) assert.Equal(t, 200, status, scope) assert.Nil(t, uuids, scope) assert.Equal(t, map[string][]string{"gcp": {"424242"}}, externals, scope) } - status, uuids, externals := analyticsScopeCallPath(t, path, []string{"data"}, "u-gcp", accounts) + status, uuids, externals := analyticsScopeCallPath(t, path, []string{"u-gcp"}, "u-gcp", accounts) assert.Equal(t, 200, status, "UUID request") assert.Equal(t, []string{"u-gcp"}, uuids) assert.Equal(t, map[string][]string{"gcp": {"424242"}}, externals) @@ -230,7 +230,7 @@ func TestHandleRequest_Analytics_RawExternalIDProviderScopedOnEveryEndpoint(t *t assert.Equal(t, 200, status, "unrestricted") assert.Equal(t, map[string][]string{"": {"424242"}}, externals, "unrestricted raw id is unchanged") - status, _, externals = analyticsScopeCallPath(t, path, []string{"data"}, "777", accounts) + status, _, externals = analyticsScopeCallPath(t, path, []string{"u-gcp"}, "777", accounts) assert.Equal(t, 404, status, "out-of-scope raw id") assert.Nil(t, externals) }) @@ -245,11 +245,11 @@ func TestHandleRequest_Analytics_ProviderlessAccountFailsClosed(t *testing.T) { t.Parallel() accounts := []config.CloudAccount{{ID: "u-x", Name: "x", ExternalID: "555"}} - status, _, externals := analyticsScopeCall(t, []string{"x"}, "555", accounts) + status, _, externals := analyticsScopeCall(t, []string{"u-x"}, "555", accounts) assert.Equal(t, 404, status) assert.Nil(t, externals) - status, uuids, externals := analyticsScopeCall(t, []string{"x"}, "u-x", accounts) + status, uuids, externals := analyticsScopeCall(t, []string{"u-x"}, "u-x", accounts) assert.Equal(t, 200, status) assert.Equal(t, []string{"u-x"}, uuids) assert.Nil(t, externals) 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 8fd827728..2b0ce5681 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. resolveAnalyticsAccountFilter calls MatchesAccount +// analytics for account B. resolveAnalyticsAccountFilter calls AccountScope.Allows // and returns errNotFound on mismatch. // -// Regression: removing resolveAnalyticsAccountFilter (or its MatchesAccount +// Regression: removing resolveAnalyticsAccountFilter (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 1df14783b..6d82844e6 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2271,8 +2271,8 @@ const riExchangeHistoryLimit = 500 // // RI exchange records carry the 12-digit AWS account id from STS, so it is // resolved to its single registered AWS account and the scope is matched by -// that account's UUID or name. An unregistered or ambiguous id matches only a -// literal entry in the allow-list. A lookup failure errors out. +// that account's UUID. An unregistered or ambiguous id matches only a literal +// entry in the allow-list. A lookup failure errors out. func (h *Handler) loadRIExchangeHistory(ctx context.Context, session *Session) ([]config.RIExchangeRecord, error) { allowed, err := h.getAccountScope(ctx, session) if err != nil { diff --git a/internal/api/handler_ri_exchange_history_scope_integration_test.go b/internal/api/handler_ri_exchange_history_scope_integration_test.go index a02b0781b..64a3e8c22 100644 --- a/internal/api/handler_ri_exchange_history_scope_integration_test.go +++ b/internal/api/handler_ri_exchange_history_scope_integration_test.go @@ -60,7 +60,7 @@ func TestRIExchangeHistory_ScopedUserSeesOlderOwnRows_Integration(t *testing.T) } for _, tc := range []struct{ by, scope string }{ - {"uuid", acctB.ID}, {"name", acctB.Name}, {"sts account id", acctB.ExternalID}, + {"uuid", acctB.ID}, {"sts account id", acctB.ExternalID}, } { t.Run("scoped to B by "+tc.by, func(t *testing.T) { recs := history(t, tc.scope) @@ -74,6 +74,12 @@ func TestRIExchangeHistory_ScopedUserSeesOlderOwnRows_Integration(t *testing.T) }) } + // A display-name entry is not bound to an account and matches nothing + // (issue #1034), so it reaches none of B's exchanges. + t.Run("a name entry for B matches nothing", func(t *testing.T) { + require.Empty(t, history(t, acctB.Name)) + }) + t.Run("scoped to A and B still caps at 500 newest", func(t *testing.T) { recs := history(t, acctA.ID, acctB.ID) require.Len(t, recs, 500) 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 ba01d7f22..4563d55e9 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -3084,6 +3084,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: @@ -3107,6 +3112,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 @@ -3123,6 +3134,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 5d355f825..ba56a46ca 100644 --- a/internal/api/router_handlers_test.go +++ b/internal/api/router_handlers_test.go @@ -999,8 +999,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" @@ -1020,10 +1021,10 @@ func TestHandler_getRIExchangeHistory_AccountScope(t *testing.T) { }{ {name: "unrestricted sees all", scope: nil, want: 1}, {name: "scoped by account uuid", scope: []string{"u-prod"}, wantIDs: []string{stsAccount, "u-prod"}, want: 1}, - {name: "scoped by account name", scope: []string{"prod"}, wantIDs: []string{stsAccount, "prod"}, want: 1}, + {name: "a name entry matches nothing", scope: []string{"prod"}, wantIDs: []string{"prod"}, want: 0}, {name: "scoped by sts account id", scope: []string{stsAccount}, wantIDs: []string{stsAccount}, want: 1}, {name: "scoped to a different account", scope: []string{"u-dev"}, wantIDs: []string{"444455556666", "u-dev"}, want: 0}, - {name: "scoped to a different account by name", scope: []string{"dev"}, wantIDs: []string{"444455556666", "dev"}, want: 0}, + {name: "scoped to a different account by name", scope: []string{"dev"}, wantIDs: []string{"dev"}, want: 0}, {name: "same external id under another provider by uuid", scope: []string{"u-az"}, wantIDs: []string{"u-az"}, want: 0}, {name: "same external id under another provider by name", scope: []string{"az-prod"}, wantIDs: []string{"az-prod"}, 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 c251a1356..f41eb941b 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 59e7e091e..ffe3c7ef9 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) + } +}