fix(api): scope revoke-any to the caller's accounts on executed purchases

internal/api/handler_purchases_revoke.go

@@ -367,43 +367,39 @@ func (h *Handler) authorizeSessionRevoke(ctx context.Context, session *Session,
 	if err != nil {
 		return fmt.Errorf("permission check failed: %w", err)
 	}
-	if hasAny {
-		return nil
-	}
-
-	hasOwn, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionRevokeOwn, auth.ResourcePurchases)
-	if err != nil {
-		return fmt.Errorf("permission check failed: %w", err)
-	}
-	if !hasOwn {
-		return NewClientError(403, "permission denied: requires revoke-any or revoke-own on purchases")
+	if !hasAny {
+		hasOwn, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionRevokeOwn, auth.ResourcePurchases)
+		if err != nil {
+			return fmt.Errorf("permission check failed: %w", err)
+		}
+		if !hasOwn {
+			return NewClientError(403, "permission denied: requires revoke-any or revoke-own on purchases")
+		}
 	}
 
-	return h.checkRevokeOwnAccountAccess(ctx, session.UserID, record)
+	return h.checkRevokeAccountAccess(ctx, session, record, hasAny)
 }
 
-// checkRevokeOwnAccountAccess enforces the account-scope ownership constraint
-// for revoke-own: the purchase must be in a cloud account the session user
-// is allowed to access. Extracted from authorizeSessionRevoke to keep that
-// function's cyclomatic complexity within the project limit.
-func (h *Handler) checkRevokeOwnAccountAccess(ctx context.Context, userID string, record *config.PurchaseHistoryRecord) error {
-	// Purchase history rows pre-date created_by_user_id; ownership is via
-	// account access (same model as the per-account-perms middleware used
-	// elsewhere in the history view). Whether revoke-own should be tightened
-	// to creator scope instead is a product decision tracked in issue #950.
-	// Fail closed for revoke-own: if the purchase has no account association
-	// we cannot verify ownership, so deny rather than allow an unscoped revoke.
-	if record.CloudAccountID == nil || *record.CloudAccountID == "" {
-		return NewClientError(403, "permission denied: cannot verify ownership for this purchase")
-	}
+// checkRevokeAccountAccess requires the purchase to be in a cloud account the
+// session may access. revoke-any lifts no account scope (issue #386, the #92
+// class): only an unrestricted revoke-any caller skips the check.
+func (h *Handler) checkRevokeAccountAccess(ctx context.Context, session *Session, record *config.PurchaseHistoryRecord, revokeAny bool) error {
 	// Goes through getAccountScope rather than calling GetAllowedAccountsAPI
 	// directly: the direct call skipped the admin-API-key and nil-auth
 	// branches, and the `len(allowed) > 0 &&` guard read an empty list as
 	// unrestricted independently of any producer (issue #1748).
-	scope, err := h.getAccountScope(ctx, &Session{UserID: userID})
+	scope, err := h.getAccountScope(ctx, session)
 	if err != nil {
 		return fmt.Errorf("account access check failed: %w", err)
 	}
+	if revokeAny && scope.AllowsAll() {
+		return nil
+	}
+	// Purchase history rows pre-date created_by_user_id, so ownership is via
+	// account access (creator scope: issue #950). Unattributed rows fail closed.
+	if record.CloudAccountID == nil || *record.CloudAccountID == "" {
+		return NewClientError(403, "permission denied: cannot verify ownership for this purchase")
+	}
 	if !scope.Allows(*record.CloudAccountID, "") {
 		return NewClientError(403, "permission denied: purchase is in an account you do not have access to")
 	}

internal/api/handler_purchases_revoke_test.go

@@ -455,14 +455,99 @@ func TestAuthorizeSessionRevoke_RevokeAny(t *testing.T) {
 	t.Cleanup(func() { mockAuth.AssertExpectations(t) })
 
 	mockAuth.On("HasPermissionAPI", ctx, "u-1", "revoke-any", "purchases").Return(true, nil)
+	mockAuth.On("GetAllowedAccountsAPI", ctx, "u-1").Return([]string{}, nil)
 
 	h := &Handler{auth: mockAuth}
 	sess := &Session{UserID: "u-1"}
+	// Unrestricted revoke-any may revoke even a row with no account association.
 	r := &config.PurchaseHistoryRecord{}
 	err := h.authorizeSessionRevoke(ctx, sess, r)
 	require.NoError(t, err)
 }
 
+// TestAuthorizeSessionRevoke_RevokeAny_AccountScope pins issue #386: revoke-any
+// lifts the ownership requirement but not the session's account scope.
+func TestAuthorizeSessionRevoke_RevokeAny_AccountScope(t *testing.T) {
+	t.Parallel()
+	inScope := "aaaa-1111"
+	outOfScope := "bbbb-2222"
+	empty := ""
+	cases := []struct {
+		name      string
+		accountID *string
+		wantErr   string
+	}{
+		{name: "in scope", accountID: &inScope},
+		{name: "other account", accountID: &outOfScope, wantErr: "account you do not have access to"},
+		{name: "nil account", accountID: nil, wantErr: "cannot verify ownership"},
+		{name: "empty account", accountID: &empty, wantErr: "cannot verify ownership"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			ctx := context.Background()
+			mockAuth := new(MockAuthService)
+			t.Cleanup(func() { mockAuth.AssertExpectations(t) })
+			mockAuth.On("HasPermissionAPI", ctx, "u-1", "revoke-any", "purchases").Return(true, nil)
+			mockAuth.On("GetAllowedAccountsAPI", ctx, "u-1").Return([]string{inScope}, nil)
+
+			h := &Handler{auth: mockAuth}
+			err := h.authorizeSessionRevoke(ctx, &Session{UserID: "u-1"}, &config.PurchaseHistoryRecord{CloudAccountID: tc.accountID})
+			if tc.wantErr == "" {
+				require.NoError(t, err)
+				return
+			}
+			ce, ok := IsClientError(err)
+			require.True(t, ok, "expected ClientError, got %T: %v", err, err)
+			assert.Equal(t, 403, ce.code)
+			assert.Contains(t, ce.Error(), tc.wantErr)
+		})
+	}
+}
+
+// TestRevokePurchase_RevokeAnyOutOfScopeNeverCallsAzure drives the real revoke
+// endpoint for a scoped revoke-any user against an executed Azure purchase in
+// another account (issue #386): it must 403 before any Azure client is built.
+func TestRevokePurchase_RevokeAnyOutOfScopeNeverCallsAzure(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	mockStore := new(MockConfigStore)
+	mockAuth := new(MockAuthService)
+	t.Cleanup(func() {
+		mockStore.AssertExpectations(t)
+		mockAuth.AssertExpectations(t)
+	})
+
+	otherAccount := "bbbb-2222"
+	r := armReservationRecord()
+	r.CloudAccountID = &otherAccount
+	mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "u-1", Email: "u1@example.com"}, nil)
+	mockAuth.On("HasPermissionAPI", ctx, "u-1", "revoke-any", "purchases").Return(true, nil)
+	mockAuth.On("GetAllowedAccountsAPI", ctx, "u-1").Return([]string{"aaaa-1111"}, nil)
+	mockStore.On("GetExecutionByID", ctx, r.PurchaseID).Return(nil, fmt.Errorf("%w: execution", config.ErrNotFound))
+	mockStore.On("GetPurchaseHistoryByPurchaseID", ctx, r.PurchaseID).Return(r, nil)
+
+	azureCalls := 0
+	h := &Handler{
+		config: mockStore,
+		auth:   mockAuth,
+		azureRevokeFactory: &azureRevokeClientFactory{
+			newCredential: func() (azcore.TokenCredential, error) {
+				azureCalls++
+				return nil, errors.New("azure must not be reached")
+			},
+		},
+	}
+
+	req := sessionReq("tok")
+	req.Body = `{"expected_refund_amount":10,"expected_refund_currency":"USD"}`
+	_, err := h.revokePurchase(ctx, req, r.PurchaseID)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected ClientError, got %T: %v", err, err)
+	assert.Equal(t, 403, ce.code)
+	assert.Zero(t, azureCalls, "no Azure client may be built for an out-of-scope purchase")
+}
+
 func TestAuthorizeSessionRevoke_RevokeOwn_AccountAccessGranted(t *testing.T) {
 	t.Parallel()
 	ctx := context.Background()
@@ -534,6 +619,8 @@ func TestAuthorizeSessionRevoke_RevokeOwn_NilAccountID(t *testing.T) {
 
 	mockAuth.On("HasPermissionAPI", ctx, "u-1", "revoke-any", "purchases").Return(false, nil)
 	mockAuth.On("HasPermissionAPI", ctx, "u-1", "revoke-own", "purchases").Return(true, nil)
+	// Unrestricted scope must not excuse an unattributed revoke-own.
+	mockAuth.On("GetAllowedAccountsAPI", ctx, "u-1").Return([]string{}, nil)
 
 	h := &Handler{auth: mockAuth}
 	sess := &Session{UserID: "u-1"}

internal/auth/types.go

@@ -509,11 +509,11 @@ const (
 	//     "Own" is currently enforced at ACCOUNT scope, not creator scope:
 	//     a user may revoke a completed purchase in any cloud account they
 	//     are allowed to access (the check in
-	//     api.checkRevokeOwnAccountAccess via GetAllowedAccountsAPI), because
+	//     api.checkRevokeAccountAccess via GetAllowedAccountsAPI), because
 	//     purchase_history rows pre-date created_by_user_id and have no
 	//     reliable per-creator attribution. Rows with no account association
-	//     (CloudAccountID NULL) are out of reach for non-admins (fail-closed);
-	//     admins still revoke them via revoke-any.
+	//     (CloudAccountID NULL) fail closed; only an unrestricted revoke-any
+	//     caller can revoke them.
 	//     NOTE: whether revoke-own should instead be creator-scoped is a
 	//     product decision tracked in issue #950; do not tighten this to
 	//     created_by_user_id without resolving that issue first.