From 921de149b090ad48f704d2b12087a17d0286dab1 Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Sun, 19 Jul 2026 21:44:06 +0200
Subject: [PATCH 1/2] refactor(api): inject Azure revoke client factory (closes
 #1463)

calculateAzureRevoke and revokeAzurePurchase each constructed
azidentity.NewDefaultAzureCredential and the armreservations clients
inline, so no test could reach either function without walking the
live Azure credential chain (az/pwsh spawn, macOS keychain prompts).

Add Handler.newAzureRevokeClients, a factory seam mirroring the
AzureSPProber.NewClient pattern: nil means "use the production
default" (unchanged credential + client construction), and tests can
override it with the existing stubCalcRefundClient/stubReturnClient
fakes. Both handlers now route through buildAzureRevokeClients, with
new tests covering the happy path and a factory-error path for each.
---
 internal/api/handler.go                       |  11 ++
 internal/api/handler_purchases_revoke.go      |  40 +++--
 internal/api/handler_purchases_revoke_test.go | 139 ++++++++++++++++++
 3 files changed, 177 insertions(+), 13 deletions(-)

diff --git a/internal/api/handler.go b/internal/api/handler.go
index 521a70f8e..9979a614d 100644
--- a/internal/api/handler.go
+++ b/internal/api/handler.go
@@ -131,6 +131,17 @@ type Handler struct {
 	// encryption key. Empty when no credStore is configured. Used by the
 	// /health endpoint only — never logged outside that one place.
 	encryptionKeySource string
+
+	// newAzureRevokeClients builds the CalculateRefund and Return clients used
+	// by the Azure purchase-revoke handlers (calculateAzureRevoke,
+	// revokeAzurePurchase). When nil (the production default),
+	// buildAzureRevokeClients constructs them from
+	// azidentity.NewDefaultAzureCredential plus the armreservations SDK
+	// client constructors. Tests override this to inject
+	// stubCalcRefundClient/stubReturnClient so the handlers can be exercised
+	// without walking the live Azure credential chain (az CLI / PowerShell /
+	// keychain prompts).
+	newAzureRevokeClients func() (azureCalculateRefundClient, azureReturnClient, error)
 }
 
 // getRIUtilizationCache returns the Postgres-backed TTL cache for Cost
diff --git a/internal/api/handler_purchases_revoke.go b/internal/api/handler_purchases_revoke.go
index f97573f55..632565c48 100644
--- a/internal/api/handler_purchases_revoke.go
+++ b/internal/api/handler_purchases_revoke.go
@@ -409,13 +409,9 @@ func (h *Handler) calculateAzureRevoke(ctx context.Context, req *events.LambdaFu
 		return nil, err
 	}
 
-	cred, err := azidentity.NewDefaultAzureCredential(nil)
-	if err != nil {
-		return nil, fmt.Errorf("revoke/calculate: obtain credential: %w", err)
-	}
-	calcClient, err := armreservations.NewCalculateRefundClient(cred, nil)
+	calcClient, _, err := h.buildAzureRevokeClients()
 	if err != nil {
-		return nil, fmt.Errorf("revoke/calculate: create calculate-refund client: %w", err)
+		return nil, fmt.Errorf("revoke/calculate: %w", err)
 	}
 
 	quantity := int32(count) // #nosec G115 -- Azure reservation count bounded by API limits (<<math.MaxInt32) //nolint:gosec
@@ -558,22 +554,40 @@ func (h *Handler) revokeAzurePurchase(ctx context.Context, record *config.Purcha
 		return nil, NewClientError(422, "cannot determine Azure reservation order ID from purchase record; contact Azure Support to request a refund")
 	}
 
-	cred, err := azidentity.NewDefaultAzureCredential(nil)
+	calcClient, returnClient, err := h.buildAzureRevokeClients()
 	if err != nil {
-		return nil, fmt.Errorf("revoke azure: obtain credential: %w", err)
+		return nil, fmt.Errorf("revoke azure: %w", err)
+	}
+
+	return h.callAzureReturn(ctx, calcClient, returnClient, record, orderID, reservationID, expectedRefundAmount)
+}
+
+// buildAzureRevokeClients returns the injected factory's result when
+// h.newAzureRevokeClients is set (test path), or constructs the production
+// CalculateRefund and Return clients from a single
+// azidentity.NewDefaultAzureCredential (production default). Mirrors the
+// AzureSPProber.NewClient injected-factory pattern
+// (internal/commitmentopts/probe_azure.go): a nil field always means "use the
+// production default", so existing production construction sites need no
+// changes.
+func (h *Handler) buildAzureRevokeClients() (azureCalculateRefundClient, azureReturnClient, error) {
+	if h.newAzureRevokeClients != nil {
+		return h.newAzureRevokeClients()
 	}
 
+	cred, err := azidentity.NewDefaultAzureCredential(nil)
+	if err != nil {
+		return nil, nil, fmt.Errorf("obtain credential: %w", err)
+	}
 	calcClient, err := armreservations.NewCalculateRefundClient(cred, nil)
 	if err != nil {
-		return nil, fmt.Errorf("revoke azure: create calculate-refund client: %w", err)
+		return nil, nil, fmt.Errorf("create calculate-refund client: %w", err)
 	}
-
 	returnClient, err := armreservations.NewReturnClient(cred, nil)
 	if err != nil {
-		return nil, fmt.Errorf("revoke azure: create return client: %w", err)
+		return nil, nil, fmt.Errorf("create return client: %w", err)
 	}
-
-	return h.callAzureReturn(ctx, calcClient, returnClient, record, orderID, reservationID, expectedRefundAmount)
+	return calcClient, returnClient, nil
 }
 
 // callAzureReturn executes the two-step Azure reservation return:
