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..9275835e7 --- /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} +} + +// 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 OnGroupDeleted") + } + + 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_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 +} + +// OnGroupDeleted is a helper method to define mock.On call +// - ctx context.Context +// - groupID string +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_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_OnGroupDeleted_Call) Return(_a0 error) *MembershipService_OnGroupDeleted_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *MembershipService_OnGroupDeleted_Call) RunAndReturn(run func(context.Context, string) error) *MembershipService_OnGroupDeleted_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..0ccaeefeb 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 { + OnGroupDeleted(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 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.OnGroupDeleted(ctx, id); err != nil { + return fmt.Errorf("clean up group membership: %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..19da0b94f 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: OnGroupDeleted 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().OnGroupDeleted(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..bf71334ff 100644 --- a/core/group/service.go +++ b/core/group/service.go @@ -209,39 +209,17 @@ 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 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.OnGroupDeleted. +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 - if err = s.relationService.Delete(ctx, relation.Relation{Object: relation.Object{ - ID: id, - Namespace: schema.GroupPrincipal, - }}); err != nil { - 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..5028d0edb 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -1193,6 +1193,103 @@ 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) + } + + // 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)) + } + } + + // 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 + } + 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 +} + +// 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. +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.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 4a99ce85f..02e7d404d 100644 --- a/core/membership/service_test.go +++ b/core/membership/service_test.go @@ -1663,3 +1663,124 @@ 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") + }) +} + +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, 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{ + 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) + }) +} 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)