sec(auth): carve out money-verb wildcards at grant and check

internal/auth/carveout_wildcard_test.go

@@ -0,0 +1,212 @@
+package auth
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+// Issue #1901 (audit A03-001..003): the carve-out set is keyed on exact
+// (action, resource) pairs, while enforcement treats a stored resource of
+// "*" as matching every resource. So {execute, *} was not carved out at
+// grant time but granted execute:purchases at check time. Every guard that
+// consults the carve-out must see the wildcard form as carved out.
+//
+// As elsewhere in this package, refusal cases stub NO write expectation on
+// the mock store: testify panics if the write lands, so the assertions
+// cannot be vacuous.
+
+// wildcardMoneyVerbs is the attack payload from the audit reproduction.
+var wildcardMoneyVerbs = []APIPermission{
+	{Action: ActionExecute, Resource: ResourceAll},
+	{Action: ActionApproveAny, Resource: ResourceAll},
+	{Action: ActionRetryAny, Resource: ResourceAll},
+}
+
+// A03-001, the grant ceiling: an admin creating a group carrying the
+// wildcard form of a money verb.
+func TestGrantCeiling_WildcardMoneyVerbNotGrantable(t *testing.T) {
+	ctx := context.Background()
+
+	for _, perm := range wildcardMoneyVerbs {
+		t.Run("create "+perm.Action+":"+perm.Resource, func(t *testing.T) {
+			mockStore := new(MockStore)
+			t.Cleanup(func() { mockStore.AssertExpectations(t) })
+			svc := newCeilingService(t, mockStore)
+
+			stubActorPermissions(ctx, mockStore, adminOnly)
+
+			result, err := svc.CreateGroupAPI(ctx, ceilingActorID, APICreateGroupRequest{
+				Name:        "Spenders",
+				Permissions: []APIPermission{perm},
+			})
+
+			require.Error(t, err)
+			assert.Nil(t, result)
+			assert.ErrorIs(t, err, ErrPermissionNotGrantable)
+			assert.Contains(t, err.Error(), perm.Action+":"+perm.Resource)
+			mockStore.AssertNotCalled(t, "CreateGroup", mock.Anything, mock.Anything)
+		})
+	}
+
+	t.Run("update execute:*", func(t *testing.T) {
+		mockStore := new(MockStore)
+		t.Cleanup(func() { mockStore.AssertExpectations(t) })
+		svc := newCeilingService(t, mockStore)
+
+		stubActorPermissions(ctx, mockStore, adminOnly)
+		stubTargetGroup(ctx, mockStore, &Group{ID: ceilingTargetID, Name: "Administrators"})
+
+		_, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID,
+			updateReqWith(APIPermission{Action: ActionExecute, Resource: ResourceAll}))
+
+		require.Error(t, err)
+		assert.ErrorIs(t, err, ErrPermissionNotGrantable)
+		mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything)
+	})
+
+	// A default-deployment admin explicitly holds the concrete money verbs
+	// (migrations 000059/000064). Holding execute:purchases is not holding
+	// execute:*, and the carve-out refuses the wildcard regardless.
+	t.Run("purchaser admin cannot grant execute:*", func(t *testing.T) {
+		mockStore := new(MockStore)
+		t.Cleanup(func() { mockStore.AssertExpectations(t) })
+		svc := newCeilingService(t, mockStore)
+
+		stubActorPermissions(ctx, mockStore, append(
+			[]Permission{{Action: ActionAdmin, Resource: ResourceAll}},
+			DefaultPurchaserPermissions()...))
+
+		_, err := svc.CreateGroupAPI(ctx, ceilingActorID, APICreateGroupRequest{
+			Name:        "Spenders",
+			Permissions: []APIPermission{{Action: ActionExecute, Resource: ResourceAll}},
+		})
+
+		require.Error(t, err)
+		assert.ErrorIs(t, err, ErrPermissionNotGrantable)
+		mockStore.AssertNotCalled(t, "CreateGroup", mock.Anything, mock.Anything)
+	})
+}
+
+// A03-003, the self-membership guard: an admin joining a group that carries
+// the wildcard form.
+func TestSelfCarvedOutGrant_WildcardGroupBlocked(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockStore)
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+	svc := createTestService(mockStore, new(MockEmailSender))
+
+	wildcard := &Group{
+		ID:          "88888888-8888-4888-8888-888888888888",
+		Name:        "Wildcard Spenders",
+		Permissions: []Permission{{Action: ActionExecute, Resource: ResourceAll}},
+	}
+	stubSelfActor(ctx, mockStore, []string{adminGroupID}, adminGroupRow(), wildcard)
+
+	_, err := svc.UpdateUser(ctx, selfActorID, selfActorID, UpdateUserRequest{
+		GroupIDs: []string{adminGroupID, wildcard.ID},
+	})
+
+	require.Error(t, err)
+	assert.ErrorIs(t, err, ErrSelfEscalation)
+	assert.Contains(t, err.Error(), ActionExecute+":"+ResourceAll)
+	mockStore.AssertNotCalled(t, "UpdateUser", mock.Anything, mock.Anything)
+}
+
+// A03-002, API-key creation: an admin minting a key that carries the
+// wildcard form, through the real CreateAPIKey path.
+func TestCreateAPIKey_AdminCannotMintWildcardMoneyVerb(t *testing.T) {
+	ctx := context.Background()
+	adminGrp := &Group{ID: DefaultAdminGroupID, Permissions: adminOnly}
+
+	for _, perm := range wildcardMoneyVerbs {
+		t.Run(perm.Action+":"+perm.Resource, func(t *testing.T) {
+			mockStore := new(MockStore)
+			t.Cleanup(func() { mockStore.AssertExpectations(t) })
+			service := &Service{store: mockStore}
+
+			user := &User{ID: "user-123", Active: true, GroupIDs: []string{DefaultAdminGroupID}}
+			mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil)
+			mockStore.On("GetGroup", ctx, DefaultAdminGroupID).Return(adminGrp, nil)
+
+			_, _, err := service.CreateAPIKey(ctx, "user-123", "spender",
+				[]Permission{{Action: perm.Action, Resource: perm.Resource}}, nil)
+
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "user does not have permission")
+			mockStore.AssertNotCalled(t, "CreateAPIKey", mock.Anything, mock.Anything)
+		})
+	}
+
+	// Negative control: an owner whose group explicitly holds the wildcard
+	// form may mint it. The refusal keys on what the owner holds, not on
+	// the wildcard itself.
+	t.Run("explicit execute:* holder may mint execute:*", func(t *testing.T) {
+		mockStore := new(MockStore)
+		t.Cleanup(func() { mockStore.AssertExpectations(t) })
+		service := &Service{store: mockStore}
+
+		grpID := "99999999-9999-4999-8999-999999999999"
+		user := &User{ID: "user-123", Active: true, GroupIDs: []string{grpID}}
+		mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil)
+		mockStore.On("GetGroup", ctx, grpID).Return(&Group{
+			ID:          grpID,
+			Permissions: []Permission{{Action: ActionExecute, Resource: ResourceAll}},
+		}, nil)
+		mockStore.On("CreateAPIKey", ctx, mock.AnythingOfType("*auth.UserAPIKey")).Return(nil).Once()
+
+		_, _, err := service.CreateAPIKey(ctx, "user-123", "spender",
+			[]Permission{{Action: ActionExecute, Resource: ResourceAll}}, nil)
+		require.NoError(t, err)
+	})
+}
+
+// A03-002 at use time: a key that already stores the wildcard form (minted
+// before the fix) must not spend for an admin owner.
+func TestComputeEffectivePermissions_AdminKeyDropsWildcardMoneyVerb(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockStore)
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+	service := &Service{store: mockStore}
+
+	user := &User{ID: "user-123", Active: true, GroupIDs: []string{DefaultAdminGroupID}}
+	mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil)
+	mockStore.On("GetGroup", ctx, DefaultAdminGroupID).
+		Return(&Group{ID: DefaultAdminGroupID, Permissions: adminOnly}, nil)
+
+	key := &UserAPIKey{ID: "key-1", UserID: "user-123", Permissions: []Permission{
+		{Action: ActionExecute, Resource: ResourceAll},
+		{Action: ActionView, Resource: ResourcePlans},
+	}}
+
+	effective, err := service.ComputeEffectivePermissions(ctx, key, user)
+	require.NoError(t, err)
+	assert.Equal(t, []Permission{{Action: ActionView, Resource: ResourcePlans}}, effective)
+
+	keyCtx := &AuthContext{User: user, Permissions: effective}
+	assert.False(t, keyCtx.HasPermission(ActionExecute, ResourcePurchases))
+	assert.False(t, keyCtx.HasPermission(ActionExecute, ResourceRIExchange))
+}
+
+// The two enforcement matchers, asked for the wildcard form directly.
+func TestAdminWildcardCarveOuts_WildcardResource(t *testing.T) {
+	adminCtx := &AuthContext{User: &User{}, Permissions: adminOnly}
+
+	assert.False(t, adminCtx.HasPermission(ActionExecute, ResourceAll))
+	assert.False(t, adminCtx.HasPermission(ActionApproveAny, ResourceAll))
+	assert.False(t, adminCtx.HasPermission(ActionRetryAny, ResourceAll))
+	assert.False(t, permissionsAllow(adminOnly, ActionExecute, ResourceAll, nil))
+	assert.False(t, permissionsAllow(adminOnly, ActionApproveAny, ResourceAll, nil))
+	assert.False(t, permissionsAllow(adminOnly, ActionRetryAny, ResourceAll, nil))
+
+	// admin:* still covers wildcard requests for verbs that are not carved
+	// out, and an explicit wildcard holder is still matched.
+	assert.True(t, adminCtx.HasPermission(ActionView, ResourceAll))
+	assert.True(t, permissionsAllow(adminOnly, ActionCancelAny, ResourceAll, nil))
+	explicit := []Permission{{Action: ActionExecute, Resource: ResourceAll}}
+	assert.True(t, (&AuthContext{User: &User{}, Permissions: explicit}).HasPermission(ActionExecute, ResourceAll))
+	assert.True(t, permissionsAllow(explicit, ActionExecute, ResourcePurchases, nil))
+}

