test(api): model the constraint dimension in MockAuthService

internal/api/grantadmin_carveout_test.go

@@ -191,3 +191,167 @@ func TestGrantPermissionsScoped_ConstrainedCheckFailsClosedOnEmptyConstraintSets
 	require.NoError(t, err)
 	assert.True(t, has)
 }
+
+// constrainedGrantCase is one permission-side constraint plus a request that
+// falls inside it and a request that falls outside it. Holding both directions
+// in the same case is deliberate: a mock that refuses everyone passes an
+// outside-only assertion, and the pre-fix mock that allowed everyone passes an
+// inside-only one. Only the pair distinguishes them.
+type constrainedGrantCase struct {
+	permConstraints *auth.PermissionConstraints
+	inside          auth.PermissionConstraints
+	outside         auth.PermissionConstraints
+	name            string
+	action          string
+	resource        string
+}
+
+// constrainedGrantCases covers all five constraint dimensions. Each case holds
+// exactly one permission, so the dimension under test is the only thing that
+// can decide the answer.
+var constrainedGrantCases = []constrainedGrantCase{
+	{
+		name:            "providers",
+		action:          auth.ActionExecute,
+		resource:        auth.ResourceRIExchange,
+		permConstraints: &auth.PermissionConstraints{Providers: []string{"aws"}},
+		inside:          auth.PermissionConstraints{Providers: []string{"aws"}},
+		outside:         auth.PermissionConstraints{Providers: []string{"azure"}},
+	},
+	{
+		name:            "regions require every requested region",
+		action:          auth.ActionExecute,
+		resource:        auth.ResourceRIExchange,
+		permConstraints: &auth.PermissionConstraints{Regions: []string{"eastus"}},
+		inside:          auth.PermissionConstraints{Regions: []string{"eastus"}},
+		outside:         auth.PermissionConstraints{Regions: []string{"eastus", "westus"}},
+	},
+	{
+		name:            "services",
+		action:          auth.ActionExecute,
+		resource:        auth.ResourcePurchases,
+		permConstraints: &auth.PermissionConstraints{Services: []string{"ec2"}},
+		inside:          auth.PermissionConstraints{Services: []string{"ec2"}},
+		outside:         auth.PermissionConstraints{Services: []string{"rds"}},
+	},
+	{
+		name:            "account IDs",
+		action:          auth.ActionExecute,
+		resource:        auth.ResourcePurchases,
+		permConstraints: &auth.PermissionConstraints{AccountIDs: []string{"acct-a"}},
+		inside:          auth.PermissionConstraints{AccountIDs: []string{"acct-a"}},
+		outside:         auth.PermissionConstraints{AccountIDs: []string{"acct-b"}},
+	},
+	{
+		name:            "max purchase amount is the spend guard",
+		action:          auth.ActionExecute,
+		resource:        auth.ResourcePurchases,
+		permConstraints: &auth.PermissionConstraints{MaxPurchaseAmount: 1000},
+		inside:          auth.PermissionConstraints{MaxPurchaseAmount: 500},
+		outside:         auth.PermissionConstraints{MaxPurchaseAmount: 5000},
+	},
+}
+
+// TestGrantPermissionsScoped_ConstraintDimensionsAreEnforced is the issue
+// #1762 regression barrier: the mock's HasPermissionForConstraintsAPI must
+// answer from the constraint arguments, not discard them.
+//
+// It fails on the pre-fix mock -- which wired that method to a closure taking
+// only (action, resource) -- with every `outside` row returning true. Measured
+// against auth.PermissionsAllowForConstraintSets before the fix, all five
+// dimensions diverged, so a handler test asserting a constraint refusal was
+// green whether or not the handler enforced anything.
+func TestGrantPermissionsScoped_ConstraintDimensionsAreEnforced(t *testing.T) {
+	ctx := context.Background()
+
+	for _, tc := range constrainedGrantCases {
+		t.Run(tc.name, func(t *testing.T) {
+			mockAuth := new(MockAuthService)
+			t.Cleanup(func() { mockAuth.AssertExpectations(t) })
+			mockAuth.grantPermissions([]auth.Permission{{
+				Action: tc.action, Resource: tc.resource, Constraints: tc.permConstraints,
+			}})
+
+			has, err := mockAuth.HasPermissionForConstraintsAPI(ctx, "u1", tc.action, tc.resource,
+				[]auth.PermissionConstraints{tc.inside})
+			require.NoError(t, err)
+			assert.True(t, has, "a request inside the permission's constraints must be allowed")
+
+			has, err = mockAuth.HasPermissionForConstraintsAPI(ctx, "u1", tc.action, tc.resource,
+				[]auth.PermissionConstraints{tc.outside})
+			require.NoError(t, err)
+			assert.False(t, has, "a request outside the permission's constraints must be refused")
+		})
+	}
+}
+
+// TestGrantPermissionsScoped_AdminDoesNotWidenAConstrainedCarveOut covers the
+// principal shape PR #1758 made reachable: execute:ri-exchange is carved out
+// of admin:*, so an administrator reaches it only by ALSO holding an explicit
+// grant. Migration 000096 seeds that grant unconstrained, but an operator may
+// configure a constrained one, and when they do the admin permission alongside
+// it must not widen them back out.
+func TestGrantPermissionsScoped_AdminDoesNotWidenAConstrainedCarveOut(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	t.Cleanup(func() { mockAuth.AssertExpectations(t) })
+	mockAuth.grantPermissions([]auth.Permission{
+		{Action: auth.ActionAdmin, Resource: auth.ResourceAll},
+		{
+			Action: auth.ActionExecute, Resource: auth.ResourceRIExchange,
+			Constraints: &auth.PermissionConstraints{Providers: []string{"aws"}},
+		},
+	})
+
+	has, err := mockAuth.HasPermissionForConstraintsAPI(ctx, "u1", auth.ActionExecute, auth.ResourceRIExchange,
+		[]auth.PermissionConstraints{{Providers: []string{"aws"}}})
+	require.NoError(t, err)
+	assert.True(t, has, "the explicit grant covers its own provider")
+
+	has, err = mockAuth.HasPermissionForConstraintsAPI(ctx, "u1", auth.ActionExecute, auth.ResourceRIExchange,
+		[]auth.PermissionConstraints{{Providers: []string{"azure"}}})
+	require.NoError(t, err)
+	assert.False(t, has,
+		"admin:* must not satisfy a carved-out verb the explicit grant constrains to another provider")
+}
+
+// TestRequirePermissionConstraints_ConstrainedGrantIsBoundedAtHandler drives
+// the same principal through the handler gate the money paths call, so the
+// barrier covers requirePermissionConstraints' consumption of the answer and
+// not only the mock's production of it.
+func TestRequirePermissionConstraints_ConstrainedGrantIsBoundedAtHandler(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	t.Cleanup(func() { mockAuth.AssertExpectations(t) })
+	mockAuth.grantPermissions([]auth.Permission{{
+		Action:      auth.ActionExecute,
+		Resource:    auth.ResourceRIExchange,
+		Constraints: &auth.PermissionConstraints{Providers: []string{"aws"}, MaxPurchaseAmount: 1000},
+	}})
+	h := &Handler{auth: mockAuth}
+	session := &Session{UserID: "u1"}
+
+	require.NoError(t,
+		h.requirePermissionConstraints(ctx, session, auth.ResourceRIExchange, []auth.PermissionConstraints{{
+			Providers: []string{"aws"}, MaxPurchaseAmount: 500,
+		}}),
+		"a request inside every dimension must pass the gate")
+
+	for _, tc := range []struct {
+		name    string
+		request auth.PermissionConstraints
+	}{
+		{"wrong provider", auth.PermissionConstraints{Providers: []string{"azure"}, MaxPurchaseAmount: 500}},
+		{"over the spend cap", auth.PermissionConstraints{Providers: []string{"aws"}, MaxPurchaseAmount: 5000}},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			err := h.requirePermissionConstraints(ctx, session, auth.ResourceRIExchange,
+				[]auth.PermissionConstraints{tc.request})
+			require.Error(t, err)
+			ce, ok := IsClientError(err)
+			require.True(t, ok, "expected a ClientError, got %T: %v", err, err)
+			assert.Equal(t, 403, ce.code)
+			assert.Contains(t, ce.Error(), "exceeds the constraints")
+		})
+	}
+}

