From 56bcc6a5754769fb4cb808681dda45c20de7889f Mon Sep 17 00:00:00 2001 From: aman Date: Mon, 26 Oct 2026 20:55:06 +0530 Subject: [PATCH 1/3] feat(deleter): user delete checks for blockers before deleting anything CheckUserDelete lists the orgs where the user is the only member, which are deleted with the user, the orgs the user leaves, what blocks the delete, and the user's PAT count. DeleteUser runs the same check first, with invoices read from the billing provider, and refuses with every blocker before changing anything. Otherwise it deletes the orgs where the user is the only member and removes the user from the rest. A blocked delete returns FAILED_PRECONDITION with one violation per blocker. --- core/deleter/deleter.go | 51 ++- core/deleter/mocks/user_pat_service.go | 57 +++ core/deleter/mocks/user_service.go | 59 +++ core/deleter/service.go | 129 +++++- core/deleter/service_test.go | 391 ++++++++++++++++-- core/user/mocks/repository.go | 59 +++ core/user/service.go | 4 + core/user/user.go | 1 + core/userpat/service.go | 8 + core/userpat/service_test.go | 24 ++ internal/api/v1beta1connect/deleter.go | 4 +- internal/api/v1beta1connect/user.go | 5 + internal/api/v1beta1connect/user_test.go | 51 +++ internal/store/postgres/user_repository.go | 16 +- .../store/postgres/user_repository_test.go | 25 ++ 15 files changed, 846 insertions(+), 38 deletions(-) diff --git a/core/deleter/deleter.go b/core/deleter/deleter.go index 62f895fdc8..bba0f10510 100644 --- a/core/deleter/deleter.go +++ b/core/deleter/deleter.go @@ -1,6 +1,11 @@ package deleter -import "strings" +import ( + "slices" + "strings" + + "github.com/raystack/frontier/core/organization" +) // Reasons a delete can be blocked. The API error carries // them as violation types, so a client can tell the reasons apart @@ -10,6 +15,7 @@ const ( BlockerUnpaidInvoice = "UNPAID_INVOICE" BlockerNegativeTokenBalance = "NEGATIVE_TOKEN_BALANCE" BlockerResourcesExist = "RESOURCES_EXIST" + BlockerSoleOwnerWithMembers = "SOLE_OWNER_WITH_MEMBERS" ) // Kinds of entity a Blocker's Subject id refers to. @@ -20,15 +26,15 @@ const ( SubjectResourceNamespace = "resource_namespace" ) -// Blocker is one reason a project or an organization cannot be deleted right now. +// Blocker is one reason a project, an organization or a user cannot be deleted right now. type Blocker struct { // Type is one of the Blocker* constants. Type string // Subject is the id of the entity behind the reason, or the namespace // for BlockerResourcesExist. Subject string - // SubjectType is one of the Subject* constants and says what kind of - // entity the Subject id refers to. + // SubjectType is one of the Subject* constants, or the organization + // namespace, and says what kind of entity the Subject id refers to. SubjectType string // Count is the number of blocking resources, for BlockerResourcesExist. Count int @@ -37,12 +43,46 @@ type Blocker struct { Message string } +type UserDeleteCheck struct { + Blockers []Blocker + OrganizationsToDelete []OrganizationToDelete + OrganizationsToLeave []OrganizationToLeave + PATCount int +} + +type OrganizationToDelete struct { + Organization organization.Organization + Blockers []Blocker +} + +type OrganizationToLeave struct { + Organization organization.Organization + RoleTitle string + Blockers []Blocker +} + +func (c UserDeleteCheck) CanDelete() bool { + return len(c.allBlockers()) == 0 +} + +func (c UserDeleteCheck) allBlockers() []Blocker { + blockers := slices.Clone(c.Blockers) + for _, o := range c.OrganizationsToDelete { + blockers = append(blockers, o.Blockers...) + } + for _, o := range c.OrganizationsToLeave { + blockers = append(blockers, o.Blockers...) + } + return blockers +} + // BlockedError carries every blocker the up-front check found, so the // caller gets one checklist instead of discovering blockers one retry at // a time. type BlockedError struct { OrgID string ProjectID string + UserID string Blockers []Blocker } @@ -51,6 +91,9 @@ func (e *BlockedError) Error() string { if e.ProjectID != "" { target = "project" } + if e.UserID != "" { + target = "user" + } var msgs []string for _, b := range e.Blockers { if b.Message != "" { diff --git a/core/deleter/mocks/user_pat_service.go b/core/deleter/mocks/user_pat_service.go index 8b618153d3..5a1e882b1f 100644 --- a/core/deleter/mocks/user_pat_service.go +++ b/core/deleter/mocks/user_pat_service.go @@ -21,6 +21,63 @@ func (_m *UserPATService) EXPECT() *UserPATService_Expecter { return &UserPATService_Expecter{mock: &_m.Mock} } +// CountByUser provides a mock function with given fields: ctx, userID +func (_m *UserPATService) CountByUser(ctx context.Context, userID string) (int, error) { + ret := _m.Called(ctx, userID) + + if len(ret) == 0 { + panic("no return value specified for CountByUser") + } + + var r0 int + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, string) (int, error)); ok { + return rf(ctx, userID) + } + if rf, ok := ret.Get(0).(func(context.Context, string) int); ok { + r0 = rf(ctx, userID) + } else { + r0 = ret.Get(0).(int) + } + + if rf, ok := ret.Get(1).(func(context.Context, string) error); ok { + r1 = rf(ctx, userID) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// UserPATService_CountByUser_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'CountByUser' +type UserPATService_CountByUser_Call struct { + *mock.Call +} + +// CountByUser is a helper method to define mock.On call +// - ctx context.Context +// - userID string +func (_e *UserPATService_Expecter) CountByUser(ctx interface{}, userID interface{}) *UserPATService_CountByUser_Call { + return &UserPATService_CountByUser_Call{Call: _e.mock.On("CountByUser", ctx, userID)} +} + +func (_c *UserPATService_CountByUser_Call) Run(run func(ctx context.Context, userID string)) *UserPATService_CountByUser_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string)) + }) + return _c +} + +func (_c *UserPATService_CountByUser_Call) Return(_a0 int, _a1 error) *UserPATService_CountByUser_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *UserPATService_CountByUser_Call) RunAndReturn(run func(context.Context, string) (int, error)) *UserPATService_CountByUser_Call { + _c.Call.Return(run) + return _c +} + // DeleteAllByOrg provides a mock function with given fields: ctx, orgID func (_m *UserPATService) DeleteAllByOrg(ctx context.Context, orgID string) error { ret := _m.Called(ctx, orgID) diff --git a/core/deleter/mocks/user_service.go b/core/deleter/mocks/user_service.go index b9e8ad97f8..ae963ab1e4 100644 --- a/core/deleter/mocks/user_service.go +++ b/core/deleter/mocks/user_service.go @@ -128,6 +128,65 @@ func (_c *UserService_GetByIDs_Call) RunAndReturn(run func(context.Context, []st return _c } +// GetByIDsIncludingDisabled provides a mock function with given fields: ctx, userIDs +func (_m *UserService) GetByIDsIncludingDisabled(ctx context.Context, userIDs []string) ([]user.User, error) { + ret := _m.Called(ctx, userIDs) + + if len(ret) == 0 { + panic("no return value specified for GetByIDsIncludingDisabled") + } + + var r0 []user.User + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, []string) ([]user.User, error)); ok { + return rf(ctx, userIDs) + } + if rf, ok := ret.Get(0).(func(context.Context, []string) []user.User); ok { + r0 = rf(ctx, userIDs) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]user.User) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, []string) error); ok { + r1 = rf(ctx, userIDs) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// UserService_GetByIDsIncludingDisabled_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetByIDsIncludingDisabled' +type UserService_GetByIDsIncludingDisabled_Call struct { + *mock.Call +} + +// GetByIDsIncludingDisabled is a helper method to define mock.On call +// - ctx context.Context +// - userIDs []string +func (_e *UserService_Expecter) GetByIDsIncludingDisabled(ctx interface{}, userIDs interface{}) *UserService_GetByIDsIncludingDisabled_Call { + return &UserService_GetByIDsIncludingDisabled_Call{Call: _e.mock.On("GetByIDsIncludingDisabled", ctx, userIDs)} +} + +func (_c *UserService_GetByIDsIncludingDisabled_Call) Run(run func(ctx context.Context, userIDs []string)) *UserService_GetByIDsIncludingDisabled_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].([]string)) + }) + return _c +} + +func (_c *UserService_GetByIDsIncludingDisabled_Call) Return(_a0 []user.User, _a1 error) *UserService_GetByIDsIncludingDisabled_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *UserService_GetByIDsIncludingDisabled_Call) RunAndReturn(run func(context.Context, []string) ([]user.User, error)) *UserService_GetByIDsIncludingDisabled_Call { + _c.Call.Return(run) + return _c +} + // NewUserService creates a new instance of UserService. 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 NewUserService(t interface { diff --git a/core/deleter/service.go b/core/deleter/service.go index 4068f4c827..0e904065d9 100644 --- a/core/deleter/service.go +++ b/core/deleter/service.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "log/slog" + "slices" "github.com/raystack/frontier/core/audit" "github.com/raystack/frontier/core/auditrecord" @@ -94,12 +95,14 @@ type InvitationService interface { type UserService interface { GetByIDs(ctx context.Context, userIDs []string) ([]user.User, error) + GetByIDsIncludingDisabled(ctx context.Context, userIDs []string) ([]user.User, error) Delete(ctx context.Context, id string) error } type UserPATService interface { DeleteAllByUser(ctx context.Context, userID string) error DeleteAllByOrg(ctx context.Context, orgID string) error + CountByUser(ctx context.Context, userID string) (int, error) } type DomainService interface { @@ -554,19 +557,127 @@ func (d Service) RemoveUsersFromOrg(ctx context.Context, orgID string, userIDs [ return errs } -// DeleteUser visits every org the user has a policy on (disabled orgs too), -// otherwise userService.Delete would leave orphan policy rows behind. -func (d Service) DeleteUser(ctx context.Context, userID string) error { +func (d Service) CheckUserDelete(ctx context.Context, userID string) (UserDeleteCheck, error) { + check, err := d.checkUser(ctx, userID, false) + if err != nil { + return UserDeleteCheck{}, err + } + if check.PATCount, err = d.userPATService.CountByUser(ctx, userID); err != nil { + return UserDeleteCheck{}, err + } + return check, nil +} + +func (d Service) checkUser(ctx context.Context, userID string, fromProvider bool) (UserDeleteCheck, error) { orgIDs, err := d.membershipService.ListResourcesByPrincipal(ctx, authenticate.Principal{ ID: userID, Type: schema.UserPrincipal, }, schema.OrganizationNamespace, membership.ResourceFilter{}) if err != nil { - return err + return UserDeleteCheck{}, err } + var check UserDeleteCheck for _, orgID := range orgIDs { - if err = d.RemoveUsersFromOrg(ctx, orgID, []string{userID}); err != nil { - return fmt.Errorf("failed to delete user from org[%s]: %w", orgID, err) + if err := d.checkUserOrg(ctx, userID, orgID, fromProvider, &check); err != nil { + return UserDeleteCheck{}, err + } + } + return check, nil +} + +func (d Service) checkUserOrg(ctx context.Context, userID, orgID string, fromProvider bool, check *UserDeleteCheck) error { + org, err := d.orgService.GetRaw(ctx, orgID) + if errors.Is(err, organization.ErrNotExist) { + return nil + } + if err != nil { + return err + } + members, err := d.existingOrgUsers(ctx, orgID) + if err != nil { + return err + } + + var self *membership.Member + var others, otherOwners int + for i, m := range members { + if m.PrincipalID == userID { + self = &members[i] + continue + } + others++ + if isOrgOwner(m) { + otherOwners++ + } + } + + switch { + case self == nil: + return nil + case others == 0: + blockers, err := d.orgDeleteBlockers(ctx, orgID, fromProvider) + if err != nil { + return err + } + check.OrganizationsToDelete = append(check.OrganizationsToDelete, OrganizationToDelete{Organization: org, Blockers: blockers}) + default: + toLeave := OrganizationToLeave{Organization: org} + if len(self.Roles) > 0 { + toLeave.RoleTitle = self.Roles[0].Title + } + if isOrgOwner(*self) && otherOwners == 0 { + toLeave.Blockers = []Blocker{{ + Type: BlockerSoleOwnerWithMembers, + Subject: org.ID, + SubjectType: schema.OrganizationNamespace, + Message: fmt.Sprintf("organization[%s] has other members and this user is its only owner: make another member an owner or remove the other members, then retry the delete", org.Name), + }} + } + check.OrganizationsToLeave = append(check.OrganizationsToLeave, toLeave) + } + return nil +} + +func (d Service) existingOrgUsers(ctx context.Context, orgID string) ([]membership.Member, error) { + members, err := d.membershipService.ListPrincipalsByResource(ctx, orgID, schema.OrganizationNamespace, membership.MemberFilter{ + PrincipalType: schema.UserPrincipal, + }) + if err != nil { + return nil, err + } + users, err := d.userService.GetByIDsIncludingDisabled(ctx, utils.Map(members, func(m membership.Member) string { return m.PrincipalID })) + if err != nil { + return nil, err + } + existing := make(map[string]bool, len(users)) + for _, u := range users { + existing[u.ID] = true + } + return utils.Filter(members, func(m membership.Member) bool { return existing[m.PrincipalID] }), nil +} + +func isOrgOwner(m membership.Member) bool { + return slices.ContainsFunc(m.Roles, func(r role.Role) bool { return r.Name == schema.RoleOrganizationOwner }) +} + +// DeleteUser visits every org the user has a policy on (disabled orgs too), +// otherwise userService.Delete would leave orphan policy rows behind. +func (d Service) DeleteUser(ctx context.Context, userID string) error { + check, err := d.checkUser(ctx, userID, true) + if err != nil { + return err + } + if blockers := check.allBlockers(); len(blockers) > 0 { + return &BlockedError{UserID: userID, Blockers: blockers} + } + for _, o := range check.OrganizationsToDelete { + if err := d.DeleteOrganization(ctx, o.Organization.ID); err != nil && !errors.Is(err, organization.ErrNotExist) { + return fmt.Errorf("failed to delete org[%s]: %w", o.Organization.ID, err) + } + } + for _, o := range check.OrganizationsToLeave { + if err := d.RemoveUsersFromOrg(ctx, o.Organization.ID, []string{userID}); err != nil { + return fmt.Errorf("failed to delete user from org[%s]: %w", o.Organization.ID, err) } } if err := d.membershipService.RemovePrincipalPolicies(ctx, userID, schema.UserPrincipal); err != nil { @@ -711,13 +822,17 @@ func (d Service) CheckOrganizationDelete(ctx context.Context, id string) ([]Bloc if _, err := d.orgService.GetRaw(ctx, id); err != nil { return nil, err } + return d.orgDeleteBlockers(ctx, id, false) +} + +func (d Service) orgDeleteBlockers(ctx context.Context, id string, fromProvider bool) ([]Blocker, error) { customers, err := d.customerService.List(ctx, customer.Filter{ OrgID: id, }) if err != nil { return nil, err } - return d.collectBlockers(ctx, id, customers, false, nil, map[string]bool{}) + return d.collectBlockers(ctx, id, customers, fromProvider, nil, map[string]bool{}) } // collectBlockers gathers the blockers across the org's billing accounts: diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index 0c84d85404..0320cd472c 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -1827,63 +1827,408 @@ func TestRemoveUsersFromOrg(t *testing.T) { }) } +var ( + orgOwner = role.Role{Name: schema.RoleOrganizationOwner, Title: "Owner"} + orgViewer = role.Role{Name: schema.RoleOrganizationViewer, Title: "Member"} +) + +func orgMember(id string, r role.Role) membership.Member { + return membership.Member{PrincipalID: id, PrincipalType: schema.UserPrincipal, Roles: []role.Role{r}} +} + +func expectUserOrgs(m deleterMocks, orgIDs ...string) { + m.mbrSvc.EXPECT().ListResourcesByPrincipal(mock.Anything, mock.Anything, schema.OrganizationNamespace, mock.Anything). + Return(orgIDs, nil) +} + +func expectUserOrg(m deleterMocks, org organization.Organization, members []membership.Member, existing ...string) { + m.orgSvc.EXPECT().GetRaw(mock.Anything, org.ID).Return(org, nil).Once() + m.mbrSvc.EXPECT().ListPrincipalsByResource(mock.Anything, org.ID, schema.OrganizationNamespace, + membership.MemberFilter{PrincipalType: schema.UserPrincipal}).Return(members, nil) + ids := make([]string, 0, len(members)) + for _, mb := range members { + ids = append(ids, mb.PrincipalID) + } + users := make([]user.User, 0, len(existing)) + for _, id := range existing { + users = append(users, user.User{ID: id}) + } + m.usrSvc.EXPECT().GetByIDsIncludingDisabled(mock.Anything, ids).Return(users, nil) +} + +func expectNoBillingAccounts(m deleterMocks, orgID string) { + m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: orgID}).Return(nil, nil) +} + +func soleOwnerBlocker(org organization.Organization) deleter.Blocker { + return deleter.Blocker{ + Type: deleter.BlockerSoleOwnerWithMembers, + Subject: org.ID, + SubjectType: schema.OrganizationNamespace, + Message: "organization[" + org.Name + "] has other members and this user is its only owner: make another member an owner or remove the other members, then retry the delete", + } +} + +func expectEmptyOrgDelete(m deleterMocks, steps *[]string) { + m.orgSvc.EXPECT().GetRaw(mock.Anything, "org-1").Return(organization.Organization{ID: "org-1"}, nil).Once() + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}).Return(nil, nil) + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{OrgID: "org-1"}).Return(nil, nil) + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}).Return(nil, nil) + expectEmptyOrgTail(m, steps) +} + func TestDeleteUser(t *testing.T) { - t.Run("removes the user from each org, then every policy it still holds, then its PATs, then the user", func(t *testing.T) { + const userID = "user-1" + solo := organization.Organization{ID: "org-1", Name: "solo", State: organization.Disabled} + shared := organization.Organization{ID: "org-2", Name: "shared"} + + t.Run("deletes the orgs that go with the user, leaves the others, then removes every policy it still holds, its PATs, and the user", func(t *testing.T) { m := newMocks(t) var steps []string - m.mbrSvc.EXPECT().ListResourcesByPrincipal(mock.Anything, mock.Anything, schema.OrganizationNamespace, mock.Anything). - Return([]string{"org-1", "org-2"}, nil) - m.mbrSvc.EXPECT().ForceRemoveOrganizationMember(mock.Anything, mock.Anything, "user-1", schema.UserPrincipal). - Run(func(_ context.Context, orgID, _, _ string) { steps = append(steps, "org:"+orgID) }).Return(nil) - m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, "user-1", schema.UserPrincipal). + expectUserOrgs(m, solo.ID, shared.ID) + expectUserOrg(m, solo, []membership.Member{orgMember(userID, orgOwner)}, userID) + expectNoBillingAccounts(m, solo.ID) + expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgViewer), orgMember("user-2", orgOwner)}, userID, "user-2") + expectEmptyOrgDelete(m, &steps) + m.mbrSvc.EXPECT().ForceRemoveOrganizationMember(mock.Anything, shared.ID, userID, schema.UserPrincipal). + Run(func(_ context.Context, orgID, _, _ string) { steps = append(steps, "leave:"+orgID) }).Return(nil) + m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, userID, schema.UserPrincipal). + Run(func(context.Context, string, string) { steps = append(steps, "policies") }).Return(nil) + m.patSvc.EXPECT().DeleteAllByUser(mock.Anything, userID). + Run(func(context.Context, string) { steps = append(steps, "pats") }).Return(nil) + m.usrSvc.EXPECT().Delete(mock.Anything, userID). + Run(func(context.Context, string) { steps = append(steps, "user") }).Return(nil) + + err := m.build().DeleteUser(context.Background(), userID) + assert.NoError(t, err) + assert.Equal(t, []string{"org", "leave:org-2", "policies", "pats", "user"}, steps) + }) + + t.Run("refuses with every blocker, judging invoices on provider data, before changing anything", func(t *testing.T) { + m := newMocks(t) + c := customer.Customer{ID: "cust-1", OrgID: solo.ID, ProviderID: "stripe-1"} + + expectUserOrgs(m, solo.ID, shared.ID) + expectUserOrg(m, solo, []membership.Member{orgMember(userID, orgOwner)}, userID) + m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: solo.ID}).Return([]customer.Customer{c}, nil) + m.subSvc.EXPECT().List(mock.Anything, subscription.Filter{CustomerID: c.ID}).Return(nil, nil) + m.invocSvc.EXPECT().ListPayableOnProvider(mock.Anything, c). + Return([]invoice.Invoice{{ID: "inv-1", State: invoice.OpenState}}, nil) + m.creditSvc.EXPECT().GetBalance(mock.Anything, c.ID).Return(0, nil) + expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgOwner), orgMember("user-2", orgViewer)}, userID, "user-2") + + err := m.build().DeleteUser(context.Background(), userID) + assert.Equal(t, &deleter.BlockedError{ + UserID: userID, + Blockers: []deleter.Blocker{ + { + Type: deleter.BlockerUnpaidInvoice, + Subject: "inv-1", + SubjectType: deleter.SubjectInvoice, + Message: "invoice[inv-1] is unpaid: pay it via its hosted payment page, then retry the delete", + }, + soleOwnerBlocker(shared), + }, + }, err) + assert.EqualError(t, err, "user cannot be deleted yet: "+ + "invoice[inv-1] is unpaid: pay it via its hosted payment page, then retry the delete; "+ + "organization[shared] has other members and this user is its only owner: make another member an owner or remove the other members, then retry the delete") + }) + + t.Run("a failed org delete stops before the user leaves the other orgs", func(t *testing.T) { + m := newMocks(t) + + expectUserOrgs(m, solo.ID, shared.ID) + expectUserOrg(m, solo, []membership.Member{orgMember(userID, orgOwner)}, userID) + expectNoBillingAccounts(m, solo.ID) + expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgViewer), orgMember("user-2", orgOwner)}, userID, "user-2") + m.orgSvc.EXPECT().GetRaw(mock.Anything, solo.ID).Return(solo, nil).Once() + m.roleSvc.EXPECT().Get(mock.Anything, schema.RoleOrganizationOwner). + Return(role.Role{}, errors.New("no owners in this test")) + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: solo.ID, IncludeDisabled: true}). + Return(nil, errors.New("read failed")) + + err := m.build().DeleteUser(context.Background(), userID) + assert.EqualError(t, err, "failed to delete org[org-1]: read failed") + }) + + t.Run("an org deleted since the check does not stop the delete", func(t *testing.T) { + m := newMocks(t) + var steps []string + + expectUserOrgs(m, solo.ID) + expectUserOrg(m, solo, []membership.Member{orgMember(userID, orgOwner)}, userID) + expectNoBillingAccounts(m, solo.ID) + m.orgSvc.EXPECT().GetRaw(mock.Anything, solo.ID).Return(organization.Organization{}, organization.ErrNotExist).Once() + m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, userID, schema.UserPrincipal). Run(func(context.Context, string, string) { steps = append(steps, "policies") }).Return(nil) - m.patSvc.EXPECT().DeleteAllByUser(mock.Anything, "user-1"). + m.patSvc.EXPECT().DeleteAllByUser(mock.Anything, userID). Run(func(context.Context, string) { steps = append(steps, "pats") }).Return(nil) - m.usrSvc.EXPECT().Delete(mock.Anything, "user-1"). + m.usrSvc.EXPECT().Delete(mock.Anything, userID). Run(func(context.Context, string) { steps = append(steps, "user") }).Return(nil) - err := m.build().DeleteUser(context.Background(), "user-1") + err := m.build().DeleteUser(context.Background(), userID) assert.NoError(t, err) - assert.Equal(t, []string{"org:org-1", "org:org-2", "policies", "pats", "user"}, steps) + assert.Equal(t, []string{"policies", "pats", "user"}, steps) }) t.Run("an org removal failure stops the delete before the remaining policies", func(t *testing.T) { m := newMocks(t) - m.mbrSvc.EXPECT().ListResourcesByPrincipal(mock.Anything, mock.Anything, schema.OrganizationNamespace, mock.Anything). - Return([]string{"org-1"}, nil) - m.mbrSvc.EXPECT().ForceRemoveOrganizationMember(mock.Anything, "org-1", "user-1", schema.UserPrincipal). + expectUserOrgs(m, shared.ID) + expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgViewer), orgMember("user-2", orgOwner)}, userID, "user-2") + m.mbrSvc.EXPECT().ForceRemoveOrganizationMember(mock.Anything, shared.ID, userID, schema.UserPrincipal). Return(errors.New("cascade boom")) - err := m.build().DeleteUser(context.Background(), "user-1") + err := m.build().DeleteUser(context.Background(), userID) assert.ErrorContains(t, err, "cascade boom") }) t.Run("a policy removal failure stops the delete before the PATs and the user", func(t *testing.T) { m := newMocks(t) - m.mbrSvc.EXPECT().ListResourcesByPrincipal(mock.Anything, mock.Anything, schema.OrganizationNamespace, mock.Anything). - Return(nil, nil) - m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, "user-1", schema.UserPrincipal). + expectUserOrgs(m) + m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, userID, schema.UserPrincipal). Return(errors.New("spicedb unavailable")) - err := m.build().DeleteUser(context.Background(), "user-1") + err := m.build().DeleteUser(context.Background(), userID) assert.ErrorContains(t, err, "spicedb unavailable") }) t.Run("aborts before userService.Delete when PAT cleanup fails", func(t *testing.T) { m := newMocks(t) - m.mbrSvc.EXPECT().ListResourcesByPrincipal(mock.Anything, mock.Anything, schema.OrganizationNamespace, mock.Anything). - Return(nil, nil) - m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, "user-1", schema.UserPrincipal).Return(nil) - m.patSvc.EXPECT().DeleteAllByUser(mock.Anything, "user-1").Return(errors.New("pat cleanup boom")) + expectUserOrgs(m) + m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, userID, schema.UserPrincipal).Return(nil) + m.patSvc.EXPECT().DeleteAllByUser(mock.Anything, userID).Return(errors.New("pat cleanup boom")) // usrSvc.Delete must NOT be called — strict mock fails on unexpected call. - err := m.build().DeleteUser(context.Background(), "user-1") + err := m.build().DeleteUser(context.Background(), userID) assert.ErrorContains(t, err, "pat cleanup boom") }) + + t.Run("a failed check stops the delete before anything changes", func(t *testing.T) { + m := newMocks(t) + + m.mbrSvc.EXPECT().ListResourcesByPrincipal(mock.Anything, mock.Anything, schema.OrganizationNamespace, mock.Anything). + Return(nil, errors.New("read failed")) + + err := m.build().DeleteUser(context.Background(), userID) + assert.EqualError(t, err, "read failed") + }) +} + +func TestCheckUserDelete(t *testing.T) { + const userID = "user-1" + + t.Run("sorts each org by who else is in it", func(t *testing.T) { + m := newMocks(t) + solo := organization.Organization{ID: "org-solo", Name: "solo", State: organization.Disabled} + shared := organization.Organization{ID: "org-shared", Name: "shared"} + viewing := organization.Organization{ID: "org-viewing", Name: "viewing"} + blocked := organization.Organization{ID: "org-blocked", Name: "blocked"} + + expectUserOrgs(m, solo.ID, shared.ID, viewing.ID, blocked.ID) + expectUserOrg(m, solo, []membership.Member{orgMember(userID, orgOwner)}, userID) + expectNoBillingAccounts(m, solo.ID) + expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgOwner), orgMember("user-2", orgOwner)}, userID, "user-2") + expectUserOrg(m, viewing, []membership.Member{orgMember(userID, orgViewer), orgMember("user-3", orgOwner)}, userID, "user-3") + expectUserOrg(m, blocked, []membership.Member{orgMember(userID, orgOwner), orgMember("user-4", orgViewer)}, userID, "user-4") + m.patSvc.EXPECT().CountByUser(mock.Anything, userID).Return(3, nil) + + got, err := m.build().CheckUserDelete(context.Background(), userID) + assert.NoError(t, err) + assert.Equal(t, deleter.UserDeleteCheck{ + OrganizationsToDelete: []deleter.OrganizationToDelete{{Organization: solo}}, + OrganizationsToLeave: []deleter.OrganizationToLeave{ + {Organization: shared, RoleTitle: "Owner"}, + {Organization: viewing, RoleTitle: "Member"}, + {Organization: blocked, RoleTitle: "Owner", Blockers: []deleter.Blocker{soleOwnerBlocker(blocked)}}, + }, + PATCount: 3, + }, got) + assert.False(t, got.CanDelete()) + }) + + t.Run("a disabled member counts, a member whose user row is gone does not", func(t *testing.T) { + m := newMocks(t) + withDisabled := organization.Organization{ID: "org-with-disabled", Name: "with-disabled"} + withGhost := organization.Organization{ID: "org-with-ghost", Name: "with-ghost"} + + expectUserOrgs(m, withDisabled.ID, withGhost.ID) + expectUserOrg(m, withDisabled, []membership.Member{orgMember(userID, orgOwner), orgMember("disabled-user", orgViewer)}, userID, "disabled-user") + expectUserOrg(m, withGhost, []membership.Member{orgMember(userID, orgOwner), orgMember("ghost-user", orgViewer)}, userID) + expectNoBillingAccounts(m, withGhost.ID) + m.patSvc.EXPECT().CountByUser(mock.Anything, userID).Return(0, nil) + + got, err := m.build().CheckUserDelete(context.Background(), userID) + assert.NoError(t, err) + assert.Empty(t, got.Blockers) + assert.Equal(t, []deleter.OrganizationToLeave{ + {Organization: withDisabled, RoleTitle: "Owner", Blockers: []deleter.Blocker{soleOwnerBlocker(withDisabled)}}, + }, got.OrganizationsToLeave) + assert.Equal(t, []deleter.OrganizationToDelete{{Organization: withGhost}}, got.OrganizationsToDelete) + }) + + t.Run("reports the blockers of an org that goes with the user", func(t *testing.T) { + m := newMocks(t) + solo := organization.Organization{ID: "org-solo", Name: "solo"} + + expectUserOrgs(m, solo.ID) + expectUserOrg(m, solo, []membership.Member{orgMember(userID, orgOwner)}, userID) + m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: solo.ID}). + Return([]customer.Customer{{ID: "cust-1", OrgID: solo.ID}}, nil) + m.creditSvc.EXPECT().GetBalance(mock.Anything, "cust-1").Return(-5, nil) + m.patSvc.EXPECT().CountByUser(mock.Anything, userID).Return(0, nil) + + got, err := m.build().CheckUserDelete(context.Background(), userID) + assert.NoError(t, err) + assert.Equal(t, []deleter.OrganizationToDelete{{ + Organization: solo, + Blockers: []deleter.Blocker{{ + Type: deleter.BlockerNegativeTokenBalance, + Subject: "cust-1", + SubjectType: deleter.SubjectBillingAccount, + Message: "billing account[cust-1] owes 5 tokens: contact support to settle the balance, then retry the delete", + }}, + }}, got.OrganizationsToDelete) + assert.Empty(t, got.Blockers) + assert.False(t, got.CanDelete()) + }) + + t.Run("judges the invoices of an org that goes with the user on the local rows", func(t *testing.T) { + m := newMocks(t) + solo := organization.Organization{ID: "org-solo", Name: "solo"} + c := customer.Customer{ID: "cust-1", OrgID: solo.ID, ProviderID: "stripe-1"} + + expectUserOrgs(m, solo.ID) + expectUserOrg(m, solo, []membership.Member{orgMember(userID, orgOwner)}, userID) + m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: solo.ID}).Return([]customer.Customer{c}, nil) + m.subSvc.EXPECT().List(mock.Anything, subscription.Filter{CustomerID: c.ID}).Return(nil, nil) + m.invocSvc.EXPECT().List(mock.Anything, invoice.Filter{CustomerID: c.ID, NonZeroOnly: true}). + Return([]invoice.Invoice{{ID: "inv-1", State: invoice.OpenState}}, nil) + m.creditSvc.EXPECT().GetBalance(mock.Anything, c.ID).Return(0, nil) + m.patSvc.EXPECT().CountByUser(mock.Anything, userID).Return(0, nil) + + got, err := m.build().CheckUserDelete(context.Background(), userID) + assert.NoError(t, err) + assert.Equal(t, []deleter.OrganizationToDelete{{ + Organization: solo, + Blockers: []deleter.Blocker{{ + Type: deleter.BlockerUnpaidInvoice, + Subject: "inv-1", + SubjectType: deleter.SubjectInvoice, + Message: "invoice[inv-1] is unpaid: pay it via its hosted payment page, then retry the delete", + }}, + }}, got.OrganizationsToDelete) + }) + + t.Run("can delete when the user only leaves orgs that keep an owner", func(t *testing.T) { + m := newMocks(t) + shared := organization.Organization{ID: "org-shared", Name: "shared"} + + expectUserOrgs(m, shared.ID) + expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgViewer), orgMember("user-2", orgOwner)}, userID, "user-2") + m.patSvc.EXPECT().CountByUser(mock.Anything, userID).Return(1, nil) + + got, err := m.build().CheckUserDelete(context.Background(), userID) + assert.NoError(t, err) + assert.True(t, got.CanDelete()) + }) + + t.Run("skips an org that no longer exists", func(t *testing.T) { + m := newMocks(t) + expectUserOrgs(m, "org-gone") + m.orgSvc.EXPECT().GetRaw(mock.Anything, "org-gone").Return(organization.Organization{}, organization.ErrNotExist) + m.patSvc.EXPECT().CountByUser(mock.Anything, userID).Return(0, nil) + + got, err := m.build().CheckUserDelete(context.Background(), userID) + assert.NoError(t, err) + assert.Equal(t, deleter.UserDeleteCheck{}, got) + }) + + failures := []struct { + name string + setup func(m deleterMocks) + }{ + { + name: "the org list", + setup: func(m deleterMocks) { + m.mbrSvc.EXPECT().ListResourcesByPrincipal(mock.Anything, mock.Anything, schema.OrganizationNamespace, mock.Anything). + Return(nil, errors.New("read failed")) + }, + }, + { + name: "the member list", + setup: func(m deleterMocks) { + expectUserOrgs(m, "org-1") + m.orgSvc.EXPECT().GetRaw(mock.Anything, "org-1").Return(organization.Organization{ID: "org-1"}, nil) + m.mbrSvc.EXPECT().ListPrincipalsByResource(mock.Anything, "org-1", schema.OrganizationNamespace, mock.Anything). + Return(nil, errors.New("read failed")) + }, + }, + { + name: "the user lookup", + setup: func(m deleterMocks) { + expectUserOrgs(m, "org-1") + m.orgSvc.EXPECT().GetRaw(mock.Anything, "org-1").Return(organization.Organization{ID: "org-1"}, nil) + m.mbrSvc.EXPECT().ListPrincipalsByResource(mock.Anything, "org-1", schema.OrganizationNamespace, mock.Anything). + Return([]membership.Member{orgMember(userID, orgOwner)}, nil) + m.usrSvc.EXPECT().GetByIDsIncludingDisabled(mock.Anything, mock.Anything).Return(nil, errors.New("read failed")) + }, + }, + { + name: "the PAT count", + setup: func(m deleterMocks) { + expectUserOrgs(m) + m.patSvc.EXPECT().CountByUser(mock.Anything, userID).Return(0, errors.New("read failed")) + }, + }, + } + for _, tc := range failures { + t.Run("a failed read of "+tc.name+" stops the check", func(t *testing.T) { + m := newMocks(t) + tc.setup(m) + _, err := m.build().CheckUserDelete(context.Background(), userID) + assert.ErrorContains(t, err, "read failed") + }) + } +} + +func TestUserDeleteCheck_CanDelete(t *testing.T) { + blocker := []deleter.Blocker{{Type: deleter.BlockerSoleOwnerWithMembers}} + tests := []struct { + name string + check deleter.UserDeleteCheck + want bool + }{ + { + name: "nothing blocks", + check: deleter.UserDeleteCheck{ + OrganizationsToDelete: []deleter.OrganizationToDelete{{}}, + OrganizationsToLeave: []deleter.OrganizationToLeave{{}}, + }, + want: true, + }, + { + name: "a blocker on the user", + check: deleter.UserDeleteCheck{Blockers: blocker}, + }, + { + name: "a blocker on an org that goes with the user", + check: deleter.UserDeleteCheck{OrganizationsToDelete: []deleter.OrganizationToDelete{{}, {Blockers: blocker}}}, + }, + { + name: "a blocker on an org the user leaves", + check: deleter.UserDeleteCheck{OrganizationsToLeave: []deleter.OrganizationToLeave{{}, {Blockers: blocker}}}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, tt.check.CanDelete()) + }) + } } func blockerConfig() deleter.Config { diff --git a/core/user/mocks/repository.go b/core/user/mocks/repository.go index 803e8b2261..d8ccda6683 100644 --- a/core/user/mocks/repository.go +++ b/core/user/mocks/repository.go @@ -369,6 +369,65 @@ func (_c *Repository_GetByIDs_Call) RunAndReturn(run func(context.Context, []str return _c } +// GetByIDsIncludingDisabled provides a mock function with given fields: ctx, userIDs +func (_m *Repository) GetByIDsIncludingDisabled(ctx context.Context, userIDs []string) ([]user.User, error) { + ret := _m.Called(ctx, userIDs) + + if len(ret) == 0 { + panic("no return value specified for GetByIDsIncludingDisabled") + } + + var r0 []user.User + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, []string) ([]user.User, error)); ok { + return rf(ctx, userIDs) + } + if rf, ok := ret.Get(0).(func(context.Context, []string) []user.User); ok { + r0 = rf(ctx, userIDs) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]user.User) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, []string) error); ok { + r1 = rf(ctx, userIDs) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// Repository_GetByIDsIncludingDisabled_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetByIDsIncludingDisabled' +type Repository_GetByIDsIncludingDisabled_Call struct { + *mock.Call +} + +// GetByIDsIncludingDisabled is a helper method to define mock.On call +// - ctx context.Context +// - userIDs []string +func (_e *Repository_Expecter) GetByIDsIncludingDisabled(ctx interface{}, userIDs interface{}) *Repository_GetByIDsIncludingDisabled_Call { + return &Repository_GetByIDsIncludingDisabled_Call{Call: _e.mock.On("GetByIDsIncludingDisabled", ctx, userIDs)} +} + +func (_c *Repository_GetByIDsIncludingDisabled_Call) Run(run func(ctx context.Context, userIDs []string)) *Repository_GetByIDsIncludingDisabled_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].([]string)) + }) + return _c +} + +func (_c *Repository_GetByIDsIncludingDisabled_Call) Return(_a0 []user.User, _a1 error) *Repository_GetByIDsIncludingDisabled_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *Repository_GetByIDsIncludingDisabled_Call) RunAndReturn(run func(context.Context, []string) ([]user.User, error)) *Repository_GetByIDsIncludingDisabled_Call { + _c.Call.Return(run) + return _c +} + // GetByName provides a mock function with given fields: ctx, name func (_m *Repository) GetByName(ctx context.Context, name string) (user.User, error) { ret := _m.Called(ctx, name) diff --git a/core/user/service.go b/core/user/service.go index a88bf4cb9f..fbfa0f2881 100644 --- a/core/user/service.go +++ b/core/user/service.go @@ -77,6 +77,10 @@ func (s Service) GetByIDs(ctx context.Context, userIDs []string) ([]User, error) return s.repository.GetByIDs(ctx, userIDs) } +func (s Service) GetByIDsIncludingDisabled(ctx context.Context, userIDs []string) ([]User, error) { + return s.repository.GetByIDsIncludingDisabled(ctx, userIDs) +} + func (s Service) GetByEmail(ctx context.Context, email string) (User, error) { email = strings.ToLower(email) return s.repository.GetByEmail(ctx, email) diff --git a/core/user/user.go b/core/user/user.go index 1bb3b1dbaf..9b79408e1b 100644 --- a/core/user/user.go +++ b/core/user/user.go @@ -24,6 +24,7 @@ type Repository interface { GetByID(ctx context.Context, id string) (User, error) GetByEmail(ctx context.Context, email string) (User, error) GetByIDs(ctx context.Context, userIds []string) ([]User, error) + GetByIDsIncludingDisabled(ctx context.Context, userIDs []string) ([]User, error) GetByName(ctx context.Context, name string) (User, error) Create(ctx context.Context, user User) (User, error) CreateWithConsent(ctx context.Context, user User, cnst consent.Consent) (User, consent.Consent, error) diff --git a/core/userpat/service.go b/core/userpat/service.go index d2ba2033e2..6517236072 100644 --- a/core/userpat/service.go +++ b/core/userpat/service.go @@ -184,6 +184,14 @@ func (s *Service) DeleteAllByUser(ctx context.Context, userID string) error { return nil } +func (s *Service) CountByUser(ctx context.Context, userID string) (int, error) { + pats, err := s.repo.ListByUser(ctx, userID) + if err != nil { + return 0, fmt.Errorf("listing PATs for user: %w", err) + } + return len(pats), nil +} + func (s *Service) DeleteAllByOrg(ctx context.Context, orgID string) error { pats, err := s.repo.ListAllByOrg(ctx, orgID) if err != nil { diff --git a/core/userpat/service_test.go b/core/userpat/service_test.go index 17fdcda316..ba0a1efd97 100644 --- a/core/userpat/service_test.go +++ b/core/userpat/service_test.go @@ -1968,6 +1968,30 @@ func TestService_DeleteAllByUser(t *testing.T) { }) } +func TestService_CountByUser(t *testing.T) { + t.Run("counts the user's live PATs across orgs", func(t *testing.T) { + repo := mocks.NewRepository(t) + repo.EXPECT().ListByUser(mock.Anything, "user-1").Return([]models.PAT{ + {ID: "pat-1", UserID: "user-1", OrgID: "org-1"}, + {ID: "pat-2", UserID: "user-1", OrgID: "org-2"}, + }, nil) + svc := userpat.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), repo, defaultConfig, + mocks.NewOrganizationService(t), nil, nil, nil, mocks.NewAuditRecordRepository(t)) + count, err := svc.CountByUser(context.Background(), "user-1") + assert.NoError(t, err) + assert.Equal(t, 2, count) + }) + + t.Run("returns error when repo list fails", func(t *testing.T) { + repo := mocks.NewRepository(t) + repo.EXPECT().ListByUser(mock.Anything, "user-1").Return(nil, errors.New("db down")) + svc := userpat.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), repo, defaultConfig, + mocks.NewOrganizationService(t), nil, nil, nil, mocks.NewAuditRecordRepository(t)) + _, err := svc.CountByUser(context.Background(), "user-1") + assert.ErrorContains(t, err, "db down") + }) +} + func TestService_DeleteAllByOrg(t *testing.T) { newService := func(t *testing.T) (*userpat.Service, *mocks.Repository, *mocks.MembershipService, *mocks.AuditRecordRepository) { t.Helper() diff --git a/internal/api/v1beta1connect/deleter.go b/internal/api/v1beta1connect/deleter.go index 42ab12199c..103f744b25 100644 --- a/internal/api/v1beta1connect/deleter.go +++ b/internal/api/v1beta1connect/deleter.go @@ -101,10 +101,10 @@ func deleteBlockedError(ctx context.Context, blocked *deleter.BlockedError) *con }) } if detail, err := connect.NewErrorDetail(failure); err != nil { - slog.WarnContext(ctx, "failed to attach precondition failure details", "error", err, "org_id", blocked.OrgID, "project_id", blocked.ProjectID) + slog.WarnContext(ctx, "failed to attach precondition failure details", "error", err, "org_id", blocked.OrgID, "project_id", blocked.ProjectID, "user_id", blocked.UserID) } else { connectErr.AddDetail(detail) } - slog.WarnContext(ctx, "delete blocked", "org_id", blocked.OrgID, "project_id", blocked.ProjectID, "blockers", len(blocked.Blockers)) + slog.WarnContext(ctx, "delete blocked", "org_id", blocked.OrgID, "project_id", blocked.ProjectID, "user_id", blocked.UserID, "blockers", len(blocked.Blockers)) return connectErr } diff --git a/internal/api/v1beta1connect/user.go b/internal/api/v1beta1connect/user.go index 6d9886c215..b9eeafb214 100644 --- a/internal/api/v1beta1connect/user.go +++ b/internal/api/v1beta1connect/user.go @@ -12,6 +12,7 @@ import ( "github.com/pkg/errors" "github.com/raystack/frontier/core/audit" "github.com/raystack/frontier/core/authenticate" + "github.com/raystack/frontier/core/deleter" "github.com/raystack/frontier/core/group" "github.com/raystack/frontier/core/membership" "github.com/raystack/frontier/core/organization" @@ -412,6 +413,10 @@ func (h *ConnectHandler) DeleteUser(ctx context.Context, request *connect.Reques userID := request.Msg.GetId() if err := h.deleterService.DeleteUser(ctx, userID); err != nil { + var blocked *deleter.BlockedError + if errors.As(err, &blocked) { + return nil, deleteBlockedError(ctx, blocked) + } errorLogger.LogServiceError(ctx, request, "DeleteUser", err, "user_id", userID) diff --git a/internal/api/v1beta1connect/user_test.go b/internal/api/v1beta1connect/user_test.go index c6cf10c30a..85f5265f49 100644 --- a/internal/api/v1beta1connect/user_test.go +++ b/internal/api/v1beta1connect/user_test.go @@ -9,6 +9,7 @@ import ( "connectrpc.com/connect" "github.com/google/uuid" "github.com/raystack/frontier/core/authenticate" + "github.com/raystack/frontier/core/deleter" "github.com/raystack/frontier/core/group" "github.com/raystack/frontier/core/organization" "github.com/raystack/frontier/core/permission" @@ -24,6 +25,7 @@ import ( "github.com/raystack/frontier/pkg/utils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "google.golang.org/genproto/googleapis/rpc/errdetails" "google.golang.org/protobuf/types/known/structpb" "google.golang.org/protobuf/types/known/timestamppb" @@ -874,6 +876,20 @@ func TestConnectHandler_DeleteUser(t *testing.T) { want: nil, err: connect.CodeInternal, }, + { + title: "should return failed precondition if an org that goes with the user cannot be deleted", + setup: func(cd *mocks.CascadeDeleter) { + cd.EXPECT().DeleteUser(mock.Anything, userID).Return(fmt.Errorf("failed to delete org[org-1]: %w", &deleter.BlockedError{ + OrgID: "org-1", + Blockers: []deleter.Blocker{{Type: deleter.BlockerUnpaidInvoice, Subject: "inv-1"}}, + })) + }, + req: &frontierv1beta1.DeleteUserRequest{ + Id: userID, + }, + want: nil, + err: connect.CodeFailedPrecondition, + }, } for _, tt := range tests { @@ -902,6 +918,41 @@ func TestConnectHandler_DeleteUser(t *testing.T) { mockDeleterSrv.AssertExpectations(t) }) } + + t.Run("should return failed precondition with one violation per blocker when the delete is blocked", func(t *testing.T) { + mockDeleterSrv := new(mocks.CascadeDeleter) + mockDeleterSrv.EXPECT().DeleteUser(mock.Anything, userID).Return(&deleter.BlockedError{ + UserID: userID, + Blockers: []deleter.Blocker{{ + Type: deleter.BlockerSoleOwnerWithMembers, + Subject: "org-1", + SubjectType: schema.OrganizationNamespace, + Message: "organization[acme] has other members and this user is its only owner: make another member an owner or remove the other members, then retry the delete", + }}, + }) + handler := &ConnectHandler{deleterService: mockDeleterSrv} + + resp, err := handler.DeleteUser(context.Background(), connect.NewRequest(&frontierv1beta1.DeleteUserRequest{ + Id: userID, + })) + assert.Nil(t, resp) + + var connectErr *connect.Error + assert.ErrorAs(t, err, &connectErr) + assert.Equal(t, connect.CodeFailedPrecondition, connectErr.Code()) + assert.Equal(t, "user cannot be deleted yet: "+ + "organization[acme] has other members and this user is its only owner: make another member an owner or remove the other members, then retry the delete", + connectErr.Message()) + + assert.Len(t, connectErr.Details(), 1) + detail, detailErr := connectErr.Details()[0].Value() + assert.NoError(t, detailErr) + failure, ok := detail.(*errdetails.PreconditionFailure) + assert.True(t, ok) + assert.Len(t, failure.GetViolations(), 1) + assert.Equal(t, deleter.BlockerSoleOwnerWithMembers, failure.GetViolations()[0].GetType()) + assert.Equal(t, "org-1", failure.GetViolations()[0].GetSubject()) + }) } func TestConnectHandler_ListUserGroups(t *testing.T) { diff --git a/internal/store/postgres/user_repository.go b/internal/store/postgres/user_repository.go index 65cf4b323d..dbee719c80 100644 --- a/internal/store/postgres/user_repository.go +++ b/internal/store/postgres/user_repository.go @@ -293,15 +293,27 @@ func (r UserRepository) List(ctx context.Context, flt user.Filter) ([]user.User, } func (r UserRepository) GetByIDs(ctx context.Context, userIDs []string) ([]user.User, error) { + return r.getByIDs(ctx, userIDs, false) +} + +func (r UserRepository) GetByIDsIncludingDisabled(ctx context.Context, userIDs []string) ([]user.User, error) { + return r.getByIDs(ctx, userIDs, true) +} + +func (r UserRepository) getByIDs(ctx context.Context, userIDs []string, includeDisabled bool) ([]user.User, error) { if len(userIDs) == 0 { return []user.User{}, nil } var fetchedUsers []User - query, params, err := fromLive(TABLE_USERS).Select("id", "name", "email", "title", "avatar", "state").Where( + stmt := fromLive(TABLE_USERS).Select("id", "name", "email", "title", "avatar", "state").Where( goqu.Ex{ "id": goqu.Op{"in": userIDs}, - }).Where(notDisabledUserExp).ToSQL() + }) + if !includeDisabled { + stmt = stmt.Where(notDisabledUserExp) + } + query, params, err := stmt.ToSQL() if err != nil { return []user.User{}, fmt.Errorf("%w: %w", errQuery, err) } diff --git a/internal/store/postgres/user_repository_test.go b/internal/store/postgres/user_repository_test.go index 7838208da4..35f16b9628 100644 --- a/internal/store/postgres/user_repository_test.go +++ b/internal/store/postgres/user_repository_test.go @@ -324,6 +324,31 @@ func (s *UserRepositoryTestSuite) TestGetByIDs() { } } +func (s *UserRepositoryTestSuite) TestGetByIDsIncludingDisabled() { + enabled, disabled := s.users[0], s.users[1] + s.Require().NoError(s.repository.SetState(s.ctx, disabled.ID, user.Disabled)) + deleted, err := s.repository.Create(s.ctx, user.User{Name: "deleted_user_slug", Email: "deleted-user@raystack.org"}) + s.Require().NoError(err) + execSQL(s.T(), s.ctx, s.client, "UPDATE users SET deleted_at = now() WHERE id = $1", deleted.ID) + lookup := []string{enabled.ID, disabled.ID, deleted.ID} + + ids := func(users []user.User) []string { + out := make([]string, 0, len(users)) + for _, u := range users { + out = append(out, u.ID) + } + return out + } + + got, err := s.repository.GetByIDs(s.ctx, lookup) + s.Require().NoError(err) + s.Assert().ElementsMatch([]string{enabled.ID}, ids(got)) + + got, err = s.repository.GetByIDsIncludingDisabled(s.ctx, lookup) + s.Require().NoError(err) + s.Assert().ElementsMatch([]string{enabled.ID, disabled.ID}, ids(got)) +} + func (s *UserRepositoryTestSuite) TestUpdateByEmail() { type testCase struct { Description string From 1ecf3133e92bf53293549871e7df5c4f7c656549 Mon Sep 17 00:00:00 2001 From: aman Date: Wed, 28 Oct 2026 18:28:44 +0530 Subject: [PATCH 2/3] feat(deleter): a refused user delete says which org each blocker is from --- core/deleter/deleter.go | 16 ++++++++++++++-- core/deleter/service_test.go | 27 +++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/core/deleter/deleter.go b/core/deleter/deleter.go index bba0f10510..a53719f004 100644 --- a/core/deleter/deleter.go +++ b/core/deleter/deleter.go @@ -1,6 +1,7 @@ package deleter import ( + "fmt" "slices" "strings" @@ -39,7 +40,7 @@ type Blocker struct { // Count is the number of blocking resources, for BlockerResourcesExist. Count int // Message says what blocks the delete and how to clear it. Empty for - // BlockerResourcesExist. + // BlockerResourcesExist, except in a refused user delete. Message string } @@ -68,7 +69,9 @@ func (c UserDeleteCheck) CanDelete() bool { func (c UserDeleteCheck) allBlockers() []Blocker { blockers := slices.Clone(c.Blockers) for _, o := range c.OrganizationsToDelete { - blockers = append(blockers, o.Blockers...) + for _, b := range o.Blockers { + blockers = append(blockers, labelWithOrg(o.Organization.Name, b)) + } } for _, o := range c.OrganizationsToLeave { blockers = append(blockers, o.Blockers...) @@ -76,6 +79,15 @@ func (c UserDeleteCheck) allBlockers() []Blocker { return blockers } +func labelWithOrg(orgName string, b Blocker) Blocker { + text := b.Message + if text == "" { + text = b.Type + } + b.Message = fmt.Sprintf("organization[%s]: %s", orgName, text) + return b +} + // BlockedError carries every blocker the up-front check found, so the // caller gets one checklist instead of discovering blockers one retry at // a time. diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index 0320cd472c..ccc61e3a8e 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -1926,16 +1926,39 @@ func TestDeleteUser(t *testing.T) { Type: deleter.BlockerUnpaidInvoice, Subject: "inv-1", SubjectType: deleter.SubjectInvoice, - Message: "invoice[inv-1] is unpaid: pay it via its hosted payment page, then retry the delete", + Message: "organization[solo]: invoice[inv-1] is unpaid: pay it via its hosted payment page, then retry the delete", }, soleOwnerBlocker(shared), }, }, err) assert.EqualError(t, err, "user cannot be deleted yet: "+ - "invoice[inv-1] is unpaid: pay it via its hosted payment page, then retry the delete; "+ + "organization[solo]: invoice[inv-1] is unpaid: pay it via its hosted payment page, then retry the delete; "+ "organization[shared] has other members and this user is its only owner: make another member an owner or remove the other members, then retry the delete") }) + t.Run("labels the same blocker in two orgs that go with the user with each org's name", func(t *testing.T) { + m := newMocks(t) + m.deletion = blockerConfig() + other := organization.Organization{ID: "org-3", Name: "other"} + + expectUserOrgs(m, solo.ID, other.ID) + for _, org := range []organization.Organization{solo, other} { + expectUserOrg(m, org, []membership.Member{orgMember(userID, orgOwner)}, userID) + expectNoBillingAccounts(m, org.ID) + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{OrgID: org.ID}). + Return([]resource.Resource{{ID: "t-" + org.ID, NamespaceID: "storage/transfer"}}, nil) + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: org.ID, IncludeDisabled: true}).Return(nil, nil) + } + + err := m.build().DeleteUser(context.Background(), userID) + inSolo := resourcesExist("storage/transfer", 1) + inSolo.Message = "organization[solo]: RESOURCES_EXIST" + inOther := resourcesExist("storage/transfer", 1) + inOther.Message = "organization[other]: RESOURCES_EXIST" + assert.Equal(t, &deleter.BlockedError{UserID: userID, Blockers: []deleter.Blocker{inSolo, inOther}}, err) + assert.EqualError(t, err, "user cannot be deleted yet: organization[solo]: RESOURCES_EXIST; organization[other]: RESOURCES_EXIST") + }) + t.Run("a failed org delete stops before the user leaves the other orgs", func(t *testing.T) { m := newMocks(t) From 332e38b08f695014beed552666a61a88b3c7ec87 Mon Sep 17 00:00:00 2001 From: aman Date: Wed, 28 Oct 2026 18:29:09 +0530 Subject: [PATCH 3/3] refactor(deleter): the sole owner blocker uses an organization subject type --- core/deleter/deleter.go | 5 +++-- core/deleter/service.go | 2 +- core/deleter/service_test.go | 2 +- internal/api/v1beta1connect/user_test.go | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/core/deleter/deleter.go b/core/deleter/deleter.go index a53719f004..e63abf68e1 100644 --- a/core/deleter/deleter.go +++ b/core/deleter/deleter.go @@ -25,6 +25,7 @@ const ( SubjectInvoice = "billing_invoice" SubjectBillingAccount = "billing_account" SubjectResourceNamespace = "resource_namespace" + SubjectOrganization = "organization" ) // Blocker is one reason a project, an organization or a user cannot be deleted right now. @@ -34,8 +35,8 @@ type Blocker struct { // Subject is the id of the entity behind the reason, or the namespace // for BlockerResourcesExist. Subject string - // SubjectType is one of the Subject* constants, or the organization - // namespace, and says what kind of entity the Subject id refers to. + // SubjectType is one of the Subject* constants and says what kind of + // entity the Subject id refers to. SubjectType string // Count is the number of blocking resources, for BlockerResourcesExist. Count int diff --git a/core/deleter/service.go b/core/deleter/service.go index 0e904065d9..dd35825cca 100644 --- a/core/deleter/service.go +++ b/core/deleter/service.go @@ -629,7 +629,7 @@ func (d Service) checkUserOrg(ctx context.Context, userID, orgID string, fromPro toLeave.Blockers = []Blocker{{ Type: BlockerSoleOwnerWithMembers, Subject: org.ID, - SubjectType: schema.OrganizationNamespace, + SubjectType: SubjectOrganization, Message: fmt.Sprintf("organization[%s] has other members and this user is its only owner: make another member an owner or remove the other members, then retry the delete", org.Name), }} } diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index ccc61e3a8e..eba56fe423 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -1864,7 +1864,7 @@ func soleOwnerBlocker(org organization.Organization) deleter.Blocker { return deleter.Blocker{ Type: deleter.BlockerSoleOwnerWithMembers, Subject: org.ID, - SubjectType: schema.OrganizationNamespace, + SubjectType: deleter.SubjectOrganization, Message: "organization[" + org.Name + "] has other members and this user is its only owner: make another member an owner or remove the other members, then retry the delete", } } diff --git a/internal/api/v1beta1connect/user_test.go b/internal/api/v1beta1connect/user_test.go index 85f5265f49..407d7a276f 100644 --- a/internal/api/v1beta1connect/user_test.go +++ b/internal/api/v1beta1connect/user_test.go @@ -926,7 +926,7 @@ func TestConnectHandler_DeleteUser(t *testing.T) { Blockers: []deleter.Blocker{{ Type: deleter.BlockerSoleOwnerWithMembers, Subject: "org-1", - SubjectType: schema.OrganizationNamespace, + SubjectType: deleter.SubjectOrganization, Message: "organization[acme] has other members and this user is its only owner: make another member an owner or remove the other members, then retry the delete", }}, })