diff --git a/internal/api/handler_purchases_revoke_test.go b/internal/api/handler_purchases_revoke_test.go
index b3c713248..08d88712e 100644
--- a/internal/api/handler_purchases_revoke_test.go
+++ b/internal/api/handler_purchases_revoke_test.go
@@ -1347,6 +1347,145 @@ func TestRevokePurchase_GetExecutionByIDDBError_Returns500(t *testing.T) {
 	mockStore.AssertExpectations(t)
 }
 
+// --- Issue #1463: injection seam for calculateAzureRevoke / revokeAzurePurchase ---
+//
+// Before this seam, both handlers called azidentity.NewDefaultAzureCredential
+// and the armreservations SDK constructors inline, so no test could reach past
+// that point without walking the live Azure credential chain (spawning az /
+// pwsh, popping macOS keychain prompts). These tests drive both handlers
+// end-to-end through h.newAzureRevokeClients using the existing
+// stubCalcRefundClient(WithAmount)/stubReturnClient fakes, proving the
+// previously-unreachable code is now covered without any live client.
+
+// TestCalculateAzureRevoke_Success exercises calculateAzureRevoke through the
+// injected client factory (happy path): session + record load + authorization
+// + window check all pass, and the injected CalculateRefund stub's quote is
+// returned to the caller.
+func TestCalculateAzureRevoke_Success(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	mockStore := new(MockConfigStore)
+	mockAuth := new(MockAuthService)
+	t.Cleanup(func() {
+		mockStore.AssertExpectations(t)
+		mockAuth.AssertExpectations(t)
+	})
+
+	mockAuth.On("ValidateSession", ctx, "tok").Return(revokeAdminSession(), nil)
+
+	r := armReservationRecord()
+	mockStore.On("GetPurchaseHistoryByPurchaseID", ctx, r.PurchaseID).Return(r, nil)
+
+	calcClient := &stubCalcRefundClientWithAmount{amount: 75.25, currency: "USD", sessID: "s-calc"}
+	h := &Handler{
+		config: mockStore,
+		auth:   mockAuth,
+		newAzureRevokeClients: func() (azureCalculateRefundClient, azureReturnClient, error) {
+			return calcClient, nil, nil
+		},
+	}
+
+	result, err := h.calculateAzureRevoke(ctx, sessionReq("tok"), r.PurchaseID)
+	require.NoError(t, err)
+	quote, ok := result.(*revokeQuoteResult)
+	require.True(t, ok)
+	assert.InDelta(t, 75.25, quote.RefundAmount, 0.0001)
+	assert.Equal(t, "USD", quote.RefundCurrency)
+}
+
+// TestCalculateAzureRevoke_ClientFactoryError verifies that when the injected
+// factory fails (e.g. credential resolution failure), calculateAzureRevoke
+// propagates the error wrapped with the "revoke/calculate:" prefix (matching
+// the prior inline-construction error shape) rather than as a ClientError.
+func TestCalculateAzureRevoke_ClientFactoryError(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	mockStore := new(MockConfigStore)
+	mockAuth := new(MockAuthService)
+	t.Cleanup(func() {
+		mockStore.AssertExpectations(t)
+		mockAuth.AssertExpectations(t)
+	})
+
+	mockAuth.On("ValidateSession", ctx, "tok").Return(revokeAdminSession(), nil)
+
+	r := armReservationRecord()
+	mockStore.On("GetPurchaseHistoryByPurchaseID", ctx, r.PurchaseID).Return(r, nil)
+
+	factoryErr := errors.New("credential unavailable")
+	h := &Handler{
+		config: mockStore,
+		auth:   mockAuth,
+		newAzureRevokeClients: func() (azureCalculateRefundClient, azureReturnClient, error) {
+			return nil, nil, factoryErr
+		},
+	}
+
+	_, err := h.calculateAzureRevoke(ctx, sessionReq("tok"), r.PurchaseID)
+	require.Error(t, err)
+	assert.ErrorContains(t, err, "revoke/calculate:")
+	assert.ErrorIs(t, err, factoryErr)
+	_, isClientErr := IsClientError(err)
+	assert.False(t, isClientErr, "client-factory failures are server-side errors, not a ClientError")
+}
+
+// TestRevokeAzurePurchase_Success exercises revokeAzurePurchase through the
+// injected client factory (happy path): the window check passes, both the
+// injected CalculateRefund and Return stubs are exercised via callAzureReturn,
+// and the purchase is marked revoked.
+func TestRevokeAzurePurchase_Success(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	mockStore := new(MockConfigStore)
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+
+	r := armReservationRecord()
+	mockStore.On("MarkPurchaseRevoked", ctx, r.PurchaseID, mock.AnythingOfType("time.Time"), "direct-api", "", mock.Anything, mock.Anything).Return(nil)
+
+	calcClient := &stubCalcRefundClientWithAmount{amount: 12.34, currency: "USD", sessID: "s-revoke"}
+	returnClient := &stubReturnClient{resp: armreservations.ReturnClientPostResponse{}}
+	h := &Handler{
+		config: mockStore,
+		newAzureRevokeClients: func() (azureCalculateRefundClient, azureReturnClient, error) {
+			return calcClient, returnClient, nil
+		},
+	}
+
+	result, err := h.revokeAzurePurchase(ctx, r, nil)
+	require.NoError(t, err)
+	m, ok := result.(*revokePurchaseResult)
+	require.True(t, ok)
+	assert.Equal(t, "revoked", m.Status)
+	assert.Equal(t, 1, returnClient.calls)
+}
+
+// TestRevokeAzurePurchase_ClientFactoryError verifies that when the injected
+// factory fails, revokeAzurePurchase propagates the error wrapped with the
+// "revoke azure:" prefix (matching the prior inline-construction error shape)
+// and never reaches callAzureReturn (no store calls expected).
+func TestRevokeAzurePurchase_ClientFactoryError(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	mockStore := new(MockConfigStore)
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+
+	r := armReservationRecord()
+	factoryErr := errors.New("credential unavailable")
+	h := &Handler{
+		config: mockStore,
+		newAzureRevokeClients: func() (azureCalculateRefundClient, azureReturnClient, error) {
+			return nil, nil, factoryErr
+		},
+	}
+
+	_, err := h.revokeAzurePurchase(ctx, r, nil)
+	require.Error(t, err)
+	assert.ErrorContains(t, err, "revoke azure:")
+	assert.ErrorIs(t, err, factoryErr)
+	_, isClientErr := IsClientError(err)
+	assert.False(t, isClientErr, "client-factory failures are server-side errors, not a ClientError")
+}
+
 // TestRevokePurchase_ConcurrentScheduledRevoke_OneWinsOneGets410 verifies that
 // two parallel revoke requests for the same scheduled execution produce the
 // correct outcomes: the first CAS wins (canceled), the second CAS loses and

