fix(auth): bind group account scope entries to account IDs

CHANGELOG.md

@@ -45,6 +45,16 @@ are the full record until the next release.
 
 ### 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 000145 rewrites existing name entries whose account predates the
+  grant; entries it cannot bind no longer match any account by name. The
+  purchase, dashboard, ladder and RI reshape scope checks decide on the
+  record's cloud account ID rather than on a name looked up for it, and a name
+  only identifies an account through that account's own, unambiguous external
+  ID (#1034)
 - API-key creation over HTTP rejects an explicit `max_amount: 0` spend cap with a 400 instead of minting the key with no cap, matching group writes (#980)
 - Group writes reject an explicit `max_amount: 0` spend cap with a 400 instead of storing it as no cap; omit the field for no limit (#133)
 - Remove debug console.log from frontend recommendation handler

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}
@@ -186,7 +207,7 @@ func TestGetAccountScope_AccountListFailureIsIDOnly(t *testing.T) {
 
 // The other gate shapes take the same scope from getAccountScope: plan
 // accounts (requirePlanAccess / requirePlanMutationAccess) and the
-// resolveAccountNamesByID map (validatePurchaseRecommendationScope).
+// recommendation's cloud_account_id (validatePurchaseRecommendationScope).
 func TestAccountScope_SharedDisplayName_OtherGates(t *testing.T) {
 	t.Parallel()
 	ctx := context.Background()
@@ -222,3 +243,78 @@ func TestAccountScope_SharedDisplayName_OtherGates(t *testing.T) {
 		})
 	}
 }
+
+// rE7 on #1116: external IDs are free text, so an editor of in-scope account Z
+// can set Z's external ID to victim Y's internal ID (and, with a different
+// provider, to Y's provider account number). Every gate that decides on a
+// record's cloud_account_id must decide on that ID, so neither change reaches
+// Y. Before the fix the purchase gate looked the name up in a map keyed by
+// both IDs and external IDs, got Z's name for Y's ID, and resolved it back to
+// Z, which is in scope.
+var (
+	shadowVictim = config.CloudAccount{ID: "b1b1b1b1-0000-4000-8000-000000000001", Name: "prod-payments", Provider: "aws", ExternalID: "123456789012", Enabled: true}
+	shadowEditor = config.CloudAccount{ID: "b2b2b2b2-0000-4000-8000-000000000002", Name: "zz-team", Provider: "azure", ExternalID: "b1b1b1b1-0000-4000-8000-000000000001", Enabled: true}
+)
+
+func TestAccountScope_EditorControlledExternalIDGrantsNothing(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	session := &Session{UserID: "u-1"}
+	sameNumber := shadowEditor
+	sameNumber.ExternalID = shadowVictim.ExternalID // ambiguous: two accounts carry it
+
+	for _, tc := range []struct {
+		name     string
+		editor   config.CloudAccount
+		targetID string
+	}{
+		{"editor's external ID is the victim's ID", shadowEditor, shadowVictim.ID},
+		{"ambiguous external ID", sameNumber, shadowVictim.ExternalID},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			// Listed in name order, as ListCloudAccounts returns them.
+			h := sharedNameHandler(t, []string{tc.editor.ID}, []config.CloudAccount{shadowVictim, tc.editor}, nil)
+			target, own := tc.targetID, tc.editor.ID
+
+			err := h.validatePurchaseRecommendationScope(ctx, session, []config.RecommendationRecord{{CloudAccountID: &target}})
+			var ce *clientError
+			require.ErrorAs(t, err, &ce, "a purchase in %s must be refused", target)
+			assert.Equal(t, 403, ce.code)
+			assert.NoError(t, h.validatePurchaseRecommendationScope(ctx, session, []config.RecommendationRecord{{CloudAccountID: &own}}),
+				"control: the editor's own account is still purchasable")
+
+			scope, err := h.getAccountScope(ctx, session)
+			require.NoError(t, err)
+			recs := filterDashboardRecommendations(scope, []config.RecommendationRecord{{ID: "victim", CloudAccountID: &target}, {ID: "own", CloudAccountID: &own}})
+			require.Len(t, recs, 1)
+			assert.Equal(t, "own", recs[0].ID, "dashboard")
+
+			ladders, err := h.filterLadderConfigsByAllowedAccounts(ctx, session, []config.LadderConfigDB{{CloudAccountID: target}, {CloudAccountID: own}})
+			require.NoError(t, err)
+			assert.Equal(t, []config.LadderConfigDB{{CloudAccountID: own}}, ladders, "ladder")
+
+			for id, want := range map[string]bool{target: false, own: true} {
+				h.reshapeAccountResolver = func(context.Context) (string, error) { return id, nil }
+				inScope, err := h.reshapeCloudAccountInScope(ctx, session)
+				require.NoError(t, err)
+				assert.Equal(t, want, inScope, "RI reshape on %s", id)
+			}
+		})
+	}
+}
+
+// A legacy history row known only by an external ID that two registered
+// accounts carry cannot be attributed, so it stays hidden from both.
+func TestHandleRequest_AccountScope_AmbiguousExternalIDHistoryRow(t *testing.T) {
+	t.Parallel()
+	sameNumber := shadowEditor
+	sameNumber.ExternalID = shadowVictim.ExternalID
+	rows := []config.PurchaseHistoryRecord{{PurchaseID: "legacy", AccountID: shadowVictim.ExternalID, Status: "completed"}}
+
+	assert.Equal(t, []string{"legacy"}, historyScopeIDs(t, []string{shadowVictim.ID}, []config.CloudAccount{shadowVictim}, rows),
+		"control: an unambiguous external ID identifies the in-scope account")
+	for _, scope := range [][]string{{shadowVictim.ID}, {sameNumber.ID}} {
+		assert.Empty(t, historyScopeIDs(t, scope, []config.CloudAccount{shadowVictim, sameNumber}, rows), "scope %v", scope)
+	}
+}

internal/api/analytics_account_ids_integration_test.go

@@ -86,9 +86,10 @@ func TestAnalyticsAccountIDs_SumsSelectedAccountsOnly_DB(t *testing.T) {
 		assert.InDelta(t, tc.want, got.Summary.TotalMonthlySavings, 1e-9, tc.ids)
 	}
 
-	// A scoped user sees the sum of their accounts; a foreign id in the list
-	// refuses the whole request (404), never a partial or all-account total.
-	scoped := newHandler([]string{"a", "b"})
+	// A scoped user (scope entries are account IDs, #1034) sees the sum of
+	// their accounts; a foreign id in the list refuses the whole request
+	// (404), never a partial or all-account total.
+	scoped := newHandler([]string{ids["a"], ids["b"]})
 	got, err := query(scoped, ids["a"]+","+ids["b"])
 	require.NoError(t, err)
 	assert.InDelta(t, 30, got.Summary.TotalMonthlySavings, 1e-9)

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)

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)
 }