From ab1fd29c55e151fbfb67805d86a893895be00cc6 Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Thu, 14 May 2026 16:36:56 +0530 Subject: [PATCH 1/6] refactor(group): move group-delete cleanup into membership package MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirrors the project deletion pattern (handler → deleter → DeleteModel + membership cleanup): - core/membership.RemoveAllGroupMembers: deletes every policy on the group and every per-principal owner/member relation. No min-owner check; the group itself is being destroyed. - core/group.Service.DeleteModel: minimal entity teardown — Object-side relations on the group + repo delete + audit. Replaces the previous combined Delete method that mixed membership cleanup into the entity service. - core/deleter.DeleteGroup: orchestrator that calls RemoveAllGroupMembers then DeleteModel. Wired into the DeleteOrganization cascade and exposed via the CascadeDeleter interface. - DeleteGroup handler routes through deleterService instead of groupService directly. group.RemoveUsers / removeUsers stay — they serve the single-user remove path in deleter.DeleteUser (cascade where the user is being destroyed system-wide). Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/serve.go | 2 +- core/deleter/mocks/group_service.go | 22 ++--- core/deleter/mocks/membership_service.go | 83 +++++++++++++++++++ core/deleter/service.go | 21 ++++- core/deleter/service_test.go | 51 ++++++------ core/group/service.go | 31 ++----- core/membership/service.go | 38 +++++++++ core/membership/service_test.go | 62 ++++++++++++++ internal/api/v1beta1connect/group.go | 4 +- internal/api/v1beta1connect/group_test.go | 22 ++--- internal/api/v1beta1connect/interfaces.go | 2 +- .../v1beta1connect/mocks/cascade_deleter.go | 47 +++++++++++ .../api/v1beta1connect/mocks/group_service.go | 47 ----------- 13 files changed, 311 insertions(+), 121 deletions(-) create mode 100644 core/deleter/mocks/membership_service.go diff --git a/cmd/serve.go b/cmd/serve.go index d220801de..64bd9e482 100644 --- a/cmd/serve.go +++ b/cmd/serve.go @@ -574,7 +574,7 @@ func buildAPIDependencies( ) cascadeDeleter := deleter.NewCascadeDeleter(organizationService, projectService, resourceService, - groupService, policyService, roleService, invitationService, userService, serviceUserService, + groupService, membershipService, policyService, roleService, invitationService, userService, serviceUserService, customerService, subscriptionService, invoiceService, ) diff --git a/core/deleter/mocks/group_service.go b/core/deleter/mocks/group_service.go index e86a8892a..2d96d9a9d 100644 --- a/core/deleter/mocks/group_service.go +++ b/core/deleter/mocks/group_service.go @@ -23,12 +23,12 @@ func (_m *GroupService) EXPECT() *GroupService_Expecter { return &GroupService_Expecter{mock: &_m.Mock} } -// Delete provides a mock function with given fields: ctx, id -func (_m *GroupService) Delete(ctx context.Context, id string) error { +// DeleteModel provides a mock function with given fields: ctx, id +func (_m *GroupService) DeleteModel(ctx context.Context, id string) error { ret := _m.Called(ctx, id) if len(ret) == 0 { - panic("no return value specified for Delete") + panic("no return value specified for DeleteModel") } var r0 error @@ -41,31 +41,31 @@ func (_m *GroupService) Delete(ctx context.Context, id string) error { return r0 } -// GroupService_Delete_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Delete' -type GroupService_Delete_Call struct { +// GroupService_DeleteModel_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'DeleteModel' +type GroupService_DeleteModel_Call struct { *mock.Call } -// Delete is a helper method to define mock.On call +// DeleteModel is a helper method to define mock.On call // - ctx context.Context // - id string -func (_e *GroupService_Expecter) Delete(ctx interface{}, id interface{}) *GroupService_Delete_Call { - return &GroupService_Delete_Call{Call: _e.mock.On("Delete", ctx, id)} +func (_e *GroupService_Expecter) DeleteModel(ctx interface{}, id interface{}) *GroupService_DeleteModel_Call { + return &GroupService_DeleteModel_Call{Call: _e.mock.On("DeleteModel", ctx, id)} } -func (_c *GroupService_Delete_Call) Run(run func(ctx context.Context, id string)) *GroupService_Delete_Call { +func (_c *GroupService_DeleteModel_Call) Run(run func(ctx context.Context, id string)) *GroupService_DeleteModel_Call { _c.Call.Run(func(args mock.Arguments) { run(args[0].(context.Context), args[1].(string)) }) return _c } -func (_c *GroupService_Delete_Call) Return(_a0 error) *GroupService_Delete_Call { +func (_c *GroupService_DeleteModel_Call) Return(_a0 error) *GroupService_DeleteModel_Call { _c.Call.Return(_a0) return _c } -func (_c *GroupService_Delete_Call) RunAndReturn(run func(context.Context, string) error) *GroupService_Delete_Call { +func (_c *GroupService_DeleteModel_Call) RunAndReturn(run func(context.Context, string) error) *GroupService_DeleteModel_Call { _c.Call.Return(run) return _c } diff --git a/core/deleter/mocks/membership_service.go b/core/deleter/mocks/membership_service.go new file mode 100644 index 000000000..a23d0fe95 --- /dev/null +++ b/core/deleter/mocks/membership_service.go @@ -0,0 +1,83 @@ +// Code generated by mockery v2.53.5. DO NOT EDIT. + +package mocks + +import ( + context "context" + + mock "github.com/stretchr/testify/mock" +) + +// MembershipService is an autogenerated mock type for the MembershipService type +type MembershipService struct { + mock.Mock +} + +type MembershipService_Expecter struct { + mock *mock.Mock +} + +func (_m *MembershipService) EXPECT() *MembershipService_Expecter { + return &MembershipService_Expecter{mock: &_m.Mock} +} + +// RemoveAllGroupMembers provides a mock function with given fields: ctx, groupID +func (_m *MembershipService) RemoveAllGroupMembers(ctx context.Context, groupID string) error { + ret := _m.Called(ctx, groupID) + + if len(ret) == 0 { + panic("no return value specified for RemoveAllGroupMembers") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string) error); ok { + r0 = rf(ctx, groupID) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// MembershipService_RemoveAllGroupMembers_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RemoveAllGroupMembers' +type MembershipService_RemoveAllGroupMembers_Call struct { + *mock.Call +} + +// RemoveAllGroupMembers is a helper method to define mock.On call +// - ctx context.Context +// - groupID string +func (_e *MembershipService_Expecter) RemoveAllGroupMembers(ctx interface{}, groupID interface{}) *MembershipService_RemoveAllGroupMembers_Call { + return &MembershipService_RemoveAllGroupMembers_Call{Call: _e.mock.On("RemoveAllGroupMembers", ctx, groupID)} +} + +func (_c *MembershipService_RemoveAllGroupMembers_Call) Run(run func(ctx context.Context, groupID string)) *MembershipService_RemoveAllGroupMembers_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string)) + }) + return _c +} + +func (_c *MembershipService_RemoveAllGroupMembers_Call) Return(_a0 error) *MembershipService_RemoveAllGroupMembers_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *MembershipService_RemoveAllGroupMembers_Call) RunAndReturn(run func(context.Context, string) error) *MembershipService_RemoveAllGroupMembers_Call { + _c.Call.Return(run) + return _c +} + +// NewMembershipService creates a new instance of MembershipService. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewMembershipService(t interface { + mock.TestingT + Cleanup(func()) +}) *MembershipService { + mock := &MembershipService{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/core/deleter/service.go b/core/deleter/service.go index a96998524..e5176e9d9 100644 --- a/core/deleter/service.go +++ b/core/deleter/service.go @@ -65,10 +65,14 @@ type ResourceService interface { type GroupService interface { List(ctx context.Context, flt group.Filter) ([]group.Group, error) - Delete(ctx context.Context, id string) error + DeleteModel(ctx context.Context, id string) error RemoveUsers(ctx context.Context, groupID string, userIDs []string) error } +type MembershipService interface { + RemoveAllGroupMembers(ctx context.Context, groupID string) error +} + type InvitationService interface { List(ctx context.Context, flt invitation.Filter) ([]invitation.Invitation, error) Delete(ctx context.Context, id uuid.UUID) error @@ -102,6 +106,7 @@ type Service struct { orgService OrganizationService resService ResourceService groupService GroupService + membershipService MembershipService policyService PolicyService roleService RoleService invitationService InvitationService @@ -114,6 +119,7 @@ type Service struct { func NewCascadeDeleter(orgService OrganizationService, projService ProjectService, resService ResourceService, groupService GroupService, + membershipService MembershipService, policyService PolicyService, roleService RoleService, invitationService InvitationService, userService UserService, serviceUserService ServiceUserService, @@ -124,6 +130,7 @@ func NewCascadeDeleter(orgService OrganizationService, projService ProjectServic orgService: orgService, resService: resService, groupService: groupService, + membershipService: membershipService, policyService: policyService, roleService: roleService, invitationService: invitationService, @@ -165,6 +172,16 @@ func (d Service) DeleteProject(ctx context.Context, id string) error { return d.projService.DeleteModel(ctx, id) } +// DeleteGroup orchestrates teardown of a single group: clears every member's +// policies and SpiceDB relations via membership, then deletes the group +// entity itself (which clears the org<->group hierarchy relations). +func (d Service) DeleteGroup(ctx context.Context, id string) error { + if err := d.membershipService.RemoveAllGroupMembers(ctx, id); err != nil { + return fmt.Errorf("remove group members: %w", err) + } + return d.groupService.DeleteModel(ctx, id) +} + func (d Service) DeleteOrganization(ctx context.Context, id string) error { // check if delete is allowed if err := d.canDelete(ctx, id); err != nil { @@ -203,7 +220,7 @@ func (d Service) DeleteOrganization(ctx context.Context, id string) error { return err } for _, g := range groups { - if err = d.groupService.Delete(ctx, g.ID); err != nil { + if err = d.DeleteGroup(ctx, g.ID); err != nil { return fmt.Errorf("failed to delete org while deleting a group[%s]: %w", g.Name, err) } } diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index 134953cc0..b62321d33 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -26,6 +26,7 @@ func newMocks(t *testing.T) ( *mocks.ProjectService, *mocks.ResourceService, *mocks.GroupService, + *mocks.MembershipService, *mocks.PolicyService, *mocks.RoleService, *mocks.InvitationService, @@ -40,6 +41,7 @@ func newMocks(t *testing.T) ( mocks.NewProjectService(t), mocks.NewResourceService(t), mocks.NewGroupService(t), + mocks.NewMembershipService(t), mocks.NewPolicyService(t), mocks.NewRoleService(t), mocks.NewInvitationService(t), @@ -52,7 +54,7 @@ func newMocks(t *testing.T) ( func TestDeleteProject(t *testing.T) { t.Run("deletes policies, resources, then project model", func(t *testing.T) { - orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) + orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}). Return([]policy.Policy{{ID: "pol-1"}, {ID: "pol-2"}}, nil) @@ -65,38 +67,38 @@ func TestDeleteProject(t *testing.T) { projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1").Return(nil) - svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) + svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) err := svc.DeleteProject(context.Background(), "proj-1") assert.NoError(t, err) }) t.Run("returns error when policy list fails", func(t *testing.T) { - _, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) + _, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) orgSvc := mocks.NewOrganizationService(t) polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}). Return(nil, errors.New("db error")) - svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) + svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) err := svc.DeleteProject(context.Background(), "proj-1") assert.ErrorContains(t, err, "db error") }) t.Run("returns error when policy delete fails", func(t *testing.T) { - _, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) + _, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) orgSvc := mocks.NewOrganizationService(t) polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}). Return([]policy.Policy{{ID: "pol-fail"}}, nil) polSvc.EXPECT().Delete(mock.Anything, "pol-fail").Return(errors.New("delete error")) - svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) + svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) err := svc.DeleteProject(context.Background(), "proj-1") assert.ErrorContains(t, err, "pol-fail") }) t.Run("no policies — still deletes resources and project", func(t *testing.T) { - orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) + orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}). Return([]policy.Policy{}, nil) @@ -104,7 +106,7 @@ func TestDeleteProject(t *testing.T) { Return([]resource.Resource{}, nil) projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1").Return(nil) - svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) + svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) err := svc.DeleteProject(context.Background(), "proj-1") assert.NoError(t, err) }) @@ -112,7 +114,7 @@ func TestDeleteProject(t *testing.T) { func TestDeleteOrganization(t *testing.T) { t.Run("full cascade delete", func(t *testing.T) { - orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) + orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) // canDelete: no customers custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). @@ -133,10 +135,11 @@ func TestDeleteOrganization(t *testing.T) { Return([]resource.Resource{}, nil) projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1").Return(nil) - // groups + // groups (DeleteGroup: RemoveAllGroupMembers then DeleteModel) grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1"}). Return([]group.Group{{ID: "grp-1", Name: "g1"}}, nil) - grpSvc.EXPECT().Delete(mock.Anything, "grp-1").Return(nil) + mbrSvc.EXPECT().RemoveAllGroupMembers(mock.Anything, "grp-1").Return(nil) + grpSvc.EXPECT().DeleteModel(mock.Anything, "grp-1").Return(nil) // service users suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}). @@ -161,13 +164,13 @@ func TestDeleteOrganization(t *testing.T) { // finally delete org model orgSvc.EXPECT().DeleteModel(mock.Anything, "org-1").Return(nil) - svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) + svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) err := svc.DeleteOrganization(context.Background(), "org-1") assert.NoError(t, err) }) t.Run("blocked when billed customer has invoices", func(t *testing.T) { - _, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) + _, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) orgSvc := mocks.NewOrganizationService(t) custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). @@ -175,13 +178,13 @@ func TestDeleteOrganization(t *testing.T) { invocSvc.EXPECT().List(mock.Anything, invoice.Filter{CustomerID: "cust-1"}). Return([]invoice.Invoice{{ID: "inv-1"}}, nil) - svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) + svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) err := svc.DeleteOrganization(context.Background(), "org-1") assert.ErrorIs(t, err, deleter.ErrDeleteNotAllowed) }) t.Run("propagates error when service user list fails", func(t *testing.T) { - orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) + orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). Return([]customer.Customer{}, nil) @@ -194,13 +197,13 @@ func TestDeleteOrganization(t *testing.T) { suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}). Return(nil, errors.New("su list failed")) - svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) + svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) err := svc.DeleteOrganization(context.Background(), "org-1") assert.ErrorContains(t, err, "su list failed") }) t.Run("propagates error when service user delete fails", func(t *testing.T) { - orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) + orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). Return([]customer.Customer{}, nil) @@ -214,7 +217,7 @@ func TestDeleteOrganization(t *testing.T) { Return([]serviceuser.ServiceUser{{ID: "su-1"}}, nil) suSvc.EXPECT().Delete(mock.Anything, "su-1").Return(errors.New("su delete failed")) - svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) + svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) err := svc.DeleteOrganization(context.Background(), "org-1") assert.ErrorContains(t, err, "su delete failed") assert.ErrorContains(t, err, "su-1") @@ -223,7 +226,7 @@ func TestDeleteOrganization(t *testing.T) { func TestDeleteCustomers(t *testing.T) { t.Run("deletes subscriptions invoices and customer", func(t *testing.T) { - orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) + orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) c := customer.Customer{ID: "cust-1", ProviderID: "stripe-1"} custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). @@ -232,13 +235,13 @@ func TestDeleteCustomers(t *testing.T) { invocSvc.EXPECT().DeleteByCustomer(mock.Anything, c).Return(nil) custSvc.EXPECT().Delete(mock.Anything, "cust-1").Return(nil) - svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) + svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) err := svc.DeleteCustomers(context.Background(), "org-1") assert.NoError(t, err) }) t.Run("skips subscription and invoice delete when no provider", func(t *testing.T) { - orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) + orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) c := customer.Customer{ID: "cust-no-provider", ProviderID: ""} custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). @@ -246,7 +249,7 @@ func TestDeleteCustomers(t *testing.T) { // no sub or invoice delete expected custSvc.EXPECT().Delete(mock.Anything, "cust-no-provider").Return(nil) - svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) + svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) err := svc.DeleteCustomers(context.Background(), "org-1") assert.NoError(t, err) }) @@ -254,13 +257,13 @@ func TestDeleteCustomers(t *testing.T) { func TestDeleteUser(t *testing.T) { t.Run("removes user from all orgs then deletes", func(t *testing.T) { - orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) + orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) orgSvc.EXPECT().ListByUser(mock.Anything, mock.Anything, mock.Anything). Return(nil, nil) usrSvc.EXPECT().Delete(mock.Anything, "user-1").Return(nil) - svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) + svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc) err := svc.DeleteUser(context.Background(), "user-1") assert.NoError(t, err) }) diff --git a/core/group/service.go b/core/group/service.go index 70a4f1bc1..446b5acf4 100644 --- a/core/group/service.go +++ b/core/group/service.go @@ -209,39 +209,26 @@ func (s Service) Disable(ctx context.Context, id string) error { return s.repository.SetState(ctx, id, Disabled) } -func (s Service) Delete(ctx context.Context, id string) error { +// DeleteModel removes the group entity and its Object-side SpiceDB relations. +// Membership cleanup (policies + member/owner relations) is the caller's +// responsibility — see core/deleter.DeleteGroup for the orchestration that +// pairs this with membership.RemoveAllGroupMembers. +func (s Service) DeleteModel(ctx context.Context, id string) error { group, err := s.repository.GetByID(ctx, id) if err != nil { return err } - // remove all users from group - policies, err := s.policyService.List(ctx, policy.Filter{ - GroupID: id, - }) - if err != nil { - return err - } - userIDs := make([]string, 0) - for _, pol := range policies { - if pol.PrincipalType == schema.UserPrincipal || pol.PrincipalType == schema.ServiceUserPrincipal { - userIDs = append(userIDs, pol.PrincipalID) - } - } - if err := s.removeUsers(ctx, group, userIDs); err != nil { - return fmt.Errorf("failed to remove users: %w", err) - } - - // delete pending relations + // delete Object-side relations on the group (org<->group hierarchy and + // any other tuples where this group is the object) if err = s.relationService.Delete(ctx, relation.Relation{Object: relation.Object{ ID: id, Namespace: schema.GroupPrincipal, - }}); err != nil { + }}); err != nil && !errors.Is(err, relation.ErrNotExist) { return err } - err = s.repository.Delete(ctx, id) - if err != nil { + if err := s.repository.Delete(ctx, id); err != nil { return err } diff --git a/core/membership/service.go b/core/membership/service.go index 5cd0bee9e..c3119518b 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -1193,6 +1193,44 @@ func (s *Service) RemoveGroupMember(ctx context.Context, groupID, principalID, p return nil } +// RemoveAllGroupMembers tears down membership for a group that is being +// destroyed: deletes every policy on the group and every owner/member +// relation per principal. No min-owner check — the group itself is going +// away, so the invariant doesn't apply. Errors are joined; partial failures +// are logged so a retry can complete the cleanup. +func (s *Service) RemoveAllGroupMembers(ctx context.Context, groupID string) error { + policies, err := s.policyService.List(ctx, policy.Filter{GroupID: groupID}) + if err != nil { + return fmt.Errorf("list group policies: %w", err) + } + + // principals get one relation-cleanup pass each, even if they hold + // multiple policies on the group. + seen := make(map[string]struct{}, len(policies)) + var errs error + for _, p := range policies { + if delErr := s.policyService.Delete(ctx, p.ID); delErr != nil { + errs = errors.Join(errs, fmt.Errorf("delete policy %s: %w", p.ID, delErr)) + continue + } + key := p.PrincipalType + "\x00" + p.PrincipalID + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} + if relErr := s.removeRelations(ctx, groupID, schema.GroupNamespace, p.PrincipalID, p.PrincipalType); relErr != nil { + errs = errors.Join(errs, fmt.Errorf("remove relations for %s:%s: %w", p.PrincipalType, p.PrincipalID, relErr)) + } + } + if errs != nil { + s.log.ErrorContext(ctx, "partial failure cleaning up group members during group deletion; retry may be required", + "group_id", groupID, + "error", errs, + ) + } + return errs +} + // OnGroupCreated wires up SpiceDB relations for a newly-created group: // links the group to its parent organization (both directions) and adds the // creator as owner via SetGroupMemberRole. If the owner add fails, hierarchy diff --git a/core/membership/service_test.go b/core/membership/service_test.go index 4a99ce85f..c0b1bd661 100644 --- a/core/membership/service_test.go +++ b/core/membership/service_test.go @@ -1663,3 +1663,65 @@ func TestService_RemoveGroupMember(t *testing.T) { }) } } + +func TestService_RemoveAllGroupMembers(t *testing.T) { + ctx := context.Background() + groupID := uuid.New().String() + userA := uuid.New().String() + userB := uuid.New().String() + + relFor := func(name, principalID string) relation.Relation { + return relation.Relation{ + Object: relation.Object{ID: groupID, Namespace: schema.GroupNamespace}, + Subject: relation.Subject{ID: principalID, Namespace: schema.UserPrincipal}, + RelationName: name, + } + } + + t.Run("removes policies and per-principal relations, dedupes principals across policies", func(t *testing.T) { + policySvc := mocks.NewPolicyService(t) + relSvc := mocks.NewRelationService(t) + + policySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID}).Return([]policy.Policy{ + {ID: "p1", PrincipalID: userA, PrincipalType: schema.UserPrincipal}, + {ID: "p2", PrincipalID: userA, PrincipalType: schema.UserPrincipal}, + {ID: "p3", PrincipalID: userB, PrincipalType: schema.UserPrincipal}, + }, nil) + policySvc.EXPECT().Delete(ctx, "p1").Return(nil) + policySvc.EXPECT().Delete(ctx, "p2").Return(nil) + policySvc.EXPECT().Delete(ctx, "p3").Return(nil) + relSvc.EXPECT().Delete(ctx, relFor(schema.OwnerRelationName, userA)).Return(relation.ErrNotExist) + relSvc.EXPECT().Delete(ctx, relFor(schema.MemberRelationName, userA)).Return(nil) + relSvc.EXPECT().Delete(ctx, relFor(schema.OwnerRelationName, userB)).Return(nil) + relSvc.EXPECT().Delete(ctx, relFor(schema.MemberRelationName, userB)).Return(relation.ErrNotExist) + + svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), policySvc, relSvc, + mocks.NewRoleService(t), mocks.NewOrgService(t), mocks.NewUserService(t), + mocks.NewProjectService(t), mocks.NewGroupService(t), mocks.NewServiceuserService(t), + mocks.NewAuditRecordRepository(t)) + + assert.NoError(t, svc.RemoveAllGroupMembers(ctx, groupID)) + }) + + t.Run("joins errors when a policy delete fails", func(t *testing.T) { + policySvc := mocks.NewPolicyService(t) + relSvc := mocks.NewRelationService(t) + + policySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID}).Return([]policy.Policy{ + {ID: "p1", PrincipalID: userA, PrincipalType: schema.UserPrincipal}, + {ID: "p2", PrincipalID: userB, PrincipalType: schema.UserPrincipal}, + }, nil) + policySvc.EXPECT().Delete(ctx, "p1").Return(errors.New("db down")) + policySvc.EXPECT().Delete(ctx, "p2").Return(nil) + relSvc.EXPECT().Delete(ctx, relFor(schema.OwnerRelationName, userB)).Return(nil) + relSvc.EXPECT().Delete(ctx, relFor(schema.MemberRelationName, userB)).Return(nil) + + svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), policySvc, relSvc, + mocks.NewRoleService(t), mocks.NewOrgService(t), mocks.NewUserService(t), + mocks.NewProjectService(t), mocks.NewGroupService(t), mocks.NewServiceuserService(t), + mocks.NewAuditRecordRepository(t)) + + err := svc.RemoveAllGroupMembers(ctx, groupID) + assert.ErrorContains(t, err, "db down") + }) +} diff --git a/internal/api/v1beta1connect/group.go b/internal/api/v1beta1connect/group.go index 9a1b04944..1ff1b2e28 100644 --- a/internal/api/v1beta1connect/group.go +++ b/internal/api/v1beta1connect/group.go @@ -562,12 +562,12 @@ func (h *ConnectHandler) DeleteGroup(ctx context.Context, request *connect.Reque return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError) } } - if err := h.groupService.Delete(ctx, request.Msg.GetId()); err != nil { + if err := h.deleterService.DeleteGroup(ctx, request.Msg.GetId()); err != nil { switch { case errors.Is(err, group.ErrNotExist): return nil, connect.NewError(connect.CodeNotFound, ErrGroupNotFound) default: - errorLogger.LogServiceError(ctx, request, "DeleteGroup.Delete", err, + errorLogger.LogServiceError(ctx, request, "DeleteGroup.DeleteGroup", err, "group_id", request.Msg.GetId()) return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError) } diff --git a/internal/api/v1beta1connect/group_test.go b/internal/api/v1beta1connect/group_test.go index 2524e2e40..1b4457d58 100644 --- a/internal/api/v1beta1connect/group_test.go +++ b/internal/api/v1beta1connect/group_test.go @@ -1598,14 +1598,14 @@ func TestConnectHandler_DeleteGroup(t *testing.T) { randomID := utils.NewString() tests := []struct { name string - setup func(gs *mocks.GroupService, os *mocks.OrganizationService) + setup func(ds *mocks.CascadeDeleter, os *mocks.OrganizationService) request *connect.Request[frontierv1beta1.DeleteGroupRequest] want *connect.Response[frontierv1beta1.DeleteGroupResponse] wantErr error }{ { name: "should return error if organization does not exist", - setup: func(gs *mocks.GroupService, os *mocks.OrganizationService) { + setup: func(_ *mocks.CascadeDeleter, os *mocks.OrganizationService) { os.EXPECT().Get(mock.Anything, testOrgID).Return(organization.Organization{}, organization.ErrNotExist) }, request: connect.NewRequest(&frontierv1beta1.DeleteGroupRequest{ @@ -1617,7 +1617,7 @@ func TestConnectHandler_DeleteGroup(t *testing.T) { }, { name: "should return error if organization is disabled", - setup: func(gs *mocks.GroupService, os *mocks.OrganizationService) { + setup: func(_ *mocks.CascadeDeleter, os *mocks.OrganizationService) { os.EXPECT().Get(mock.Anything, testOrgID).Return(organization.Organization{}, organization.ErrDisabled) }, request: connect.NewRequest(&frontierv1beta1.DeleteGroupRequest{ @@ -1629,9 +1629,9 @@ func TestConnectHandler_DeleteGroup(t *testing.T) { }, { name: "should return error if group does not exist", - setup: func(gs *mocks.GroupService, os *mocks.OrganizationService) { + setup: func(ds *mocks.CascadeDeleter, os *mocks.OrganizationService) { os.EXPECT().Get(mock.Anything, testOrgID).Return(organization.Organization{ID: testOrgID}, nil) - gs.EXPECT().Delete(mock.Anything, randomID).Return(group.ErrNotExist) + ds.EXPECT().DeleteGroup(mock.Anything, randomID).Return(group.ErrNotExist) }, request: connect.NewRequest(&frontierv1beta1.DeleteGroupRequest{ Id: randomID, @@ -1642,9 +1642,9 @@ func TestConnectHandler_DeleteGroup(t *testing.T) { }, { name: "should delete group successfully", - setup: func(gs *mocks.GroupService, os *mocks.OrganizationService) { + setup: func(ds *mocks.CascadeDeleter, os *mocks.OrganizationService) { os.EXPECT().Get(mock.Anything, testOrgID).Return(organization.Organization{ID: testOrgID}, nil) - gs.EXPECT().Delete(mock.Anything, randomID).Return(nil) + ds.EXPECT().DeleteGroup(mock.Anything, randomID).Return(nil) }, request: connect.NewRequest(&frontierv1beta1.DeleteGroupRequest{ Id: randomID, @@ -1657,14 +1657,14 @@ func TestConnectHandler_DeleteGroup(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - mockGroupSvc := new(mocks.GroupService) + mockDeleterSvc := new(mocks.CascadeDeleter) mockOrgSvc := new(mocks.OrganizationService) if tt.setup != nil { - tt.setup(mockGroupSvc, mockOrgSvc) + tt.setup(mockDeleterSvc, mockOrgSvc) } h := ConnectHandler{ - groupService: mockGroupSvc, - orgService: mockOrgSvc, + deleterService: mockDeleterSvc, + orgService: mockOrgSvc, } got, err := h.DeleteGroup(context.Background(), tt.request) if tt.wantErr != nil { diff --git a/internal/api/v1beta1connect/interfaces.go b/internal/api/v1beta1connect/interfaces.go index 1e60224b7..077569cb4 100644 --- a/internal/api/v1beta1connect/interfaces.go +++ b/internal/api/v1beta1connect/interfaces.go @@ -306,7 +306,6 @@ type GroupService interface { RemoveUsers(ctx context.Context, groupID string, userID []string) error Enable(ctx context.Context, id string) error Disable(ctx context.Context, id string) error - Delete(ctx context.Context, id string) error } type EventService interface { @@ -388,6 +387,7 @@ type NamespaceService interface { type CascadeDeleter interface { DeleteProject(ctx context.Context, id string) error DeleteOrganization(ctx context.Context, id string) error + DeleteGroup(ctx context.Context, id string) error RemoveUsersFromOrg(ctx context.Context, orgID string, userIDs []string) error DeleteUser(ctx context.Context, userID string) error } diff --git a/internal/api/v1beta1connect/mocks/cascade_deleter.go b/internal/api/v1beta1connect/mocks/cascade_deleter.go index 82acb6e70..cb3c272db 100644 --- a/internal/api/v1beta1connect/mocks/cascade_deleter.go +++ b/internal/api/v1beta1connect/mocks/cascade_deleter.go @@ -21,6 +21,53 @@ func (_m *CascadeDeleter) EXPECT() *CascadeDeleter_Expecter { return &CascadeDeleter_Expecter{mock: &_m.Mock} } +// DeleteGroup provides a mock function with given fields: ctx, id +func (_m *CascadeDeleter) DeleteGroup(ctx context.Context, id string) error { + ret := _m.Called(ctx, id) + + if len(ret) == 0 { + panic("no return value specified for DeleteGroup") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string) error); ok { + r0 = rf(ctx, id) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// CascadeDeleter_DeleteGroup_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'DeleteGroup' +type CascadeDeleter_DeleteGroup_Call struct { + *mock.Call +} + +// DeleteGroup is a helper method to define mock.On call +// - ctx context.Context +// - id string +func (_e *CascadeDeleter_Expecter) DeleteGroup(ctx interface{}, id interface{}) *CascadeDeleter_DeleteGroup_Call { + return &CascadeDeleter_DeleteGroup_Call{Call: _e.mock.On("DeleteGroup", ctx, id)} +} + +func (_c *CascadeDeleter_DeleteGroup_Call) Run(run func(ctx context.Context, id string)) *CascadeDeleter_DeleteGroup_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string)) + }) + return _c +} + +func (_c *CascadeDeleter_DeleteGroup_Call) Return(_a0 error) *CascadeDeleter_DeleteGroup_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *CascadeDeleter_DeleteGroup_Call) RunAndReturn(run func(context.Context, string) error) *CascadeDeleter_DeleteGroup_Call { + _c.Call.Return(run) + return _c +} + // DeleteOrganization provides a mock function with given fields: ctx, id func (_m *CascadeDeleter) DeleteOrganization(ctx context.Context, id string) error { ret := _m.Called(ctx, id) diff --git a/internal/api/v1beta1connect/mocks/group_service.go b/internal/api/v1beta1connect/mocks/group_service.go index 65885111d..2046ea845 100644 --- a/internal/api/v1beta1connect/mocks/group_service.go +++ b/internal/api/v1beta1connect/mocks/group_service.go @@ -82,53 +82,6 @@ func (_c *GroupService_Create_Call) RunAndReturn(run func(context.Context, group return _c } -// Delete provides a mock function with given fields: ctx, id -func (_m *GroupService) Delete(ctx context.Context, id string) error { - ret := _m.Called(ctx, id) - - if len(ret) == 0 { - panic("no return value specified for Delete") - } - - var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string) error); ok { - r0 = rf(ctx, id) - } else { - r0 = ret.Error(0) - } - - return r0 -} - -// GroupService_Delete_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Delete' -type GroupService_Delete_Call struct { - *mock.Call -} - -// Delete is a helper method to define mock.On call -// - ctx context.Context -// - id string -func (_e *GroupService_Expecter) Delete(ctx interface{}, id interface{}) *GroupService_Delete_Call { - return &GroupService_Delete_Call{Call: _e.mock.On("Delete", ctx, id)} -} - -func (_c *GroupService_Delete_Call) Run(run func(ctx context.Context, id string)) *GroupService_Delete_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context), args[1].(string)) - }) - return _c -} - -func (_c *GroupService_Delete_Call) Return(_a0 error) *GroupService_Delete_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *GroupService_Delete_Call) RunAndReturn(run func(context.Context, string) error) *GroupService_Delete_Call { - _c.Call.Return(run) - return _c -} - // Disable provides a mock function with given fields: ctx, id func (_m *GroupService) Disable(ctx context.Context, id string) error { ret := _m.Called(ctx, id) From e96fa88eb8c090d34a05798eac9fd73ce5579fd3 Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Fri, 15 May 2026 08:40:41 +0530 Subject: [PATCH 2/6] fix(group): also clean up org<->group hierarchy on delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DeleteModel only clears Object-side relations on the group, which misses the inverse hierarchy tuple created in OnGroupCreated: organization#member@group#member (Object is the org, Subject is the group — pre-existing leak in the old group.Service.Delete; surfaced during this refactor's audit.) Adds membership.OnGroupDeleted as the symmetric counterpart to OnGroupCreated: bundles RemoveAllGroupMembers + unlinkGroupFromOrg so both hierarchy directions plus member policies/relations are cleaned up in one call. deleter.DeleteGroup now invokes it before DeleteModel. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/deleter/mocks/membership_service.go | 22 +++++----- core/deleter/service.go | 10 ++--- core/deleter/service_test.go | 4 +- core/membership/service.go | 23 +++++++++++ core/membership/service_test.go | 52 ++++++++++++++++++++++++ 5 files changed, 93 insertions(+), 18 deletions(-) diff --git a/core/deleter/mocks/membership_service.go b/core/deleter/mocks/membership_service.go index a23d0fe95..9275835e7 100644 --- a/core/deleter/mocks/membership_service.go +++ b/core/deleter/mocks/membership_service.go @@ -21,12 +21,12 @@ func (_m *MembershipService) EXPECT() *MembershipService_Expecter { return &MembershipService_Expecter{mock: &_m.Mock} } -// RemoveAllGroupMembers provides a mock function with given fields: ctx, groupID -func (_m *MembershipService) RemoveAllGroupMembers(ctx context.Context, groupID string) error { +// OnGroupDeleted provides a mock function with given fields: ctx, groupID +func (_m *MembershipService) OnGroupDeleted(ctx context.Context, groupID string) error { ret := _m.Called(ctx, groupID) if len(ret) == 0 { - panic("no return value specified for RemoveAllGroupMembers") + panic("no return value specified for OnGroupDeleted") } var r0 error @@ -39,31 +39,31 @@ func (_m *MembershipService) RemoveAllGroupMembers(ctx context.Context, groupID return r0 } -// MembershipService_RemoveAllGroupMembers_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RemoveAllGroupMembers' -type MembershipService_RemoveAllGroupMembers_Call struct { +// MembershipService_OnGroupDeleted_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'OnGroupDeleted' +type MembershipService_OnGroupDeleted_Call struct { *mock.Call } -// RemoveAllGroupMembers is a helper method to define mock.On call +// OnGroupDeleted is a helper method to define mock.On call // - ctx context.Context // - groupID string -func (_e *MembershipService_Expecter) RemoveAllGroupMembers(ctx interface{}, groupID interface{}) *MembershipService_RemoveAllGroupMembers_Call { - return &MembershipService_RemoveAllGroupMembers_Call{Call: _e.mock.On("RemoveAllGroupMembers", ctx, groupID)} +func (_e *MembershipService_Expecter) OnGroupDeleted(ctx interface{}, groupID interface{}) *MembershipService_OnGroupDeleted_Call { + return &MembershipService_OnGroupDeleted_Call{Call: _e.mock.On("OnGroupDeleted", ctx, groupID)} } -func (_c *MembershipService_RemoveAllGroupMembers_Call) Run(run func(ctx context.Context, groupID string)) *MembershipService_RemoveAllGroupMembers_Call { +func (_c *MembershipService_OnGroupDeleted_Call) Run(run func(ctx context.Context, groupID string)) *MembershipService_OnGroupDeleted_Call { _c.Call.Run(func(args mock.Arguments) { run(args[0].(context.Context), args[1].(string)) }) return _c } -func (_c *MembershipService_RemoveAllGroupMembers_Call) Return(_a0 error) *MembershipService_RemoveAllGroupMembers_Call { +func (_c *MembershipService_OnGroupDeleted_Call) Return(_a0 error) *MembershipService_OnGroupDeleted_Call { _c.Call.Return(_a0) return _c } -func (_c *MembershipService_RemoveAllGroupMembers_Call) RunAndReturn(run func(context.Context, string) error) *MembershipService_RemoveAllGroupMembers_Call { +func (_c *MembershipService_OnGroupDeleted_Call) RunAndReturn(run func(context.Context, string) error) *MembershipService_OnGroupDeleted_Call { _c.Call.Return(run) return _c } diff --git a/core/deleter/service.go b/core/deleter/service.go index e5176e9d9..0ccaeefeb 100644 --- a/core/deleter/service.go +++ b/core/deleter/service.go @@ -70,7 +70,7 @@ type GroupService interface { } type MembershipService interface { - RemoveAllGroupMembers(ctx context.Context, groupID string) error + OnGroupDeleted(ctx context.Context, groupID string) error } type InvitationService interface { @@ -173,11 +173,11 @@ func (d Service) DeleteProject(ctx context.Context, id string) error { } // DeleteGroup orchestrates teardown of a single group: clears every member's -// policies and SpiceDB relations via membership, then deletes the group -// entity itself (which clears the org<->group hierarchy relations). +// policies and relations plus the org<->group hierarchy relations via +// membership, then deletes the group entity itself. func (d Service) DeleteGroup(ctx context.Context, id string) error { - if err := d.membershipService.RemoveAllGroupMembers(ctx, id); err != nil { - return fmt.Errorf("remove group members: %w", err) + if err := d.membershipService.OnGroupDeleted(ctx, id); err != nil { + return fmt.Errorf("clean up group membership: %w", err) } return d.groupService.DeleteModel(ctx, id) } diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index b62321d33..19da0b94f 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -135,10 +135,10 @@ func TestDeleteOrganization(t *testing.T) { Return([]resource.Resource{}, nil) projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1").Return(nil) - // groups (DeleteGroup: RemoveAllGroupMembers then DeleteModel) + // groups (DeleteGroup: OnGroupDeleted then DeleteModel) grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1"}). Return([]group.Group{{ID: "grp-1", Name: "g1"}}, nil) - mbrSvc.EXPECT().RemoveAllGroupMembers(mock.Anything, "grp-1").Return(nil) + mbrSvc.EXPECT().OnGroupDeleted(mock.Anything, "grp-1").Return(nil) grpSvc.EXPECT().DeleteModel(mock.Anything, "grp-1").Return(nil) // service users diff --git a/core/membership/service.go b/core/membership/service.go index c3119518b..acb028ee0 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -1231,6 +1231,29 @@ func (s *Service) RemoveAllGroupMembers(ctx context.Context, groupID string) err return errs } +// OnGroupDeleted tears down all SpiceDB state created in OnGroupCreated: +// per-principal policies and member/owner relations, plus the two +// org<->group hierarchy relations. The group entity itself is left for the +// caller (group.Service.DeleteModel) to remove. +// +// Errors are joined; partial failures are logged so a retry can complete +// the cleanup. +func (s *Service) OnGroupDeleted(ctx context.Context, groupID string) error { + grp, err := s.groupService.Get(ctx, groupID) + if err != nil { + return err + } + + var errs error + if err := s.RemoveAllGroupMembers(ctx, groupID); err != nil { + errs = errors.Join(errs, fmt.Errorf("remove group members: %w", err)) + } + if err := s.unlinkGroupFromOrg(ctx, groupID, grp.OrganizationID); err != nil { + errs = errors.Join(errs, fmt.Errorf("unlink group from org: %w", err)) + } + return errs +} + // OnGroupCreated wires up SpiceDB relations for a newly-created group: // links the group to its parent organization (both directions) and adds the // creator as owner via SetGroupMemberRole. If the owner add fails, hierarchy diff --git a/core/membership/service_test.go b/core/membership/service_test.go index c0b1bd661..10926558e 100644 --- a/core/membership/service_test.go +++ b/core/membership/service_test.go @@ -1725,3 +1725,55 @@ func TestService_RemoveAllGroupMembers(t *testing.T) { assert.ErrorContains(t, err, "db down") }) } + +func TestService_OnGroupDeleted(t *testing.T) { + ctx := context.Background() + orgID := uuid.New().String() + groupID := uuid.New().String() + grp := group.Group{ID: groupID, OrganizationID: orgID, Title: "T"} + + t.Run("removes members then unlinks group from org", func(t *testing.T) { + policySvc := mocks.NewPolicyService(t) + relSvc := mocks.NewRelationService(t) + grpSvc := mocks.NewGroupService(t) + + grpSvc.EXPECT().Get(ctx, groupID).Return(grp, nil) + policySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID}).Return([]policy.Policy{}, nil) + + // unlinkGroupFromOrg: both hierarchy relations + relSvc.EXPECT().Delete(ctx, relation.Relation{ + Object: relation.Object{ID: groupID, Namespace: schema.GroupNamespace}, + Subject: relation.Subject{ID: orgID, Namespace: schema.OrganizationNamespace}, + RelationName: schema.OrganizationRelationName, + }).Return(nil) + relSvc.EXPECT().Delete(ctx, relation.Relation{ + Object: relation.Object{ID: orgID, Namespace: schema.OrganizationNamespace}, + Subject: relation.Subject{ + ID: groupID, + Namespace: schema.GroupNamespace, + SubRelationName: schema.MemberRelationName, + }, + RelationName: schema.MemberRelationName, + }).Return(nil) + + svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), policySvc, relSvc, + mocks.NewRoleService(t), mocks.NewOrgService(t), mocks.NewUserService(t), + mocks.NewProjectService(t), grpSvc, mocks.NewServiceuserService(t), + mocks.NewAuditRecordRepository(t)) + + assert.NoError(t, svc.OnGroupDeleted(ctx, groupID)) + }) + + t.Run("returns error if group not found", func(t *testing.T) { + grpSvc := mocks.NewGroupService(t) + grpSvc.EXPECT().Get(ctx, groupID).Return(group.Group{}, group.ErrNotExist) + + svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), + mocks.NewPolicyService(t), mocks.NewRelationService(t), + mocks.NewRoleService(t), mocks.NewOrgService(t), mocks.NewUserService(t), + mocks.NewProjectService(t), grpSvc, mocks.NewServiceuserService(t), + mocks.NewAuditRecordRepository(t)) + + assert.ErrorIs(t, svc.OnGroupDeleted(ctx, groupID), group.ErrNotExist) + }) +} From 9d086f72004536740f2f6b19b391b0e88d13ef6a Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Fri, 15 May 2026 09:02:52 +0530 Subject: [PATCH 3/6] fix(group): clean up group-as-principal policies on delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OnGroupDeleted previously only deleted policies where the group was the *resource* (memberships into the group). Policies where the group is the *principal* on some other resource — e.g. CreatePolicy granting a group access to a project — were leaked when the group was destroyed, leaving orphaned bindings. Adds removeGroupAsPrincipalPolicies which queries policies by principal and deletes them, called from OnGroupDeleted alongside the existing member + hierarchy cleanup. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/membership/service.go | 35 +++++++++++++++++++++++++++++---- core/membership/service_test.go | 9 ++++++++- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/core/membership/service.go b/core/membership/service.go index acb028ee0..9bef3934f 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -1231,10 +1231,12 @@ func (s *Service) RemoveAllGroupMembers(ctx context.Context, groupID string) err return errs } -// OnGroupDeleted tears down all SpiceDB state created in OnGroupCreated: -// per-principal policies and member/owner relations, plus the two -// org<->group hierarchy relations. The group entity itself is left for the -// caller (group.Service.DeleteModel) to remove. +// OnGroupDeleted tears down all SpiceDB state created during the group's +// lifetime: per-member policies and owner/member relations, policies where +// the group itself is the principal on other resources (e.g. group granted +// a role on a project), and the two org<->group hierarchy relations. The +// group entity itself is left for the caller (group.Service.DeleteModel) +// to remove. // // Errors are joined; partial failures are logged so a retry can complete // the cleanup. @@ -1248,12 +1250,37 @@ func (s *Service) OnGroupDeleted(ctx context.Context, groupID string) error { if err := s.RemoveAllGroupMembers(ctx, groupID); err != nil { errs = errors.Join(errs, fmt.Errorf("remove group members: %w", err)) } + if err := s.removeGroupAsPrincipalPolicies(ctx, groupID); err != nil { + errs = errors.Join(errs, fmt.Errorf("remove group-as-principal policies: %w", err)) + } if err := s.unlinkGroupFromOrg(ctx, groupID, grp.OrganizationID); err != nil { errs = errors.Join(errs, fmt.Errorf("unlink group from org: %w", err)) } return errs } +// removeGroupAsPrincipalPolicies deletes every policy where the given group +// is the principal — e.g. policies created by `CreatePolicy(principal=group:X, +// resource=project:Y, role=viewer)` that grant the group access to other +// resources. policyService.Delete is expected to also remove the matching +// rolebinding relation in SpiceDB. +func (s *Service) removeGroupAsPrincipalPolicies(ctx context.Context, groupID string) error { + policies, err := s.policyService.List(ctx, policy.Filter{ + PrincipalType: schema.GroupPrincipal, + PrincipalID: groupID, + }) + if err != nil { + return fmt.Errorf("list group-as-principal policies: %w", err) + } + var errs error + for _, p := range policies { + if delErr := s.policyService.Delete(ctx, p.ID); delErr != nil { + errs = errors.Join(errs, fmt.Errorf("delete policy %s: %w", p.ID, delErr)) + } + } + return errs +} + // OnGroupCreated wires up SpiceDB relations for a newly-created group: // links the group to its parent organization (both directions) and adds the // creator as owner via SetGroupMemberRole. If the owner add fails, hierarchy diff --git a/core/membership/service_test.go b/core/membership/service_test.go index 10926558e..02e7d404d 100644 --- a/core/membership/service_test.go +++ b/core/membership/service_test.go @@ -1732,13 +1732,20 @@ func TestService_OnGroupDeleted(t *testing.T) { groupID := uuid.New().String() grp := group.Group{ID: groupID, OrganizationID: orgID, Title: "T"} - t.Run("removes members then unlinks group from org", func(t *testing.T) { + t.Run("removes members, group-as-principal policies, and unlinks from org", func(t *testing.T) { policySvc := mocks.NewPolicyService(t) relSvc := mocks.NewRelationService(t) grpSvc := mocks.NewGroupService(t) grpSvc.EXPECT().Get(ctx, groupID).Return(grp, nil) + // RemoveAllGroupMembers — no member policies policySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID}).Return([]policy.Policy{}, nil) + // removeGroupAsPrincipalPolicies — one policy granting this group access elsewhere + policySvc.EXPECT().List(ctx, policy.Filter{ + PrincipalType: schema.GroupPrincipal, + PrincipalID: groupID, + }).Return([]policy.Policy{{ID: "principal-p1"}}, nil) + policySvc.EXPECT().Delete(ctx, "principal-p1").Return(nil) // unlinkGroupFromOrg: both hierarchy relations relSvc.EXPECT().Delete(ctx, relation.Relation{ From 645783ef6bd7956580bb239c8b6c4b5b5915321e Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Fri, 15 May 2026 09:34:57 +0530 Subject: [PATCH 4/6] fix(group): address CodeRabbit feedback on #1615 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - core/group: use schema.GroupNamespace (semantic constant for the group-as-resource Object) instead of schema.GroupPrincipal in DeleteModel's wildcard relation delete. Both values are the same string today, but GroupNamespace is the correct constant for this call site. - core/membership.RemoveAllGroupMembers: split into two passes. First pass attempts every policy delete and tracks principals with any failure. Second pass clears direct relations only for principals whose policies were all deleted. This avoids leaving a surviving policy without its matching SpiceDB tuples when a per-policy delete fails mid-loop — the previous code could strip relations after one policy succeeded for a principal even though a later policy delete for the same principal failed. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/group/service.go | 2 +- core/membership/service.go | 23 ++++++++++++++++------- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/core/group/service.go b/core/group/service.go index 446b5acf4..113a8b2ec 100644 --- a/core/group/service.go +++ b/core/group/service.go @@ -223,7 +223,7 @@ func (s Service) DeleteModel(ctx context.Context, id string) error { // any other tuples where this group is the object) if err = s.relationService.Delete(ctx, relation.Relation{Object: relation.Object{ ID: id, - Namespace: schema.GroupPrincipal, + Namespace: schema.GroupNamespace, }}); err != nil && !errors.Is(err, relation.ErrNotExist) { return err } diff --git a/core/membership/service.go b/core/membership/service.go index 9bef3934f..5028d0edb 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -1204,24 +1204,33 @@ func (s *Service) RemoveAllGroupMembers(ctx context.Context, groupID string) err return fmt.Errorf("list group policies: %w", err) } - // principals get one relation-cleanup pass each, even if they hold - // multiple policies on the group. - seen := make(map[string]struct{}, len(policies)) + // First pass: delete every policy. Track which principals had any + // delete failure so we don't strip their SpiceDB relations while a + // surviving policy still references them. + principals := make(map[string]policy.Policy, len(policies)) + failed := make(map[string]struct{}, len(policies)) var errs error for _, p := range policies { + key := p.PrincipalType + "\x00" + p.PrincipalID + principals[key] = p if delErr := s.policyService.Delete(ctx, p.ID); delErr != nil { + failed[key] = struct{}{} errs = errors.Join(errs, fmt.Errorf("delete policy %s: %w", p.ID, delErr)) - continue } - key := p.PrincipalType + "\x00" + p.PrincipalID - if _, ok := seen[key]; ok { + } + + // Second pass: clean up direct relations only for principals whose + // policies were all deleted successfully. The rest get retried on the + // next attempt once their lingering policies are removed. + for key, p := range principals { + if _, hadFailure := failed[key]; hadFailure { continue } - seen[key] = struct{}{} if relErr := s.removeRelations(ctx, groupID, schema.GroupNamespace, p.PrincipalID, p.PrincipalType); relErr != nil { errs = errors.Join(errs, fmt.Errorf("remove relations for %s:%s: %w", p.PrincipalType, p.PrincipalID, relErr)) } } + if errs != nil { s.log.ErrorContext(ctx, "partial failure cleaning up group members during group deletion; retry may be required", "group_id", groupID, From ae9422f9f28077ffe44741c527505024c5f5764c Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Fri, 15 May 2026 09:46:37 +0530 Subject: [PATCH 5/6] refactor(group): move relation cleanup fully into membership package Wildcard Object-side relation sweep now lives in membership.OnGroupDeleted alongside the policy/member/hierarchy cleanup. group.DeleteModel is reduced to entity-only cleanup (repo delete + audit log) so all SpiceDB writes are owned by a single package. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/group/service.go | 15 +++------------ core/membership/service.go | 8 ++++++++ core/membership/service_test.go | 4 ++++ 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/core/group/service.go b/core/group/service.go index 113a8b2ec..bf71334ff 100644 --- a/core/group/service.go +++ b/core/group/service.go @@ -209,25 +209,16 @@ func (s Service) Disable(ctx context.Context, id string) error { return s.repository.SetState(ctx, id, Disabled) } -// DeleteModel removes the group entity and its Object-side SpiceDB relations. -// Membership cleanup (policies + member/owner relations) is the caller's +// DeleteModel removes the group entity from storage and emits the audit +// event. All SpiceDB policy and relation cleanup is the caller's // responsibility — see core/deleter.DeleteGroup for the orchestration that -// pairs this with membership.RemoveAllGroupMembers. +// pairs this with membership.OnGroupDeleted. func (s Service) DeleteModel(ctx context.Context, id string) error { group, err := s.repository.GetByID(ctx, id) if err != nil { return err } - // delete Object-side relations on the group (org<->group hierarchy and - // any other tuples where this group is the object) - if err = s.relationService.Delete(ctx, relation.Relation{Object: relation.Object{ - ID: id, - Namespace: schema.GroupNamespace, - }}); err != nil && !errors.Is(err, relation.ErrNotExist) { - return err - } - if err := s.repository.Delete(ctx, id); err != nil { return err } diff --git a/core/membership/service.go b/core/membership/service.go index 5028d0edb..f41deba37 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -1265,6 +1265,14 @@ func (s *Service) OnGroupDeleted(ctx context.Context, groupID string) error { if err := s.unlinkGroupFromOrg(ctx, groupID, grp.OrganizationID); err != nil { errs = errors.Join(errs, fmt.Errorf("unlink group from org: %w", err)) } + // Defensive sweep: wildcard-delete any tuple where the group is the Object + // that didn't get cleaned above (e.g. legacy direct relations created + // without a matching policy). + if err := s.relationService.Delete(ctx, relation.Relation{ + Object: relation.Object{ID: groupID, Namespace: schema.GroupNamespace}, + }); err != nil && !errors.Is(err, relation.ErrNotExist) { + errs = errors.Join(errs, fmt.Errorf("sweep group object-side relations: %w", err)) + } return errs } diff --git a/core/membership/service_test.go b/core/membership/service_test.go index 02e7d404d..d7ea117c6 100644 --- a/core/membership/service_test.go +++ b/core/membership/service_test.go @@ -1762,6 +1762,10 @@ func TestService_OnGroupDeleted(t *testing.T) { }, RelationName: schema.MemberRelationName, }).Return(nil) + // defensive sweep: wildcard delete of any remaining Object-side tuples + relSvc.EXPECT().Delete(ctx, relation.Relation{ + Object: relation.Object{ID: groupID, Namespace: schema.GroupNamespace}, + }).Return(nil) svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), policySvc, relSvc, mocks.NewRoleService(t), mocks.NewOrgService(t), mocks.NewUserService(t), From 2b30c2a98e0361a1bd7873d784b9c3aa6534381b Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Fri, 15 May 2026 09:58:14 +0530 Subject: [PATCH 6/6] refactor(group): drop wildcard relation sweep from OnGroupDeleted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sweep was a workaround for a leak in policy.Delete (resource-side grant tuple not cleaned up — tracked in #1617). It doesn't belong in the membership package: every relation and policy created during a group's lifetime is now matched 1:1 with an explicit delete via the same service primitive. The leak will be addressed at policy.Delete. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/membership/service.go | 8 -------- core/membership/service_test.go | 4 ---- 2 files changed, 12 deletions(-) diff --git a/core/membership/service.go b/core/membership/service.go index f41deba37..5028d0edb 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -1265,14 +1265,6 @@ func (s *Service) OnGroupDeleted(ctx context.Context, groupID string) error { if err := s.unlinkGroupFromOrg(ctx, groupID, grp.OrganizationID); err != nil { errs = errors.Join(errs, fmt.Errorf("unlink group from org: %w", err)) } - // Defensive sweep: wildcard-delete any tuple where the group is the Object - // that didn't get cleaned above (e.g. legacy direct relations created - // without a matching policy). - if err := s.relationService.Delete(ctx, relation.Relation{ - Object: relation.Object{ID: groupID, Namespace: schema.GroupNamespace}, - }); err != nil && !errors.Is(err, relation.ErrNotExist) { - errs = errors.Join(errs, fmt.Errorf("sweep group object-side relations: %w", err)) - } return errs } diff --git a/core/membership/service_test.go b/core/membership/service_test.go index d7ea117c6..02e7d404d 100644 --- a/core/membership/service_test.go +++ b/core/membership/service_test.go @@ -1762,10 +1762,6 @@ func TestService_OnGroupDeleted(t *testing.T) { }, RelationName: schema.MemberRelationName, }).Return(nil) - // defensive sweep: wildcard delete of any remaining Object-side tuples - relSvc.EXPECT().Delete(ctx, relation.Relation{ - Object: relation.Object{ID: groupID, Namespace: schema.GroupNamespace}, - }).Return(nil) svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), policySvc, relSvc, mocks.NewRoleService(t), mocks.NewOrgService(t), mocks.NewUserService(t),