diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 9da77fade..064ca9001 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -423,9 +423,12 @@ func (h *Handler) requireDeleteOrCancelPurchasePermission(ctx context.Context, r // // Gate logic: // - stateless admin API key: always permitted. -// - update-any:purchases or cancel-any:purchases: permitted without an -// owner check (full-scope management and cancel-any operator paths). -// - otherwise: fetch the execution and require creator match. +// - update-any:purchases: permitted without an owner check or Constraints. +// - cancel-any:purchases: permitted without an owner check when its +// Constraints cover every recommendation (issue #678); a Constraints +// denial falls back to the creator branch. +// - otherwise: fetch the execution and require creator match, then apply +// cancel-own Constraints if the creator also holds cancel-own. // Legacy rows with a NULL creator are out of reach for non-privileged // users (matching cancel-own / approve-own / retry-own behavior). func (h *Handler) authorizePlannedPurchaseCancel(ctx context.Context, session *Session, executionID string) error { @@ -435,30 +438,91 @@ func (h *Handler) authorizePlannedPurchaseCancel(ctx context.Context, session *S if h.auth == nil { return NewClientError(500, "authentication service not configured") } - // update-any and cancel-any both grant full-scope access without an owner check. - for _, verb := range []string{auth.ActionUpdateAny, auth.ActionCancelAny} { - has, err := h.auth.HasPermissionAPI(ctx, session.UserID, verb, auth.ResourcePurchases) - if err != nil { - return fmt.Errorf("permission check failed: %w", err) - } - if has { - return nil + // update-any grants full-scope access without an owner check or + // Constraints (tracked by #961). + hasUpdate, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionUpdateAny, auth.ResourcePurchases) + if err != nil { + return fmt.Errorf("permission check failed: %w", err) + } + if hasUpdate { + return nil + } + hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionCancelAny, auth.ResourcePurchases) + if err != nil { + return fmt.Errorf("permission check failed: %w", err) + } + execution, err := h.loadPlannedExecution(ctx, executionID) + if err != nil { + return err + } + // cancel-any is bounded by its Constraints (issue #678); a 403 falls back + // to the creator branch exactly as authorizeSessionCancel does. + var anyErr error + if hasAny { + // Wrapped for consistency; DELETE planned has no token fall-through. + anyErr = wrapConstraintDenied(h.requirePermissionConstraints(ctx, session, auth.ActionCancelAny, auth.ResourcePurchases, revokeExecutionConstraintSets(execution))) + if anyErr == nil || !isForbidden(anyErr) { + return anyErr } } + return h.authorizePlannedCreatorCancel(ctx, session, execution, anyErr) +} + +// loadPlannedExecution fetches the row, mapping a missing one to errNotFound. +func (h *Handler) loadPlannedExecution(ctx context.Context, executionID string) (*config.PurchaseExecution, error) { execution, err := h.config.GetExecutionByID(ctx, executionID) if errors.Is(err, config.ErrNotFound) { - return errNotFound + return nil, errNotFound } if err != nil { - return fmt.Errorf("failed to get execution: %w", err) + return nil, fmt.Errorf("failed to get execution: %w", err) } + return execution, nil +} + +// authorizePlannedCreatorCancel is the creator branch of +// authorizePlannedPurchaseCancel. anyErr is the cancel-any Constraints denial +// (nil when the session has no cancel-any); it is returned instead of the +// ownership 403 so a constraint refusal stays terminal. +func (h *Handler) authorizePlannedCreatorCancel(ctx context.Context, session *Session, execution *config.PurchaseExecution, anyErr error) error { // requireDeleteOrCancelPurchasePermission already validated the session; - // UserID is either the API-key sentinel (handled above) or a real user UUID - // from ValidateSession. The nil/empty guard here is therefore defensive. + // UserID is either the API-key sentinel (handled by the caller) or a real + // user UUID from ValidateSession. The nil/empty guard here is defensive. if execution.CreatedByUserID == nil || *execution.CreatedByUserID != session.UserID { + if anyErr != nil { + return anyErr + } return NewClientError(403, "permission denied: cannot cancel another user's scheduled purchase") } - return nil + // A creator match alone is enough (delete:purchases holders need no + // cancel-own, #961); a creator who also holds cancel-own is bounded by it. + hasOwn, err := h.sessionHoldsVerb(ctx, session, auth.ActionCancelOwn) + if err != nil { + return err + } + if !hasOwn { + return nil + } + return wrapConstraintDenied(h.requirePermissionConstraints(ctx, session, auth.ActionCancelOwn, auth.ResourcePurchases, revokeExecutionConstraintSets(execution))) +} + +// sessionHoldsVerb reports whether the principal that requirePermissionConstraints +// evaluates holds verb on purchases: the owner for a user session, the KEY's +// effective permissions for a user-API-key session. The key path probes with an +// empty, non-strict set, which any held permission satisfies whatever its +// Constraints, so a denial means the verb is not held. +func (h *Handler) sessionHoldsVerb(ctx context.Context, session *Session, verb string) (bool, error) { + var has bool + var err error + if session.UserAPIKeyID != "" { + has, err = h.auth.HasAPIKeyPermissionForConstraintsAPI(ctx, session.UserAPIKeyID, session.UserID, verb, auth.ResourcePurchases, []auth.PermissionConstraints{{}}) + } else { + has, err = h.auth.HasPermissionAPI(ctx, session.UserID, verb, auth.ResourcePurchases) + } + if err != nil { + return false, fmt.Errorf("permission check failed: %w", err) + } + return has, nil } func (h *Handler) deletePlannedPurchase(ctx context.Context, req *events.LambdaFunctionURLRequest, executionID string) (*DeletePlannedPurchaseResponse, error) { diff --git a/internal/api/handler_purchases_cancel_constraints_test.go b/internal/api/handler_purchases_cancel_constraints_test.go index 8ef09c57a..d58535f73 100644 --- a/internal/api/handler_purchases_cancel_constraints_test.go +++ b/internal/api/handler_purchases_cancel_constraints_test.go @@ -276,3 +276,168 @@ func TestRevokeViaEmailToken_SessionBranchEnforcesCancelConstraints(t *testing.T store.AssertCalled(t, "TransitionExecutionStatus", mock.Anything, cancelExecID, mock.Anything, "revocation_requested", mock.Anything) }) } + +func deletePlannedReq() *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{Headers: map[string]string{"authorization": "Bearer sess-tok"}} +} + +func TestDeletePlannedPurchase_CancelConstraints(t *testing.T) { + t.Parallel() + ctx := context.Background() + creator := revokeConstraintsUser + del := auth.Permission{Action: auth.ActionDelete, Resource: auth.ResourcePurchases} + run := func(creatorID *string, perms ...auth.Permission) (*DeletePlannedPurchaseResponse, *MockConfigStore, error) { + exec := cancelRouteExec("pending") + exec.CreatedByUserID = creatorID + h, store := cancelRouteHandler(exec, perms...) + resp, err := h.deletePlannedPurchase(ctx, deletePlannedReq(), cancelExecID) + return resp, store, err + } + notTransitioned := func(t *testing.T, store *MockConfigStore) { + t.Helper() + store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + } + + t.Run("constrained cancel-any out of scope is denied", func(t *testing.T) { + t.Parallel() + _, store, err := run(nil, awsOnly(auth.ActionCancelAny)) + requireRevokeForbidden(t, err, constraintDenied) + notTransitioned(t, store) + }) + t.Run("unconstrained and in-scope cancel-any cancel", func(t *testing.T) { + t.Parallel() + for _, c := range []*auth.PermissionConstraints{nil, {Providers: []string{"azure"}}} { + resp, _, err := run(nil, revokePerm(auth.ActionCancelAny, c)) + require.NoError(t, err) + assert.Equal(t, "canceled", resp.Status) + } + }) + t.Run("constrained any falls back to creator with unconstrained own", func(t *testing.T) { + t.Parallel() + resp, _, err := run(&creator, awsOnly(auth.ActionCancelAny), revokePerm(auth.ActionCancelOwn, nil)) + require.NoError(t, err) + assert.Equal(t, "canceled", resp.Status) + }) + t.Run("constrained any for a non-creator keeps the constraint message", func(t *testing.T) { + t.Parallel() + other := "someone-else" + _, store, err := run(&other, awsOnly(auth.ActionCancelAny), revokePerm(auth.ActionCancelOwn, nil)) + requireRevokeForbidden(t, err, constraintDenied) + notTransitioned(t, store) + }) + t.Run("creator with constrained cancel-own out of scope is denied", func(t *testing.T) { + t.Parallel() + _, store, err := run(&creator, awsOnly(auth.ActionCancelOwn)) + requireRevokeForbidden(t, err, constraintDenied) + notTransitioned(t, store) + }) + t.Run("creator with delete and no cancel-own still deletes", func(t *testing.T) { + t.Parallel() + resp, _, err := run(&creator, del) + require.NoError(t, err) + assert.Equal(t, "canceled", resp.Status) + }) + t.Run("row with no creator is denied for delete-only callers", func(t *testing.T) { + t.Parallel() + _, store, err := run(nil, del) + requireRevokeForbidden(t, err, "another user's scheduled purchase") + notTransitioned(t, store) + }) + t.Run("update-any is unchanged and unconstrained", func(t *testing.T) { + t.Parallel() + resp, _, err := run(nil, revokePerm(auth.ActionUpdateAny, &auth.PermissionConstraints{Providers: []string{"aws"}})) + require.NoError(t, err) + assert.Equal(t, "canceled", resp.Status) + }) +} + +// plannedKeyHandler models a delete:purchases user-API-key request whose owner +// created the execution and holds cancel-own. keyCancelOwn is what the KEY holds +// (probe), keyInScope what its cancel-own Constraints allow for the execution. +func plannedKeyHandler(keyCancelOwn, keyInScope bool) (*Handler, *MockConfigStore) { + ctx := mock.Anything + owner, keyID := revokeConstraintsUser, "key-1" + exec := cancelRouteExec("pending") + exec.CreatedByUserID = &owner + store := new(MockConfigStore) + store.On("GetExecutionByID", ctx, cancelExecID).Return(exec, nil) + store.On("TransitionExecutionStatus", ctx, cancelExecID, mock.Anything, "canceled", mock.Anything). + Return(&config.PurchaseExecution{ExecutionID: cancelExecID, Status: "canceled"}, nil).Maybe() + store.On("DeleteSuppressionsByExecutionTx", ctx, ctx, cancelExecID).Return(nil).Maybe() + m := new(MockAuthService) + m.On("HasAPIKeyPermissionAPI", ctx, "user-key-value", auth.ActionDelete, auth.ResourcePurchases).Return(owner, keyID, true, nil) + m.On("GetAllowedAccountsAPI", ctx, owner).Return([]string{}, nil) + m.On("HasPermissionAPI", ctx, owner, mock.Anything, auth.ResourcePurchases).Return(false, nil).Maybe() + m.On("HasPermissionAPI", ctx, owner, auth.ActionCancelOwn, auth.ResourcePurchases).Return(true, nil).Maybe() + m.On("HasAPIKeyPermissionForConstraintsAPI", ctx, keyID, owner, auth.ActionCancelOwn, auth.ResourcePurchases, []auth.PermissionConstraints{{}}).Return(keyCancelOwn, nil) + m.On("HasAPIKeyPermissionForConstraintsAPI", ctx, keyID, owner, auth.ActionCancelOwn, auth.ResourcePurchases, mock.Anything).Return(keyInScope, nil).Maybe() + return &Handler{config: store, auth: m}, store +} + +func TestDeletePlannedPurchase_APIKeyCancelOwnFollowsTheKey(t *testing.T) { + t.Parallel() + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "user-key-value"}} + t.Run("delete-only key keeps working although the owner holds cancel-own", func(t *testing.T) { + t.Parallel() + h, _ := plannedKeyHandler(false, false) + resp, err := h.deletePlannedPurchase(context.Background(), req, cancelExecID) + require.NoError(t, err) + assert.Equal(t, "canceled", resp.Status) + }) + t.Run("key with constrained cancel-own out of scope is denied", func(t *testing.T) { + t.Parallel() + h, store := plannedKeyHandler(true, false) + _, err := h.deletePlannedPurchase(context.Background(), req, cancelExecID) + requireRevokeForbidden(t, err, constraintDenied) + store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) + t.Run("key with in-scope cancel-own is allowed", func(t *testing.T) { + t.Parallel() + h, _ := plannedKeyHandler(true, true) + _, err := h.deletePlannedPurchase(context.Background(), req, cancelExecID) + require.NoError(t, err) + }) +} + +func TestDeletePlannedPurchase_ConstraintStoreErrorDoesNotFallBack(t *testing.T) { + t.Parallel() + ctx := context.Background() + boom := errors.New("constraint store down") + exec := cancelRouteExec("pending") + owner := revokeConstraintsUser + exec.CreatedByUserID = &owner + store := new(MockConfigStore) + store.On("GetExecutionByID", mock.Anything, cancelExecID).Return(exec, nil) + m := new(MockAuthService) + m.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{UserID: owner}, nil) + m.On("GetAllowedAccountsAPI", mock.Anything, owner).Return([]string{}, nil) + m.On("HasPermissionAPI", mock.Anything, owner, auth.ActionCancelAny, auth.ResourcePurchases).Return(true, nil) + m.On("HasPermissionAPI", mock.Anything, owner, mock.Anything, auth.ResourcePurchases).Return(false, nil).Maybe() + m.On("HasPermissionForConstraintsAPI", mock.Anything, owner, auth.ActionCancelAny, auth.ResourcePurchases, mock.Anything).Return(false, boom) + // cancel-own is deliberately denied: a fallback would let the creator delete. + h := &Handler{config: store, auth: m} + _, err := h.deletePlannedPurchase(ctx, deletePlannedReq(), cancelExecID) + require.ErrorIs(t, err, boom) + store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +func TestDeletePlannedPurchase_APIKeyVerbProbeErrorRefuses(t *testing.T) { + t.Parallel() + boom := errors.New("key store down") + owner, keyID := revokeConstraintsUser, "key-1" + exec := cancelRouteExec("pending") + exec.CreatedByUserID = &owner + store := new(MockConfigStore) + store.On("GetExecutionByID", mock.Anything, cancelExecID).Return(exec, nil) + m := new(MockAuthService) + m.On("HasAPIKeyPermissionAPI", mock.Anything, "user-key-value", auth.ActionDelete, auth.ResourcePurchases).Return(owner, keyID, true, nil) + m.On("GetAllowedAccountsAPI", mock.Anything, owner).Return([]string{}, nil) + m.On("HasPermissionAPI", mock.Anything, owner, mock.Anything, auth.ResourcePurchases).Return(false, nil).Maybe() + m.On("HasAPIKeyPermissionForConstraintsAPI", mock.Anything, keyID, owner, auth.ActionCancelOwn, auth.ResourcePurchases, []auth.PermissionConstraints{{}}).Return(false, boom) + + h := &Handler{config: store, auth: m} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "user-key-value"}} + _, err := h.deletePlannedPurchase(context.Background(), req, cancelExecID) + require.ErrorIs(t, err, boom) + store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 9facf9d3e..e547a9e86 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -5274,6 +5274,8 @@ func buildCancelOwnHandler(userID, creatorID string) (*Handler, *MockConfigStore mockAuth.On("HasPermissionAPI", mock.Anything, userID, "update-any", "purchases").Return(false, nil).Maybe() mockAuth.On("GetAllowedAccountsAPI", mock.Anything, userID).Return([]string{}, nil).Maybe() + mockAuth.allowConstraintChecks() + creator := creatorID exec := &config.PurchaseExecution{ExecutionID: ownExecID, Status: "pending", CreatedByUserID: &creator} mockConfig := new(MockConfigStore) @@ -5383,6 +5385,9 @@ func TestHandler_deletePlannedPurchase_UserAPIKey_Allowed(t *testing.T) { // since the owning user holds neither update-any nor cancel-any. mockAuth.On("HasPermissionAPI", ctx, ownerUserID, auth.ActionUpdateAny, auth.ResourcePurchases).Return(false, nil) mockAuth.On("HasPermissionAPI", ctx, ownerUserID, auth.ActionCancelAny, auth.ResourcePurchases).Return(false, nil) + // The key holds delete:purchases only: probing its cancel-own is denied. + mockAuth.On("HasAPIKeyPermissionForConstraintsAPI", ctx, keyID, ownerUserID, auth.ActionCancelOwn, auth.ResourcePurchases, []auth.PermissionConstraints{{}}). + Return(false, nil) creator := ownerUserID execution := &config.PurchaseExecution{ExecutionID: execID, Status: "pending", CreatedByUserID: &creator} diff --git a/internal/api/router_660_permission_flips_test.go b/internal/api/router_660_permission_flips_test.go index fc23c0fec..c3635af39 100644 --- a/internal/api/router_660_permission_flips_test.go +++ b/internal/api/router_660_permission_flips_test.go @@ -346,6 +346,8 @@ func TestDeletePlannedPurchase_PermissionGate(t *testing.T) { // authorizePlannedPurchaseCancel also checks cancel-any when update-any // is false (issue #1400); register it so the mock does not panic. mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-any", "purchases").Return(false, nil) + // The creator branch asks whether cancel-own is also held (issue #678). + mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-own", "purchases").Return(false, nil) creator := userID mockStore := new(MockConfigStore) mockStore.On("GetExecutionByID", ctx, execID). @@ -397,6 +399,7 @@ func TestDeletePlannedPurchase_PermissionGate(t *testing.T) { mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-own", "purchases").Return(true, nil) mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) mockAuth.On("HasPermissionAPI", ctx, userID, "update-any", "purchases").Return(false, nil) + mockAuth.allowConstraintChecks() t.Cleanup(func() { mockAuth.AssertExpectations(t) }) creator := userID