fix(api): four handler bugs found while documenting the API (#1102)

go.mod

@@ -59,7 +59,7 @@ require (
 	go.opentelemetry.io/otel/metric v1.45.0 // indirect
 	go.opentelemetry.io/otel/trace v1.45.0 // indirect
 	golang.org/x/crypto v0.58.0
-	golang.org/x/net v0.61.0 // indirect
+	golang.org/x/net v0.62.0 // indirect
 	golang.org/x/oauth2 v0.37.0
 	golang.org/x/sync v0.24.0
 	golang.org/x/sys v0.49.0 // indirect

go.sum

@@ -367,8 +367,8 @@ golang.org/x/crypto v0.58.0 h1:COkYLr4k7nDI4r6QtMML2AfzeIswWHAJthLS6K/M/54=
 golang.org/x/crypto v0.58.0/go.mod h1:Xh+kl5A+M0gMiWDytH2Hhr5Z7WbNCpv/eB/EFgfSk+w=
 golang.org/x/mod v0.42.0 h1:ICeL3t2L30ljSPHHxDnR4fqVcBt/zR2zJAb9juavmQg=
 golang.org/x/mod v0.42.0/go.mod h1:u5fzIrRInkL8MaO1cbRkUHT64v303YIT5KEqpNr7jRs=
-golang.org/x/net v0.61.0 h1:RR+6j/BTBCrggOLugxa+66R8FvoN6MHlzDlZIbN9YFw=
-golang.org/x/net v0.61.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI=
+golang.org/x/net v0.62.0 h1:18ELalfM5+XNRwzM2nYqXsZQ9ByS1uhj2O/U7+5jqRY=
+golang.org/x/net v0.62.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI=
 golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98=
 golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58=
 golang.org/x/sync v0.24.0 h1:iqiDrGoDgyuvAKrhLQthUEzU/R378N78gXbBjxed7oA=

internal/api/handler_accounts.go

@@ -614,6 +614,15 @@ func (h *Handler) updateAccount(ctx context.Context, httpReq *events.LambdaFunct
 		return nil, err
 	}
 
+	// The provider is fixed when the account is created: UpdateCloudAccount
+	// does not write it, and credentials, service overrides and plan
+	// memberships all depend on it. Refuse a change rather than echo back a
+	// provider that was never saved.
+	if req.Provider != existing.Provider {
+		return nil, NewClientError(400, fmt.Sprintf(
+			"provider cannot be changed (this account is %s); delete the account and add it again under the new provider", existing.Provider))
+	}
+
 	account := cloudAccountFromRequest(req)
 	account.ID = id
 	account.CreatedAt = existing.CreatedAt

internal/api/handler_accounts_hostcheck_test.go

@@ -180,13 +180,14 @@ func TestApproveRegistration_EmptyRoleARNRequiresHostAccount(t *testing.T) {
 				Return(&config.AccountRegistration{ID: hostCheckRegID, Status: "pending", Provider: "aws", ExternalID: tc.externalID}, nil)
 			// Registered so a regression fails on the assertions below
 			// instead of panicking on an unexpected call.
-			store.On("TransitionRegistrationStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
-				Return(errors.New("registration must not be transitioned"))
+			store.On("ApproveAccountRegistration", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
+				Return(errors.New("registration must not be approved"))
 			h := hostCheckHandler(store, tc.fake, tc.cfgErr)
 			req := &events.LambdaFunctionURLRequest{Body: roleARNBody(tc.externalID, "")}
 			result, err := h.approveRegistration(ctx, req, hostCheckRegID)
 			assert.Nil(t, result)
 			requireNotHost400(t, err, tc.wantMsg)
+			store.AssertNotCalled(t, "ApproveAccountRegistration")
 			store.AssertNotCalled(t, "TransitionRegistrationStatus")
 			store.AssertNotCalled(t, "CreateCloudAccount")
 		})
@@ -196,12 +197,11 @@ func TestApproveRegistration_EmptyRoleARNRequiresHostAccount(t *testing.T) {
 		store := new(MockConfigStore)
 		store.On("GetAccountRegistration", mock.Anything, hostCheckRegID).
 			Return(&config.AccountRegistration{ID: hostCheckRegID, Status: "pending", Provider: "aws", ExternalID: probeHostAcct}, nil)
-		store.On("TransitionRegistrationStatus", mock.Anything, mock.Anything, "pending", mock.Anything).Return(nil)
-		store.On("UpdateAccountRegistration", mock.Anything, mock.Anything).Return(nil)
+		store.On("ApproveAccountRegistration", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
 		h := hostCheckHandler(store, &probeSTS{}, nil)
 		req := &events.LambdaFunctionURLRequest{Body: roleARNBody(probeHostAcct, "")}
 		_, err := h.approveRegistration(ctx, req, hostCheckRegID)
 		require.NoError(t, err)
-		store.AssertCalled(t, "CreateCloudAccount")
+		store.AssertCalled(t, "ApproveAccountRegistration", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
 	})
 }

internal/api/handler_accounts_provider_immutable_test.go

@@ -0,0 +1,74 @@
+package api
+
+import (
+	"context"
+	"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/config"
+)
+
+// Regression tests for issue #1102 item 3: PUT /api/accounts/{id} validated
+// the request's provider and echoed it back, but UpdateCloudAccount never
+// writes the provider column, so the change was silently dropped. A provider
+// change is now refused with a 400 before anything is saved.
+
+func updateAccountRequest(body, id string) *events.LambdaFunctionURLRequest {
+	req := adminRequest(body)
+	req.RequestContext.HTTP.Method = "PUT"
+	req.RequestContext.HTTP.Path = "/api/accounts/" + id
+	return req
+}
+
+func TestUpdateAccount_ProviderChangeIsRejected(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	setupAdminAuth(ctx, mockAuth)
+
+	existing := sampleAccount() // provider aws
+	store := new(MockConfigStore)
+	store.On("GetCloudAccount", ctx, existing.ID).Return(&existing, nil)
+	// Registered so the pre-fix handler reaches the save and returns the
+	// echoed account instead of panicking on an unexpected call.
+	store.On("UpdateCloudAccount", ctx, mock.Anything).Return(nil).Maybe()
+	handler := &Handler{auth: mockAuth, config: store}
+
+	body := `{"name":"Test Account","provider":"gcp","external_id":"123456789012","gcp_auth_mode":"application_default"}`
+	result, err := handler.updateAccount(ctx, updateAccountRequest(body, existing.ID), existing.ID)
+
+	assert.Nil(t, result, "a provider that is never saved must not be echoed back")
+	require.Error(t, err)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "want a client error, got %v", err)
+	assert.Equal(t, 400, ce.code)
+	assert.Contains(t, ce.message, "provider cannot be changed")
+	assert.Contains(t, ce.message, "aws")
+	store.AssertNotCalled(t, "UpdateCloudAccount", mock.Anything, mock.Anything)
+}
+
+func TestUpdateAccount_SameProviderIsSaved(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	setupAdminAuth(ctx, mockAuth)
+
+	existing := sampleAccount()
+	store := new(MockConfigStore)
+	store.On("GetCloudAccount", ctx, existing.ID).Return(&existing, nil)
+	store.On("UpdateCloudAccount", ctx, mock.MatchedBy(func(a *config.CloudAccount) bool {
+		return a.ID == existing.ID && a.Provider == "aws" && a.Name == "Renamed"
+	})).Return(nil).Once()
+	handler := &Handler{auth: mockAuth, config: store}
+
+	body := `{"name":"Renamed","provider":"aws","external_id":"123456789012"}`
+	result, err := handler.updateAccount(ctx, updateAccountRequest(body, existing.ID), existing.ID)
+	require.NoError(t, err)
+	saved, ok := result.(*config.CloudAccount)
+	require.True(t, ok)
+	assert.Equal(t, "aws", saved.Provider)
+	assert.Equal(t, "Renamed", saved.Name)
+	store.AssertExpectations(t)
+}

internal/api/handler_analytics.go

@@ -131,9 +131,9 @@ func (h *Handler) getHistoryAnalytics(ctx context.Context, req *events.LambdaFun
 	}
 
 	// Parse parameters
-	interval := params["interval"]
-	if interval == "" {
-		interval = "hourly"
+	interval, err := analyticsInterval(params["interval"])
+	if err != nil {
+		return nil, err
 	}
 
 	// Normalise + validate the global-filter provider chip, mirroring
@@ -198,9 +198,9 @@ func (h *Handler) getHistoryBreakdown(ctx context.Context, req *events.LambdaFun
 		return nil, NewClientError(503, "analytics not configured")
 	}
 
-	dimension := params["dimension"]
-	if dimension == "" {
-		dimension = "service"
+	dimension, err := analyticsDimension(params["dimension"])
+	if err != nil {
+		return nil, err
 	}
 
 	// Scope check plus account -> dual-column inputs; see getHistoryAnalytics.
@@ -232,6 +232,32 @@ func (h *Handler) getHistoryBreakdown(ctx context.Context, req *events.LambdaFun
 	}, nil
 }
 
+// analyticsInterval returns the requested bucket interval, defaulting to
+// hourly. A value the store cannot bucket by is a 400 here; without this check
+// it reached intervalToTruncUnit inside QueryHistory and came back as a 500.
+func analyticsInterval(raw string) (string, error) {
+	if raw == "" {
+		return "hourly", nil
+	}
+	if _, err := intervalToTruncUnit(raw); err != nil {
+		return "", NewClientError(400, "invalid interval: must be one of hourly, daily, weekly, monthly")
+	}
+	return raw, nil
+}
+
+// analyticsDimension returns the requested breakdown dimension, defaulting to
+// service. A value the store cannot group by is a 400 here; without this check
+// it reached dimensionToColumn inside QueryBreakdown and came back as a 500.
+func analyticsDimension(raw string) (string, error) {
+	if raw == "" {
+		return "service", nil
+	}
+	if _, err := dimensionToColumn(raw); err != nil {
+		return "", NewClientError(400, "invalid dimension: must be one of service, provider, region, account")
+	}
+	return raw, nil
+}
+
 // requestedAnalyticsAccounts returns the accounts an analytics request names.
 // The plural account_ids (comma-separated UUIDs, validated and capped by
 // parseAccountIDs) wins when both it and the legacy singular account_id are

internal/api/handler_analytics_invalid_params_test.go

@@ -0,0 +1,102 @@
+package api
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+// Regression tests for issue #1102 item 1: an unsupported analytics interval
+// or dimension used to reach the Postgres client, fail its allowlist and come
+// back as a 500. It is a bad request, so the handler now answers 400 before
+// any query runs.
+
+func TestHandler_getHistoryAnalytics_UnsupportedIntervalIs400(t *testing.T) {
+	for _, interval := range []string{"yearly", "minute", "HOURLY", "hour"} {
+		t.Run(interval, func(t *testing.T) {
+			ctx := context.Background()
+			mockClient := new(MockAnalyticsClient)
+			// What the Postgres client answers for an interval outside its
+			// allowlist, so the pre-fix handler fails this test with a 500
+			// instead of panicking on an unexpected call.
+			mockClient.On("QueryHistory", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, interval).
+				Return(nil, nil, fmt.Errorf("unsupported interval %q", interval)).Maybe()
+			mockAuth, req := adminAnalyticsReq(ctx)
+			handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)}
+
+			_, err := handler.getHistoryAnalytics(ctx, req, map[string]string{"interval": interval})
+			require.Error(t, err)
+			ce, ok := IsClientError(err)
+			require.True(t, ok, "want a client error, got %v", err)
+			assert.Equal(t, 400, ce.code)
+			assert.Contains(t, ce.message, "invalid interval")
+			mockClient.AssertNotCalled(t, "QueryHistory", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+		})
+	}
+}
+
+func TestHandler_getHistoryAnalytics_SupportedIntervalsReachStore(t *testing.T) {
+	for _, interval := range []string{"hourly", "daily", "weekly", "monthly"} {
+		t.Run(interval, func(t *testing.T) {
+			ctx := context.Background()
+			mockClient := new(MockAnalyticsClient)
+			mockClient.On("QueryHistory", ctx, []string(nil), map[string][]string(nil), "", mock.Anything, mock.Anything, interval).
+				Return([]HistoryDataPoint{}, (*HistorySummary)(nil), nil)
+			mockAuth, req := adminAnalyticsReq(ctx)
+			handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)}
+
+			result, err := handler.getHistoryAnalytics(ctx, req, map[string]string{"interval": interval})
+			require.NoError(t, err)
+			resp, ok := result.(*AnalyticsResponse)
+			require.True(t, ok)
+			assert.Equal(t, interval, resp.Interval)
+			mockClient.AssertExpectations(t)
+		})
+	}
+}
+
+func TestHandler_getHistoryBreakdown_UnsupportedDimensionIs400(t *testing.T) {
+	for _, dimension := range []string{"tenant", "account_id", "SERVICE", "service_type"} {
+		t.Run(dimension, func(t *testing.T) {
+			ctx := context.Background()
+			mockClient := new(MockAnalyticsClient)
+			// The Postgres client's answer for an unknown dimension; see above.
+			mockClient.On("QueryBreakdown", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, dimension).
+				Return(nil, fmt.Errorf("unsupported dimension %q", dimension)).Maybe()
+			mockAuth, req := adminAnalyticsReq(ctx)
+			handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)}
+
+			_, err := handler.getHistoryBreakdown(ctx, req, map[string]string{"dimension": dimension})
+			require.Error(t, err)
+			ce, ok := IsClientError(err)
+			require.True(t, ok, "want a client error, got %v", err)
+			assert.Equal(t, 400, ce.code)
+			assert.Contains(t, ce.message, "invalid dimension")
+			mockClient.AssertNotCalled(t, "QueryBreakdown", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+		})
+	}
+}
+
+func TestHandler_getHistoryBreakdown_SupportedDimensionsReachStore(t *testing.T) {
+	for _, dimension := range []string{"service", "provider", "region", "account"} {
+		t.Run(dimension, func(t *testing.T) {
+			ctx := context.Background()
+			mockClient := new(MockAnalyticsClient)
+			mockClient.On("QueryBreakdown", ctx, []string(nil), map[string][]string(nil), mock.Anything, mock.Anything, dimension).
+				Return(map[string]BreakdownValue{}, nil)
+			mockAuth, req := adminAnalyticsReq(ctx)
+			handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)}
+
+			result, err := handler.getHistoryBreakdown(ctx, req, map[string]string{"dimension": dimension})
+			require.NoError(t, err)
+			resp, ok := result.(*BreakdownResponse)
+			require.True(t, ok)
+			assert.Equal(t, dimension, resp.Dimension)
+			mockClient.AssertExpectations(t)
+		})
+	}
+}

