diff --git a/internal/api/handler.go b/internal/api/handler.go index bfaaefcda..e564ce75e 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -10,6 +10,7 @@ import ( "time" "github.com/LeanerCloud/CUDly/internal/accounts" + "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/config" "github.com/LeanerCloud/CUDly/internal/credentials" "github.com/LeanerCloud/CUDly/internal/email" @@ -242,6 +243,49 @@ func (h *Handler) requirePermission(ctx context.Context, req *events.LambdaFunct return session, nil } +// unattributedAccountConstraint is the request-side AccountIDs value passed +// to requirePermissionConstraints when a request cannot be attributed to a +// registered cloud account (a recommendation without a cloud_account_id, or +// an RI exchange on a deployment whose running account is not registered). +// The auth matcher treats an EMPTY request-side list as "dimension not +// specified = satisfied", so omitting AccountIDs would let a permission +// constrained to specific accounts authorize an unattributed request +// (fail-open). Sending this sentinel keeps the list non-empty: it can never +// equal a real cloud account UUID, so a permission constrained to real +// accounts denies the request (fail closed), while a permission without an +// AccountIDs constraint still matches via the empty-permission-side rule. +const unattributedAccountConstraint = "unattributed" + +// requirePermissionConstraints re-checks an already-authenticated session +// against request-derived permission constraint sets, so the Constraints +// (MaxPurchaseAmount, Providers, Services, Regions, AccountIDs) configured on +// the granting group permission are enforced at execution time (SEC-01, +// issue #1141). Callers must have passed requirePermission for the same +// action/resource first; this adds the constraint dimension once the request +// body is parsed and validated. The stateless admin API key is a full-access +// infrastructure credential with no user row, so it bypasses the check just +// like it bypasses requirePermission's per-user lookup. Fails closed on a +// missing auth service or a lookup error. +func (h *Handler) requirePermissionConstraints(ctx context.Context, session *Session, action, resource string, constraintSets []auth.PermissionConstraints) error { + if session == nil { + return fmt.Errorf("internal error: nil session passed to requirePermissionConstraints") + } + if session.UserID == apiKeyAdminUserID { + return nil + } + if h.auth == nil { + return fmt.Errorf("authentication service not configured") + } + has, err := h.auth.HasPermissionForConstraintsAPI(ctx, session.UserID, action, resource, constraintSets) + if err != nil { + return fmt.Errorf("permission constraint check failed: %w", err) + } + if !has { + return NewClientError(403, fmt.Sprintf("permission denied: this request exceeds the constraints configured on your %s permission for %s", action, resource)) + } + return nil +} + // getAllowedAccounts returns the list of account IDs the user is allowed to // access. Empty slice means all access (Administrators-group members carry the // "*" wildcard, which GetAllowedAccountsAPI surfaces as unrestricted). The diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index e70c1947b..05692fa18 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -74,6 +74,10 @@ func scopedAuthMock(ctx context.Context) *MockAuthService { // Grant every permission so role-gating doesn't interfere with what we // actually want to test (account-level scoping). m.On("HasPermissionAPI", ctx, permsScopedUserID, mock.Anything, mock.Anything).Return(true, nil) + // Likewise grant the SEC-01 execution-time constraint check; constraint + // behaviour has its own dedicated tests. + m.On("HasPermissionForConstraintsAPI", ctx, permsScopedUserID, mock.Anything, mock.Anything, mock.Anything). + Return(true, nil).Maybe() m.On("GetAllowedAccountsAPI", ctx, permsScopedUserID).Return([]string{permsAccA}, nil) return m } diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 3a7cee1f1..5de9d0cd8 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1548,9 +1548,56 @@ func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *event if err := validateCapacityConsistency(execReq.Recommendations, execReq.CapacityPercent); err != nil { return ExecutePurchaseRequest{}, nil, err } + // Enforce the per-permission Constraints (MaxPurchaseAmount, Providers, + // Services, Regions, AccountIDs) configured on the granting + // execute:purchases permission (SEC-01, issue #1141). Runs after the + // per-rec validation above so provider tokens are already normalized. + // Each recommendation must individually be granted by a permission; the + // amount cap is checked against the batch's total upfront cost so it + // cannot be evaded by splitting a large purchase across recs. + if err := h.requirePermissionConstraints(ctx, session, "execute", "purchases", purchaseConstraintSets(execReq.Recommendations)); err != nil { + return ExecutePurchaseRequest{}, nil, err + } return execReq, session, nil } +// purchaseConstraintSets builds one auth.PermissionConstraints per +// recommendation in a web execute request, for the SEC-01 constraint +// enforcement in validateExecutePurchaseRequest. Single-value Provider/ +// Service/Region/AccountIDs lists make the auth service's any-overlap +// matcher equivalent to strict containment, so a batch cannot pass on the +// strength of one in-scope rec while another rec is out of scope. +// MaxPurchaseAmount carries the batch's total upfront cost (the same basis +// as the global $10M sanity cap in validateAndTotalRecommendations) on +// every set. AccountIDs is ALWAYS populated: a rec without a CloudAccountID +// carries unattributedAccountConstraint so an AccountIDs-constrained +// permission denies it (the auth matcher treats an empty request-side list +// as satisfied, so omitting the dimension would fail open). Session-level +// allowed_accounts scoping is independently enforced by +// validatePurchaseRecommendationScope. +func purchaseConstraintSets(recs []config.RecommendationRecord) []auth.PermissionConstraints { + var totalUpfront float64 + for i := range recs { + totalUpfront += recs[i].UpfrontCost + } + sets := make([]auth.PermissionConstraints, 0, len(recs)) + for i := range recs { + rec := &recs[i] + accountID := unattributedAccountConstraint + if rec.CloudAccountID != nil && *rec.CloudAccountID != "" { + accountID = *rec.CloudAccountID + } + sets = append(sets, auth.PermissionConstraints{ + Providers: []string{rec.Provider}, + Services: []string{rec.Service}, + Regions: []string{rec.Region}, + AccountIDs: []string{accountID}, + MaxPurchaseAmount: totalUpfront, + }) + } + return sets +} + // normalizeCapacityPercent defaults an absent/zero capacity_percent to 100 // and rejects anything outside [1, 100]. capacity_percent is audit-only but // still bounded: a value outside the range is a client bug worth surfacing diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 946a456ff..fc837166f 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/config" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" @@ -3327,6 +3328,7 @@ func TestHandler_executePurchase_DirectExec_NoPermission(t *testing.T) { // Base execute:purchases grant — passes the validateExecutePurchaseRequest // gate but does not carry execute-any or execute-own for the direct path. mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "execute", "purchases").Return(true, nil) + mockAuth.allowConstraintChecks() mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "execute-any", "purchases").Return(false, nil) mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "execute-own", "purchases").Return(false, nil) // Scope check: no allowed_accounts restriction for this test. @@ -3346,6 +3348,99 @@ func TestHandler_executePurchase_DirectExec_NoPermission(t *testing.T) { assert.Contains(t, ce.Error(), "execute-any or execute-own") } +// TestHandler_executePurchase_PermissionConstraintsDenied is the SEC-01 +// (#1141) regression test: a session whose execute:purchases permission is +// granted (the bare verb/resource gate passes, exactly as it does for a +// constrained permission) but whose per-permission Constraints reject the +// request must receive a 403 BEFORE any execution is persisted. Pre-fix the +// handler never consulted the constraints, so this request was saved and +// proceeded to the approval flow. +func TestHandler_executePurchase_PermissionConstraintsDenied(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + // No store expectations registered: the request must be rejected before + // SavePurchaseExecution / GetPendingExecutions are reached. + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + userSession := &Session{ + UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd", + Email: "capped@example.com", + } + mockAuth.On("ValidateSession", ctx, "capped-token").Return(userSession, nil) + // The bare verb/resource gate passes - this is exactly what happens for + // a permission that carries Constraints, because HasPermissionAPI checks + // with nil request-side constraints. + mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "execute", "purchases").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, userSession.UserID).Return([]string{}, nil) + // The constraint check must receive one set per recommendation, each + // carrying the batch's TOTAL upfront cost ($3000 + $2500 = $5500) and + // that rec's provider/service/region as single-value lists. These recs + // carry no cloud_account_id, so AccountIDs must be the unattributed + // sentinel (never empty, which the auth matcher treats as satisfied). + mockAuth.On("HasPermissionForConstraintsAPI", ctx, userSession.UserID, "execute", "purchases", + mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { + if len(sets) != 2 { + return false + } + for _, c := range sets { + if c.MaxPurchaseAmount != 5500.0 { + return false + } + if !assert.ObjectsAreEqual([]string{unattributedAccountConstraint}, c.AccountIDs) { + return false + } + } + return assert.ObjectsAreEqual([]string{"aws"}, sets[0].Providers) && + assert.ObjectsAreEqual([]string{"ec2"}, sets[0].Services) && + assert.ObjectsAreEqual([]string{"us-east-1"}, sets[0].Regions) && + assert.ObjectsAreEqual([]string{"aws"}, sets[1].Providers) && + assert.ObjectsAreEqual([]string{"ec2"}, sets[1].Services) && + assert.ObjectsAreEqual([]string{"eu-west-1"}, sets[1].Regions) + })).Return(false, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer capped-token"}, + Body: `{"recommendations": [ + {"id": "rec-1", "provider": "aws", "service": "ec2", "region": "us-east-1", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 3000.0, "savings": 50.0}, + {"id": "rec-2", "provider": "aws", "service": "ec2", "region": "eu-west-1", "count": 2, "term": 1, "payment": "all-upfront", "upfront_cost": 2500.0, "savings": 100.0} + ]}`, + } + _, err := handler.executePurchase(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got: %v", err) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "constraints") +} + +// TestPurchaseConstraintSets_AccountDimensionAlwaysPopulated pins the SEC-01 +// fail-closed shape of the AccountIDs dimension: every constraint set must +// carry a non-empty AccountIDs list. The auth matcher treats an empty +// request-side list as "dimension not specified = satisfied", so a rec +// without a cloud_account_id must carry the unattributed sentinel; omitting +// the dimension would let an AccountIDs-constrained execute:purchases +// permission authorize an unattributed purchase (fail-open). +func TestPurchaseConstraintSets_AccountDimensionAlwaysPopulated(t *testing.T) { + attributed := "11111111-2222-3333-4444-555555555555" + empty := "" + sets := purchaseConstraintSets([]config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 100, CloudAccountID: &attributed}, + {Provider: "aws", Service: "rds", Region: "eu-west-1", UpfrontCost: 200, CloudAccountID: nil}, + {Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 300, CloudAccountID: &empty}, + }) + require.Len(t, sets, 3) + assert.Equal(t, []string{attributed}, sets[0].AccountIDs) + assert.Equal(t, []string{unattributedAccountConstraint}, sets[1].AccountIDs) + assert.Equal(t, []string{unattributedAccountConstraint}, sets[2].AccountIDs) + for i, c := range sets { + assert.NotEmpty(t, c.AccountIDs, "set %d must always carry the AccountIDs dimension", i) + assert.Equal(t, 600.0, c.MaxPurchaseAmount, "set %d must carry the batch total upfront", i) + } +} + // TestHandler_executePurchase_DirectExec_ExecuteAny verifies that a session // with execute-any:purchases can direct-execute a purchase (no ownership // check). The handler must call ApproveAndExecute and return status=completed. @@ -3368,6 +3463,7 @@ func TestHandler_executePurchase_DirectExec_ExecuteAny(t *testing.T) { // removed in issue #940 — HasPermissionAPI is now always consulted. // First, the outer gate: requirePermission("execute","purchases"). mockAuth.On("HasPermissionAPI", ctx, adminSession.UserID, "execute", "purchases").Return(true, nil) + mockAuth.allowConstraintChecks() // Then the direct-execute gate: authorizeSessionExecuteDirect("execute-any"). mockAuth.On("HasPermissionAPI", ctx, adminSession.UserID, "execute-any", "purchases").Return(true, nil) // Scope check: no allowed_accounts restriction for this test. @@ -3410,6 +3506,7 @@ func TestHandler_executePurchase_DirectExec_ExecuteOwn_Owner(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "owner-token").Return(ownerSession, nil) mockAuth.On("HasPermissionAPI", ctx, ownerID, "execute", "purchases").Return(true, nil) + mockAuth.allowConstraintChecks() mockAuth.On("HasPermissionAPI", ctx, ownerID, "execute-any", "purchases").Return(false, nil) mockAuth.On("HasPermissionAPI", ctx, ownerID, "execute-own", "purchases").Return(true, nil) mockAuth.On("GetAllowedAccountsAPI", ctx, ownerID).Return([]string{}, nil) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index ec3e117bd..1f83fe542 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -20,6 +20,7 @@ import ( "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/config" "github.com/LeanerCloud/CUDly/internal/credentials" + "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/exchange" "github.com/LeanerCloud/CUDly/pkg/logging" awsprovider "github.com/LeanerCloud/CUDly/providers/aws" @@ -716,7 +717,8 @@ func validateExecuteExchangeBody(body ExchangeExecuteRequestBody) error { // Requires execute:ri-exchange (deliberately separate from execute:purchases) // because RI exchanges are financially irreversible once submitted to AWS. func (h *Handler) executeExchange(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { - if _, err := h.requirePermission(ctx, req, "execute", "ri-exchange"); err != nil { + session, err := h.requirePermission(ctx, req, "execute", "ri-exchange") + if err != nil { return nil, err } @@ -735,6 +737,34 @@ func (h *Handler) executeExchange(ctx context.Context, req *events.LambdaFunctio region := body.Region + // Enforce the per-permission Constraints configured on the granting + // execute:ri-exchange permission (SEC-01, issue #1141). RI exchanges + // are AWS EC2 only and region-scoped, and operate on the RIs of the + // deployment's own AWS account, so AccountIDs carries the registered + // cloud account the running deployment resolves to (fail closed on a + // resolution error; unattributedAccountConstraint when the deployment + // maps to no registered account, so an AccountIDs-constrained + // permission denies). The amount cap is checked against the caller's + // max_payment_due_usd guardrail, which ExecuteExchange independently + // enforces against the actual quoted payment due. + maxPayment, _ := maxRat.Float64() + cloudAccountID, err := h.resolveReshapeCloudAccountID(ctx) + if err != nil { + return nil, fmt.Errorf("failed to resolve cloud account scope: %w", err) + } + if cloudAccountID == "" { + cloudAccountID = unattributedAccountConstraint + } + if err := h.requirePermissionConstraints(ctx, session, "execute", "ri-exchange", []auth.PermissionConstraints{{ + AccountIDs: []string{cloudAccountID}, + Providers: []string{string(common.ProviderAWS)}, + Services: []string{string(common.ServiceEC2)}, + Regions: []string{region}, + MaxPurchaseAmount: maxPayment, + }}); err != nil { + return nil, err + } + exchangeID, quote, err := exchange.ExecuteExchange(ctx, exchange.ExchangeExecuteRequest{ Region: region, ReservedIDs: body.RIIDs, diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 250180c49..de7479c65 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -1018,6 +1018,9 @@ func (m *mockAuthForExchange) ListGroupsAPI(_ context.Context) (any, error) func (m *mockAuthForExchange) HasPermissionAPI(_ context.Context, _, _, _ string) (bool, error) { return true, nil } +func (m *mockAuthForExchange) HasPermissionForConstraintsAPI(_ context.Context, _, _, _ string, _ []auth.PermissionConstraints) (bool, error) { + return true, nil +} func (m *mockAuthForExchange) GetUserPermissionsAPI(_ context.Context, _ string) (any, error) { return nil, nil } @@ -1288,6 +1291,126 @@ func TestExecuteExchange_EmptyRegionReturns400(t *testing.T) { assert.Contains(t, err.Error(), "region is required") } +// TestExecuteExchange_PermissionConstraintsDenied is the SEC-01 (#1141) +// regression test for the exchange path: a session that passes the bare +// execute:ri-exchange gate (as a constrained permission does) but whose +// per-permission Constraints reject the request must receive a 403 before +// the exchange is submitted to AWS. The constraint set must carry the AWS +// EC2 scope, the request's region, the deployment's registered cloud +// account, and the max_payment_due_usd guardrail as the amount. +func TestExecuteExchange_PermissionConstraintsDenied(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + const deploymentAccountID = "11111111-2222-3333-4444-555555555555" + userSession := &Session{ + UserID: "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee", + Email: "exchanger@example.com", + } + mockAuth.On("ValidateSession", ctx, "exchange-token").Return(userSession, nil) + mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "execute", "ri-exchange").Return(true, nil) + mockAuth.On("HasPermissionForConstraintsAPI", ctx, userSession.UserID, "execute", "ri-exchange", + mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { + if len(sets) != 1 { + return false + } + c := sets[0] + return assert.ObjectsAreEqual([]string{deploymentAccountID}, c.AccountIDs) && + assert.ObjectsAreEqual([]string{"aws"}, c.Providers) && + assert.ObjectsAreEqual([]string{"ec2"}, c.Services) && + assert.ObjectsAreEqual([]string{"eu-central-1"}, c.Regions) && + c.MaxPurchaseAmount == 250.50 + })).Return(false, nil) + + h := &Handler{ + auth: mockAuth, + reshapeAccountResolver: func(_ context.Context) (string, error) { + return deploymentAccountID, nil + }, + } + _, err := h.executeExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer exchange-token"}, + Body: `{"ri_ids":["ri-123"],"target_offering_id":"off-1","max_payment_due_usd":"250.50","region":"eu-central-1"}`, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a ClientError, got: %v", err) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "constraints") +} + +// TestExecuteExchange_AccountResolutionErrorFailsClosed pins the fail-closed +// behavior of the SEC-01 AccountIDs dimension: when the deployment's cloud +// account cannot be resolved (STS error, account lookup failure), the +// handler must abort BEFORE the constraint check and the AWS call rather +// than evaluating constraints without the account dimension. +func TestExecuteExchange_AccountResolutionErrorFailsClosed(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + // No HasPermissionForConstraintsAPI expectation: it must NOT be called. + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + userSession := &Session{ + UserID: "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee", + Email: "exchanger@example.com", + } + mockAuth.On("ValidateSession", ctx, "exchange-token").Return(userSession, nil) + mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "execute", "ri-exchange").Return(true, nil) + + h := &Handler{ + auth: mockAuth, + reshapeAccountResolver: func(_ context.Context) (string, error) { + return "", fmt.Errorf("sts get-caller-identity denied") + }, + } + _, err := h.executeExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer exchange-token"}, + Body: `{"ri_ids":["ri-123"],"target_offering_id":"off-1","max_payment_due_usd":"250.50","region":"eu-central-1"}`, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "resolve cloud account scope") +} + +// TestExecuteExchange_UnattributedAccountStillConstrained pins the sentinel +// behavior: when the deployment resolves to no registered cloud account +// (non-AWS host, bootstrap), the constraint set must still carry a non-empty +// AccountIDs list (the unattributed sentinel) so a permission constrained to +// specific accounts denies the exchange instead of matching the auth +// layer's "empty request list = satisfied" rule (fail closed). +func TestExecuteExchange_UnattributedAccountStillConstrained(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + userSession := &Session{ + UserID: "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee", + Email: "exchanger@example.com", + } + mockAuth.On("ValidateSession", ctx, "exchange-token").Return(userSession, nil) + mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "execute", "ri-exchange").Return(true, nil) + mockAuth.On("HasPermissionForConstraintsAPI", ctx, userSession.UserID, "execute", "ri-exchange", + mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { + return len(sets) == 1 && + assert.ObjectsAreEqual([]string{unattributedAccountConstraint}, sets[0].AccountIDs) + })).Return(false, nil) + + h := &Handler{ + auth: mockAuth, + reshapeAccountResolver: func(_ context.Context) (string, error) { + return "", nil + }, + } + _, err := h.executeExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer exchange-token"}, + Body: `{"ri_ids":["ri-123"],"target_offering_id":"off-1","max_payment_due_usd":"250.50","region":"eu-central-1"}`, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a ClientError, got: %v", err) + assert.Equal(t, 403, ce.code) +} + // TestGetExchangeQuote_EmptyRegionResolvesFromSDK pins finding 01-L4: // getExchangeQuote must resolve the region from the AWS SDK chain when // the caller omits it, matching getReshapeRecommendations, instead of diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index d3bbad7ed..da56b4569 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -3,6 +3,7 @@ package api import ( "context" + "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/config" "github.com/LeanerCloud/CUDly/internal/credentials" "github.com/LeanerCloud/CUDly/internal/mocks" @@ -251,12 +252,27 @@ func (m *MockAuthService) HasPermissionAPI(ctx context.Context, userID, action, return args.Bool(0), args.Error(1) } +func (m *MockAuthService) HasPermissionForConstraintsAPI(ctx context.Context, userID, action, resource string, constraintSets []auth.PermissionConstraints) (bool, error) { + args := m.Called(ctx, userID, action, resource, constraintSets) + return args.Bool(0), args.Error(1) +} + func (m *MockAuthService) GetUserPermissionsAPI(ctx context.Context, userID string) (any, error) { args := m.Called(ctx, userID) return args.Get(0), args.Error(1) } -// grantAdmin makes every HasPermissionAPI check succeed, modeling an +// allowConstraintChecks stubs the SEC-01 execution-time permission +// constraint check (HasPermissionForConstraintsAPI) to succeed for any +// request, modelling a granting permission with no Constraints configured. +// Tests that target constraint behaviour register an explicit expectation +// instead. +func (m *MockAuthService) allowConstraintChecks() { + m.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(true, nil).Maybe() +} + +// grantAdmin makes every HasPermissionAPI check succeed, modelling an // Administrators-group member. Authorization is group-membership-only after // issue #907, so admin-gated handlers resolve "is admin" / specific permissions // through HasPermissionAPI rather than a Session.Role short-circuit; tests that @@ -267,6 +283,10 @@ func (m *MockAuthService) GetUserPermissionsAPI(ctx context.Context, userID stri func (m *MockAuthService) grantAdmin() { m.On("HasPermissionAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(true, nil).Maybe() + // Admins hold {admin, *}, which grants every constraint set, so the + // SEC-01 execution-time constraint check succeeds too. + m.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(true, nil).Maybe() // Administrators-group members carry the "*" wildcard, surfaced as // unrestricted access (nil/empty). Handlers that scope by account call // GetAllowedAccountsAPI after the permission check, so stub it too. diff --git a/internal/api/types.go b/internal/api/types.go index 9a53fa546..74dd07d10 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -6,6 +6,7 @@ import ( "time" "github.com/LeanerCloud/CUDly/internal/analytics" + "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/commitmentopts" "github.com/LeanerCloud/CUDly/internal/config" "github.com/LeanerCloud/CUDly/internal/credentials" @@ -210,6 +211,12 @@ type AuthServiceInterface interface { ListGroupsAPI(ctx context.Context) (any, error) // Permission checking HasPermissionAPI(ctx context.Context, userID, action, resource string) (bool, error) + // HasPermissionForConstraintsAPI checks action on resource against + // request-derived constraint sets so per-permission Constraints + // (MaxPurchaseAmount, Providers, Services, Regions, AccountIDs) are + // enforced at execution time. Every constraint set must be granted by + // at least one of the user's permissions (SEC-01, issue #1141). + HasPermissionForConstraintsAPI(ctx context.Context, userID, action, resource string, constraintSets []auth.PermissionConstraints) (bool, error) // GetUserPermissionsAPI returns the effective permission set for a user // (union of all group permissions). Used by GET /api/auth/me/permissions. // Returns []auth.APIPermission converted to []PermissionEntry by the handler. diff --git a/internal/auth/service_api.go b/internal/auth/service_api.go index df1eca79a..c961e27fd 100644 --- a/internal/auth/service_api.go +++ b/internal/auth/service_api.go @@ -359,6 +359,33 @@ func (s *Service) HasPermissionAPI(ctx context.Context, userID, action, resource return s.HasPermission(ctx, userID, action, resource, nil) } +// HasPermissionForConstraintsAPI checks that the user holds action on +// resource for EVERY request-derived constraint set, so per-permission +// Constraints (MaxPurchaseAmount, Providers, Services, Regions, AccountIDs) +// configured on a group permission are enforced at execution time instead of +// being silently ignored (SEC-01, issue #1141). A single effective-permission +// fetch covers all sets; each set must be granted by at least one permission +// (different sets may be satisfied by different permissions, matching the +// union semantics of group grants). +// +// Fail closed: an empty constraintSets slice is a caller bug, not a grant - +// it returns an explicit error rather than allowing. +func (s *Service) HasPermissionForConstraintsAPI(ctx context.Context, userID, action, resource string, constraintSets []PermissionConstraints) (bool, error) { + if len(constraintSets) == 0 { + return false, fmt.Errorf("no permission constraint sets provided for %s on %s", action, resource) + } + permissions, err := s.GetUserPermissions(ctx, userID) + if err != nil { + return false, err + } + for i := range constraintSets { + if !s.permissionsAllow(permissions, action, resource, &constraintSets[i]) { + return false, nil + } + } + return true, nil +} + // GetUserPermissionsAPI returns the effective permission set for a user via // the API. Calls GetUserPermissions (the same union path the server enforces // with) and converts each Permission to an APIPermission for the wire format. diff --git a/internal/auth/service_api_test.go b/internal/auth/service_api_test.go index 7956accc2..9c9827f04 100644 --- a/internal/auth/service_api_test.go +++ b/internal/auth/service_api_test.go @@ -628,6 +628,131 @@ func TestService_HasPermissionAPI(t *testing.T) { }) } +// TestService_HasPermissionForConstraintsAPI is the SEC-01 (#1141) regression +// suite: per-permission Constraints (MaxPurchaseAmount, Providers, Services, +// Regions) configured on a group permission must be enforced when the request +// supplies constraint sets, instead of being silently ignored the way the +// nil-constraints HasPermissionAPI path ignores them. +func TestService_HasPermissionForConstraintsAPI(t *testing.T) { + ctx := context.Background() + + // stubConstrainedPurchaser models the real failing scenario from the + // finding: an operator scoped a group's execute:purchases permission + // with MaxPurchaseAmount=$1000, providers=[aws], regions=[us-east-1]. + stubConstrainedPurchaser := func(mockStore *MockStore) { + user := &User{ID: "purchaser-1", GroupIDs: []string{"purchasers"}} + mockStore.On("GetUserByID", ctx, "purchaser-1").Return(user, nil).Once() + mockStore.On("GetGroup", ctx, "purchasers").Return(&Group{ + ID: "purchasers", + Name: "Purchasers", + Permissions: []Permission{{ + Action: ActionExecute, + Resource: ResourcePurchases, + Constraints: &PermissionConstraints{ + Providers: []string{"aws"}, + Regions: []string{"us-east-1"}, + MaxPurchaseAmount: 1000, + }, + }}, + }, nil).Once() + } + + t.Run("purchase over MaxPurchaseAmount is denied", func(t *testing.T) { + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + stubConstrainedPurchaser(mockStore) + + has, err := service.HasPermissionForConstraintsAPI(ctx, "purchaser-1", ActionExecute, ResourcePurchases, []PermissionConstraints{ + {Providers: []string{"aws"}, Regions: []string{"us-east-1"}, MaxPurchaseAmount: 5000}, + }) + require.NoError(t, err) + assert.False(t, has, "a $5000 purchase must not pass a $1000 MaxPurchaseAmount cap") + mockStore.AssertExpectations(t) + }) + + t.Run("purchase within all constraints is allowed", func(t *testing.T) { + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + stubConstrainedPurchaser(mockStore) + + has, err := service.HasPermissionForConstraintsAPI(ctx, "purchaser-1", ActionExecute, ResourcePurchases, []PermissionConstraints{ + {Providers: []string{"aws"}, Regions: []string{"us-east-1"}, MaxPurchaseAmount: 500}, + }) + require.NoError(t, err) + assert.True(t, has) + mockStore.AssertExpectations(t) + }) + + t.Run("out-of-scope provider is denied", func(t *testing.T) { + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + stubConstrainedPurchaser(mockStore) + + has, err := service.HasPermissionForConstraintsAPI(ctx, "purchaser-1", ActionExecute, ResourcePurchases, []PermissionConstraints{ + {Providers: []string{"azure"}, Regions: []string{"us-east-1"}, MaxPurchaseAmount: 500}, + }) + require.NoError(t, err) + assert.False(t, has, "an azure purchase must not pass a providers=[aws] constraint") + mockStore.AssertExpectations(t) + }) + + t.Run("batch with one out-of-scope set is denied", func(t *testing.T) { + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + stubConstrainedPurchaser(mockStore) + + has, err := service.HasPermissionForConstraintsAPI(ctx, "purchaser-1", ActionExecute, ResourcePurchases, []PermissionConstraints{ + {Providers: []string{"aws"}, Regions: []string{"us-east-1"}, MaxPurchaseAmount: 500}, + {Providers: []string{"aws"}, Regions: []string{"eu-west-1"}, MaxPurchaseAmount: 500}, + }) + require.NoError(t, err) + assert.False(t, has, "every constraint set must be granted; an out-of-region rec fails the batch") + mockStore.AssertExpectations(t) + }) + + t.Run("admin passes any constraint set", func(t *testing.T) { + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + adminUser := &User{ID: "admin-123", GroupIDs: []string{DefaultAdminGroupID}} + mockStore.On("GetUserByID", ctx, "admin-123").Return(adminUser, nil).Once() + mockStore.On("GetGroup", ctx, DefaultAdminGroupID).Return(&Group{ + ID: DefaultAdminGroupID, + Permissions: []Permission{{Action: ActionAdmin, Resource: ResourceAll}}, + }, nil).Once() + + has, err := service.HasPermissionForConstraintsAPI(ctx, "admin-123", ActionExecute, ResourcePurchases, []PermissionConstraints{ + {Providers: []string{"gcp"}, Regions: []string{"europe-west1"}, MaxPurchaseAmount: 9_999_999}, + }) + require.NoError(t, err) + assert.True(t, has) + mockStore.AssertExpectations(t) + }) + + t.Run("empty constraint sets fail loud", func(t *testing.T) { + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + has, err := service.HasPermissionForConstraintsAPI(ctx, "purchaser-1", ActionExecute, ResourcePurchases, nil) + require.Error(t, err, "an empty constraint-set slice is a caller bug, not a grant") + assert.False(t, has) + mockStore.AssertExpectations(t) + }) + + t.Run("store error propagates and denies", func(t *testing.T) { + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + mockStore.On("GetUserByID", ctx, "err-1").Return(nil, fmt.Errorf("database error")).Once() + + has, err := service.HasPermissionForConstraintsAPI(ctx, "err-1", ActionExecute, ResourcePurchases, []PermissionConstraints{ + {Providers: []string{"aws"}}, + }) + require.Error(t, err) + assert.False(t, has) + mockStore.AssertExpectations(t) + }) +} + // Test error paths and edge cases // TestUserToAPIUser_EmptyGroups verifies the nil→[]string{} substitution diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index 9a983ae53..d38c915de 100644 --- a/internal/auth/service_group.go +++ b/internal/auth/service_group.go @@ -161,9 +161,17 @@ func (s *Service) HasPermission(ctx context.Context, userID, action, resource st return false, err } + return s.permissionsAllow(permissions, action, resource, constraints), nil +} + +// permissionsAllow reports whether any permission in the effective set grants +// action on resource under the given request-side constraints. Extracted from +// HasPermission so batch callers (HasPermissionForConstraintsAPI) can evaluate +// several constraint sets against a single permission fetch. +func (s *Service) permissionsAllow(permissions []Permission, action, resource string, constraints *PermissionConstraints) bool { for _, perm := range permissions { if checkAdminPermission(perm) { - return true, nil + return true } if !checkPermissionMatch(perm, action, resource) { @@ -174,10 +182,10 @@ func (s *Service) HasPermission(ctx context.Context, userID, action, resource st continue } - return true, nil + return true } - return false, nil + return false } func checkAdminPermission(perm Permission) bool { diff --git a/internal/server/app.go b/internal/server/app.go index bcc390c4b..17fa87fbe 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -1138,6 +1138,10 @@ func (a *authServiceAdapter) HasPermissionAPI(ctx context.Context, userID, actio return a.service.HasPermissionAPI(ctx, userID, action, resource) } +func (a *authServiceAdapter) HasPermissionForConstraintsAPI(ctx context.Context, userID, action, resource string, constraintSets []auth.PermissionConstraints) (bool, error) { + return a.service.HasPermissionForConstraintsAPI(ctx, userID, action, resource, constraintSets) +} + func (a *authServiceAdapter) GetUserPermissionsAPI(ctx context.Context, userID string) (any, error) { return a.service.GetUserPermissionsAPI(ctx, userID) }