internal/api/mocks_test.go

@@ -2,7 +2,6 @@ package api
 
 import (
 	"context"
-	"fmt"
 	"sync"
 
 	"github.com/LeanerCloud/CUDly/internal/auth"
@@ -294,21 +293,41 @@ func permissionDecision(args mock.Arguments, action, resource string) bool {
 
 func (m *MockAuthService) HasPermissionForConstraintsAPI(ctx context.Context, userID, action, resource string, constraintSets []auth.PermissionConstraints) (bool, error) {
 	args := m.Called(ctx, userID, action, resource, constraintSets)
-	// The decision-function path (registered by grantPermissionsScoped) must
-	// mirror auth.Service.HasPermissionForConstraintsAPI's fail-closed
-	// contract: an empty constraintSets is a caller bug, not a grant. Explicit
+	// The decision-function path (registered by grantPermissionsScoped) reads
+	// the constraint arguments, so it answers from auth's real matchers rather
+	// than from a re-statement of their contract. Explicit
 	// mock.On(...).Return(bool, err) expectations for constrained-permission
 	// tests are untouched -- those already encode the intended outcome for
 	// whatever constraintSets the test passes, via permissionDecision below.
-	if decide, ok := args.Get(0).(func(action, resource string) bool); ok {
-		if len(constraintSets) == 0 {
-			return false, fmt.Errorf("no permission constraint sets provided for %s on %s", action, resource)
+	if decide, ok := args.Get(0).(constraintDecisionFunc); ok {
+		if err := args.Error(1); err != nil {
+			return false, err
 		}
-		return decide(action, resource), args.Error(1)
+		return decide(action, resource, constraintSets)
+	}
+	// permissionDecision's func(action, resource) bool answers from the verb
+	// alone. On THIS method that IS the #1762 defect: constraintSets is
+	// discarded, so every request outside the permission's constraints reads
+	// as allowed. Reject it here rather than letting a future registration
+	// reintroduce the divergence silently. Note that this aborts the test
+	// binary, so a wholesale revert of grantPermissionsScoped's wiring dies
+	// here instead of reporting the per-dimension failures in
+	// TestGrantPermissionsScoped_ConstraintDimensionsAreEnforced; the reach
+	// this adds is over a one-off inline registration, which no test asserts.
+	if _, blind := args.Get(0).(func(action, resource string) bool); blind {
+		panic("MockAuthService.HasPermissionForConstraintsAPI: a func(action, resource) bool return " +
+			"discards the constraint arguments (issue #1762); register a constraintDecisionFunc")
 	}
 	return permissionDecision(args, action, resource), args.Error(1)
 }
 
+// constraintDecisionFunc is the constraint-AWARE mock return registered by
+// grantPermissionsScoped, for HasPermissionForConstraintsAPI only. A named
+// type rather than a bare signature so the registration site says which of
+// the two decision shapes it means, and so the blind-signature check above
+// has something unambiguous to reject.
+type constraintDecisionFunc func(action, resource string, constraintSets []auth.PermissionConstraints) (bool, error)
+
 func (m *MockAuthService) GetUserPermissionsAPI(ctx context.Context, userID string) (any, error) {
 	args := m.Called(ctx, userID)
 	return args.Get(0), args.Error(1)
@@ -391,10 +410,13 @@ func (m *MockAuthService) grantPermissions(perms []auth.Permission) {
 	m.grantPermissionsScoped(perms, nil)
 }
 
-// grantPermissionsScoped is the shared implementation. The permission checks
-// are answered by auth.AuthContext.HasPermission rather than by a constant, so
-// a handler asking for a verb this principal does not hold is DENIED exactly
-// as it would be in production.
+// grantPermissionsScoped is the shared implementation. Both halves of the
+// authorization decision are answered by the real auth code rather than by a
+// constant: the verb check by auth.AuthContext.HasPermission, the constraint
+// check by auth.PermissionsAllowForConstraintSets. A handler asking for a verb
+// this principal does not hold, or for a request outside the Constraints
+// configured on the permission that grants it, is DENIED exactly as it would
+// be in production.
 //
 // .Maybe() is retained: many handlers legitimately return before reaching a
 // permission check (bad body, bad UUID). Asserting the check happened is a
@@ -407,11 +429,29 @@ func (m *MockAuthService) grantPermissionsScoped(perms []auth.Permission, accoun
 	}
 	m.On("HasPermissionAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
 		Return(decide, nil).Maybe()
-	// The SEC-01 execution-time constraint check. A principal holding only
-	// admin:* satisfies any constraint set for the verbs it holds, and holds
-	// none of the carved-out verbs, so the same decision function applies.
+	// The SEC-01 execution-time constraint check. This previously reused
+	// `decide`, which takes only (action, resource) and therefore discarded
+	// the constraint arguments outright, so the mock answered "allowed" for
+	// all five constraint dimensions when the request fell OUTSIDE the
+	// permission's constraints (issue #1762).
+	//
+	// The shortcut was justified on the grounds that a principal holding only
+	// admin:* satisfies any constraint set for the verbs it holds. That is
+	// true, and stayed true through #1758: both halves short-circuit on the
+	// admin:* wildcard before reaching any constraint comparison, so for such
+	// a principal the constraint arguments are ignored whatever Constraints
+	// the admin permission itself carries, and #1758's carve-out of
+	// execute:ri-exchange only changed which verbs both DENY in lockstep.
+	// What the justification never covered is the helper it was attached to.
+	// grantPermissionsScoped takes an ARBITRARY permission set, and a
+	// permission carrying Constraints is exactly what the check exists to
+	// bound.
+	decideConstrained := constraintDecisionFunc(
+		func(action, resource string, constraintSets []auth.PermissionConstraints) (bool, error) {
+			return auth.PermissionsAllowForConstraintSets(perms, action, resource, constraintSets)
+		})
 	m.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).
-		Return(decide, nil).Maybe()
+		Return(decideConstrained, nil).Maybe()
 	m.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).
 		Return(accounts, nil).Maybe()
 }

internal/auth/service_api.go

@@ -413,17 +413,49 @@ func (s *Service) HasPermissionAPI(ctx context.Context, userID, action, resource
 // 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.
+// it returns an explicit error rather than allowing. Checked here as well as
+// in PermissionsAllowForConstraintSets so a caller bug never reaches the
+// store, which the "empty constraint sets fail loud" case asserts by leaving
+// the store mock with no registered expectation.
 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)
+		return false, errNoConstraintSets(action, resource)
 	}
 	permissions, err := s.GetUserPermissions(ctx, userID)
 	if err != nil {
 		return false, err
 	}
+	return PermissionsAllowForConstraintSets(permissions, action, resource, constraintSets)
+}
+
+// errNoConstraintSets is the shared fail-closed error for an empty
+// constraint-set slice, so the three entry points that guard against it
+// (this file's two, plus HasAPIKeyPermissionForConstraintsAPI) cannot drift
+// apart in wording.
+func errNoConstraintSets(action, resource string) error {
+	return fmt.Errorf("no permission constraint sets provided for %s on %s", action, resource)
+}
+
+// PermissionsAllowForConstraintSets is the decision half of
+// HasPermissionForConstraintsAPI with the permission fetch removed: it
+// reports whether an already-resolved permission set grants action on
+// resource for EVERY request-derived constraint set.
+//
+// Exported so a caller that already holds the effective permission set
+// answers from this code rather than from a re-implementation of it. The
+// api package's MockAuthService is the caller that motivated it: it modeled
+// the permission set faithfully but discarded the constraint arguments
+// entirely, so it answered "allowed" for requests on every one of the five
+// constraint dimensions that this function denies, and a handler test
+// asserting a constraint refusal was measuring the mock (issue #1762).
+//
+// Fail closed: an empty constraintSets slice is a caller bug, not a grant.
+func PermissionsAllowForConstraintSets(permissions []Permission, action, resource string, constraintSets []PermissionConstraints) (bool, error) {
+	if len(constraintSets) == 0 {
+		return false, errNoConstraintSets(action, resource)
+	}
 	for i := range constraintSets {
-		if !s.permissionsAllow(permissions, action, resource, &constraintSets[i]) {
+		if !permissionsAllow(permissions, action, resource, &constraintSets[i]) {
 			return false, nil
 		}
 	}

internal/auth/service_apikeys_api.go

@@ -377,7 +377,7 @@ func (s *Service) HasAPIKeyPermissionAPI(ctx context.Context, apiKey, action, re
 // failure returns an error and callers must deny.
 func (s *Service) HasAPIKeyPermissionForConstraintsAPI(ctx context.Context, keyID, 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)
+		return false, errNoConstraintSets(action, resource)
 	}
 	key, err := s.store.GetAPIKeyByID(ctx, keyID)
 	if err != nil {
@@ -405,10 +405,10 @@ func (s *Service) HasAPIKeyPermissionForConstraintsAPI(ctx context.Context, keyI
 	//   - The key's effective permissions (key's constraint limits, e.g. MaxPurchaseAmount).
 	//   - The owner's group permissions (owner's constraint limits).
 	for i := range constraintSets {
-		if !s.permissionsAllow(perms, action, resource, &constraintSets[i]) {
+		if !permissionsAllow(perms, action, resource, &constraintSets[i]) {
 			return false, nil
 		}
-		if !s.permissionsAllow(ownerAuthCtx.Permissions, action, resource, &constraintSets[i]) {
+		if !permissionsAllow(ownerAuthCtx.Permissions, action, resource, &constraintSets[i]) {
 			return false, nil
 		}
 	}