From 876142578db2276936e261dda02fd11475a043c8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 5 Oct 2026 03:19:17 +0200 Subject: [PATCH 1/2] fix(api): scope revoke-any to the caller's accounts on executed purchases authorizeSessionRevoke returned early for revoke-any, so a user holding revoke-any but restricted to some cloud accounts could quote and return an executed Azure reservation in an account outside their scope. The scheduled-execution revoke path already gates on requireExecutionAccess (issue #92); the purchase-history path now applies the same account scope to revoke-any as to revoke-own. Only an unrestricted revoke-any caller may revoke a row with no cloud account association; scoped callers fail closed on it. Refs #386 --- internal/api/handler_purchases_revoke.go | 48 +++++----- internal/api/handler_purchases_revoke_test.go | 87 +++++++++++++++++++ 2 files changed, 109 insertions(+), 26 deletions(-) diff --git a/internal/api/handler_purchases_revoke.go b/internal/api/handler_purchases_revoke.go index 391dae0b..92c5645a 100644 --- a/internal/api/handler_purchases_revoke.go +++ b/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") } diff --git a/internal/api/handler_purchases_revoke_test.go b/internal/api/handler_purchases_revoke_test.go index 193c1bdf..930bea9c 100644 --- a/internal/api/handler_purchases_revoke_test.go +++ b/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"} From 4e0892740b68292d94930f9d6a0ec5fbbaf4ee3e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 5 Oct 2026 08:28:22 +0200 Subject: [PATCH 2/2] docs(auth): fix the revoke access comment The comment named the removed checkRevokeOwnAccountAccess and said admins revoke unattributed rows via revoke-any. Only an unrestricted revoke-any caller can, per checkRevokeAccountAccess. --- internal/auth/types.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/auth/types.go b/internal/auth/types.go index f3cd809d..91f55956 100644 --- a/internal/auth/types.go +++ b/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.