diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index af2c99a92..68593e95b 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -436,14 +436,14 @@ func (h *Handler) requireDeleteOrCancelPurchasePermission(ctx context.Context, r // // Gate logic: // - stateless admin API key: always permitted. -// - 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). +// - update-any:purchases, then cancel-any:purchases: each is permitted +// without an owner check when its Constraints cover every recommendation +// (issues #961 and #678). A Constraints denial falls through to the next +// full-scope verb and then to the creator branch. +// - otherwise: 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 { if session.UserID == apiKeyAdminUserID { return nil @@ -451,36 +451,52 @@ func (h *Handler) authorizePlannedPurchaseCancel(ctx context.Context, session *S if h.auth == nil { return NewClientError(500, "authentication service not configured") } - // update-any grants full-scope access without an owner check or - // Constraints (tracked by #961). Both full-scope verbs are evaluated - // against the session's effective permissions: the key's for a user API - // key, never the owner's alone (issue #979). - hasUpdate, err := h.sessionHoldsVerb(ctx, session, auth.ActionUpdateAny) - if err != nil { - return err - } - if hasUpdate { - return nil - } - hasAny, err := h.sessionHoldsVerb(ctx, session, auth.ActionCancelAny) + execution, err := h.loadPlannedExecution(ctx, executionID) if err != nil { return err } - execution, err := h.loadPlannedExecution(ctx, executionID) - if err != nil { + granted, denied, err := h.authorizePlannedFullScope(ctx, session, execution) + if err != nil || granted { 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 { + return h.authorizePlannedCreatorCancel(ctx, session, execution, denied) +} + +// plannedFullScopeVerbs are the verbs that let DELETE planned act on another +// user's row, in the order they are tried. +var plannedFullScopeVerbs = []string{auth.ActionUpdateAny, auth.ActionCancelAny} + +// authorizePlannedFullScope tries each full-scope verb the session holds +// against the strict per-recommendation Constraints of the STORED row +// (revokeExecutionConstraintSets), never the request. granted reports that one +// of them covers the row. denied is the last Constraints 403, returned to a +// non-creator in place of the ownership 403 so the refusal stays terminal; it +// is nil when the session holds neither verb. Any other error (a probe or +// constraint-store failure) is returned as err and never read as a denial, so +// it cannot fall through to a weaker branch. Both verbs are evaluated against +// the session's effective permissions: the key's for a user API key, never the +// owner's alone (issue #979). +func (h *Handler) authorizePlannedFullScope(ctx context.Context, session *Session, execution *config.PurchaseExecution) (granted bool, denied, err error) { + sets := revokeExecutionConstraintSets(execution) + for _, verb := range plannedFullScopeVerbs { + held, probeErr := h.sessionHoldsVerb(ctx, session, verb) + if probeErr != nil { + return false, nil, probeErr + } + if !held { + continue + } // 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 + verbErr := wrapConstraintDenied(h.requirePermissionConstraints(ctx, session, verb, auth.ResourcePurchases, sets)) + if verbErr == nil { + return true, nil, nil + } + if !isForbidden(verbErr) { + return false, nil, verbErr } + denied = verbErr } - return h.authorizePlannedCreatorCancel(ctx, session, execution, anyErr) + return false, denied, nil } // loadPlannedExecution fetches the row, mapping a missing one to errNotFound. @@ -496,9 +512,9 @@ func (h *Handler) loadPlannedExecution(ctx context.Context, executionID string) } // 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. +// authorizePlannedPurchaseCancel. anyErr is the update-any or cancel-any +// Constraints denial (nil when the session holds neither); 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 by the caller) or a real diff --git a/internal/api/handler_purchases_cancel_constraints_test.go b/internal/api/handler_purchases_cancel_constraints_test.go index 2235f440e..951d3c26d 100644 --- a/internal/api/handler_purchases_cancel_constraints_test.go +++ b/internal/api/handler_purchases_cancel_constraints_test.go @@ -345,11 +345,11 @@ func TestDeletePlannedPurchase_CancelConstraints(t *testing.T) { requireRevokeForbidden(t, err, "another user's scheduled purchase") notTransitioned(t, store) }) - t.Run("update-any is unchanged and unconstrained", func(t *testing.T) { + t.Run("update-any out of scope is denied (#961)", 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) + _, store, err := run(nil, awsOnly(auth.ActionUpdateAny)) + requireRevokeForbidden(t, err, constraintDenied) + notTransitioned(t, store) }) } diff --git a/internal/api/handler_purchases_planned_update_any_test.go b/internal/api/handler_purchases_planned_update_any_test.go new file mode 100644 index 000000000..4506a232c --- /dev/null +++ b/internal/api/handler_purchases_planned_update_any_test.go @@ -0,0 +1,221 @@ +package api + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Issue #961: on DELETE /api/purchases/planned/{id}, update-any:purchases +// granted full scope on the verb alone and never evaluated the granting +// permission's Constraints, so a session whose update-any is bounded to one +// provider could cancel any provider's scheduled purchase. Answers come from +// the real matcher via grantPermissionsScoped (bearer sessions) or a real +// auth.Service (API keys); the cancelRouteExec row is azure/compute/ +// westeurope on account "acct-1". + +const plannedOtherUser = "someone-else" + +// runUpdateAnyDelete drives deletePlannedPurchase for a row created by creator +// (nil = legacy NULL creator), optionally replacing its recommendations. +func runUpdateAnyDelete(creator *string, recs []config.RecommendationRecord, perms ...auth.Permission) (*DeletePlannedPurchaseResponse, *MockConfigStore, error) { + exec := cancelRouteExec("pending") + exec.CreatedByUserID = creator + if recs != nil { + exec.Recommendations = recs + } + h, store := cancelRouteHandler(exec, perms...) + resp, err := h.deletePlannedPurchase(context.Background(), deletePlannedReq(), cancelExecID) + return resp, store, err +} + +func requireNotCanceled(t *testing.T, store *MockConfigStore) { + t.Helper() + store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +func requireCanceled(t *testing.T, resp *DeletePlannedPurchaseResponse, err error) { + t.Helper() + require.NoError(t, err) + require.NotNil(t, resp) + assert.Equal(t, "canceled", resp.Status) +} + +func TestDeletePlannedPurchase_UpdateAnyConstraintDimensions(t *testing.T) { + t.Parallel() + other := plannedOtherUser + denying := map[string]*auth.PermissionConstraints{ + "providers": {Providers: []string{"aws"}}, + "services": {Services: []string{"rds"}}, + "regions": {Regions: []string{"eastus"}}, + "account ids": {AccountIDs: []string{"other-acct"}}, + } + allowing := map[string]*auth.PermissionConstraints{ + "providers": {Providers: []string{"azure"}}, + "services": {Services: []string{"compute"}}, + "regions": {Regions: []string{"westeurope"}}, + "account ids": {AccountIDs: []string{"acct-1"}}, + "none": nil, + } + for dim, c := range denying { + t.Run("deny/"+dim, func(t *testing.T) { + t.Parallel() + _, store, err := runUpdateAnyDelete(&other, nil, revokePerm(auth.ActionUpdateAny, c)) + requireRevokeForbidden(t, err, constraintDenied) + requireNotCanceled(t, store) + }) + } + for dim, c := range allowing { + t.Run("allow/"+dim, func(t *testing.T) { + t.Parallel() + resp, _, err := runUpdateAnyDelete(&other, nil, revokePerm(auth.ActionUpdateAny, c)) + requireCanceled(t, resp, err) + }) + } +} + +func TestDeletePlannedPurchase_UpdateAnyStrictScopeAndRecommendations(t *testing.T) { + t.Parallel() + other := plannedOtherUser + azureOnly := revokePerm(auth.ActionUpdateAny, &auth.PermissionConstraints{Providers: []string{"azure"}}) + t.Run("unknown region is refused by a region-bounded grant", func(t *testing.T) { + t.Parallel() + recs := []config.RecommendationRecord{recFor("acct-1", "azure", "compute", "")} + _, store, err := runUpdateAnyDelete(&other, recs, revokePerm(auth.ActionUpdateAny, &auth.PermissionConstraints{Regions: []string{"westeurope"}})) + requireRevokeForbidden(t, err, constraintDenied) + requireNotCanceled(t, store) + }) + t.Run("every recommendation must be in scope", func(t *testing.T) { + t.Parallel() + recs := []config.RecommendationRecord{azureRec(), recFor("acct-uuid", "aws", "ec2", "us-east-1")} + _, store, err := runUpdateAnyDelete(&other, recs, azureOnly) + requireRevokeForbidden(t, err, constraintDenied) + requireNotCanceled(t, store) + }) + t.Run("a row with no recommendations is refused by a bounded grant", func(t *testing.T) { + t.Parallel() + _, store, err := runUpdateAnyDelete(&other, []config.RecommendationRecord{}, azureOnly) + requireRevokeForbidden(t, err, constraintDenied) + requireNotCanceled(t, store) + }) + t.Run("a row with no recommendations is allowed by an unbounded grant", func(t *testing.T) { + t.Parallel() + resp, _, err := runUpdateAnyDelete(&other, []config.RecommendationRecord{}, revokePerm(auth.ActionUpdateAny, nil)) + requireCanceled(t, resp, err) + }) +} + +// A bounded update-any denial falls back to cancel-any and then to the +// creator branch, as #678 does for cancel-any; a non-creator keeps the +// constraint message. +func TestDeletePlannedPurchase_UpdateAnyFallback(t *testing.T) { + t.Parallel() + creator, other := revokeConstraintsUser, plannedOtherUser + del := auth.Permission{Action: auth.ActionDelete, Resource: auth.ResourcePurchases} + updateOut := awsOnly(auth.ActionUpdateAny) + allowed := map[string]struct { + creator *string + perms []auth.Permission + }{ + "to unbounded cancel-any": {&other, []auth.Permission{updateOut, revokePerm(auth.ActionCancelAny, nil)}}, + "to in-scope cancel-any": {&other, []auth.Permission{updateOut, revokePerm(auth.ActionCancelAny, &auth.PermissionConstraints{Providers: []string{"azure"}})}}, + "to the creator with delete": {&creator, []auth.Permission{updateOut, del}}, + "to the creator with in-scope own": {&creator, []auth.Permission{updateOut, revokePerm(auth.ActionCancelOwn, nil)}}, + } + for name, tc := range allowed { + t.Run("falls back "+name, func(t *testing.T) { + t.Parallel() + resp, _, err := runUpdateAnyDelete(tc.creator, nil, tc.perms...) + requireCanceled(t, resp, err) + }) + } + denied := map[string]struct { + creator *string + perms []auth.Permission + }{ + "non-creator with delete": {&other, []auth.Permission{updateOut, del}}, + "non-creator with bounded cancel-any": {&other, []auth.Permission{updateOut, awsOnly(auth.ActionCancelAny), del}}, + "legacy row with no creator": {nil, []auth.Permission{updateOut, del}}, + "creator with out-of-scope cancel-own": {&creator, []auth.Permission{updateOut, awsOnly(auth.ActionCancelOwn)}}, + } + for name, tc := range denied { + t.Run("denies "+name, func(t *testing.T) { + t.Parallel() + _, store, err := runUpdateAnyDelete(tc.creator, nil, tc.perms...) + requireRevokeForbidden(t, err, constraintDenied) + requireNotCanceled(t, store) + }) + } +} + +func TestDeletePlannedPurchase_UpdateAnyAdministratorsGroup(t *testing.T) { + t.Parallel() + other := plannedOtherUser + t.Run("unbounded admin still cancels", func(t *testing.T) { + t.Parallel() + resp, _, err := runUpdateAnyDelete(&other, nil, auth.Permission{Action: auth.ActionAdmin, Resource: auth.ResourceAll}) + requireCanceled(t, resp, err) + }) + t.Run("admin bounded to another provider is refused", func(t *testing.T) { + t.Parallel() + admin := auth.Permission{Action: auth.ActionAdmin, Resource: auth.ResourceAll, Constraints: &auth.PermissionConstraints{Providers: []string{"aws"}}} + _, store, err := runUpdateAnyDelete(&other, nil, admin) + requireRevokeForbidden(t, err, constraintDenied) + requireNotCanceled(t, store) + }) +} + +// An error evaluating update-any's Constraints is not a denial: it must not +// fall back to cancel-any or to the creator branch, both of which would allow. +func TestDeletePlannedPurchase_UpdateAnyConstraintErrorDoesNotFallBack(t *testing.T) { + t.Parallel() + boom := errors.New("constraint store down") + owner := revokeConstraintsUser + exec := cancelRouteExec("pending") + exec.CreatedByUserID = &owner + store := new(MockConfigStore) + store.On("GetExecutionByID", mock.Anything, cancelExecID).Return(exec, nil) + // Reached only if the error were swallowed; requireNotCanceled reports it. + store.On("TransitionExecutionStatus", mock.Anything, cancelExecID, mock.Anything, mock.Anything, mock.Anything). + Return(nil, errKeyScopeReached).Maybe() + 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, mock.Anything, auth.ResourcePurchases).Return(true, nil) + m.On("HasPermissionForConstraintsAPI", mock.Anything, owner, auth.ActionUpdateAny, auth.ResourcePurchases, mock.Anything).Return(false, boom) + m.On("HasPermissionForConstraintsAPI", mock.Anything, owner, mock.Anything, auth.ResourcePurchases, mock.Anything).Return(true, nil).Maybe() + + h := &Handler{config: store, auth: m} + _, err := h.deletePlannedPurchase(context.Background(), deletePlannedReq(), cancelExecID) + require.ErrorIs(t, err, boom) + requireNotCanceled(t, store) +} + +// A user API key is bounded by the KEY's update-any Constraints (key +// intersected with its owner), evaluated by the real auth.Service. The +// keyScopeExec row is aws/ec2/us-east-1, created by another user. +func TestDeletePlannedPurchase_APIKeyUpdateAnyFollowsKeyConstraints(t *testing.T) { + rt := keyScopeRoutes[0] + keyPerms := func(provider string) []auth.Permission { + return []auth.Permission{ + {Action: auth.ActionDelete, Resource: auth.ResourcePurchases}, + revokePerm(auth.ActionUpdateAny, &auth.PermissionConstraints{Providers: []string{provider}}), + } + } + t.Run("key bounded to another provider is refused", func(t *testing.T) { + code, reached := serveKeyScope(t, rt, newKeyScopeAuth(keyPerms("azure")), keyScopeVictim, keyHeaders()) + assert.Equal(t, 403, code) + assert.False(t, reached, "a refused request must not reach the mutation") + }) + t.Run("key bounded to the row's provider is allowed", func(t *testing.T) { + _, reached := serveKeyScope(t, rt, newKeyScopeAuth(keyPerms("aws")), keyScopeVictim, keyHeaders()) + assert.True(t, reached) + }) +} diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 6bd68c935..3419a81d4 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1746,6 +1746,8 @@ func TestHandler_deletePlannedPurchase(t *testing.T) { canceled := &config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "canceled"} mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() + // authorizePlannedPurchaseCancel reads the row to bound update-any by its Constraints (#961). + mockStore.On("GetExecutionByID", ctx, "11111111-1111-1111-1111-111111111111").Return(&config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "pending"}, nil).Once() mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(canceled, nil) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1791,6 +1793,8 @@ func TestHandler_deletePlannedPurchase_DisablesPlan(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() + // authorizePlannedPurchaseCancel reads the row to bound update-any by its Constraints (#961). + mockStore.On("GetExecutionByID", ctx, execID).Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending"}, nil).Once() mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(canceled, nil) mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil) // Assert that UpdatePurchasePlan is called with enabled=false. @@ -1843,6 +1847,8 @@ func TestHandler_deletePlannedPurchase_AlreadyDisabledPlan(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() + // authorizePlannedPurchaseCancel reads the row to bound update-any by its Constraints (#961). + mockStore.On("GetExecutionByID", ctx, execID).Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending"}, nil).Once() mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(canceled, nil) mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil) @@ -2098,9 +2104,12 @@ func TestHandler_deletePlannedPurchase_BackendErrorReturns5xx(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() + // authorizePlannedPurchaseCancel reads the row to bound update-any by its Constraints (#961). + mockStore.On("GetExecutionByID", ctx, execID).Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending"}, nil).Once() mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(nil, dbErr) // On a real backend error we must NOT fall into the recovery path: - // GetExecutionByID must not be called. AssertExpectations verifies this. + // GetExecutionByID must not be called a second time. The .Once() above + // makes a recovery-path read fail the test. handler := &Handler{config: mockStore, auth: mockAuth} @@ -2289,6 +2298,8 @@ func TestHandler_deletePlannedPurchase_NilExecution(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() + // authorizePlannedPurchaseCancel reads the row to bound update-any by its Constraints (#961). + mockStore.On("GetExecutionByID", ctx, "99999999-9999-9999-9999-999999999999").Return(&config.PurchaseExecution{ExecutionID: "99999999-9999-9999-9999-999999999999", Status: "pending"}, nil).Once() mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) handler := &Handler{config: mockStore, auth: mockAuth}