fix(auth): enforce constraints on an admin-scoped permission on every check

internal/auth/admin_constraints_test.go

@@ -0,0 +1,126 @@
+package auth
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Issue #956: an {admin, *} permission that carries Constraints must be
+// narrowed by them on every check, not only under StrictScope. Before the
+// fix, permissionsAllow applied an admin permission's Constraints only when
+// the request set StrictScope, so a key created as {admin, *} with
+// accounts=[A] or a max_amount passed every non-strict check for account B
+// or for any amount.
+
+func constrainedAdmin(c *PermissionConstraints) Permission {
+	return Permission{Action: ActionAdmin, Resource: ResourceAll, Constraints: c}
+}
+
+func TestPermissionsAllow_ConstrainedAdminNonStrict(t *testing.T) {
+	t.Parallel()
+	held := constrainedAdmin(&PermissionConstraints{
+		AccountIDs: []string{"acct-a"}, Providers: []string{"aws"}, Services: []string{"ec2"},
+		Regions: []string{"us-east-1"}, MaxPurchaseAmount: 500,
+	})
+	cases := []struct {
+		name string
+		req  PermissionConstraints
+		want bool
+	}{
+		{"other account denied", PermissionConstraints{AccountIDs: []string{"acct-b"}}, false},
+		{"own account allowed", PermissionConstraints{AccountIDs: []string{"acct-a"}}, true},
+		{"amount over cap denied", PermissionConstraints{MaxPurchaseAmount: 501}, false},
+		{"amount at cap allowed", PermissionConstraints{MaxPurchaseAmount: 500}, true},
+		{"other provider denied", PermissionConstraints{Providers: []string{"azure"}}, false},
+		{"other service denied", PermissionConstraints{Services: []string{"rds"}}, false},
+		{"other region denied", PermissionConstraints{Regions: []string{"eu-west-1"}}, false},
+		{"full in-scope request allowed", PermissionConstraints{
+			AccountIDs: []string{"acct-a"}, Providers: []string{"aws"}, Services: []string{"ec2"},
+			Regions: []string{"us-east-1"}, MaxPurchaseAmount: 100,
+		}, true},
+		// The empty probe sessionHoldsVerb sends must still report the verb
+		// as held: an empty request dimension is "not specified".
+		{"empty holds-verb probe allowed", PermissionConstraints{}, true},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			got, err := PermissionsAllowForConstraintSets([]Permission{held}, ActionExecuteAny, ResourcePurchases, []PermissionConstraints{tc.req})
+			require.NoError(t, err)
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}
+
+func TestPermissionsAllow_AdminWithoutConstraintsUnchanged(t *testing.T) {
+	t.Parallel()
+	admin := []Permission{{Action: ActionAdmin, Resource: ResourceAll}}
+	req := &PermissionConstraints{AccountIDs: []string{"acct-b"}, MaxPurchaseAmount: 1e6}
+	assert.True(t, permissionsAllow(admin, ActionExecuteAny, ResourcePurchases, req))
+	assert.True(t, permissionsAllow(admin, ActionUpdate, ResourceConfig, nil))
+	// Carve-outs still apply to an admin permission, constrained or not.
+	assert.False(t, permissionsAllow(admin, ActionExecute, ResourcePurchases, req))
+	constrained := []Permission{constrainedAdmin(&PermissionConstraints{AccountIDs: []string{"acct-a"}})}
+	assert.False(t, permissionsAllow(constrained, ActionExecute, ResourcePurchases, &PermissionConstraints{AccountIDs: []string{"acct-a"}}))
+}
+
+// A constrained admin grant is narrowed even when another grant in the set
+// matches the action: the decision is per permission, and an in-scope
+// explicit grant still allows.
+func TestPermissionsAllow_ConstrainedAdminAlongsideExplicitGrant(t *testing.T) {
+	t.Parallel()
+	perms := []Permission{
+		constrainedAdmin(&PermissionConstraints{AccountIDs: []string{"acct-a"}}),
+		{Action: ActionUpdate, Resource: ResourceConfig, Constraints: &PermissionConstraints{AccountIDs: []string{"acct-c"}}},
+	}
+	assert.False(t, permissionsAllow(perms, ActionUpdate, ResourceConfig, &PermissionConstraints{AccountIDs: []string{"acct-b"}}))
+	assert.True(t, permissionsAllow(perms, ActionUpdate, ResourceConfig, &PermissionConstraints{AccountIDs: []string{"acct-a"}}))
+	assert.True(t, permissionsAllow(perms, ActionUpdate, ResourceConfig, &PermissionConstraints{AccountIDs: []string{"acct-c"}}))
+}
+
+// End to end through the service: an Administrators-group owner holds an
+// unconstrained {admin, *}, so the owner gate passes everything; only the
+// key's own constraints can refuse account B or an amount over the cap.
+func TestHasAPIKeyPermissionForConstraintsAPI_ConstrainedAdminKey(t *testing.T) {
+	key := effKey(constrainedAdmin(&PermissionConstraints{AccountIDs: []string{"acct-a"}, MaxPurchaseAmount: 500}))
+	ownerPerms := []Permission{{Action: ActionAdmin, Resource: ResourceAll}}
+	cases := []struct {
+		name string
+		req  PermissionConstraints
+		want bool
+	}{
+		{"account B refused", PermissionConstraints{AccountIDs: []string{"acct-b"}, MaxPurchaseAmount: 100}, false},
+		{"amount over key cap refused", PermissionConstraints{AccountIDs: []string{"acct-a"}, MaxPurchaseAmount: 1000}, false},
+		{"in scope allowed", PermissionConstraints{AccountIDs: []string{"acct-a"}, MaxPurchaseAmount: 100}, true},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			s := &Service{store: effStore(t, key, effOwner(), ownerPerms)}
+			got, err := s.HasAPIKeyPermissionForConstraintsAPI(context.Background(), effKeyID, effOwnerID,
+				ActionExecuteAny, ResourcePurchases, []PermissionConstraints{tc.req})
+			require.NoError(t, err)
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}
+
+// The same rule holds for a group-granted constrained admin permission on the
+// session path (HasPermissionForConstraintsAPI).
+func TestHasPermissionForConstraintsAPI_ConstrainedAdminGroup(t *testing.T) {
+	groupPerms := []Permission{constrainedAdmin(&PermissionConstraints{AccountIDs: []string{"acct-a"}})}
+	s := &Service{store: effStore(t, nil, effOwner(), groupPerms)}
+	ctx := context.Background()
+
+	got, err := s.HasPermissionForConstraintsAPI(ctx, effOwnerID, ActionUpdate, ResourceConfig,
+		[]PermissionConstraints{{AccountIDs: []string{"acct-b"}}})
+	require.NoError(t, err)
+	assert.False(t, got, "a group's {admin, *} constrained to acct-a must not authorize acct-b")
+
+	got, err = s.HasPermissionForConstraintsAPI(ctx, effOwnerID, ActionUpdate, ResourceConfig,
+		[]PermissionConstraints{{AccountIDs: []string{"acct-a"}}})
+	require.NoError(t, err)
+	assert.True(t, got)
+}

internal/auth/service_apikeys.go

@@ -182,6 +182,13 @@ func (s *Service) validateCreateAPIKeyRequest(user *User, password, name string,
 // user's permissions. Administrators-group members hold {admin, *}, so the
 // per-permission HasPermission check below already passes for every requested
 // permission; no role-based short-circuit is needed.
+//
+// Only action and resource are checked here. A key permission's Constraints
+// are not compared with the owner's at creation; they are enforced at check
+// time, where HasAPIKeyPermissionForConstraintsAPI requires every request to
+// pass both the key's constraints and the owner's. That holds for an
+// {admin, *} key permission too: permissionsAllow applies its Constraints on
+// every check, not only under StrictScope (issue #956).
 func (s *Service) validateAPIKeyPermissions(ctx context.Context, user *User, permissions []Permission) error {
 	// Get user's auth context to check their permissions
 	authCtx, err := s.GetAuthContext(ctx, user.ID)

internal/auth/service_group.go

@@ -323,6 +323,14 @@ func (s *Service) HasPermission(ctx context.Context, userID, action, resource st
 // adminCarvedOuts (separation of duties, issue #923). For those, admin falls
 // through to the explicit-permission check below instead of short-circuiting.
 //
+// An admin:* permission that carries Constraints is narrowed by them on every
+// check, strict or not, exactly like any other permission (issue #956). It
+// used to apply them only under StrictScope, so a key created as {admin, *}
+// with accounts=[A] or a max_amount passed a non-strict check for account B
+// or for any amount: the scoping its creator asked for was silently ignored.
+// An unconstrained admin:* (the seeded Administrators group) is unaffected,
+// since checkPermissionConstraints passes a permission with no Constraints.
+//
 // A package function rather than a Service method: the decision reads nothing
 // off the receiver, and PermissionsAllowForConstraintSets exposes it to
 // callers that hold an already-resolved permission set but no Service.
@@ -332,7 +340,7 @@ func permissionsAllow(permissions []Permission, action, resource string, constra
 			if coversCarvedOut(Permission{Action: action, Resource: resource}) {
 				continue
 			}
-			if constraints != nil && constraints.StrictScope && !checkPermissionConstraints(perm, constraints) {
+			if !checkPermissionConstraints(perm, constraints) {
 				continue
 			}
 			return true

internal/server/apikey_admin_constraints_test.go

@@ -0,0 +1,73 @@
+package server
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/api"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/auth"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+// Issue #956: a key created through the API as {admin, *} with accounts=[A]
+// and a max_amount keeps those constraints on create (#957), and they must
+// also be enforced on the non-strict constraint check the API layer runs for
+// user-API-key sessions (requirePermissionConstraints ->
+// HasAPIKeyPermissionForConstraintsAPI). The owner is an Administrators
+// member with an unconstrained {admin, *}, so the owner gate passes
+// everything and only the key's own constraints can refuse the request.
+func TestAuthServiceAdapter_ConstrainedAdminKeyRefusedOutOfScope(t *testing.T) {
+	adapter, mockStore := createMockAuthService(t)
+	ctx := context.Background()
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+
+	const ownerPass = "CorrectHorse@123" // throwaway fixture credential
+	owner := &auth.User{
+		ID: "user-1", Email: "test@example.com", Active: true,
+		PasswordHash: auth.TestPasswordHash(t, ownerPass),
+		GroupIDs:     []string{auth.DefaultAdminGroupID},
+	}
+	mockStore.On("GetUserByID", ctx, "user-1").Return(owner, nil)
+	mockStore.On("GetGroup", ctx, auth.DefaultAdminGroupID).Return(&auth.Group{
+		ID:          auth.DefaultAdminGroupID,
+		Permissions: []auth.Permission{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}},
+	}, nil)
+	var stored *auth.UserAPIKey
+	mockStore.On("CreateAPIKey", ctx, mock.AnythingOfType("*auth.UserAPIKey")).
+		Run(func(args mock.Arguments) { stored = args.Get(1).(*auth.UserAPIKey) }).Return(nil)
+
+	expiresAt := time.Now().Add(24 * time.Hour)
+	_, err := adapter.CreateAPIKeyAPI(ctx, "user-1", api.CreateAPIKeyRequest{
+		Name:      "admin-scoped",
+		Password:  ownerPass,
+		ExpiresAt: &expiresAt,
+		Permissions: []api.Permission{{
+			Action: auth.ActionAdmin, Resource: auth.ResourceAll,
+			Constraints: &api.PermissionConstraint{Accounts: []string{"acct-a"}, MaxAmount: 500},
+		}},
+	})
+	require.NoError(t, err)
+	require.NotNil(t, stored)
+	mockStore.On("GetAPIKeyByID", ctx, stored.ID).Return(stored, nil)
+
+	cases := []struct {
+		name string
+		req  auth.PermissionConstraints
+		want bool
+	}{
+		{"account B refused", auth.PermissionConstraints{AccountIDs: []string{"acct-b"}, MaxPurchaseAmount: 100}, false},
+		{"amount over key cap refused", auth.PermissionConstraints{AccountIDs: []string{"acct-a"}, MaxPurchaseAmount: 501}, false},
+		{"in scope allowed", auth.PermissionConstraints{AccountIDs: []string{"acct-a"}, MaxPurchaseAmount: 500}, true},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := adapter.HasAPIKeyPermissionForConstraintsAPI(ctx, stored.ID, "user-1",
+				auth.ActionExecuteAny, auth.ResourcePurchases, []auth.PermissionConstraints{tc.req})
+			require.NoError(t, err)
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}