diff --git a/internal/api/handler_config_test.go b/internal/api/handler_config_test.go index 688cfb29b..d11f4e8f5 100644 --- a/internal/api/handler_config_test.go +++ b/internal/api/handler_config_test.go @@ -1236,6 +1236,7 @@ func TestHandler_updateConfig_RequiresUpdateConfigPermission(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t); mockAuth.AssertExpectations(t) }) // A non-admin user who was granted view:config (by migration 000088) but // not update:config. This mirrors the Standard-Users permission set after diff --git a/internal/api/handler_ladder.go b/internal/api/handler_ladder.go index c641134de..f8010e246 100644 --- a/internal/api/handler_ladder.go +++ b/internal/api/handler_ladder.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" + "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/config" "github.com/aws/aws-lambda-go/events" ) @@ -14,10 +15,18 @@ import ( // LADDER CONFIG HANDLERS // ========================================== -// getLadderConfigs returns all per-account ladder configurations. -// Requires view:config permission. +// getLadderConfigs returns the ladder configurations for accounts the caller +// is allowed to see. Requires view:config permission. +// +// Migration 000088 granted view:config to the Standard User and Read-Only +// User groups (previously admin-only), so this handler can no longer return +// every account's config unconditionally -- a scoped non-admin user would +// otherwise enumerate other accounts' spend caps and ramp schedules. Filter +// through filterLadderConfigsByAllowedAccounts before returning, mirroring +// filterPurchaseHistoryByAllowedAccounts (issue #1030 scoping pattern). func (h *Handler) getLadderConfigs(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { - if _, err := h.requirePermission(ctx, req, "view", "config"); err != nil { + session, err := h.requirePermission(ctx, req, "view", "config") + if err != nil { return nil, err } @@ -26,9 +35,36 @@ func (h *Handler) getLadderConfigs(ctx context.Context, req *events.LambdaFuncti return nil, fmt.Errorf("failed to list ladder configs: %w", err) } + configs, err = h.filterLadderConfigsByAllowedAccounts(ctx, session, configs) + if err != nil { + return nil, err + } + return map[string]any{"configs": configs}, nil } +// filterLadderConfigsByAllowedAccounts drops configs whose CloudAccountID is +// 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) + if err != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", err) + } + if auth.IsUnrestrictedAccess(allowed) { + 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]) { + filtered = append(filtered, c) + } + } + return filtered, nil +} + // upsertLadderConfig inserts or updates the per-account ladder configuration // for the given (cloud_account_id, provider) pair extracted from the request // body. Requires update:config permission. diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index ccdecde6a..c98ca538f 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -23,10 +23,12 @@ package api // 10. GET /ri-exchange/instances (getAllowedAccounts scope, issue #1030) // 11. GET /ri-exchange/utilization (getAllowedAccounts scope, issue #1030) // 12. GET /ri-exchange/reshape (getAllowedAccounts scope, issue #1030) +// 13. GET /ladder/configs (filterLadderConfigsByAllowedAccounts, issue #1428 follow-up) import ( "context" "encoding/json" + "errors" "testing" "time" @@ -1367,3 +1369,110 @@ func TestPerAccountPerms_GetReshapeRecommendations_AdminSeesAll(t *testing.T) { // proceeded past the scope gate into the actual reshape logic. _ = typed } + +// ─── 13. GET /ladder/configs ────────────────────────────────────────────────── + +// TestPerAccountPerms_GetLadderConfigs_ScopedUserSeesOnlyOwnAccount is the +// real failing scenario from the cross-account data leak: migration 000088 +// grants view:config to non-admin groups, but getLadderConfigs previously +// returned every account's LadderConfigDB (spend caps, ramp schedules) +// unconditionally. A scoped user (allowed_accounts: [permsAccA]) must see +// only permsAccA's config when configs exist for both accounts. +// +// Regression: without filterLadderConfigsByAllowedAccounts, this test fails +// because the account-B config is included in the response. +func TestPerAccountPerms_GetLadderConfigs_ScopedUserSeesOnlyOwnAccount(t *testing.T) { + ctx := context.Background() + + cfgA := config.LadderConfigDB{ID: "cfg-a", CloudAccountID: permsAccA, Provider: "aws"} + cfgB := config.LadderConfigDB{ID: "cfg-b", CloudAccountID: permsAccB, Provider: "aws"} + + mockStore := new(MockConfigStore) + mockStore.On("GetLadderConfigs", ctx).Return([]config.LadderConfigDB{cfgA, cfgB}, nil) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return permsAccountList(), nil + } + + handler := &Handler{ + auth: scopedAuthMock(ctx), + config: mockStore, + } + + resp, err := handler.getLadderConfigs(ctx, scopedReq()) + require.NoError(t, err) + body, ok := resp.(map[string]any) + require.True(t, ok, "expected map[string]any, got %T", resp) + configs, ok := body["configs"].([]config.LadderConfigDB) + require.True(t, ok, "expected []config.LadderConfigDB, got %T", body["configs"]) + + ids := make([]string, len(configs)) + for i, c := range configs { + ids[i] = c.ID + } + assert.ElementsMatch(t, []string{"cfg-a"}, ids, + "scoped user must see only account-A's ladder config; account-B config must be filtered out") +} + +// TestPerAccountPerms_GetLadderConfigs_AllowedAccountsLookupError verifies +// that when GetAllowedAccountsAPI returns an error, getLadderConfigs fails +// closed (returns the error, not unfiltered configs). +func TestPerAccountPerms_GetLadderConfigs_AllowedAccountsLookupError(t *testing.T) { + ctx := context.Background() + + cfgA := config.LadderConfigDB{ID: "cfg-a", CloudAccountID: permsAccA, Provider: "aws"} + cfgB := config.LadderConfigDB{ID: "cfg-b", CloudAccountID: permsAccB, Provider: "aws"} + + mockStore := new(MockConfigStore) + mockStore.On("GetLadderConfigs", ctx).Return([]config.LadderConfigDB{cfgA, cfgB}, nil) + + // Scoped auth mock, but GetAllowedAccountsAPI fails + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, permsScopedToken).Return(&Session{ + UserID: permsScopedUserID, + Email: "scoped@example.com", + }, nil) + mockAuth.On("HasPermissionAPI", ctx, permsScopedUserID, mock.Anything, mock.Anything).Return(true, nil) + injectedErr := errors.New("allowed accounts lookup failed") + mockAuth.On("GetAllowedAccountsAPI", ctx, permsScopedUserID).Return(nil, injectedErr) + + handler := &Handler{ + auth: mockAuth, + config: mockStore, + } + + resp, err := handler.getLadderConfigs(ctx, scopedReq()) + require.Error(t, err, "expected getLadderConfigs to return an error when allowed-accounts lookup fails") + assert.ErrorContains(t, err, "failed to get allowed accounts", + "error should wrap the allowed-accounts lookup failure") + assert.Nil(t, resp, "response must be nil when allowed-accounts lookup fails (fail-closed)") +} + +// TestPerAccountPerms_GetLadderConfigs_AdminSeesAll verifies that an admin +// (unrestricted) session bypasses the allowed_accounts gate and receives +// every account's ladder config. +func TestPerAccountPerms_GetLadderConfigs_AdminSeesAll(t *testing.T) { + ctx := context.Background() + + cfgA := config.LadderConfigDB{ID: "cfg-a", CloudAccountID: permsAccA, Provider: "aws"} + cfgB := config.LadderConfigDB{ID: "cfg-b", CloudAccountID: permsAccB, Provider: "aws"} + + mockStore := new(MockConfigStore) + mockStore.On("GetLadderConfigs", ctx).Return([]config.LadderConfigDB{cfgA, cfgB}, nil) + + mockAuth, req := adminDashboardReq(ctx) + handler := &Handler{auth: mockAuth, config: mockStore} + + resp, err := handler.getLadderConfigs(ctx, req) + require.NoError(t, err) + body, ok := resp.(map[string]any) + require.True(t, ok, "expected map[string]any, got %T", resp) + configs, ok := body["configs"].([]config.LadderConfigDB) + require.True(t, ok, "expected []config.LadderConfigDB, got %T", body["configs"]) + + ids := make([]string, len(configs)) + for i, c := range configs { + ids[i] = c.ID + } + assert.ElementsMatch(t, []string{"cfg-a", "cfg-b"}, ids, + "admin must see every account's ladder config") +}