From 9203891f7eeb1421ecc999de183b98aa5b68cb90 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 06:16:28 +0200 Subject: [PATCH 1/4] refactor(auth): represent account scope as a type whose zero value denies Follow-up to the fail-closed producers. Those closed the three known paths that yielded an empty list, but the representation still conflated absence with unrestricted, so a fourth producer would reintroduce the bug -- and two consumers already read empty as "everything" on their own, independently of any producer: handler_marketplace.go:435 if len(allowed) == 0 { return nil } handler_purchases_revoke.go:398 if len(allowed) > 0 && !stringInSlice(...) Neither could be reached by fixing producers. The point of a type change is that it makes them stop compiling rather than merely stop being reachable. AccountScope{Accounts []string; Unrestricted bool} replaces the untyped []string. Its defining property is that the ZERO VALUE DENIES: AccountScope{} is restricted to nothing, so a forgotten initialisation, a value returned on an error path, or a field a later change fails to populate all fail closed. The doc comment states that as a requirement and explains why the flag must not be inverted to `Restricted bool` for readability -- that spelling would make the zero value grant everything and rebuild the original bug with the compiler's blessing. Unrestricted is now always set positively. The two legitimate unrestricted principals say so explicitly: the stateless admin API key returns UnrestrictedScope(), and a resolved list carrying "*" or no configuration converts through ScopeFromLegacyList, which is safe only because the resolver has already failed closed on an unestablishable scope. getAllowedAccounts becomes getAccountScope and returns AccountScope. The compiler then found every consumer, including one the enumeration had missed: filterReservationsByScopeIndex took the legacy []string and was invisible to a call-site scan. That is 19 sites, not the 18 counted by hand. Migration is behaviour-preserving for every legitimate principal, pinned by a parity test asserting AllowsAll agrees with IsUnrestrictedAccess and Allows agrees with MatchesAccount across every legacy input shape. Refs #1748. --- .../api/account_scope_fail_closed_test.go | 19 +-- internal/api/handler.go | 25 ++-- internal/api/handler_accounts.go | 13 +- internal/api/handler_analytics.go | 7 +- internal/api/handler_dashboard.go | 13 +- internal/api/handler_history.go | 7 +- internal/api/handler_ladder.go | 7 +- internal/api/handler_marketplace.go | 14 +- internal/api/handler_purchases_revoke.go | 8 +- internal/api/handler_recommendations.go | 7 +- internal/api/handler_ri_exchange.go | 26 ++-- internal/api/scoping.go | 23 ++- internal/auth/account_scope.go | 119 ++++++++++++++++ internal/auth/account_scope_test.go | 132 ++++++++++++++++++ 14 files changed, 338 insertions(+), 82 deletions(-) create mode 100644 internal/auth/account_scope.go create mode 100644 internal/auth/account_scope_test.go diff --git a/internal/api/account_scope_fail_closed_test.go b/internal/api/account_scope_fail_closed_test.go index b05c4b343..07701e02c 100644 --- a/internal/api/account_scope_fail_closed_test.go +++ b/internal/api/account_scope_fail_closed_test.go @@ -5,7 +5,6 @@ import ( "errors" "testing" - "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -31,10 +30,11 @@ func TestGetAllowedAccounts_FailsClosedWhenAuthMissing(t *testing.T) { ctx := context.Background() h := &Handler{auth: nil} - got, err := h.getAllowedAccounts(ctx, &Session{UserID: scopeSessionUser}) + got, err := h.getAccountScope(ctx, &Session{UserID: scopeSessionUser}) require.Error(t, err, "a nil auth service must refuse, not grant unrestricted access") - assert.Nil(t, got) + assert.False(t, got.AllowsAll(), "the scope returned alongside the error must not be unrestricted") + assert.False(t, got.Allows("any-account", ""), "and must grant no account at all") assert.Contains(t, err.Error(), "cannot establish account scope") } @@ -49,10 +49,11 @@ func TestGetAllowedAccounts_PropagatesResolverFailure(t *testing.T) { m.On("GetAllowedAccountsAPI", ctx, scopeSessionUser).Return([]string(nil), boom) h := &Handler{auth: m} - got, err := h.getAllowedAccounts(ctx, &Session{UserID: scopeSessionUser}) + got, err := h.getAccountScope(ctx, &Session{UserID: scopeSessionUser}) require.Error(t, err) - assert.Nil(t, got) + assert.False(t, got.AllowsAll(), "a failed resolution must not yield an unrestricted scope") + assert.False(t, got.Allows("any-account", "")) } // End-to-end through the shared scoping seam every scoped handler uses: an @@ -84,10 +85,10 @@ func TestGetAllowedAccounts_AdminAPIKeyStillUnrestricted(t *testing.T) { ctx := context.Background() h := &Handler{auth: new(MockAuthService)} - got, err := h.getAllowedAccounts(ctx, &Session{UserID: apiKeyAdminUserID}) + got, err := h.getAccountScope(ctx, &Session{UserID: apiKeyAdminUserID}) require.NoError(t, err, "the admin API key must remain unrestricted") - assert.True(t, auth.IsUnrestrictedAccess(got)) + assert.True(t, got.AllowsAll(), "expressed as a SET flag, never as an empty list") } // The other two legitimate unrestricted principals, resolved through the auth @@ -111,10 +112,10 @@ func TestGetAllowedAccounts_LegitimateUnrestrictedPrincipalsPass(t *testing.T) { m.On("GetAllowedAccountsAPI", ctx, scopeSessionUser).Return(tc.resolved, nil) h := &Handler{auth: m} - got, err := h.getAllowedAccounts(ctx, &Session{UserID: scopeSessionUser}) + got, err := h.getAccountScope(ctx, &Session{UserID: scopeSessionUser}) require.NoError(t, err, "a successfully resolved scope must not be refused") - assert.True(t, auth.IsUnrestrictedAccess(got), + assert.True(t, got.AllowsAll(), "a successful resolution to an empty/wildcard scope still means all accounts") }) } diff --git a/internal/api/handler.go b/internal/api/handler.go index b68d9bd0e..69da724bb 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -527,18 +527,27 @@ func (h *Handler) requirePermissionConstraints(ctx context.Context, session *Ses // access. Empty slice means all access (Administrators-group members carry the // "*" wildcard, which GetAllowedAccountsAPI surfaces as unrestricted). The // stateless admin API key has no user row, so it short-circuits to all access. -func (h *Handler) getAllowedAccounts(ctx context.Context, session *Session) ([]string, error) { +func (h *Handler) getAccountScope(ctx context.Context, session *Session) (auth.AccountScope, error) { if session.UserID == apiKeyAdminUserID { - return nil, nil // stateless admin API key = all access + // Positively unrestricted: the stateless admin API key is an + // infrastructure credential with no user row. Expressed as a set flag, + // never as an empty list (issue #1748). + return auth.UnrestrictedScope(), nil } if h.auth == nil { - // Fail closed. Returning an empty list here meant "all accounts", so a - // handler running without an auth service granted every caller access - // to every cloud account (issue #1748). Auth components must fail - // closed when nil, never fall through. - return nil, fmt.Errorf("authentication service not configured: cannot establish account scope") + // Fail closed. Auth components must fail closed when nil, never fall + // through. The zero AccountScope returned alongside this error denies + // everything even if a caller ignores the error. + return auth.AccountScope{}, fmt.Errorf("authentication service not configured: cannot establish account scope") } - return h.auth.GetAllowedAccountsAPI(ctx, session.UserID) + accounts, err := h.auth.GetAllowedAccountsAPI(ctx, session.UserID) + if err != nil { + return auth.AccountScope{}, err + } + // Safe here and only here: GetAllowedAccountsAPI has already failed closed + // on an unestablishable scope, so an empty list at this point genuinely + // means "no restriction configured". + return auth.ScopeFromLegacyList(accounts), nil } // setSecurityHeaders adds comprehensive security headers to the response. diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index 0ea530c7f..13eb07ac9 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -13,7 +13,6 @@ import ( "golang.org/x/oauth2" "github.com/LeanerCloud/CUDly/internal/accounts" - "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/config" "github.com/LeanerCloud/CUDly/internal/credentials" "github.com/LeanerCloud/CUDly/internal/oidc" @@ -105,15 +104,15 @@ func (h *Handler) listAccounts(ctx context.Context, req *events.LambdaFunctionUR // Filter by allowed accts if the user has restricted access. // An empty list or one containing "*" grants unrestricted access. // Otherwise each entry is matched against the account's ID or Name. - allowedAccounts, err := h.getAllowedAccounts(ctx, session) + allowedAccounts, err := h.getAccountScope(ctx, session) if err != nil { return nil, fmt.Errorf("failed to get allowed accounts: %w", err) } - if !auth.IsUnrestrictedAccess(allowedAccounts) { + if !allowedAccounts.AllowsAll() { filtered := accts[:0] for _rvc := range accts { acct := accts[_rvc] - if auth.MatchesAccount(allowedAccounts, acct.ID, acct.Name) { + if allowedAccounts.Allows(acct.ID, acct.Name) { filtered = append(filtered, acct) } } @@ -162,18 +161,18 @@ func (h *Handler) listAccountsMinimal(ctx context.Context, req *events.LambdaFun return nil, fmt.Errorf("accounts: %w", err) } - allowedAccounts, err := h.getAllowedAccounts(ctx, session) + allowedAccounts, err := h.getAccountScope(ctx, session) if err != nil { return nil, fmt.Errorf("failed to get allowed accounts: %w", err) } - unrestricted := auth.IsUnrestrictedAccess(allowedAccounts) + unrestricted := allowedAccounts.AllowsAll() // Build the minimal projection in place, applying allowed_accounts scoping // during the copy so a restricted user only ever sees their entitled rows. summaries := make([]AccountSummary, 0, len(accts)) for i := range accts { acct := &accts[i] - if !unrestricted && !auth.MatchesAccount(allowedAccounts, acct.ID, acct.Name) { + if !unrestricted && !allowedAccounts.Allows(acct.ID, acct.Name) { continue } summaries = append(summaries, AccountSummary{ diff --git a/internal/api/handler_analytics.go b/internal/api/handler_analytics.go index 9302d1dd1..3b6e0fe69 100644 --- a/internal/api/handler_analytics.go +++ b/internal/api/handler_analytics.go @@ -7,7 +7,6 @@ import ( "time" "github.com/LeanerCloud/CUDly/internal/analytics" - "github.com/LeanerCloud/CUDly/internal/auth" "github.com/aws/aws-lambda-go/events" ) @@ -232,18 +231,18 @@ func (h *Handler) getHistoryBreakdown(ctx context.Context, req *events.LambdaFun // without account_id or for an account outside their allowed_accounts list. // Admin/unrestricted sessions pass through (account_id may be empty). func (h *Handler) validateAnalyticsAccountScope(ctx context.Context, session *Session, accountID string) error { - allowed, err := h.getAllowedAccounts(ctx, session) + allowed, err := h.getAccountScope(ctx, session) if err != nil { return fmt.Errorf("failed to get allowed accounts: %w", err) } - if auth.IsUnrestrictedAccess(allowed) { + if allowed.AllowsAll() { return nil } if accountID == "" { return NewClientError(400, "account_id is required for scoped users") } nameByID := h.resolveAccountNamesByID(ctx) - if !auth.MatchesAccount(allowed, accountID, nameByID[accountID]) { + if !allowed.Allows(accountID, nameByID[accountID]) { return errNotFound } return nil diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index efbd28981..3dd624a72 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -8,7 +8,6 @@ import ( "strings" "time" - "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/config" "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/aws/aws-lambda-go/events" @@ -139,11 +138,11 @@ func (h *Handler) resolveDashboardAccountScope(ctx context.Context, params map[s // so a scoped user with zero accessible accounts sees zeroed KPIs rather than // everyone's data. func (h *Handler) resolveAllowedAccountScope(ctx context.Context, session *Session) (uuids []string, externalIDsByProvider map[string][]string, err error) { - allowed, err := h.getAllowedAccounts(ctx, session) + allowed, err := h.getAccountScope(ctx, session) if err != nil { return nil, nil, fmt.Errorf("failed to get allowed accounts: %w", err) } - if auth.IsUnrestrictedAccess(allowed) { + if allowed.AllowsAll() { return nil, nil, nil } accounts, err := h.config.ListCloudAccounts(ctx, config.CloudAccountFilter{}) @@ -155,7 +154,7 @@ func (h *Handler) resolveAllowedAccountScope(ctx context.Context, session *Sessi allowedUUIDs := []string{} for _rvc := range accounts { a := accounts[_rvc] - if auth.MatchesAccount(allowed, a.ID, a.Name) { + if allowed.Allows(a.ID, a.Name) { allowedUUIDs = append(allowedUUIDs, a.ID) } } @@ -167,11 +166,11 @@ func (h *Handler) resolveAllowedAccountScope(ctx context.Context, session *Sessi // to the recommendations list before aggregation so scoped users don't see // cross-account totals. Admin/unrestricted sessions pass through unchanged. func (h *Handler) filterDashboardRecommendations(ctx context.Context, session *Session, recs []config.RecommendationRecord) ([]config.RecommendationRecord, error) { - allowed, err := h.getAllowedAccounts(ctx, session) + allowed, err := h.getAccountScope(ctx, session) if err != nil { return nil, fmt.Errorf("failed to get allowed accounts: %w", err) } - if auth.IsUnrestrictedAccess(allowed) { + if allowed.AllowsAll() { return recs, nil } @@ -182,7 +181,7 @@ func (h *Handler) filterDashboardRecommendations(ctx context.Context, session *S continue } id := *recs[_rvc].CloudAccountID - if auth.MatchesAccount(allowed, id, nameByID[id]) { + if allowed.Allows(id, nameByID[id]) { filtered = append(filtered, recs[_rvc]) } } diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index f1cfcba97..53abf9139 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -8,7 +8,6 @@ import ( "strconv" "time" - "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/config" "github.com/LeanerCloud/CUDly/internal/runtime" "github.com/LeanerCloud/CUDly/pkg/logging" @@ -991,11 +990,11 @@ func appendMissing(dst []string, vals ...string) []string { // another user's multi-account in-flight row (including CreatedByUserEmail PII // and dollar amounts) is not visible to unrelated scoped users. func (h *Handler) filterPurchaseHistoryByAllowedAccounts(ctx context.Context, session *Session, purchases []config.PurchaseHistoryRecord) ([]config.PurchaseHistoryRecord, error) { - allowed, err := h.getAllowedAccounts(ctx, session) + allowed, err := h.getAccountScope(ctx, session) if err != nil { return nil, fmt.Errorf("failed to get allowed accounts: %w", err) } - if auth.IsUnrestrictedAccess(allowed) { + if allowed.AllowsAll() { return purchases, nil } nameByID := h.resolveAccountNamesByID(ctx) @@ -1013,7 +1012,7 @@ func (h *Handler) filterPurchaseHistoryByAllowedAccounts(ctx context.Context, se } continue } - if auth.MatchesAccount(allowed, p.AccountID, nameByID[p.AccountID]) { + if allowed.Allows(p.AccountID, nameByID[p.AccountID]) { filtered = append(filtered, p) } } diff --git a/internal/api/handler_ladder.go b/internal/api/handler_ladder.go index f8010e246..6d1ace5f6 100644 --- a/internal/api/handler_ladder.go +++ b/internal/api/handler_ladder.go @@ -6,7 +6,6 @@ import ( "encoding/json" "fmt" - "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/config" "github.com/aws/aws-lambda-go/events" ) @@ -47,18 +46,18 @@ func (h *Handler) getLadderConfigs(ctx context.Context, req *events.LambdaFuncti // outside the session's allowed_accounts. Admin/unrestricted sessions pass // through unchanged. func (h *Handler) filterLadderConfigsByAllowedAccounts(ctx context.Context, session *Session, configs []config.LadderConfigDB) ([]config.LadderConfigDB, error) { - allowed, err := h.getAllowedAccounts(ctx, session) + allowed, err := h.getAccountScope(ctx, session) if err != nil { return nil, fmt.Errorf("failed to get allowed accounts: %w", err) } - if auth.IsUnrestrictedAccess(allowed) { + if allowed.AllowsAll() { return configs, nil } nameByID := h.resolveAccountNamesByID(ctx) filtered := make([]config.LadderConfigDB, 0, len(configs)) for _rvc := range configs { c := configs[_rvc] - if auth.MatchesAccount(allowed, c.CloudAccountID, nameByID[c.CloudAccountID]) { + if allowed.Allows(c.CloudAccountID, nameByID[c.CloudAccountID]) { filtered = append(filtered, c) } } diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index 8dbcd89c2..ebc06950e 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -427,19 +427,17 @@ func (h *Handler) authorizeAllowedAccount(ctx context.Context, session *Session, return nil } } - allowed, err := h.getAllowedAccounts(ctx, session) + scope, err := h.getAccountScope(ctx, session) if err != nil { return fmt.Errorf("failed to check allowed accounts: %w", err) } - // Empty list means "no restriction" (the user has access to all accounts). - if len(allowed) == 0 { + // Was a hand-rolled `len(allowed) == 0` meaning "no restriction", which + // read an unestablishable scope as unrestricted independently of any + // producer (issue #1748). AccountScope makes unrestricted an explicit + // flag, so the zero value denies. + if scope.Allows(cloudAccountID, "") { return nil } - for _, id := range allowed { - if id == "*" || id == cloudAccountID { - return nil - } - } return NewClientError(403, "permission denied: purchase is in a cloud account not covered by your session's allowed accounts") } diff --git a/internal/api/handler_purchases_revoke.go b/internal/api/handler_purchases_revoke.go index ecbde82cd..0e2517bab 100644 --- a/internal/api/handler_purchases_revoke.go +++ b/internal/api/handler_purchases_revoke.go @@ -391,11 +391,15 @@ func (h *Handler) checkRevokeOwnAccountAccess(ctx context.Context, userID string if record.CloudAccountID == nil || *record.CloudAccountID == "" { return NewClientError(403, "permission denied: cannot verify ownership for this purchase") } - allowed, err := h.auth.GetAllowedAccountsAPI(ctx, userID) + // Goes through getAccountScope rather than calling GetAllowedAccountsAPI + // directly: the direct call skipped the admin-API-key and nil-auth + // branches, and the `len(allowed) > 0 &&` guard read an empty list as + // unrestricted independently of any producer (issue #1748). + scope, err := h.getAccountScope(ctx, &Session{UserID: userID}) if err != nil { return fmt.Errorf("account access check failed: %w", err) } - if len(allowed) > 0 && !stringInSlice(*record.CloudAccountID, allowed) { + if !scope.Allows(*record.CloudAccountID, "") { return NewClientError(403, "permission denied: purchase is in an account you do not have access to") } return nil diff --git a/internal/api/handler_recommendations.go b/internal/api/handler_recommendations.go index 1cd3ce8b2..4ae1e314e 100644 --- a/internal/api/handler_recommendations.go +++ b/internal/api/handler_recommendations.go @@ -6,7 +6,6 @@ import ( "fmt" "time" - "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/config" "github.com/LeanerCloud/CUDly/pkg/common" "github.com/aws/aws-lambda-go/events" @@ -122,11 +121,11 @@ func (h *Handler) getRecommendations(ctx context.Context, req *events.LambdaFunc // those belonging to accounts the user is allowed to access. Returns the // unmodified slice when the user has unrestricted access (empty allowed list). func (h *Handler) filterRecommendationsByAllowedAccounts(ctx context.Context, session *Session, recs []config.RecommendationRecord) ([]config.RecommendationRecord, error) { - allowedAccounts, err := h.getAllowedAccounts(ctx, session) + allowedAccounts, err := h.getAccountScope(ctx, session) if err != nil { return nil, fmt.Errorf("failed to get allowed accounts: %w", err) } - if auth.IsUnrestrictedAccess(allowedAccounts) { + if allowedAccounts.AllowsAll() { return recs, nil } @@ -150,7 +149,7 @@ func (h *Handler) filterRecommendationsByAllowedAccounts(ctx context.Context, se continue } id := *rec.CloudAccountID - if auth.MatchesAccount(allowedAccounts, id, nameByID[id]) { + if allowedAccounts.Allows(id, nameByID[id]) { filtered = append(filtered, rec) } } diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index a298663d5..20bd94926 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -364,11 +364,11 @@ func filterAzureReservationsBySubscription(reservations []azurecompute.Exchangea // logging the identifiers would just relocate the disclosure this filter // exists to prevent. func (h *Handler) filterAzureReservationsByScope(ctx context.Context, session *Session, reservations []azurecompute.ExchangeableReservation) ([]azurecompute.ExchangeableReservation, error) { - allowed, err := h.getAllowedAccounts(ctx, session) + allowed, err := h.getAccountScope(ctx, session) if err != nil { return nil, fmt.Errorf("failed to get allowed accounts: %w", err) } - if auth.IsUnrestrictedAccess(allowed) { + if allowed.AllowsAll() { // client.ListExchangeableReservations may return a nil slice for zero // results; every other path here returns a non-nil empty slice, so // normalize here too rather than letting an admin session alone see @@ -417,7 +417,7 @@ func azureScopeIndex(accounts []config.CloudAccount) map[string]config.CloudAcco // list covers (auth.MatchesAccount). Fails closed: a reservation with no // BillingScopeID, or one that resolves to no registered CloudAccount, is // dropped rather than kept. -func filterReservationsByScopeIndex(reservations []azurecompute.ExchangeableReservation, scopeToAccount map[string]config.CloudAccount, allowed []string) []azurecompute.ExchangeableReservation { +func filterReservationsByScopeIndex(reservations []azurecompute.ExchangeableReservation, scopeToAccount map[string]config.CloudAccount, allowed auth.AccountScope) []azurecompute.ExchangeableReservation { filtered := make([]azurecompute.ExchangeableReservation, 0, len(reservations)) for i := range reservations { r := reservations[i] @@ -428,7 +428,7 @@ func filterReservationsByScopeIndex(reservations []azurecompute.ExchangeableRese if !ok { continue } - if auth.MatchesAccount(allowed, account.ID, account.Name) { + if allowed.Allows(account.ID, account.Name) { filtered = append(filtered, r) } } @@ -792,18 +792,18 @@ func exchangeRegions(targets []AzureExchangeTargetBody, sources []AzureExchangeS // Unrestricted / admin sessions short-circuit before the account fetch, // mirroring requireExecutionAccess. func (h *Handler) requireAzureSubscriptionScope(ctx context.Context, session *Session, subscriptionID string) error { - allowed, err := h.getAllowedAccounts(ctx, session) + allowed, err := h.getAccountScope(ctx, session) if err != nil { return fmt.Errorf("failed to get allowed accounts: %w", err) } - if auth.IsUnrestrictedAccess(allowed) { + if allowed.AllowsAll() { return nil } account, err := h.config.GetCloudAccountByExternalID(ctx, "azure", subscriptionID) if err != nil { return fmt.Errorf("failed to resolve cloud account scope: %w", err) } - if account == nil || !auth.MatchesAccount(allowed, account.ID, account.Name) { + if account == nil || !allowed.Allows(account.ID, account.Name) { return errNotFound } return nil @@ -1262,11 +1262,11 @@ func (h *Handler) loadAWSConfigWithRegion(ctx context.Context, region string) (a // Used by listConvertibleRIs, getRIUtilization, and getReshapeRecommendations // to eliminate duplicated account-scoping blocks. func (h *Handler) reshapeCloudAccountInScope(ctx context.Context, session *Session) (bool, error) { - allowed, aErr := h.getAllowedAccounts(ctx, session) + allowed, aErr := h.getAccountScope(ctx, session) if aErr != nil { return false, fmt.Errorf("failed to get allowed accounts: %w", aErr) } - if auth.IsUnrestrictedAccess(allowed) { + if allowed.AllowsAll() { return true, nil } cloudAccountID, aErr := h.resolveReshapeCloudAccountID(ctx) @@ -1274,7 +1274,7 @@ func (h *Handler) reshapeCloudAccountInScope(ctx context.Context, session *Sessi return false, fmt.Errorf("failed to resolve cloud account scope: %w", aErr) } nameByID := h.resolveAccountNamesByID(ctx) - return auth.MatchesAccount(allowed, cloudAccountID, nameByID[cloudAccountID]), nil + return allowed.Allows(cloudAccountID, nameByID[cloudAccountID]), nil } // resolveReshapeCloudAccountID returns the cloud account ID for the running @@ -1992,16 +1992,16 @@ func (h *Handler) getRIExchangeHistory(ctx context.Context, req *events.LambdaFu // Filter records by the session's allowed_accounts against the record's // AccountID. Scoped users don't see history for accounts outside their // scope. Admin / unrestricted sessions pass through unchanged. - allowed, err := h.getAllowedAccounts(ctx, session) + allowed, err := h.getAccountScope(ctx, session) if err != nil { return nil, fmt.Errorf("failed to get allowed accounts: %w", err) } - if !auth.IsUnrestrictedAccess(allowed) { + if !allowed.AllowsAll() { nameByID := h.resolveAccountNamesByID(ctx) filtered := records[:0] for _rvc := range records { r := records[_rvc] - if auth.MatchesAccount(allowed, r.AccountID, nameByID[r.AccountID]) { + if allowed.Allows(r.AccountID, nameByID[r.AccountID]) { filtered = append(filtered, r) } } diff --git a/internal/api/scoping.go b/internal/api/scoping.go index 05a69e76e..c769c7c6b 100644 --- a/internal/api/scoping.go +++ b/internal/api/scoping.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" - "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/config" ) @@ -32,14 +31,14 @@ func (h *Handler) requireAccountAccess(ctx context.Context, session *Session, ac return nil, errNotFound } - allowed, err := h.getAllowedAccounts(ctx, session) + allowed, err := h.getAccountScope(ctx, session) if err != nil { return nil, fmt.Errorf("failed to get allowed accounts: %w", err) } - if auth.IsUnrestrictedAccess(allowed) { + if allowed.AllowsAll() { return account, nil } - if !auth.MatchesAccount(allowed, account.ID, account.Name) { + if !allowed.Allows(account.ID, account.Name) { return nil, errNotFound } return account, nil @@ -55,11 +54,11 @@ func (h *Handler) requireAccountAccess(ctx context.Context, session *Session, ac // caller passes here. This is the plan-level analog of requireAccountAccess // and is used by the plans/purchases/ri-exchange per-record scoping. func (h *Handler) requirePlanAccess(ctx context.Context, session *Session, planID string) error { - allowed, err := h.getAllowedAccounts(ctx, session) + allowed, err := h.getAccountScope(ctx, session) if err != nil { return fmt.Errorf("failed to get allowed accounts: %w", err) } - if auth.IsUnrestrictedAccess(allowed) { + if allowed.AllowsAll() { return nil } accounts, err := h.config.GetPlanAccounts(ctx, planID) @@ -68,7 +67,7 @@ func (h *Handler) requirePlanAccess(ctx context.Context, session *Session, planI } for _rvc := range accounts { acct := accounts[_rvc] - if auth.MatchesAccount(allowed, acct.ID, acct.Name) { + if allowed.Allows(acct.ID, acct.Name) { return nil } } @@ -81,11 +80,11 @@ func (h *Handler) requirePlanAccess(ctx context.Context, session *Session, planI // CloudAccountID are rejected when the session is scoped — we can't // attribute them, and silently letting them through would bypass the filter. func (h *Handler) validatePurchaseRecommendationScope(ctx context.Context, session *Session, recs []config.RecommendationRecord) error { - allowed, err := h.getAllowedAccounts(ctx, session) + allowed, err := h.getAccountScope(ctx, session) if err != nil { return fmt.Errorf("failed to get allowed accounts: %w", err) } - if auth.IsUnrestrictedAccess(allowed) { + if allowed.AllowsAll() { return nil } nameByID := h.resolveAccountNamesByID(ctx) @@ -95,7 +94,7 @@ func (h *Handler) validatePurchaseRecommendationScope(ctx context.Context, sessi return NewClientError(400, fmt.Sprintf("recommendation %d has no cloud_account_id; scoped users cannot execute unattributed recommendations", i)) } id := *rec.CloudAccountID - if !auth.MatchesAccount(allowed, id, nameByID[id]) { + if !allowed.Allows(id, nameByID[id]) { return NewClientError(403, fmt.Sprintf("recommendation %d targets account %s which is outside your allowed_accounts", i, id)) } } @@ -114,11 +113,11 @@ func (h *Handler) validatePurchaseRecommendationScope(ctx context.Context, sessi // round-trip (and to keep existing unit-test fixtures for admin operations // working without adding execution mocks). func (h *Handler) requireExecutionAccess(ctx context.Context, session *Session, executionID string) error { - allowed, err := h.getAllowedAccounts(ctx, session) + allowed, err := h.getAccountScope(ctx, session) if err != nil { return fmt.Errorf("failed to get allowed accounts: %w", err) } - if auth.IsUnrestrictedAccess(allowed) { + if allowed.AllowsAll() { return nil } execution, err := h.config.GetExecutionByID(ctx, executionID) diff --git a/internal/auth/account_scope.go b/internal/auth/account_scope.go new file mode 100644 index 000000000..5146e2ba8 --- /dev/null +++ b/internal/auth/account_scope.go @@ -0,0 +1,119 @@ +package auth + +import "strings" + +// AccountScope is the set of cloud accounts a principal may access. +// +// # The zero value MUST deny. This is a requirement, not an observation. +// +// AccountScope{} is {Unrestricted: false, Accounts: nil} -- restricted to +// nothing, granting access to no account at all. That is deliberate and +// load-bearing: a forgotten initialisation, a value returned on an error path, +// or a field a later change fails to populate all fail CLOSED. +// +// Do NOT invert the flag to `Restricted bool` for readability. That reads +// marginally better and is the entire bug this type exists to remove: it would +// make the zero value grant everything, so absence would once again mean +// unrestricted -- with a compiler now blessing it. Issue #1748 is precisely +// that failure in its untyped form: `[]string` used both for "no restriction +// configured" and for "we could not work out your scope", so every resolution +// failure silently granted access to every account. +// +// Unrestricted must therefore always be set POSITIVELY by a caller that has +// established the principal genuinely has unlimited access -- the stateless +// 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, mirroring MatchesAccount. + Accounts []string + + // Unrestricted grants access to every account. Set it only where + // unlimited access has been positively established. + Unrestricted bool +} + +// 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. +func UnrestrictedScope() AccountScope { + return AccountScope{Unrestricted: true} +} + +// ScopeForAccounts returns a scope limited to the given accounts. +// +// An empty list yields a scope that grants NOTHING, which is the opposite of +// the legacy []string convention where empty meant "all accounts". Callers +// converting a legacy list must decide which they mean rather than passing it +// through; ScopeFromLegacyList exists for the one place that still has to. +func ScopeForAccounts(accounts []string) AccountScope { + return AccountScope{Accounts: accounts} +} + +// ScopeFromLegacyList converts a resolved []string using the historical +// convention where empty or a "*" entry means unrestricted. +// +// It is safe ONLY for a list that came from a SUCCESSFUL resolution, because +// it cannot tell "no restriction configured" from "resolution failed" -- they +// are the same value, which is issue #1748. Callers must have already failed +// closed on the failure case (see Service.ResolveAllowedAccounts) before +// calling this. +func ScopeFromLegacyList(accounts []string) AccountScope { + if IsUnrestrictedAccess(accounts) { + return UnrestrictedScope() + } + return ScopeForAccounts(accounts) +} + +// Allows reports whether the scope permits the given cloud account, matched by +// internal ID or display name. Pass "" for the name when unavailable. +// +// A restricted scope with no accounts allows nothing, which is what makes the +// zero value deny. +func (s AccountScope) Allows(accountID, accountName string) bool { + if s.Unrestricted { + return true + } + for _, a := range s.Accounts { + if a == accountID { + return true + } + if accountName != "" && a == accountName { + return true + } + } + return false +} + +// AllowsAll reports whether the scope is unrestricted. Prefer Allows; this +// exists for the few sites that must branch on unrestricted-ness itself, such +// as skipping a filter entirely. +func (s AccountScope) AllowsAll() bool { + return s.Unrestricted +} + +// FilterIDs returns the subset of ids the scope permits. An unrestricted scope +// returns ids unchanged. +func (s AccountScope) FilterIDs(ids []string) []string { + if s.Unrestricted { + return ids + } + out := make([]string, 0, len(ids)) + for _, id := range ids { + if s.Allows(id, "") { + out = append(out, id) + } + } + return out +} + +// String renders the scope for error messages and logs. +func (s AccountScope) String() string { + if s.Unrestricted { + return "all accounts" + } + if len(s.Accounts) == 0 { + return "no accounts" + } + return strings.Join(s.Accounts, ", ") +} diff --git a/internal/auth/account_scope_test.go b/internal/auth/account_scope_test.go new file mode 100644 index 000000000..bcbe9c2cb --- /dev/null +++ b/internal/auth/account_scope_test.go @@ -0,0 +1,132 @@ +package auth + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The defining property of AccountScope (issue #1748): THE ZERO VALUE DENIES. +// +// This is the reason the type exists. The untyped []string it replaces used +// the same value -- empty -- for "no restriction configured" and for "we could +// not establish your scope", so every resolution failure silently granted +// access to every account. +// +// If someone later inverts the flag to `Restricted bool` because it reads +// better, the zero value starts granting everything and the bug returns with +// the compiler's blessing. These tests are the barrier. + +func TestAccountScope_ZeroValueDeniesEverything(t *testing.T) { + var zero AccountScope + + assert.False(t, zero.AllowsAll(), "the zero value must NOT be unrestricted") + assert.False(t, zero.Allows("any-account-id", ""), "the zero value must allow no account") + assert.False(t, zero.Allows("any-account-id", "any-name")) + assert.Empty(t, zero.FilterIDs([]string{"a", "b", "c"}), + "the zero value must filter every account out") + assert.Equal(t, "no accounts", zero.String()) +} + +// A struct returned on an error path, or one whose fields a later change fails +// to populate, must deny. Same property, stated the way it will actually be +// encountered. +func TestAccountScope_UninitialisedOnErrorPathDenies(t *testing.T) { + failing := func() (AccountScope, error) { + var s AccountScope // deliberately not populated + return s, assert.AnError + } + s, err := failing() + require.Error(t, err) + assert.False(t, s.Allows("acct-A", ""), "a scope returned alongside an error must not grant access") +} + +func TestAccountScope_UnrestrictedMustBeSetPositively(t *testing.T) { + assert.True(t, UnrestrictedScope().AllowsAll()) + assert.True(t, UnrestrictedScope().Allows("literally-anything", "")) + assert.Equal(t, []string{"a", "b"}, UnrestrictedScope().FilterIDs([]string{"a", "b"})) + assert.Equal(t, "all accounts", UnrestrictedScope().String()) +} + +// ScopeForAccounts with an EMPTY list grants nothing -- the opposite of the +// legacy []string convention. Pinned because this inversion is the whole point +// and is exactly what a careless migration would get backwards. +func TestAccountScope_EmptyExplicitListGrantsNothing(t *testing.T) { + s := ScopeForAccounts(nil) + assert.False(t, s.AllowsAll(), "an empty explicit list is NOT unrestricted") + assert.False(t, s.Allows("acct-A", "")) + + s2 := ScopeForAccounts([]string{}) + assert.False(t, s2.AllowsAll()) + assert.False(t, s2.Allows("acct-A", "")) +} + +func TestAccountScope_RestrictedAllowsOnlyItsOwn(t *testing.T) { + s := ScopeForAccounts([]string{"acct-A", "prod-name"}) + + assert.True(t, s.Allows("acct-A", ""), "matches by ID") + assert.True(t, s.Allows("some-uuid", "prod-name"), "matches by display name") + 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()) + assert.Equal(t, []string{"acct-A"}, s.FilterIDs([]string{"acct-A", "acct-B"})) +} + +// ScopeFromLegacyList preserves the historical convention, and is safe only +// for an already-successful resolution. Both spellings of unrestricted must +// convert to a positively-set flag rather than to an empty restricted scope. +func TestAccountScope_FromLegacyList(t *testing.T) { + for _, tc := range []struct { + name string + in []string + wantUnrestricted bool + }{ + {"nil means unrestricted under the legacy convention", nil, true}, + {"empty means unrestricted under the legacy convention", []string{}, true}, + {"wildcard means unrestricted", []string{"*"}, true}, + {"wildcard among others still means unrestricted", []string{"acct-A", "*"}, true}, + {"a real list stays restricted", []string{"acct-A"}, false}, + } { + t.Run(tc.name, func(t *testing.T) { + s := ScopeFromLegacyList(tc.in) + assert.Equal(t, tc.wantUnrestricted, s.AllowsAll()) + if tc.wantUnrestricted { + assert.True(t, s.Allows("anything", "")) + } else { + assert.True(t, s.Allows("acct-A", "")) + assert.False(t, s.Allows("acct-B", "")) + } + }) + } +} + +// The migration must not accidentally promote a scoped principal to +// unrestricted -- the control the team lead asked for explicitly. +func TestAccountScope_ScopedPrincipalStaysLimited(t *testing.T) { + s := ScopeFromLegacyList([]string{"acct-A", "acct-B"}) + + require.False(t, s.AllowsAll(), "a genuinely scoped principal must not become unrestricted") + assert.True(t, s.Allows("acct-A", "")) + assert.True(t, s.Allows("acct-B", "")) + assert.False(t, s.Allows("acct-C", "")) + 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) { + for _, legacy := range [][]string{ + nil, {}, {"*"}, {"acct-A"}, {"acct-A", "acct-B"}, {"prod-name"}, + } { + 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) + } + } +} From 849916e73ac70335997f2c4f7f2dc905a2f58ef0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 06:36:27 +0200 Subject: [PATCH 2/4] docs(auth): state what actually makes ScopeFromLegacyList safe The doc comment justified the empty-means-unrestricted conversion as "safe only for a successful resolution". That was true but underspecified, and the underspecification hid a real dependency: under the earlier resolver, which refused only TOTAL failure, this function would have converted a widened empty union into UnrestrictedScope() -- carrying #1748 forward inside the new type with the compiler blessing it. The guarantee it depends on is not "the resolver returned successfully". It is "the resolver refuses any empty union that a skipped group could have caused". Spell out all three refused inputs so a future change to any of them shows up as a change to this function's precondition. Adds a test pinning the boundary: the legacy conversion maps both spellings of empty to unrestricted, while ScopeForAccounts -- the constructor for raw, unvetted input -- treats empty as nothing. Refs #1748. --- internal/auth/account_scope.go | 29 ++++++++++++++++++++++++----- internal/auth/account_scope_test.go | 20 ++++++++++++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/internal/auth/account_scope.go b/internal/auth/account_scope.go index 5146e2ba8..d33ed87ac 100644 --- a/internal/auth/account_scope.go +++ b/internal/auth/account_scope.go @@ -53,11 +53,30 @@ func ScopeForAccounts(accounts []string) AccountScope { // ScopeFromLegacyList converts a resolved []string using the historical // convention where empty or a "*" entry means unrestricted. // -// It is safe ONLY for a list that came from a SUCCESSFUL resolution, because -// it cannot tell "no restriction configured" from "resolution failed" -- they -// are the same value, which is issue #1748. Callers must have already failed -// closed on the failure case (see Service.ResolveAllowedAccounts) before -// calling this. +// ScopeFromLegacyList(nil) and ScopeFromLegacyList([]) both return +// UnrestrictedScope(). That is deliberate and it is also the dangerous part, +// so be precise about what makes it safe. +// +// This function CANNOT distinguish "no restriction configured" from "we could +// not work out this principal's scope" -- they are the same empty value, which +// is issue #1748 in its entirety. It therefore carries no safety of its own. +// It is safe only because Service.ResolveAllowedAccounts has ALREADY refused +// every input that would make empty mean the wrong thing: +// +// - a store error, or a missing user row -> error, never reaches here +// - every group unresolvable -> error +// - SOME group unresolvable AND the surviving union empty -> error +// +// That third condition is the one to keep in view. An earlier version of the +// resolver refused only total failure, and under it this function WOULD have +// converted a widened empty union into UnrestrictedScope() -- carrying the bug +// forward inside the new type, with the compiler blessing it. The guarantee +// this depends on is not "the resolver returns successfully"; it is "the +// resolver refuses any empty union that a skipped group could have caused". +// +// Do not call this on a []string from any other source. If a new producer +// appears, give it the same guarantee first or build the AccountScope +// directly with ScopeForAccounts / UnrestrictedScope. func ScopeFromLegacyList(accounts []string) AccountScope { if IsUnrestrictedAccess(accounts) { return UnrestrictedScope() diff --git a/internal/auth/account_scope_test.go b/internal/auth/account_scope_test.go index bcbe9c2cb..c4b8d447b 100644 --- a/internal/auth/account_scope_test.go +++ b/internal/auth/account_scope_test.go @@ -130,3 +130,23 @@ func TestAccountScope_ParityWithLegacyHelpers(t *testing.T) { } } } + +// ScopeFromLegacyList's safety is entirely borrowed from the resolver, so pin +// the boundary: it maps BOTH spellings of empty to unrestricted, which is +// correct only for inputs the resolver has already vetted. +// +// This is the test that would have caught the type change carrying #1748 +// forward: if the resolver stopped refusing a widened empty union, this +// conversion would silently produce UnrestrictedScope() from it. +func TestAccountScope_LegacyConversionIsBorrowedSafety(t *testing.T) { + assert.True(t, ScopeFromLegacyList(nil).AllowsAll(), + "empty means unrestricted under the legacy convention -- safe ONLY post-resolver") + assert.True(t, ScopeFromLegacyList([]string{}).AllowsAll()) + + // The corollary: a caller that has NOT been through the resolver must not + // use this. ScopeForAccounts is the safe constructor for raw input, and it + // treats empty as "nothing" rather than "everything". + assert.False(t, ScopeForAccounts([]string{}).AllowsAll(), + "the raw-input constructor must NOT inherit the legacy empty-means-all rule") + assert.False(t, ScopeForAccounts(nil).Allows("acct-A", "")) +} From 090706074e2dbf95942c128ad74e76e2b9d483f0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 07:07:31 +0200 Subject: [PATCH 3/4] style(api): rename a local that shadowed the accounts package gocritic importShadow: the local holding the resolved allow-list was named `accounts`, shadowing the imported internal/accounts package. Refs #1748. --- internal/api/handler.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/api/handler.go b/internal/api/handler.go index 69da724bb..9adf23d2d 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -540,14 +540,14 @@ func (h *Handler) getAccountScope(ctx context.Context, session *Session) (auth.A // everything even if a caller ignores the error. return auth.AccountScope{}, fmt.Errorf("authentication service not configured: cannot establish account scope") } - accounts, err := h.auth.GetAllowedAccountsAPI(ctx, session.UserID) + resolved, err := h.auth.GetAllowedAccountsAPI(ctx, session.UserID) if err != nil { return auth.AccountScope{}, err } // Safe here and only here: GetAllowedAccountsAPI has already failed closed // on an unestablishable scope, so an empty list at this point genuinely // means "no restriction configured". - return auth.ScopeFromLegacyList(accounts), nil + return auth.ScopeFromLegacyList(resolved), nil } // setSecurityHeaders adds comprehensive security headers to the response. From d0c2dc19e8dc31d0b24df68a95e3b8af9279cb6e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 09:48:06 +0200 Subject: [PATCH 4/4] docs(api,auth): correct comments that still describe the replaced representation D1 -- getAccountScope's godoc was wrong three ways at once, on the one seam this change pivots on. It named getAllowedAccounts (removed), described the return as a list of account IDs (it is an AccountScope), and stated "empty slice means all access" -- the exact hazard AccountScope abolishes, and now false of the value returned, whose empty form DENIES. A reader trusting it would have drawn precisely the inverted conclusion. Rewritten to state the zero-value-denies property, that unrestricted is only ever set positively, and that every unestablishable path returns an error beside a zero scope. D2 -- five further comments still described the removed representation: handler.go:224, handler_dashboard.go, handler_ri_exchange.go and two in scoping.go, plus internal/auth/group_ceiling.go which referenced "getAllowedAccounts in internal/api" -- a name this change removes, so the break belongs here even though the file is not otherwise touched. One test comment naming the live seam is corrected too; the remaining test-file mentions are accurate past-tense history of the defect and are left alone. D3 -- ScopeForAccounts treats "*" as a LITERAL account identifier, which inverts the legacy meaning rather than merely differing from it: ScopeForAccounts(["*"]) AllowsAll=false Allows("acct-A")=false ScopeFromLegacyList(["*"]) AllowsAll=true Allows("acct-A")=true Unreachable today, since production builds scopes only through ScopeFromLegacyList and UnrestrictedScope. But the obvious future call, ScopeForAccounts(group.AllowedAccounts), would silently produce a scope that denies everything for any wildcard-carrying group -- and all seven seeded groups ship allowed_accounts = ARRAY['*']. The doc warned only about the empty list; it now names the wildcard asymmetry and says which constructor to use. Documentation only: no behaviour change, verified by execution for D3. Refs #1748. --- internal/api/grantscoped_test.go | 2 +- internal/api/handler.go | 20 +++++++++++++++----- internal/api/handler_dashboard.go | 2 +- internal/api/handler_ri_exchange.go | 4 ++-- internal/api/scoping.go | 5 +++-- internal/auth/account_scope.go | 22 +++++++++++++++++----- internal/auth/group_ceiling.go | 2 +- 7 files changed, 40 insertions(+), 17 deletions(-) diff --git a/internal/api/grantscoped_test.go b/internal/api/grantscoped_test.go index 659f4304e..2a36c39d8 100644 --- a/internal/api/grantscoped_test.go +++ b/internal/api/grantscoped_test.go @@ -19,7 +19,7 @@ import ( // are green": the suite had no restriction to enforce. // // These pin the shared seam every scoped handler funnels through -// (getAllowedAccounts -> requireAccountAccess / requirePlanAccess), so a +// (getAccountScope -> requireAccountAccess / requirePlanAccess), so a // regression in the seam fails here rather than silently in production. const ( diff --git a/internal/api/handler.go b/internal/api/handler.go index 9adf23d2d..f6fd36612 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -221,7 +221,7 @@ func NewHandler(cfg HandlerConfig) *Handler { // API-key session. It has no backing user row in the auth store, so any code // that would otherwise resolve group-derived permissions for it must treat it // as full-access up front (the API key is an infrastructure credential, not a -// user). See requirePermission / requireAdmin / getAllowedAccounts. +// user). See requirePermission / requireAdmin / getAccountScope. // It is also the actor identity threaded into the group-write grant ceiling, // which recognizes it and measures the key against a bare {admin, *} holding // (auth.AdminAPIKeyActorID) rather than failing the user lookup. @@ -523,10 +523,20 @@ func (h *Handler) requirePermissionConstraints(ctx context.Context, session *Ses return nil } -// getAllowedAccounts returns the list of account IDs the user is allowed to -// access. Empty slice means all access (Administrators-group members carry the -// "*" wildcard, which GetAllowedAccountsAPI surfaces as unrestricted). The -// stateless admin API key has no user row, so it short-circuits to all access. +// getAccountScope returns the auth.AccountScope describing which cloud +// accounts the session may reach. +// +// The scope is a value, not a list, and its ZERO FORM DENIES: an +// AccountScope{} grants no account at all. That is the inverse of the +// []string it replaced, where an empty slice meant ALL access -- the +// representation confusion behind issue #1748. Unrestricted access is carried +// as an explicit flag and is only ever set positively, here for the stateless +// admin API key (an infrastructure credential with no user row) and by +// auth.ScopeFromLegacyList for a resolved list that is empty or carries "*". +// +// Fails closed: every path that cannot establish the scope returns an error +// alongside a zero AccountScope, so a caller that ignores the error still +// denies. func (h *Handler) getAccountScope(ctx context.Context, session *Session) (auth.AccountScope, error) { if session.UserID == apiKeyAdminUserID { // Positively unrestricted: the stateless admin API key is an diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index 3dd624a72..1d27bff5a 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -128,7 +128,7 @@ func (h *Handler) resolveDashboardAccountScope(ctx context.Context, params map[s // into the dual-column purchase-history filter inputs so dashboard commitment // metrics and the inventory endpoints (fetchCommitmentRecords) never include // accounts the session can't access (issue #956). It lists -// the cloud accounts, keeps those the session matches via auth.MatchesAccount, +// the cloud accounts, keeps those the session scope allows (AccountScope.Allows), // and resolves their UUIDs through resolveAccountFilterIDs (same code path as an // explicit filter). // diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 20bd94926..5fc7dbc1d 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -413,8 +413,8 @@ func azureScopeIndex(accounts []config.CloudAccount) map[string]config.CloudAcco } // filterReservationsByScopeIndex narrows reservations down to the rows whose -// BillingScopeID resolves, via scopeToAccount, to a CloudAccount the allowed -// list covers (auth.MatchesAccount). Fails closed: a reservation with no +// BillingScopeID resolves, via scopeToAccount, to a CloudAccount the session +// scope allows (AccountScope.Allows). Fails closed: a reservation with no // BillingScopeID, or one that resolves to no registered CloudAccount, is // dropped rather than kept. func filterReservationsByScopeIndex(reservations []azurecompute.ExchangeableReservation, scopeToAccount map[string]config.CloudAccount, allowed auth.AccountScope) []azurecompute.ExchangeableReservation { diff --git a/internal/api/scoping.go b/internal/api/scoping.go index c769c7c6b..f8f4bdc13 100644 --- a/internal/api/scoping.go +++ b/internal/api/scoping.go @@ -9,7 +9,7 @@ import ( ) // requireAccountAccess fetches the account by ID, then verifies the session's -// allowed_accounts list grants access via auth.MatchesAccount. Returns the +// AccountScope allows it (AccountScope.Allows). Returns the // fetched account on success so callers can avoid a second GetCloudAccount // call. // @@ -246,7 +246,8 @@ func (h *Handler) resolveSingleAccountFilterIDs(ctx context.Context, accountID s // VARCHAR). Without both keys, the name lookup always misses for one path. // // Returns an empty map on error; callers fall through to ID-only matching, -// which is still safe (MatchesAccount falls back to ID comparison). +// which is still safe (AccountScope.Allows falls back to ID comparison when +// the name is empty). func (h *Handler) resolveAccountNamesByID(ctx context.Context) map[string]string { accounts, err := h.config.ListCloudAccounts(ctx, config.CloudAccountFilter{}) if err != nil { diff --git a/internal/auth/account_scope.go b/internal/auth/account_scope.go index d33ed87ac..45dc40ddf 100644 --- a/internal/auth/account_scope.go +++ b/internal/auth/account_scope.go @@ -40,12 +40,24 @@ func UnrestrictedScope() AccountScope { return AccountScope{Unrestricted: true} } -// ScopeForAccounts returns a scope limited to the given accounts. +// ScopeForAccounts returns a scope limited to the given accounts, treating +// every entry as a LITERAL account identifier. // -// An empty list yields a scope that grants NOTHING, which is the opposite of -// the legacy []string convention where empty meant "all accounts". Callers -// converting a legacy list must decide which they mean rather than passing it -// through; ScopeFromLegacyList exists for the one place that still has to. +// Two asymmetries with the legacy []string convention, and both invert its +// meaning rather than merely differing from it: +// +// - 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 +// denies everything else, where ScopeFromLegacyList(["*"]) is unrestricted. +// +// The second matters for the obvious future call. Passing a group's stored +// list straight through -- ScopeForAccounts(group.AllowedAccounts) -- silently +// produces a scope that DENIES EVERYTHING for any wildcard-carrying group, and +// all seven seeded groups ship allowed_accounts = ARRAY['*']. Use +// ScopeFromLegacyList for a value that came out of the resolver, and this +// constructor only for a list you have already decided is literal. func ScopeForAccounts(accounts []string) AccountScope { return AccountScope{Accounts: accounts} } diff --git a/internal/auth/group_ceiling.go b/internal/auth/group_ceiling.go index c89729514..01005767c 100644 --- a/internal/auth/group_ceiling.go +++ b/internal/auth/group_ceiling.go @@ -143,7 +143,7 @@ func validateRequestedPermissions(requested []Permission) error { // Fails closed on an unidentified actor or a resolution error. // // The stateless admin API key has no user row and is unrestricted everywhere -// else (see getAllowedAccounts in internal/api), so it is unrestricted here. +// else (see getAccountScope in internal/api), so it is unrestricted here. func (s *Service) grantCeilingAccounts(ctx context.Context, actorUserID string) ([]string, error) { if actorUserID == "" { return nil, fmt.Errorf("%w: the acting user could not be identified", ErrPermissionCeiling)