From 0911163146d2894680394435c48babb1a1529421 Mon Sep 17 00:00:00 2001 From: Bethuel Mmbaga Date: Thu, 8 Aug 2024 18:01:38 +0300 Subject: [PATCH] Add batch delete for groups and users (#2370) * Refactor user deletion logic and introduce batch delete * Prevent self-deletion for users * Add delete multiple groups * Refactor group deletion with validation * Fix tests * Add bulk delete functions for Users and Groups in account manager interface and mocks * Add tests for DeleteGroups method in group management * Add tests for DeleteUsers method in users management --- management/server/account.go | 2 + management/server/group.go | 239 +++++++++++------- management/server/group_test.go | 145 ++++++++++- management/server/mock_server/account_mock.go | 18 ++ management/server/user.go | 146 ++++++++--- management/server/user_test.go | 151 +++++++++++ 6 files changed, 575 insertions(+), 126 deletions(-) diff --git a/management/server/account.go b/management/server/account.go index e99e0e7f3..ca53ebad0 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -68,6 +68,7 @@ type AccountManager interface { SaveSetupKey(ctx context.Context, accountID string, key *SetupKey, userID string) (*SetupKey, error) CreateUser(ctx context.Context, accountID, initiatorUserID string, key *UserInfo) (*UserInfo, error) DeleteUser(ctx context.Context, accountID, initiatorUserID string, targetUserID string) error + DeleteRegularUsers(ctx context.Context, accountID, initiatorUserID string, targetUserIDs []string) error InviteUser(ctx context.Context, accountID string, initiatorUserID string, targetUserID string) error ListSetupKeys(ctx context.Context, accountID, userID string) ([]*SetupKey, error) SaveUser(ctx context.Context, accountID, initiatorUserID string, update *User) (*UserInfo, error) @@ -101,6 +102,7 @@ type AccountManager interface { SaveGroup(ctx context.Context, accountID, userID string, group *nbgroup.Group) error SaveGroups(ctx context.Context, accountID, userID string, newGroups []*nbgroup.Group) error DeleteGroup(ctx context.Context, accountId, userId, groupID string) error + DeleteGroups(ctx context.Context, accountId, userId string, groupIDs []string) error ListGroups(ctx context.Context, accountId string) ([]*nbgroup.Group, error) GroupAddPeer(ctx context.Context, accountId, groupID, peerID string) error GroupDeletePeer(ctx context.Context, accountId, groupID, peerID string) error diff --git a/management/server/group.go b/management/server/group.go index 37a6fc305..49720f347 100644 --- a/management/server/group.go +++ b/management/server/group.go @@ -2,8 +2,12 @@ package server import ( "context" + "errors" "fmt" + "slices" + nbdns "github.com/netbirdio/netbird/dns" + "github.com/netbirdio/netbird/route" "github.com/rs/xid" log "github.com/sirupsen/logrus" @@ -243,7 +247,7 @@ func difference(a, b []string) []string { return diff } -// DeleteGroup object of the peers +// DeleteGroup object of the peers. func (am *DefaultAccountManager) DeleteGroup(ctx context.Context, accountId, userId, groupID string) error { unlock := am.Store.AcquireWriteLockByUID(ctx, accountId) defer unlock() @@ -253,96 +257,14 @@ func (am *DefaultAccountManager) DeleteGroup(ctx context.Context, accountId, use return err } - g, ok := account.Groups[groupID] + group, ok := account.Groups[groupID] if !ok { return nil } - // disable a deleting integration group if the initiator is not an admin service user - if g.Issued == nbgroup.GroupIssuedIntegration { - executingUser := account.Users[userId] - if executingUser == nil { - return status.Errorf(status.NotFound, "user not found") - } - if executingUser.Role != UserRoleAdmin || !executingUser.IsServiceUser { - return status.Errorf(status.PermissionDenied, "only service users with admin power can delete integration group") - } + if err = validateDeleteGroup(account, group, userId); err != nil { + return err } - - // check route links - for _, r := range account.Routes { - for _, g := range r.Groups { - if g == groupID { - return &GroupLinkError{"route", string(r.NetID)} - } - } - for _, g := range r.PeerGroups { - if g == groupID { - return &GroupLinkError{"route", string(r.NetID)} - } - } - } - - // check DNS links - for _, dns := range account.NameServerGroups { - for _, g := range dns.Groups { - if g == groupID { - return &GroupLinkError{"name server groups", dns.Name} - } - } - } - - // check ACL links - for _, policy := range account.Policies { - for _, rule := range policy.Rules { - for _, src := range rule.Sources { - if src == groupID { - return &GroupLinkError{"policy", policy.Name} - } - } - - for _, dst := range rule.Destinations { - if dst == groupID { - return &GroupLinkError{"policy", policy.Name} - } - } - } - } - - // check setup key links - for _, setupKey := range account.SetupKeys { - for _, grp := range setupKey.AutoGroups { - if grp == groupID { - return &GroupLinkError{"setup key", setupKey.Name} - } - } - } - - // check user links - for _, user := range account.Users { - for _, grp := range user.AutoGroups { - if grp == groupID { - return &GroupLinkError{"user", user.Id} - } - } - } - - // check DisabledManagementGroups - for _, disabledMgmGrp := range account.DNSSettings.DisabledManagementGroups { - if disabledMgmGrp == groupID { - return &GroupLinkError{"disabled DNS management groups", g.Name} - } - } - - // check integrated peer validator groups - if account.Settings.Extra != nil { - for _, integratedPeerValidatorGroups := range account.Settings.Extra.IntegratedValidatorGroups { - if groupID == integratedPeerValidatorGroups { - return &GroupLinkError{"integrated validator", g.Name} - } - } - } - delete(account.Groups, groupID) account.Network.IncSerial() @@ -350,13 +272,57 @@ func (am *DefaultAccountManager) DeleteGroup(ctx context.Context, accountId, use return err } - am.StoreEvent(ctx, userId, groupID, accountId, activity.GroupDeleted, g.EventMeta()) + am.StoreEvent(ctx, userId, groupID, accountId, activity.GroupDeleted, group.EventMeta()) am.updateAccountPeers(ctx, account) return nil } +// DeleteGroups deletes groups from an account. +// Note: This function does not acquire the global lock. +// It is the caller's responsibility to ensure proper locking is in place before invoking this method. +// +// If an error occurs while deleting a group, the function skips it and continues deleting other groups. +// Errors are collected and returned at the end. +func (am *DefaultAccountManager) DeleteGroups(ctx context.Context, accountId, userId string, groupIDs []string) error { + account, err := am.Store.GetAccount(ctx, accountId) + if err != nil { + return err + } + + var allErrors error + + deletedGroups := make([]*nbgroup.Group, 0, len(groupIDs)) + for _, groupID := range groupIDs { + group, ok := account.Groups[groupID] + if !ok { + continue + } + + if err := validateDeleteGroup(account, group, userId); err != nil { + allErrors = errors.Join(allErrors, fmt.Errorf("failed to delete group %s: %w", groupID, err)) + continue + } + + delete(account.Groups, groupID) + deletedGroups = append(deletedGroups, group) + } + + account.Network.IncSerial() + if err = am.Store.SaveAccount(ctx, account); err != nil { + return err + } + + for _, g := range deletedGroups { + am.StoreEvent(ctx, userId, g.ID, accountId, activity.GroupDeleted, g.EventMeta()) + } + + am.updateAccountPeers(ctx, account) + + return allErrors +} + // ListGroups objects of the peers func (am *DefaultAccountManager) ListGroups(ctx context.Context, accountID string) ([]*nbgroup.Group, error) { unlock := am.Store.AcquireWriteLockByUID(ctx, accountID) @@ -440,3 +406,102 @@ func (am *DefaultAccountManager) GroupDeletePeer(ctx context.Context, accountID, return nil } + +func validateDeleteGroup(account *Account, group *nbgroup.Group, userID string) error { + // disable a deleting integration group if the initiator is not an admin service user + if group.Issued == nbgroup.GroupIssuedIntegration { + executingUser := account.Users[userID] + if executingUser == nil { + return status.Errorf(status.NotFound, "user not found") + } + if executingUser.Role != UserRoleAdmin || !executingUser.IsServiceUser { + return status.Errorf(status.PermissionDenied, "only service users with admin power can delete integration group") + } + } + + if isLinked, linkedRoute := isGroupLinkedToRoute(account.Routes, group.ID); isLinked { + return &GroupLinkError{"route", string(linkedRoute.NetID)} + } + + if isLinked, linkedDns := isGroupLinkedToDns(account.NameServerGroups, group.ID); isLinked { + return &GroupLinkError{"name server groups", linkedDns.Name} + } + + if isLinked, linkedPolicy := isGroupLinkedToPolicy(account.Policies, group.ID); isLinked { + return &GroupLinkError{"policy", linkedPolicy.Name} + } + + if isLinked, linkedSetupKey := isGroupLinkedToSetupKey(account.SetupKeys, group.ID); isLinked { + return &GroupLinkError{"setup key", linkedSetupKey.Name} + } + + if isLinked, linkedUser := isGroupLinkedToUser(account.Users, group.ID); isLinked { + return &GroupLinkError{"user", linkedUser.Id} + } + + if slices.Contains(account.DNSSettings.DisabledManagementGroups, group.ID) { + return &GroupLinkError{"disabled DNS management groups", group.Name} + } + + if account.Settings.Extra != nil { + if slices.Contains(account.Settings.Extra.IntegratedValidatorGroups, group.ID) { + return &GroupLinkError{"integrated validator", group.Name} + } + } + + return nil +} + +// isGroupLinkedToRoute checks if a group is linked to any route in the account. +func isGroupLinkedToRoute(routes map[route.ID]*route.Route, groupID string) (bool, *route.Route) { + for _, r := range routes { + if slices.Contains(r.Groups, groupID) || slices.Contains(r.PeerGroups, groupID) { + return true, r + } + } + return false, nil +} + +// isGroupLinkedToPolicy checks if a group is linked to any policy in the account. +func isGroupLinkedToPolicy(policies []*Policy, groupID string) (bool, *Policy) { + for _, policy := range policies { + for _, rule := range policy.Rules { + if slices.Contains(rule.Sources, groupID) || slices.Contains(rule.Destinations, groupID) { + return true, policy + } + } + } + return false, nil +} + +// isGroupLinkedToDns checks if a group is linked to any nameserver group in the account. +func isGroupLinkedToDns(nameServerGroups map[string]*nbdns.NameServerGroup, groupID string) (bool, *nbdns.NameServerGroup) { + for _, dns := range nameServerGroups { + for _, g := range dns.Groups { + if g == groupID { + return true, dns + } + } + } + return false, nil +} + +// isGroupLinkedToSetupKey checks if a group is linked to any setup key in the account. +func isGroupLinkedToSetupKey(setupKeys map[string]*SetupKey, groupID string) (bool, *SetupKey) { + for _, setupKey := range setupKeys { + if slices.Contains(setupKey.AutoGroups, groupID) { + return true, setupKey + } + } + return false, nil +} + +// isGroupLinkedToUser checks if a group is linked to any user in the account. +func isGroupLinkedToUser(users map[string]*User, groupID string) (bool, *User) { + for _, user := range users { + if slices.Contains(user.AutoGroups, groupID) { + return true, user + } + } + return false, nil +} diff --git a/management/server/group_test.go b/management/server/group_test.go index 373d72964..89b68ad6c 100644 --- a/management/server/group_test.go +++ b/management/server/group_test.go @@ -3,12 +3,14 @@ package server import ( "context" "errors" + "fmt" "testing" nbdns "github.com/netbirdio/netbird/dns" nbgroup "github.com/netbirdio/netbird/management/server/group" "github.com/netbirdio/netbird/management/server/status" "github.com/netbirdio/netbird/route" + "github.com/stretchr/testify/assert" ) const ( @@ -21,7 +23,7 @@ func TestDefaultAccountManager_CreateGroup(t *testing.T) { t.Error("failed to create account manager") } - account, err := initTestGroupAccount(am) + _, account, err := initTestGroupAccount(am) if err != nil { t.Error("failed to init testing account") } @@ -56,7 +58,7 @@ func TestDefaultAccountManager_DeleteGroup(t *testing.T) { t.Error("failed to create account manager") } - account, err := initTestGroupAccount(am) + _, account, err := initTestGroupAccount(am) if err != nil { t.Error("failed to init testing account") } @@ -132,7 +134,136 @@ func TestDefaultAccountManager_DeleteGroup(t *testing.T) { } } -func initTestGroupAccount(am *DefaultAccountManager) (*Account, error) { +func TestDefaultAccountManager_DeleteGroups(t *testing.T) { + am, err := createManager(t) + assert.NoError(t, err, "Failed to create account manager") + + manager, account, err := initTestGroupAccount(am) + assert.NoError(t, err, "Failed to init testing account") + + groups := make([]*nbgroup.Group, 10) + for i := 0; i < 10; i++ { + groups[i] = &nbgroup.Group{ + ID: fmt.Sprintf("group-%d", i+1), + AccountID: account.Id, + Name: fmt.Sprintf("group-%d", i+1), + Issued: nbgroup.GroupIssuedAPI, + } + } + + err = manager.SaveGroups(context.Background(), account.Id, groupAdminUserID, groups) + assert.NoError(t, err, "Failed to save test groups") + + testCases := []struct { + name string + groupIDs []string + expectedReasons []string + expectedDeleted []string + expectedNotDeleted []string + }{ + { + name: "route", + groupIDs: []string{"grp-for-route"}, + expectedReasons: []string{"route"}, + }, + { + name: "route with peer groups", + groupIDs: []string{"grp-for-route2"}, + expectedReasons: []string{"route"}, + }, + { + name: "name server groups", + groupIDs: []string{"grp-for-name-server-grp"}, + expectedReasons: []string{"name server groups"}, + }, + { + name: "policy", + groupIDs: []string{"grp-for-policies"}, + expectedReasons: []string{"policy"}, + }, + { + name: "setup keys", + groupIDs: []string{"grp-for-keys"}, + expectedReasons: []string{"setup key"}, + }, + { + name: "users", + groupIDs: []string{"grp-for-users"}, + expectedReasons: []string{"user"}, + }, + { + name: "integration", + groupIDs: []string{"grp-for-integration"}, + expectedReasons: []string{"only service users with admin power can delete integration group"}, + }, + { + name: "successfully delete multiple groups", + groupIDs: []string{"group-1", "group-2"}, + expectedDeleted: []string{"group-1", "group-2"}, + }, + { + name: "delete non-existent group", + groupIDs: []string{"non-existent-group"}, + expectedDeleted: []string{"non-existent-group"}, + }, + { + name: "delete multiple groups with mixed results", + groupIDs: []string{"group-3", "grp-for-policies", "group-4", "grp-for-users"}, + expectedReasons: []string{"policy", "user"}, + expectedDeleted: []string{"group-3", "group-4"}, + expectedNotDeleted: []string{"grp-for-policies", "grp-for-users"}, + }, + { + name: "delete groups with multiple errors", + groupIDs: []string{"grp-for-policies", "grp-for-users"}, + expectedReasons: []string{"policy", "user"}, + expectedNotDeleted: []string{"grp-for-policies", "grp-for-users"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err = am.DeleteGroups(context.Background(), account.Id, groupAdminUserID, tc.groupIDs) + if len(tc.expectedReasons) > 0 { + assert.Error(t, err) + var foundExpectedErrors int + + wrappedErr, ok := err.(interface{ Unwrap() []error }) + assert.Equal(t, ok, true) + + for _, e := range wrappedErr.Unwrap() { + var sErr *status.Error + if errors.As(e, &sErr) { + assert.Contains(t, tc.expectedReasons, sErr.Message, "unexpected error message") + foundExpectedErrors++ + } + + var gErr *GroupLinkError + if errors.As(e, &gErr) { + assert.Contains(t, tc.expectedReasons, gErr.Resource, "unexpected error resource") + foundExpectedErrors++ + } + } + assert.Equal(t, len(tc.expectedReasons), foundExpectedErrors, "not all expected errors were found") + } else { + assert.NoError(t, err) + } + + for _, groupID := range tc.expectedDeleted { + _, err := am.GetGroup(context.Background(), account.Id, groupID, groupAdminUserID) + assert.Error(t, err, "group should have been deleted: %s", groupID) + } + + for _, groupID := range tc.expectedNotDeleted { + group, err := am.GetGroup(context.Background(), account.Id, groupID, groupAdminUserID) + assert.NoError(t, err, "group should not have been deleted: %s", groupID) + assert.NotNil(t, group, "group should exist: %s", groupID) + } + }) + } +} + +func initTestGroupAccount(am *DefaultAccountManager) (*DefaultAccountManager, *Account, error) { accountID := "testingAcc" domain := "example.com" @@ -236,7 +367,7 @@ func initTestGroupAccount(am *DefaultAccountManager) (*Account, error) { err := am.Store.SaveAccount(context.Background(), account) if err != nil { - return nil, err + return nil, nil, err } _ = am.SaveGroup(context.Background(), accountID, groupAdminUserID, groupForRoute) @@ -247,5 +378,9 @@ func initTestGroupAccount(am *DefaultAccountManager) (*Account, error) { _ = am.SaveGroup(context.Background(), accountID, groupAdminUserID, groupForUsers) _ = am.SaveGroup(context.Background(), accountID, groupAdminUserID, groupForIntegration) - return am.Store.GetAccount(context.Background(), account.Id) + acc, err := am.Store.GetAccount(context.Background(), account.Id) + if err != nil { + return nil, nil, err + } + return am, acc, nil } diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index a66bdee2b..495325252 100644 --- a/management/server/mock_server/account_mock.go +++ b/management/server/mock_server/account_mock.go @@ -42,6 +42,7 @@ type MockAccountManager struct { SaveGroupFunc func(ctx context.Context, accountID, userID string, group *group.Group) error SaveGroupsFunc func(ctx context.Context, accountID, userID string, groups []*group.Group) error DeleteGroupFunc func(ctx context.Context, accountID, userId, groupID string) error + DeleteGroupsFunc func(ctx context.Context, accountId, userId string, groupIDs []string) error ListGroupsFunc func(ctx context.Context, accountID string) ([]*group.Group, error) GroupAddPeerFunc func(ctx context.Context, accountID, groupID, peerID string) error GroupDeletePeerFunc func(ctx context.Context, accountID, groupID, peerID string) error @@ -67,6 +68,7 @@ type MockAccountManager struct { SaveOrAddUserFunc func(ctx context.Context, accountID, userID string, user *server.User, addIfNotExists bool) (*server.UserInfo, error) SaveOrAddUsersFunc func(ctx context.Context, accountID, initiatorUserID string, update []*server.User, addIfNotExists bool) ([]*server.UserInfo, error) DeleteUserFunc func(ctx context.Context, accountID string, initiatorUserID string, targetUserID string) error + DeleteRegularUsersFunc func(ctx context.Context, accountID, initiatorUserID string, targetUserIDs []string) error CreatePATFunc func(ctx context.Context, accountID string, initiatorUserID string, targetUserId string, tokenName string, expiresIn int) (*server.PersonalAccessTokenGenerated, error) DeletePATFunc func(ctx context.Context, accountID string, initiatorUserID string, targetUserId string, tokenID string) error GetPATFunc func(ctx context.Context, accountID string, initiatorUserID string, targetUserId string, tokenID string) (*server.PersonalAccessToken, error) @@ -326,6 +328,14 @@ func (am *MockAccountManager) DeleteGroup(ctx context.Context, accountId, userId return status.Errorf(codes.Unimplemented, "method DeleteGroup is not implemented") } +// DeleteGroups mock implementation of DeleteGroups from server.AccountManager interface +func (am *MockAccountManager) DeleteGroups(ctx context.Context, accountId, userId string, groupIDs []string) error { + if am.DeleteGroupsFunc != nil { + return am.DeleteGroupsFunc(ctx, accountId, userId, groupIDs) + } + return status.Errorf(codes.Unimplemented, "method DeleteGroups is not implemented") +} + // ListGroups mock implementation of ListGroups from server.AccountManager interface func (am *MockAccountManager) ListGroups(ctx context.Context, accountID string) ([]*group.Group, error) { if am.ListGroupsFunc != nil { @@ -528,6 +538,14 @@ func (am *MockAccountManager) DeleteUser(ctx context.Context, accountID string, return status.Errorf(codes.Unimplemented, "method DeleteUser is not implemented") } +// DeleteRegularUsers mocks DeleteRegularUsers of the AccountManager interface +func (am *MockAccountManager) DeleteRegularUsers(ctx context.Context, accountID string, initiatorUserID string, targetUserIDs []string) error { + if am.DeleteRegularUsersFunc != nil { + return am.DeleteRegularUsersFunc(ctx, accountID, initiatorUserID, targetUserIDs) + } + return status.Errorf(codes.Unimplemented, "method DeleteRegularUsers is not implemented") +} + func (am *MockAccountManager) InviteUser(ctx context.Context, accountID string, initiatorUserID string, targetUserID string) error { if am.InviteUserFunc != nil { return am.InviteUserFunc(ctx, accountID, initiatorUserID, targetUserID) diff --git a/management/server/user.go b/management/server/user.go index b8afcda3a..8fd0a1627 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -2,6 +2,7 @@ package server import ( "context" + "errors" "fmt" "strings" "time" @@ -472,51 +473,18 @@ func (am *DefaultAccountManager) DeleteUser(ctx context.Context, accountID, init } func (am *DefaultAccountManager) deleteRegularUser(ctx context.Context, account *Account, initiatorUserID, targetUserID string) error { - tuEmail, tuName, err := am.getEmailAndNameOfTargetUser(ctx, account.Id, initiatorUserID, targetUserID) - if err != nil { - log.WithContext(ctx).Errorf("failed to resolve email address: %s", err) - return err - } - - if !isNil(am.idpManager) { - // Delete if the user already exists in the IdP.Necessary in cases where a user account - // was created where a user account was provisioned but the user did not sign in - _, err = am.idpManager.GetUserDataByID(ctx, targetUserID, idp.AppMetadata{WTAccountID: account.Id}) - if err == nil { - err = am.deleteUserFromIDP(ctx, targetUserID, account.Id) - if err != nil { - log.WithContext(ctx).Debugf("failed to delete user from IDP: %s", targetUserID) - return err - } - } else { - log.WithContext(ctx).Debugf("skipped deleting user %s from IDP, error: %v", targetUserID, err) - } - } - - err = am.deleteUserPeers(ctx, initiatorUserID, targetUserID, account) + meta, err := am.prepareUserDeletion(ctx, account, initiatorUserID, targetUserID) if err != nil { return err } - u, err := account.FindUser(targetUserID) - if err != nil { - log.WithContext(ctx).Errorf("failed to find user %s for deletion, this should never happen: %s", targetUserID, err) - } - - var tuCreatedAt time.Time - if u != nil { - tuCreatedAt = u.CreatedAt - } - delete(account.Users, targetUserID) err = am.Store.SaveAccount(ctx, account) if err != nil { return err } - meta := map[string]any{"name": tuName, "email": tuEmail, "created_at": tuCreatedAt} am.StoreEvent(ctx, initiatorUserID, targetUserID, account.Id, activity.UserDeleted, meta) - am.updateAccountPeers(ctx, account) return nil @@ -1190,6 +1158,116 @@ func (am *DefaultAccountManager) getEmailAndNameOfTargetUser(ctx context.Context return "", "", fmt.Errorf("user info not found for user: %s", targetId) } +// DeleteRegularUsers deletes regular users from an account. +// Note: This function does not acquire the global lock. +// It is the caller's responsibility to ensure proper locking is in place before invoking this method. +// +// If an error occurs while deleting the user, the function skips it and continues deleting other users. +// Errors are collected and returned at the end. +func (am *DefaultAccountManager) DeleteRegularUsers(ctx context.Context, accountID, initiatorUserID string, targetUserIDs []string) error { + account, err := am.Store.GetAccount(ctx, accountID) + if err != nil { + return err + } + + executingUser := account.Users[initiatorUserID] + if executingUser == nil { + return status.Errorf(status.NotFound, "user not found") + } + if !executingUser.HasAdminPower() { + return status.Errorf(status.PermissionDenied, "only users with admin power can delete users") + } + + var allErrors error + + deletedUsersMeta := make(map[string]map[string]any) + for _, targetUserID := range targetUserIDs { + if initiatorUserID == targetUserID { + allErrors = errors.Join(allErrors, errors.New("self deletion is not allowed")) + continue + } + + targetUser := account.Users[targetUserID] + if targetUser == nil { + allErrors = errors.Join(allErrors, fmt.Errorf("target user: %s not found", targetUserID)) + continue + } + + if targetUser.Role == UserRoleOwner { + allErrors = errors.Join(allErrors, fmt.Errorf("unable to delete a user: %s with owner role", targetUserID)) + continue + } + + // disable deleting integration user if the initiator is not admin service user + if targetUser.Issued == UserIssuedIntegration && !executingUser.IsServiceUser { + allErrors = errors.Join(allErrors, errors.New("only integration service user can delete this user")) + continue + } + + meta, err := am.prepareUserDeletion(ctx, account, initiatorUserID, targetUserID) + if err != nil { + allErrors = errors.Join(allErrors, fmt.Errorf("failed to delete user %s: %s", targetUserID, err)) + continue + } + + delete(account.Users, targetUserID) + deletedUsersMeta[targetUserID] = meta + } + + err = am.Store.SaveAccount(ctx, account) + if err != nil { + return fmt.Errorf("failed to delete users: %w", err) + } + + am.updateAccountPeers(ctx, account) + + for targetUserID, meta := range deletedUsersMeta { + am.StoreEvent(ctx, initiatorUserID, targetUserID, account.Id, activity.UserDeleted, meta) + } + + return allErrors +} + +func (am *DefaultAccountManager) prepareUserDeletion(ctx context.Context, account *Account, initiatorUserID, targetUserID string) (map[string]any, error) { + tuEmail, tuName, err := am.getEmailAndNameOfTargetUser(ctx, account.Id, initiatorUserID, targetUserID) + if err != nil { + log.WithContext(ctx).Errorf("failed to resolve email address: %s", err) + return nil, err + } + + if !isNil(am.idpManager) { + // Delete if the user already exists in the IdP. Necessary in cases where a user account + // was created where a user account was provisioned but the user did not sign in + _, err = am.idpManager.GetUserDataByID(ctx, targetUserID, idp.AppMetadata{WTAccountID: account.Id}) + if err == nil { + err = am.deleteUserFromIDP(ctx, targetUserID, account.Id) + if err != nil { + log.WithContext(ctx).Debugf("failed to delete user from IDP: %s", targetUserID) + return nil, err + } + } else { + log.WithContext(ctx).Debugf("skipped deleting user %s from IDP, error: %v", targetUserID, err) + } + } + + err = am.deleteUserPeers(ctx, initiatorUserID, targetUserID, account) + if err != nil { + return nil, err + } + + u, err := account.FindUser(targetUserID) + if err != nil { + log.WithContext(ctx).Errorf("failed to find user %s for deletion, this should never happen: %s", targetUserID, err) + } + + var tuCreatedAt time.Time + if u != nil { + tuCreatedAt = u.CreatedAt + } + + return map[string]any{"name": tuName, "email": tuEmail, "created_at": tuCreatedAt}, nil +} + func findUserInIDPUserdata(userID string, userData []*idp.UserData) (*idp.UserData, bool) { for _, user := range userData { if user.ID == userID { diff --git a/management/server/user_test.go b/management/server/user_test.go index 99d2792df..272060276 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -662,6 +662,157 @@ func TestUser_DeleteUser_regularUser(t *testing.T) { } +func TestUser_DeleteUser_RegularUsers(t *testing.T) { + store := newStore(t) + defer store.Close(context.Background()) + account := newAccountWithId(context.Background(), mockAccountID, mockUserID, "") + + targetId := "user2" + account.Users[targetId] = &User{ + Id: targetId, + IsServiceUser: true, + ServiceUserName: "user2username", + } + targetId = "user3" + account.Users[targetId] = &User{ + Id: targetId, + IsServiceUser: false, + Issued: UserIssuedAPI, + } + targetId = "user4" + account.Users[targetId] = &User{ + Id: targetId, + IsServiceUser: false, + Issued: UserIssuedIntegration, + } + + targetId = "user5" + account.Users[targetId] = &User{ + Id: targetId, + IsServiceUser: false, + Issued: UserIssuedAPI, + Role: UserRoleOwner, + } + account.Users["user6"] = &User{ + Id: "user6", + IsServiceUser: false, + Issued: UserIssuedAPI, + } + account.Users["user7"] = &User{ + Id: "user7", + IsServiceUser: false, + Issued: UserIssuedAPI, + } + account.Users["user8"] = &User{ + Id: "user8", + IsServiceUser: false, + Issued: UserIssuedAPI, + Role: UserRoleAdmin, + } + account.Users["user9"] = &User{ + Id: "user9", + IsServiceUser: false, + Issued: UserIssuedAPI, + Role: UserRoleAdmin, + } + + err := store.SaveAccount(context.Background(), account) + if err != nil { + t.Fatalf("Error when saving account: %s", err) + } + + am := DefaultAccountManager{ + Store: store, + eventStore: &activity.InMemoryEventStore{}, + integratedPeerValidator: MocIntegratedValidator{}, + } + + testCases := []struct { + name string + userIDs []string + expectedReasons []string + expectedDeleted []string + expectedNotDeleted []string + }{ + { + name: "Delete service user successfully ", + userIDs: []string{"user2"}, + expectedDeleted: []string{"user2"}, + }, + { + name: "Delete regular user successfully", + userIDs: []string{"user3"}, + expectedDeleted: []string{"user3"}, + }, + { + name: "Delete integration regular user permission denied", + userIDs: []string{"user4"}, + expectedReasons: []string{"only integration service user can delete this user"}, + expectedNotDeleted: []string{"user4"}, + }, + { + name: "Delete user with owner role should return permission denied", + userIDs: []string{"user5"}, + expectedReasons: []string{"unable to delete a user: user5 with owner role"}, + expectedNotDeleted: []string{"user5"}, + }, + { + name: "Delete multiple users with mixed results", + userIDs: []string{"user5", "user5", "user6", "user7"}, + expectedReasons: []string{"only integration service user can delete this user", "unable to delete a user: user5 with owner role"}, + expectedDeleted: []string{"user6", "user7"}, + expectedNotDeleted: []string{"user4", "user5"}, + }, + { + name: "Delete non-existent user", + userIDs: []string{"non-existent-user"}, + expectedReasons: []string{"target user: non-existent-user not found"}, + expectedNotDeleted: []string{}, + }, + { + name: "Delete multiple regular users successfully", + userIDs: []string{"user8", "user9"}, + expectedDeleted: []string{"user8", "user9"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err = am.DeleteRegularUsers(context.Background(), mockAccountID, mockUserID, tc.userIDs) + if len(tc.expectedReasons) > 0 { + assert.Error(t, err) + var foundExpectedErrors int + + wrappedErr, ok := err.(interface{ Unwrap() []error }) + assert.Equal(t, ok, true) + + for _, e := range wrappedErr.Unwrap() { + assert.Contains(t, tc.expectedReasons, e.Error(), "unexpected error message") + foundExpectedErrors++ + } + + assert.Equal(t, len(tc.expectedReasons), foundExpectedErrors, "not all expected errors were found") + } else { + assert.NoError(t, err) + } + + acc, err := am.GetAccountByUserOrAccountID(context.Background(), "", account.Id, "") + assert.NoError(t, err) + + for _, id := range tc.expectedDeleted { + _, exists := acc.Users[id] + assert.False(t, exists, "user should have been deleted: %s", id) + } + + for _, id := range tc.expectedNotDeleted { + user, exists := acc.Users[id] + assert.True(t, exists, "user should not have been deleted: %s", id) + assert.NotNil(t, user, "user should exist: %s", id) + } + }) + } +} + func TestDefaultAccountManager_GetUser(t *testing.T) { store := newStore(t) defer store.Close(context.Background())