diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index 76ec0b71..9167200b 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -484,6 +484,9 @@ func (m *mockConfigStore) UpdateAccountRegistration(_ context.Context, _ *config func (m *mockConfigStore) TransitionRegistrationStatus(_ context.Context, _ *config.AccountRegistration, _ string, _ *string) error { return nil } +func (m *mockConfigStore) ApproveAccountRegistration(_ context.Context, _ *config.AccountRegistration, _ *config.CloudAccount, _ *string) error { + return nil +} func (m *mockConfigStore) DeleteAccountRegistration(_ context.Context, _ string) error { return nil } diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index c1d5baad..cd4f2957 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -581,6 +581,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 diff --git a/internal/api/handler_accounts_hostcheck_test.go b/internal/api/handler_accounts_hostcheck_test.go index eef79252..a606bdf4 100644 --- a/internal/api/handler_accounts_hostcheck_test.go +++ b/internal/api/handler_accounts_hostcheck_test.go @@ -173,15 +173,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) - store.AssertNotCalled(t, "TransitionRegistrationStatus") - store.AssertNotCalled(t, "CreateCloudAccount") + store.AssertNotCalled(t, "ApproveAccountRegistration") }) } @@ -189,12 +188,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) }) } diff --git a/internal/api/handler_accounts_provider_immutable_test.go b/internal/api/handler_accounts_provider_immutable_test.go new file mode 100644 index 00000000..497f6ef9 --- /dev/null +++ b/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) +} diff --git a/internal/api/handler_analytics.go b/internal/api/handler_analytics.go index cd1bfd05..534119a3 100644 --- a/internal/api/handler_analytics.go +++ b/internal/api/handler_analytics.go @@ -127,9 +127,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 @@ -190,9 +190,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. @@ -220,6 +220,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 +} + // resolveAnalyticsAccountFilter enforces the session's allowed_accounts scope // on the requested account_id and returns the dual-column filter the analytics // queries run with. Scoped users must name an account that their scope allows; diff --git a/internal/api/handler_analytics_invalid_params_test.go b/internal/api/handler_analytics_invalid_params_test.go new file mode 100644 index 00000000..68549faa --- /dev/null +++ b/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) + }) + } +} diff --git a/internal/api/handler_purchases_revoke_azure_id_integration_test.go b/internal/api/handler_purchases_revoke_azure_id_integration_test.go new file mode 100644 index 00000000..e6f2b354 --- /dev/null +++ b/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") + }) +} diff --git a/internal/api/handler_registrations.go b/internal/api/handler_registrations.go index b101d4cd..a195d172 100644 --- a/internal/api/handler_registrations.go +++ b/internal/api/handler_registrations.go @@ -290,18 +290,8 @@ func (h *Handler) approveRegistration(ctx context.Context, httpReq *events.Lambd return nil, err } - // Atomically transition to "approved" first — prevents double-approval. - reg.Status = "approved" h.setReviewMetadata(ctx, reg, httpReq) - // reviewed_by may be "admin-api-key" (not a UUID FK); validate before passing as actor. - if err := h.config.TransitionRegistrationStatus(ctx, reg, "pending", validUUIDPtrOrNil(reg.ReviewedBy)); err != nil { - if errors.Is(err, config.ErrRegistrationConflict) { - return nil, NewClientError(409, "registration was already processed by another request") - } - return nil, fmt.Errorf("registrations: transition: %w", err) - } - // Create the cloud account (only one request reaches here). now := time.Now() account := cloudAccountFromRequest(acctReq) account.ID = uuid.New().String() @@ -316,20 +306,18 @@ func (h *Handler) approveRegistration(ctx context.Context, httpReq *events.Lambd account.CreatedAt = now account.UpdatedAt = now - if err := h.config.CreateCloudAccount(ctx, account); err != nil { - return nil, classifyStoreError(err, fmt.Sprintf("an account with external ID %q already exists for %s", acctReq.ExternalID, acctReq.Provider)) + // Approve, create the account and link them in one transaction, so a + // failed account insert leaves the registration pending rather than + // approved with no account (issue #1102). The conditional status change + // inside it also stops a double approval. reviewed_by may be + // "admin-api-key" (not a UUID FK); validate before passing as actor. + if err := h.config.ApproveAccountRegistration(ctx, reg, account, validUUIDPtrOrNil(reg.ReviewedBy)); err != nil { + return nil, classifyApproveRegistrationError(err, acctReq) } // Auto-store credentials if the registration included them. h.storeRegistrationCredentials(ctx, reg, account.ID) - // Link the cloud account and wipe the credential payload from the registration record. - reg.CloudAccountID = &account.ID - reg.RegCredentialPayload = "" // Don't keep encrypted credentials longer than needed. - if err := h.config.UpdateAccountRegistration(ctx, reg); err != nil { - logging.Warnf("registration %s approved but failed to link cloud_account_id: %v", reg.ID, err) - } - h.notifyRegistrant(reg, email.RegistrationDecisionData{ AccountName: reg.AccountName, Provider: reg.Provider, ExternalID: reg.ExternalID, Decision: "approved", @@ -338,6 +326,19 @@ func (h *Handler) approveRegistration(ctx context.Context, httpReq *events.Lambd return account, nil } +// classifyApproveRegistrationError maps an ApproveAccountRegistration failure +// to the response: 409 when another request already decided the registration +// or the account is already registered, 500 otherwise. +func classifyApproveRegistrationError(err error, acctReq CloudAccountRequest) error { + if errors.Is(err, config.ErrRegistrationConflict) { + return NewClientError(409, "registration was already processed by another request") + } + if isDuplicateKeyError(err) { + return NewClientError(409, fmt.Sprintf("an account with external ID %q already exists for %s", acctReq.ExternalID, acctReq.Provider)) + } + return fmt.Errorf("registrations: approve: %w", err) +} + // accountHasCredentialFreePath returns true when the account's auth // mode resolves a credential without any stored secret — either via // ambient instance credentials (role assumption, managed identity, diff --git a/internal/api/handler_registrations_approve_integration_test.go b/internal/api/handler_registrations_approve_integration_test.go new file mode 100644 index 00000000..2ae68dfb --- /dev/null +++ b/internal/api/handler_registrations_approve_integration_test.go @@ -0,0 +1,234 @@ +//go:build integration + +package api + +import ( + "context" + "encoding/json" + "fmt" + "sync" + "testing" + "time" + + "github.com/aws/aws-lambda-go/events" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Real-PostgreSQL tests for issue #1102 item 2: approving a registration +// flipped it to approved and only then created the cloud account, in separate +// statements, so a failed create left it approved with no account. Approval, +// account creation and the link now share one transaction. + +func newRegistrationForApproval(ctx context.Context, t *testing.T, f *createConcurrencyFixture, externalID string) string { + t.Helper() + reg := &config.AccountRegistration{ + ReferenceToken: "tok-" + externalID, Status: "pending", Provider: "azure", + ExternalID: externalID, AccountName: "Fixture " + externalID, ContactEmail: "owner@example.com", + } + require.NoError(t, f.store.CreateAccountRegistration(ctx, reg)) + return reg.ID +} + +func approveRequest(t *testing.T, externalID string) *events.LambdaFunctionURLRequest { + t.Helper() + body, err := json.Marshal(CloudAccountRequest{ + Name: "Fixture " + externalID, Provider: "azure", ExternalID: externalID, AzureAuthMode: "managed_identity", + }) + require.NoError(t, err) + return &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}, Body: string(body)} +} + +type registrationRow struct { + status string + cloudAccountID *string + reviewedAt *time.Time +} + +func readRegistration(ctx context.Context, t *testing.T, pool *pgxpool.Pool, id string) registrationRow { + t.Helper() + var r registrationRow + require.NoError(t, pool.QueryRow(ctx, + `SELECT status, cloud_account_id::text, reviewed_at FROM account_registrations WHERE id = $1`, id, + ).Scan(&r.status, &r.cloudAccountID, &r.reviewedAt)) + return r +} + +func countAccounts(ctx context.Context, t *testing.T, pool *pgxpool.Pool, externalID string) int { + t.Helper() + var n int + require.NoError(t, pool.QueryRow(ctx, + `SELECT count(*) FROM cloud_accounts WHERE provider = 'azure' AND external_id = $1`, externalID).Scan(&n)) + return n +} + +// A failed account insert must leave the registration exactly as it was. +func TestApproveRegistration_FailedAccountCreateLeavesRegistrationPending(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second) + t.Cleanup(cancel) + f := newCreateConcurrencyFixture(ctx, t) + + const ext = "fixture-subscription-dup" + require.NoError(t, f.store.CreateCloudAccount(ctx, &config.CloudAccount{ + Name: "Already registered", Provider: "azure", ExternalID: ext, Enabled: true, + })) + regID := newRegistrationForApproval(ctx, t, f, ext) + + _, err := f.handler.approveRegistration(ctx, approveRequest(t, ext), regID) + requireClientCode(t, err, 409) + + row := readRegistration(ctx, t, f.pool, regID) + assert.Equal(t, "pending", row.status, "a failed approval must not leave the registration approved") + assert.Nil(t, row.cloudAccountID) + assert.Nil(t, row.reviewedAt) + assert.Equal(t, 1, countAccounts(ctx, t, f.pool, ext)) + + // Still pending, so the admin can act on it again. + _, err = f.handler.rejectRegistration(ctx, approveRequest(t, ext), regID) + require.NoError(t, err) +} + +// Concurrent approvals of one registration: exactly one wins, one account is +// created, and the registration is linked to it. +func TestApproveRegistration_ConcurrentApprovalsCreateOneAccount(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second) + t.Cleanup(cancel) + f := newCreateConcurrencyFixture(ctx, t) + + const ext = "fixture-subscription-race" + regID := newRegistrationForApproval(ctx, t, f, ext) + + const workers = 8 + var wg sync.WaitGroup + start := make(chan struct{}) + results := make([]any, workers) + errs := make([]error, workers) + for i := range workers { + wg.Add(1) + go func() { + defer wg.Done() + <-start + results[i], errs[i] = f.handler.approveRegistration(ctx, approveRequest(t, ext), regID) + }() + } + close(start) + wg.Wait() + + var winner *config.CloudAccount + for i := range workers { + if errs[i] == nil { + require.Nil(t, winner, "more than one approval succeeded") + winner = results[i].(*config.CloudAccount) + continue + } + requireClientCode(t, errs[i], 409) + } + require.NotNil(t, winner, "no approval succeeded") + + row := readRegistration(ctx, t, f.pool, regID) + assert.Equal(t, "approved", row.status) + require.NotNil(t, row.cloudAccountID) + assert.Equal(t, winner.ID, *row.cloudAccountID) + assert.Equal(t, 1, countAccounts(ctx, t, f.pool, ext)) +} + +// installSlowAccountInsert makes every cloud_accounts insert sleep, and fail +// when the external_id ends in "-fail", so an approval holds its transaction +// open long enough for a concurrent reject to queue behind it. +func installSlowAccountInsert(ctx context.Context, t *testing.T, pool *pgxpool.Pool) { + t.Helper() + _, err := pool.Exec(ctx, ` + CREATE FUNCTION test_slow_account_insert() RETURNS trigger AS $$ + BEGIN + PERFORM pg_sleep(1); + IF NEW.external_id LIKE '%-fail' THEN + RAISE EXCEPTION 'injected cloud_accounts insert failure'; + END IF; + RETURN NEW; + END $$ LANGUAGE plpgsql; + CREATE TRIGGER test_slow_account_insert BEFORE INSERT ON cloud_accounts + FOR EACH ROW EXECUTE FUNCTION test_slow_account_insert();`) + require.NoError(t, err) +} + +// waitForActivity polls pg_stat_activity until a backend matches the predicate. +func waitForActivity(ctx context.Context, t *testing.T, pool *pgxpool.Pool, predicate string) { + t.Helper() + require.Eventually(t, func() bool { + var n int + err := pool.QueryRow(ctx, `SELECT count(*) FROM pg_stat_activity WHERE `+predicate).Scan(&n) + return err == nil && n > 0 + }, 10*time.Second, 10*time.Millisecond, "no backend matched %s", predicate) +} + +// A reject that arrives while an approval is mid-transaction waits for it. +// When the approval commits, the reject gets 409; when the approval fails and +// rolls back, the reject proceeds and no account or approval is left behind. +func TestApproveRegistration_RejectQueuedBehindApproval(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) + t.Cleanup(cancel) + f := newCreateConcurrencyFixture(ctx, t) + installSlowAccountInsert(ctx, t, f.pool) + + for _, tc := range []struct { + ext string + approveCode int // 0: approval succeeds + wantStatus string + wantRejectCode int // 0: reject succeeds + wantAccounts int + }{ + {ext: "fixture-subscription-commit", wantStatus: "approved", wantRejectCode: 409, wantAccounts: 1}, + {ext: "fixture-subscription-fail", approveCode: 500, wantStatus: "rejected", wantAccounts: 0}, + } { + t.Run(tc.ext, func(t *testing.T) { + regID := newRegistrationForApproval(ctx, t, f, tc.ext) + + approveDone := make(chan error, 1) + go func() { + _, err := f.handler.approveRegistration(ctx, approveRequest(t, tc.ext), regID) + approveDone <- err + }() + // The approval has changed the status and is now inside the + // account insert, holding the registration row lock. + waitForActivity(ctx, t, f.pool, `wait_event = 'PgSleep' AND query LIKE '%INSERT INTO cloud_accounts%'`) + + rejectDone := make(chan error, 1) + go func() { + _, err := f.handler.rejectRegistration(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + }, regID) + rejectDone <- err + }() + // The reject's status change is blocked on that lock, not run after. + waitForActivity(ctx, t, f.pool, `wait_event_type = 'Lock' AND query LIKE '%UPDATE account_registrations%'`) + + approveErr := <-approveDone + rejectErr := <-rejectDone + switch tc.approveCode { + case 0: + require.NoError(t, approveErr) + default: + require.Error(t, approveErr) + _, isClient := IsClientError(approveErr) + assert.False(t, isClient, "an injected database failure is a 500, got %v", approveErr) + } + if tc.wantRejectCode == 0 { + require.NoError(t, rejectErr) + } else { + requireClientCode(t, rejectErr, tc.wantRejectCode) + } + + row := readRegistration(ctx, t, f.pool, regID) + assert.Equal(t, tc.wantStatus, row.status) + assert.Equal(t, tc.wantAccounts, countAccounts(ctx, t, f.pool, tc.ext)) + if tc.wantAccounts == 0 { + assert.Nil(t, row.cloudAccountID, fmt.Sprintf("registration %s", regID)) + } else { + assert.NotNil(t, row.cloudAccountID) + } + }) + } +} diff --git a/internal/api/handler_registrations_approve_test.go b/internal/api/handler_registrations_approve_test.go new file mode 100644 index 00000000..d0d35270 --- /dev/null +++ b/internal/api/handler_registrations_approve_test.go @@ -0,0 +1,114 @@ +package api + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Unit tests for approveRegistration after issue #1102 item 2. Approval used +// to flip the registration to approved, then create the account in a separate +// statement, so a failed create left an approved registration with no account. +// The handler now makes one ApproveAccountRegistration call, which does all of +// it in a transaction; the real-Postgres atomicity and race tests are in +// handler_registrations_approve_integration_test.go. + +const approveRegID = "5a5a5a5a-1102-4102-8102-000000000001" + +const approveRegBody = `{"name":"Fixture Sub","provider":"azure","external_id":"fixture-subscription-1102","azure_auth_mode":"managed_identity"}` + +func pendingRegistrationFixture() *config.AccountRegistration { + return &config.AccountRegistration{ + ID: approveRegID, Status: "pending", Provider: "azure", + ExternalID: "fixture-subscription-1102", AccountName: "Fixture Sub", + } +} + +// newApproveRegistrationHandler wires an admin session and a pending +// registration. The pre-#1102 steps are registered with .Maybe() so a +// regression to them fails the AssertNotCalled checks instead of panicking. +func newApproveRegistrationHandler(ctx context.Context, approveErr error) (*Handler, *MockConfigStore) { + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + store := new(MockConfigStore) + store.On("GetAccountRegistration", ctx, approveRegID).Return(pendingRegistrationFixture(), nil) + store.On("ApproveAccountRegistration", ctx, mock.Anything, mock.Anything, mock.Anything).Return(approveErr) + store.On("TransitionRegistrationStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + store.On("CreateCloudAccount", mock.Anything, mock.Anything).Return(nil).Maybe() + store.On("UpdateAccountRegistration", mock.Anything, mock.Anything).Return(nil).Maybe() + return &Handler{auth: mockAuth, config: store}, store +} + +// assertNoPiecemealApproval checks that none of the separate, non-atomic +// steps the handler used before #1102 ran. +func assertNoPiecemealApproval(t *testing.T, store *MockConfigStore) { + t.Helper() + store.AssertNotCalled(t, "TransitionRegistrationStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + store.AssertNotCalled(t, "CreateCloudAccount", mock.Anything, mock.Anything) + store.AssertNotCalled(t, "UpdateAccountRegistration", mock.Anything, mock.Anything) +} + +func TestApproveRegistration_ApprovesAndCreatesInOneStoreCall(t *testing.T) { + ctx := context.Background() + h, store := newApproveRegistrationHandler(ctx, nil) + + result, err := h.approveRegistration(ctx, adminRequest(approveRegBody), approveRegID) + require.NoError(t, err) + account, ok := result.(*config.CloudAccount) + require.True(t, ok, "got %T", result) + + store.AssertCalled(t, "ApproveAccountRegistration", ctx, + mock.MatchedBy(func(r *config.AccountRegistration) bool { + return r.ID == approveRegID && r.ReviewedAt != nil && r.ReviewedBy != nil && + *r.ReviewedBy == adminAccountSession().UserID + }), + mock.MatchedBy(func(a *config.CloudAccount) bool { + return a.ID == account.ID && a.Provider == "azure" && + a.ExternalID == "fixture-subscription-1102" && a.Enabled + }), + mock.MatchedBy(func(actor *string) bool { return actor != nil && *actor == adminAccountSession().UserID }), + ) + assertNoPiecemealApproval(t, store) +} + +func TestApproveRegistration_StoreErrorsMapToResponses(t *testing.T) { + cases := []struct { + name string + storeErr error + code int // 0: not a client error (500) + message string + }{ + {"already decided", config.ErrRegistrationConflict, 409, "already processed"}, + {"account already registered", fmt.Errorf("failed to create cloud account: %w", + errors.New(`ERROR: duplicate key value violates unique constraint "cloud_accounts_provider_external_id_key" (SQLSTATE 23505)`)), + 409, `external ID "fixture-subscription-1102" already exists for azure`}, + {"database failure", errors.New("connection reset"), 0, "registrations: approve: connection reset"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + h, store := newApproveRegistrationHandler(ctx, tc.storeErr) + + result, err := h.approveRegistration(ctx, adminRequest(approveRegBody), approveRegID) + assert.Nil(t, result) + require.Error(t, err) + ce, ok := IsClientError(err) + if tc.code == 0 { + require.False(t, ok, "a database failure must stay a 500, got client error %v", err) + assert.ErrorContains(t, err, tc.message) + } else { + require.True(t, ok, "want a client error, got %v", err) + assert.Equal(t, tc.code, ce.code) + assert.Contains(t, ce.message, tc.message) + } + assertNoPiecemealApproval(t, store) + }) + } +} diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 9a078709..6783f6b8 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -599,6 +599,10 @@ type StoreInterface interface { // TransitionRegistrationStatus atomically updates a registration's workflow fields. // actor is the UUID of the reviewer (nil for system-initiated transitions). TransitionRegistrationStatus(ctx context.Context, reg *AccountRegistration, fromStatus string, actor *string) error + // ApproveAccountRegistration atomically approves a pending registration, + // creates its cloud account and links them. Returns ErrRegistrationConflict + // when the registration is no longer pending; on any error nothing is saved. + ApproveAccountRegistration(ctx context.Context, reg *AccountRegistration, account *CloudAccount, actor *string) error DeleteAccountRegistration(ctx context.Context, id string) error // Purchase suppressions. Written inside a WithTx block during bulk diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 668d1bd2..c556ac83 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1903,6 +1903,13 @@ func getExecutionByIDOn(ctx context.Context, q executionQuerier, executionID str ` executions, err := queryExecutionsOn(ctx, q, query, executionID) + if isInvalidTextRepresentation(err) { + // execution_id is a UUID column, and Postgres rejects an ID it cannot + // cast instead of matching no row. No execution can have such an ID, + // so it is not found. Callers such as the purchase revoke route look up + // IDs that may be an Azure ARM path and fall through on ErrNotFound. + return nil, fmt.Errorf("%w: execution %s", ErrNotFound, executionID) + } if err != nil { return nil, err } @@ -1914,6 +1921,18 @@ func getExecutionByIDOn(ctx context.Context, q executionQuerier, executionID str return &executions[0], nil } +// pgInvalidTextRepresentation is the SQLSTATE Postgres raises when a text +// value cannot be cast to the column type, for example a non-UUID string +// compared with a uuid column. +const pgInvalidTextRepresentation = "22P02" + +// isInvalidTextRepresentation reports whether err is Postgres refusing a text value +// for a typed column (SQLSTATE 22P02). +func isInvalidTextRepresentation(err error) bool { + var pgErr *pgconn.PgError + return errors.As(err, &pgErr) && pgErr.Code == pgInvalidTextRepresentation +} + // GetExecutionByPlanAndDate retrieves execution for a specific plan and date. func (s *PostgresStore) GetExecutionByPlanAndDate(ctx context.Context, planID string, scheduledDate time.Time) (*PurchaseExecution, error) { query := ` diff --git a/internal/config/store_postgres_execution_non_uuid_integration_test.go b/internal/config/store_postgres_execution_non_uuid_integration_test.go new file mode 100644 index 00000000..60e2657a --- /dev/null +++ b/internal/config/store_postgres_execution_non_uuid_integration_test.go @@ -0,0 +1,49 @@ +//go:build integration + +package config + +// Real-PostgreSQL test for issue #1102 item 5. purchase_executions.execution_id +// is a UUID column, and POST /api/purchases/{id}/revoke asks GetExecutionByID +// about every ID before falling back to purchase_history. An Azure purchase ID +// is an ARM path, which Postgres refuses to cast to uuid (SQLSTATE 22P02), so +// the lookup failed with an error the handler answered as a 500 instead of the +// ErrNotFound that sends the revoke on to the purchase_history path. + +import ( + "context" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +func TestGetExecutionByID_NonUUIDIsNotFoundPostgres(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second) + defer cancel() + store := setupLadderStore(ctx, t) + + for _, id := range []string{ + "/subscriptions/00000000-0000-0000-0000-000000000000/providers/Microsoft.Capacity/reservationOrders/order-1/reservations/res-1", + "/providers/Microsoft.Capacity/reservationOrders/order-1", + "pid-1", + "", + } { + exec, err := store.GetExecutionByID(ctx, id) + require.ErrorIs(t, err, ErrNotFound, "id %q", id) + require.Nil(t, exec) + } + + // The connection is still usable afterwards, and a well-formed UUID behaves + // as before: missing is ErrNotFound, present is returned. + _, err := store.GetExecutionByID(ctx, uuid.NewString()) + require.ErrorIs(t, err, ErrNotFound) + + execID := uuid.NewString() + require.NoError(t, store.SavePurchaseExecution(ctx, &PurchaseExecution{ + ExecutionID: execID, Status: "scheduled", ScheduledDate: time.Now(), + })) + got, err := store.GetExecutionByID(ctx, execID) + require.NoError(t, err) + require.Equal(t, execID, got.ExecutionID) +} diff --git a/internal/config/store_postgres_execution_non_uuid_test.go b/internal/config/store_postgres_execution_non_uuid_test.go new file mode 100644 index 00000000..a8a84ccc --- /dev/null +++ b/internal/config/store_postgres_execution_non_uuid_test.go @@ -0,0 +1,56 @@ +package config + +// Unit tests pinning how narrowly GetExecutionByID turns a driver error into +// ErrNotFound (issue #1102 item 5). Only SQLSTATE 22P02 (a value the uuid +// column cannot hold) means "no such execution"; any other Postgres error is a +// real failure and must not reach callers as a 404. The real-Postgres side of +// 22P02 is covered by TestGetExecutionByID_NonUUIDIsNotFoundPostgres. + +import ( + "context" + "testing" + + "github.com/jackc/pgx/v5/pgconn" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func getExecutionByIDWithDriverError(t *testing.T, dbErr error) error { + t.Helper() + mock := newMock(t) + defer mock.Close() + mock.ExpectQuery("FROM purchase_executions"). + WithArgs("exec-1"). + WillReturnError(dbErr) + + exec, err := storeWith(mock).GetExecutionByID(context.Background(), "exec-1") + require.NoError(t, mock.ExpectationsWereMet()) + require.Error(t, err) + assert.Nil(t, exec) + return err +} + +func TestGetExecutionByID_InvalidTextRepresentationIsNotFound(t *testing.T) { + err := getExecutionByIDWithDriverError(t, &pgconn.PgError{Code: "22P02", + Message: `invalid input syntax for type uuid: "exec-1"`}) + assert.ErrorIs(t, err, ErrNotFound) +} + +func TestGetExecutionByID_OtherPostgresErrorsAreNotNotFound(t *testing.T) { + for _, code := range []string{ + "57P01", // admin_shutdown + "53300", // too_many_connections + "42P01", // undefined_table + "22P03", // invalid_binary_representation, the code next to 22P02 + } { + t.Run(code, func(t *testing.T) { + pgErr := &pgconn.PgError{Code: code, Message: "boom"} + err := getExecutionByIDWithDriverError(t, pgErr) + + assert.NotErrorIs(t, err, ErrNotFound) + var got *pgconn.PgError + require.ErrorAs(t, err, &got, "driver error must stay reachable for callers") + assert.Same(t, pgErr, got) + }) + } +} diff --git a/internal/config/store_postgres_registrations.go b/internal/config/store_postgres_registrations.go index 0cfc2250..f5e4b8b6 100644 --- a/internal/config/store_postgres_registrations.go +++ b/internal/config/store_postgres_registrations.go @@ -9,6 +9,7 @@ import ( "time" "github.com/google/uuid" + "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgconn" ) @@ -245,6 +246,34 @@ func (s *PostgresStore) TransitionRegistrationStatus(ctx context.Context, reg *A return nil } +// ApproveAccountRegistration moves a pending registration to approved, creates +// its cloud account and links the two, all in one transaction. If any step +// fails nothing is written, so a registration is never left approved with no +// account (issue #1102). +// +// The status change runs first and is conditional on status = 'pending'. Its +// row lock makes a concurrent approve or reject of the same registration wait +// for this transaction: after a commit the other request matches no row and +// gets ErrRegistrationConflict; after a rollback it proceeds as if this one +// never ran. A failed account insert (for example the provider + external_id +// already being registered) returns the driver error and leaves the +// registration pending. +func (s *PostgresStore) ApproveAccountRegistration(ctx context.Context, reg *AccountRegistration, account *CloudAccount, actor *string) error { + return s.WithTx(ctx, func(tx pgx.Tx) error { + txStore := &PostgresStore{db: tx} + reg.Status = "approved" + reg.CloudAccountID = nil + if err := txStore.TransitionRegistrationStatus(ctx, reg, "pending", actor); err != nil { + return err + } + if err := txStore.CreateCloudAccount(ctx, account); err != nil { + return err + } + reg.CloudAccountID = &account.ID + return txStore.UpdateAccountRegistration(ctx, reg) + }) +} + // DeleteAccountRegistration removes a registration record by ID. func (s *PostgresStore) DeleteAccountRegistration(ctx context.Context, id string) error { _, err := s.db.Exec(ctx, `DELETE FROM account_registrations WHERE id = $1`, id) diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index e946ed5f..33f391d0 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -1761,6 +1761,12 @@ func (m *MockConfigStore) TransitionRegistrationStatus(ctx context.Context, reg return args.Error(0) } +func (m *MockConfigStore) ApproveAccountRegistration(ctx context.Context, reg *config.AccountRegistration, account *config.CloudAccount, actor *string) error { + m.record("ApproveAccountRegistration", ctx, reg, account, actor) + args := m.Called(ctx, reg, account, actor) + return args.Error(0) +} + func (m *MockConfigStore) DeleteAccountRegistration(ctx context.Context, id string) error { m.record("DeleteAccountRegistration", ctx, id) args := m.Called(ctx, id) diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index 845618a1..3ecdf15d 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -335,6 +335,9 @@ func (m *mockConfigStoreForHealth) UpdateAccountRegistration(_ context.Context, func (m *mockConfigStoreForHealth) TransitionRegistrationStatus(_ context.Context, _ *config.AccountRegistration, _ string, _ *string) error { return nil } +func (m *mockConfigStoreForHealth) ApproveAccountRegistration(_ context.Context, _ *config.AccountRegistration, _ *config.CloudAccount, _ *string) error { + return nil +} func (m *mockConfigStoreForHealth) DeleteAccountRegistration(_ context.Context, _ string) error { return nil }