From 4abfc3314cebe41b127821cdf7137d5de82f91c9 Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Sun, 19 Jul 2026 22:31:20 +0200
Subject: [PATCH 2/2] fix(api): construct only required Azure revoke clients

---
 internal/api/handler.go                       | 14 ++---
 internal/api/handler_purchases_revoke.go      | 57 ++++++++++++++-----
 internal/api/handler_purchases_revoke_test.go | 56 +++++++++++++++---
 3 files changed, 93 insertions(+), 34 deletions(-)

diff --git a/internal/api/handler.go b/internal/api/handler.go
index 9979a614d..752684919 100644
--- a/internal/api/handler.go
+++ b/internal/api/handler.go
@@ -132,16 +132,10 @@ type Handler struct {
 	// /health endpoint only — never logged outside that one place.
 	encryptionKeySource string
 
-	// newAzureRevokeClients builds the CalculateRefund and Return clients used
-	// by the Azure purchase-revoke handlers (calculateAzureRevoke,
-	// revokeAzurePurchase). When nil (the production default),
-	// buildAzureRevokeClients constructs them from
-	// azidentity.NewDefaultAzureCredential plus the armreservations SDK
-	// client constructors. Tests override this to inject
-	// stubCalcRefundClient/stubReturnClient so the handlers can be exercised
-	// without walking the live Azure credential chain (az CLI / PowerShell /
-	// keychain prompts).
-	newAzureRevokeClients func() (azureCalculateRefundClient, azureReturnClient, error)
+	// Optional Azure revoke client factory injected by tests. When nil, the
+	// revoke handlers use the production Azure credential and client
+	// constructors.
+	azureRevokeFactory *azureRevokeClientFactory
 }
 
 // getRIUtilizationCache returns the Postgres-backed TTL cache for Cost
