diff --git a/core/deleter/deleter.go b/core/deleter/deleter.go index 62f895fdc..e63abf68e 100644 --- a/core/deleter/deleter.go +++ b/core/deleter/deleter.go @@ -1,6 +1,12 @@ package deleter -import "strings" +import ( + "fmt" + "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 +16,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. @@ -18,9 +25,10 @@ const ( SubjectInvoice = "billing_invoice" SubjectBillingAccount = "billing_account" SubjectResourceNamespace = "resource_namespace" + SubjectOrganization = "organization" ) -// 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 @@ -33,16 +41,61 @@ 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 } +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 { + for _, b := range o.Blockers { + blockers = append(blockers, labelWithOrg(o.Organization.Name, b)) + } + } + for _, o := range c.OrganizationsToLeave { + blockers = append(blockers, o.Blockers...) + } + 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. type BlockedError struct { OrgID string ProjectID string + UserID string Blockers []Blocker } @@ -51,6 +104,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 8b618153d..5a1e882b1 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 b9e8ad97f..ae963ab1e 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 4068f4c82..dd35825cc 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: 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), + }} + } + 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 0c84d8540..eba56fe42 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -1827,63 +1827,431 @@ 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: 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", + } +} + +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, "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{"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: "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: "+ + "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) + + 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, 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{"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 803e8b226..d8ccda668 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 a88bf4cb9..fbfa0f288 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 1bb3b1dba..9b79408e1 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 d2ba2033e..651723607 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 17fdcda31..ba0a1efd9 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 42ab12199..103f744b2 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 6d9886c21..b9eeafb21 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 c6cf10c30..407d7a276 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: 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", + }}, + }) + 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 65cf4b323..dbee719c8 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 7838208da..35f16b962 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