internal/api/handler_purchases_revoke_azure_id_integration_test.go

@@ -0,0 +1,80 @@
+//go:build integration
+
+package api
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
+	"github.com/aws/aws-lambda-go/events"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
+)
+
+// TestRevokePurchase_AzureARMPurchaseIDOnRealPostgres is the regression test
+// for issue #1102 item 5. revokePurchase first asks GetExecutionByID whether
+// the ID is a scheduled execution. An Azure purchase ID is an ARM path, which
+// Postgres refused to cast to the uuid execution_id column (SQLSTATE 22P02), so
+// every Azure revoke failed with a 500 before reaching purchase_history. The
+// unit tests mocked that lookup as ErrNotFound and never saw it.
+//
+// The store and the purchase_history row are real; only the two Azure SDK
+// clients are stubs, so no cloud call is made.
+func TestRevokePurchase_AzureARMPurchaseIDOnRealPostgres(t *testing.T) {
+	ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second)
+	t.Cleanup(cancel)
+	f := newCreateConcurrencyFixture(ctx, t)
+
+	var acctID string
+	require.NoError(t, f.pool.QueryRow(ctx,
+		"SELECT id::text FROM cloud_accounts WHERE external_id = 'fixture-subscription-a'").Scan(&acctID))
+
+	const purchaseID = "/subscriptions/00000000-0000-0000-0000-00000000000a/providers/Microsoft.Capacity/reservationOrders/order-1102/reservations/res-1102"
+	purchasedAt := time.Now().UTC().Add(-24 * time.Hour)
+	require.NoError(t, f.store.SavePurchaseHistory(ctx, &config.PurchaseHistoryRecord{
+		AccountID: "fixture-subscription-a", CloudAccountID: &acctID, PurchaseID: purchaseID,
+		Timestamp: purchasedAt, Provider: "azure", Service: "compute", Region: "eastus",
+		ResourceType: "Standard_D2s_v3", Count: 1, Term: 1, Payment: "monthly",
+		RevocationWindowClosesAt: config.RevocationWindowClosesAtFor("azure", purchasedAt),
+	}))
+
+	returnClient := &stubReturnClient{}
+	f.handler.azureRevokeFactory = &azureRevokeClientFactory{
+		newCredential: func() (azcore.TokenCredential, error) { return nil, nil },
+		newCalculateRefundClient: func(azcore.TokenCredential) (azureCalculateRefundClient, error) {
+			return &stubCalcRefundClientWithAmount{amount: 42.5, currency: "USD", sessID: "sess-1102"}, nil
+		},
+		newReturnClient: func(azcore.TokenCredential) (azureReturnClient, error) { return returnClient, nil },
+	}
+	req := func(body string) *events.LambdaFunctionURLRequest {
+		return &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}, Body: body}
+	}
+
+	t.Run("unknown ARM id is a 404, not a 500", func(t *testing.T) {
+		_, err := f.handler.revokePurchase(ctx, req(""), "/providers/Microsoft.Capacity/reservationOrders/no-such-order/reservations/none")
+		require.Error(t, err)
+		ce, ok := IsClientError(err)
+		require.True(t, ok, "want a client error, got %v", err)
+		assert.Equal(t, 404, ce.code)
+		assert.Zero(t, returnClient.calls)
+	})
+
+	t.Run("Azure reservation is returned and recorded", func(t *testing.T) {
+		result, err := f.handler.revokePurchase(ctx,
+			req(`{"expected_refund_amount":42.5,"expected_refund_currency":"USD"}`), purchaseID)
+		require.NoError(t, err)
+		res, ok := result.(*revokePurchaseResult)
+		require.True(t, ok, "got %T", result)
+		assert.Equal(t, "revoked", res.Status)
+		assert.Equal(t, 1, returnClient.calls)
+
+		var revokedAt *time.Time
+		require.NoError(t, f.pool.QueryRow(ctx,
+			"SELECT revoked_at FROM purchase_history WHERE purchase_id = $1", purchaseID).Scan(&revokedAt))
+		assert.NotNil(t, revokedAt, "the revoke must be persisted on the purchase_history row")
+	})
+}