diff --git a/internal/api/handler_purchases_revoke.go b/internal/api/handler_purchases_revoke.go
index 632565c48..a43de2b76 100644
--- a/internal/api/handler_purchases_revoke.go
+++ b/internal/api/handler_purchases_revoke.go
@@ -93,6 +93,12 @@ type azureCalculateRefundClient interface {
 	Post(ctx context.Context, reservationOrderID string, body armreservations.CalculateRefundRequest, options *armreservations.CalculateRefundClientPostOptions) (armreservations.CalculateRefundClientPostResponse, error)
 }
 
+type azureRevokeClientFactory struct {
+	newCredential            func() (azcore.TokenCredential, error)
+	newCalculateRefundClient func(azcore.TokenCredential) (azureCalculateRefundClient, error)
+	newReturnClient          func(azcore.TokenCredential) (azureReturnClient, error)
+}
+
 // revokePurchaseResult is the JSON body returned on a successful revocation.
 type revokePurchaseResult struct {
 	Status     string `json:"status"`
@@ -409,7 +415,7 @@ func (h *Handler) calculateAzureRevoke(ctx context.Context, req *events.LambdaFu
 		return nil, err
 	}
 
-	calcClient, _, err := h.buildAzureRevokeClients()
+	calcClient, err := h.buildAzureCalculateRefundClient()
 	if err != nil {
 		return nil, fmt.Errorf("revoke/calculate: %w", err)
 	}
@@ -562,28 +568,49 @@ func (h *Handler) revokeAzurePurchase(ctx context.Context, record *config.Purcha
 	return h.callAzureReturn(ctx, calcClient, returnClient, record, orderID, reservationID, expectedRefundAmount)
 }
 
-// buildAzureRevokeClients returns the injected factory's result when
-// h.newAzureRevokeClients is set (test path), or constructs the production
-// CalculateRefund and Return clients from a single
-// azidentity.NewDefaultAzureCredential (production default). Mirrors the
-// AzureSPProber.NewClient injected-factory pattern
-// (internal/commitmentopts/probe_azure.go): a nil field always means "use the
-// production default", so existing production construction sites need no
-// changes.
-func (h *Handler) buildAzureRevokeClients() (azureCalculateRefundClient, azureReturnClient, error) {
-	if h.newAzureRevokeClients != nil {
-		return h.newAzureRevokeClients()
+func (h *Handler) getAzureRevokeFactory() azureRevokeClientFactory {
+	if h.azureRevokeFactory != nil {
+		return *h.azureRevokeFactory
+	}
+	return azureRevokeClientFactory{
+		newCredential: func() (azcore.TokenCredential, error) {
+			return azidentity.NewDefaultAzureCredential(nil)
+		},
+		newCalculateRefundClient: func(cred azcore.TokenCredential) (azureCalculateRefundClient, error) {
+			return armreservations.NewCalculateRefundClient(cred, nil)
+		},
+		newReturnClient: func(cred azcore.TokenCredential) (azureReturnClient, error) {
+			return armreservations.NewReturnClient(cred, nil)
+		},
 	}
+}
 
-	cred, err := azidentity.NewDefaultAzureCredential(nil)
+func (h *Handler) buildAzureCalculateRefundClient() (azureCalculateRefundClient, error) {
+	factory := h.getAzureRevokeFactory()
+	cred, err := factory.newCredential()
+	if err != nil {
+		return nil, fmt.Errorf("obtain credential: %w", err)
+	}
+	calcClient, err := factory.newCalculateRefundClient(cred)
+	if err != nil {
+		return nil, fmt.Errorf("create calculate-refund client: %w", err)
+	}
+	return calcClient, nil
+}
+
+// buildAzureRevokeClients constructs both clients from one credential because
+// the revoke flow calls CalculateRefund immediately before Return.
+func (h *Handler) buildAzureRevokeClients() (azureCalculateRefundClient, azureReturnClient, error) {
+	factory := h.getAzureRevokeFactory()
+	cred, err := factory.newCredential()
 	if err != nil {
 		return nil, nil, fmt.Errorf("obtain credential: %w", err)
 	}
-	calcClient, err := armreservations.NewCalculateRefundClient(cred, nil)
+	calcClient, err := factory.newCalculateRefundClient(cred)
 	if err != nil {
 		return nil, nil, fmt.Errorf("create calculate-refund client: %w", err)
 	}
-	returnClient, err := armreservations.NewReturnClient(cred, nil)
+	returnClient, err := factory.newReturnClient(cred)
 	if err != nil {
 		return nil, nil, fmt.Errorf("create return client: %w", err)
 	}
diff --git a/internal/api/handler_purchases_revoke_test.go b/internal/api/handler_purchases_revoke_test.go
index 08d88712e..234bc876b 100644
--- a/internal/api/handler_purchases_revoke_test.go
+++ b/internal/api/handler_purchases_revoke_test.go
@@ -1353,7 +1353,7 @@ func TestRevokePurchase_GetExecutionByIDDBError_Returns500(t *testing.T) {
 // and the armreservations SDK constructors inline, so no test could reach past
 // that point without walking the live Azure credential chain (spawning az /
 // pwsh, popping macOS keychain prompts). These tests drive both handlers
-// end-to-end through h.newAzureRevokeClients using the existing
+// end-to-end through h.azureRevokeFactory using the existing
 // stubCalcRefundClient(WithAmount)/stubReturnClient fakes, proving the
 // previously-unreachable code is now covered without any live client.
 
@@ -1377,11 +1377,25 @@ func TestCalculateAzureRevoke_Success(t *testing.T) {
 	mockStore.On("GetPurchaseHistoryByPurchaseID", ctx, r.PurchaseID).Return(r, nil)
 
 	calcClient := &stubCalcRefundClientWithAmount{amount: 75.25, currency: "USD", sessID: "s-calc"}
+	credentialCalls := 0
+	calculateClientCalls := 0
+	returnClientCalls := 0
 	h := &Handler{
 		config: mockStore,
 		auth:   mockAuth,
-		newAzureRevokeClients: func() (azureCalculateRefundClient, azureReturnClient, error) {
-			return calcClient, nil, nil
+		azureRevokeFactory: &azureRevokeClientFactory{
+			newCredential: func() (azcore.TokenCredential, error) {
+				credentialCalls++
+				return nil, nil
+			},
+			newCalculateRefundClient: func(azcore.TokenCredential) (azureCalculateRefundClient, error) {
+				calculateClientCalls++
+				return calcClient, nil
+			},
+			newReturnClient: func(azcore.TokenCredential) (azureReturnClient, error) {
+				returnClientCalls++
+				return &stubReturnClient{}, nil
+			},
 		},
 	}
 
@@ -1391,6 +1405,9 @@ func TestCalculateAzureRevoke_Success(t *testing.T) {
 	require.True(t, ok)
 	assert.InDelta(t, 75.25, quote.RefundAmount, 0.0001)
 	assert.Equal(t, "USD", quote.RefundCurrency)
+	assert.Equal(t, 1, credentialCalls)
+	assert.Equal(t, 1, calculateClientCalls)
+	assert.Zero(t, returnClientCalls, "calculate must not construct an unused Return client")
 }
 
 // TestCalculateAzureRevoke_ClientFactoryError verifies that when the injected
@@ -1416,8 +1433,10 @@ func TestCalculateAzureRevoke_ClientFactoryError(t *testing.T) {
 	h := &Handler{
 		config: mockStore,
 		auth:   mockAuth,
-		newAzureRevokeClients: func() (azureCalculateRefundClient, azureReturnClient, error) {
-			return nil, nil, factoryErr
+		azureRevokeFactory: &azureRevokeClientFactory{
+			newCredential: func() (azcore.TokenCredential, error) {
+				return nil, factoryErr
+			},
 		},
 	}
 
@@ -1444,10 +1463,24 @@ func TestRevokeAzurePurchase_Success(t *testing.T) {
 
 	calcClient := &stubCalcRefundClientWithAmount{amount: 12.34, currency: "USD", sessID: "s-revoke"}
 	returnClient := &stubReturnClient{resp: armreservations.ReturnClientPostResponse{}}
+	credentialCalls := 0
+	calculateClientCalls := 0
+	returnClientCalls := 0
 	h := &Handler{
 		config: mockStore,
-		newAzureRevokeClients: func() (azureCalculateRefundClient, azureReturnClient, error) {
-			return calcClient, returnClient, nil
+		azureRevokeFactory: &azureRevokeClientFactory{
+			newCredential: func() (azcore.TokenCredential, error) {
+				credentialCalls++
+				return nil, nil
+			},
+			newCalculateRefundClient: func(azcore.TokenCredential) (azureCalculateRefundClient, error) {
+				calculateClientCalls++
+				return calcClient, nil
+			},
+			newReturnClient: func(azcore.TokenCredential) (azureReturnClient, error) {
+				returnClientCalls++
+				return returnClient, nil
+			},
 		},
 	}
 
@@ -1457,6 +1490,9 @@ func TestRevokeAzurePurchase_Success(t *testing.T) {
 	require.True(t, ok)
 	assert.Equal(t, "revoked", m.Status)
 	assert.Equal(t, 1, returnClient.calls)
+	assert.Equal(t, 1, credentialCalls)
+	assert.Equal(t, 1, calculateClientCalls)
+	assert.Equal(t, 1, returnClientCalls)
 }
 
 // TestRevokeAzurePurchase_ClientFactoryError verifies that when the injected
@@ -1473,8 +1509,10 @@ func TestRevokeAzurePurchase_ClientFactoryError(t *testing.T) {
 	factoryErr := errors.New("credential unavailable")
 	h := &Handler{
 		config: mockStore,
-		newAzureRevokeClients: func() (azureCalculateRefundClient, azureReturnClient, error) {
-			return nil, nil, factoryErr
+		azureRevokeFactory: &azureRevokeClientFactory{
+			newCredential: func() (azcore.TokenCredential, error) {
+				return nil, factoryErr
+			},
 		},
 	}