internal/auth/group_ceiling.go

@@ -82,7 +82,7 @@ func (s *Service) checkGrantCeiling(ctx context.Context, actorUserID string, req
 		return err
 	}
 	for _, req := range requested {
-		if adminCarvedOuts[[2]string{req.Action, req.Resource}] {
+		if coversCarvedOut(req) {
 			if permissionCoveredBy(existing, req) {
 				continue
 			}
@@ -350,7 +350,7 @@ func grantCeilingAllows(actorPerms []Permission, req Permission) bool {
 			// constraint set. Carved-out pairs never reach here (the caller
 			// rejects them first), but mirror permissionsAllow anyway so the
 			// two stay in lockstep if the carve-out set grows (#1644).
-			if adminCarvedOuts[[2]string{req.Action, req.Resource}] {
+			if coversCarvedOut(req) {
 				continue
 			}
 			return true

internal/auth/group_ceiling_permissions_test.go

@@ -230,7 +230,7 @@ func TestGrantCeiling_ConstraintContainment(t *testing.T) {
 	ctx := context.Background()
 
 	const fixtureAction, fixtureResource = ActionView, ResourcePlans
-	require.False(t, adminCarvedOuts[[2]string{fixtureAction, fixtureResource}],
+	require.False(t, coversCarvedOut(Permission{Action: fixtureAction, Resource: fixtureResource}),
 		"fixture verb %s:%s must not be carved out, or the carve-out check "+
 			"refuses before this test's containment logic ever runs", fixtureAction, fixtureResource)
 

internal/auth/service_group.go

@@ -329,7 +329,7 @@ func (s *Service) HasPermission(ctx context.Context, userID, action, resource st
 func permissionsAllow(permissions []Permission, action, resource string, constraints *PermissionConstraints) bool {
 	for _, perm := range permissions {
 		if checkAdminPermission(perm) {
-			if adminCarvedOuts[[2]string{action, resource}] {
+			if coversCarvedOut(Permission{Action: action, Resource: resource}) {
 				continue
 			}
 			return true

internal/auth/service_user.go

@@ -548,7 +548,7 @@ func (s *Service) guardSelfCarvedOutGrant(ctx context.Context, held []Permission
 // applies: an actor holding only admin:* does not "already hold" these.
 func (s *Service) firstUnheldCarvedOut(group *Group, held []Permission) *Permission {
 	for i, perm := range group.Permissions {
-		if !adminCarvedOuts[[2]string{perm.Action, perm.Resource}] {
+		if !coversCarvedOut(perm) {
 			continue
 		}
 		if permissionsAllow(held, perm.Action, perm.Resource, nil) {

internal/auth/types.go

@@ -133,6 +133,22 @@ var adminCarvedOuts = map[[2]string]bool{
 	{ActionExecute, ResourceRIExchange}: true,
 }
 
+// coversCarvedOut reports whether perm, as stated, would satisfy any
+// carved-out pair under enforcement's own matching rules
+// (checkPermissionMatch). A stored resource of "*" matches every resource
+// there, so {execute, *} covers execute:purchases and execute:ri-exchange and
+// is carved out exactly as those pairs are (issue #1901). Every carve-out
+// test goes through here so the grant-time guards and the enforcement
+// matchers cannot disagree about what the set covers.
+func coversCarvedOut(perm Permission) bool {
+	for pair := range adminCarvedOuts {
+		if checkPermissionMatch(perm, pair[0], pair[1]) {
+			return true
+		}
+	}
+	return false
+}
+
 // HasPermission checks if the auth context has a specific permission.
 // Authorization is derived purely from group-granted permissions: a user
 // who is a member of the Administrators group holds {ActionAdmin, ResourceAll}
@@ -148,7 +164,7 @@ func (ctx *AuthContext) HasPermission(action, resource string) bool {
 		// Admin permission grants all access EXCEPT the carved-out
 		// money-spending verbs (separation of duties, issue #923).
 		if perm.Action == ActionAdmin && perm.Resource == ResourceAll {
-			if adminCarvedOuts[[2]string{action, resource}] {
+			if coversCarvedOut(Permission{Action: action, Resource: resource}) {
 				// Fall through to explicit-permission check below.
 				continue
 			}

internal/auth/types_test.go

@@ -172,3 +172,35 @@ func TestAdminAndPurchaserCanExecutePurchases(t *testing.T) {
 	assert.True(t, ctx.HasPermission(ActionRetryAny, ResourcePurchases))
 	assert.True(t, ctx.HasPermission(ActionDelete, ResourceUsers))
 }
+
+func TestCoversCarvedOut(t *testing.T) {
+	cases := []struct {
+		perm Permission
+		want bool
+	}{
+		{Permission{Action: ActionExecute, Resource: ResourcePurchases}, true},
+		{Permission{Action: ActionApproveAny, Resource: ResourcePurchases}, true},
+		{Permission{Action: ActionRetryAny, Resource: ResourcePurchases}, true},
+		{Permission{Action: ActionExecute, Resource: ResourceRIExchange}, true},
+		{Permission{Action: ActionExecute, Resource: ResourceAll}, true},
+		{Permission{Action: ActionApproveAny, Resource: ResourceAll}, true},
+		{Permission{Action: ActionRetryAny, Resource: ResourceAll}, true},
+		// Not carved out: other verbs, even with the wildcard.
+		{Permission{Action: ActionAdmin, Resource: ResourceAll}, false},
+		{Permission{Action: ActionView, Resource: ResourceAll}, false},
+		{Permission{Action: ActionCancelAny, Resource: ResourceAll}, false},
+		{Permission{Action: ActionExecute, Resource: ResourcePlans}, false},
+		// Forms enforcement does not treat as wildcards, so neither does
+		// the carve-out: blank, prefix, and different case match nothing
+		// at check time (checkPermissionMatch compares exactly).
+		{Permission{Action: ActionExecute, Resource: ""}, false},
+		{Permission{Action: ActionExecute, Resource: "purchases*"}, false},
+		{Permission{Action: ActionExecute, Resource: "Purchases"}, false},
+		{Permission{Action: "Execute", Resource: ResourceAll}, false},
+	}
+	for _, tc := range cases {
+		t.Run(tc.perm.Action+":"+tc.perm.Resource, func(t *testing.T) {
+			assert.Equal(t, tc.want, coversCarvedOut(tc.perm))
+		})
+	}
+}