diff --git a/internal/auth/carveout_wildcard_test.go b/internal/auth/carveout_wildcard_test.go new file mode 100644 index 000000000..4eb4d24ba --- /dev/null +++ b/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)) +} diff --git a/internal/auth/group_ceiling.go b/internal/auth/group_ceiling.go index 1fa48b057..7f090609e 100644 --- a/internal/auth/group_ceiling.go +++ b/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 diff --git a/internal/auth/group_ceiling_permissions_test.go b/internal/auth/group_ceiling_permissions_test.go index b4318b5b9..2dbbb75f3 100644 --- a/internal/auth/group_ceiling_permissions_test.go +++ b/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) diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index 994d22502..2f27227cd 100644 --- a/internal/auth/service_group.go +++ b/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 diff --git a/internal/auth/service_user.go b/internal/auth/service_user.go index 1fb02bf03..fd84ef8b5 100644 --- a/internal/auth/service_user.go +++ b/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) { diff --git a/internal/auth/types.go b/internal/auth/types.go index 46ec0a26c..9f91a5e92 100644 --- a/internal/auth/types.go +++ b/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 } diff --git a/internal/auth/types_test.go b/internal/auth/types_test.go index 80981f492..dfcdce022 100644 --- a/internal/auth/types_test.go +++ b/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)) + }) + } +}