diff --git a/management/server/account.go b/management/server/account.go index 0f3b5e6eb7b..a0c6fd0b014 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -67,7 +67,7 @@ type AccountManager interface { SaveSetupKey(ctx context.Context, accountID string, key *types.SetupKey, userID string) (*types.SetupKey, error) CreateUser(ctx context.Context, accountID, initiatorUserID string, key *types.UserInfo) (*types.UserInfo, error) DeleteUser(ctx context.Context, accountID, initiatorUserID string, targetUserID string) error - DeleteRegularUsers(ctx context.Context, accountID, initiatorUserID string, targetUserIDs []string) error + DeleteRegularUsers(ctx context.Context, accountID, initiatorUserID string, targetUserIDs []string, userInfos map[string]*types.UserInfo) error InviteUser(ctx context.Context, accountID string, initiatorUserID string, targetUserID string) error ListSetupKeys(ctx context.Context, accountID, userID string) ([]*types.SetupKey, error) SaveUser(ctx context.Context, accountID, initiatorUserID string, update *types.User) (*types.UserInfo, error) @@ -96,7 +96,7 @@ type AccountManager interface { DeletePAT(ctx context.Context, accountID string, initiatorUserID string, targetUserID string, tokenID string) error GetPAT(ctx context.Context, accountID string, initiatorUserID string, targetUserID string, tokenID string) (*types.PersonalAccessToken, error) GetAllPATs(ctx context.Context, accountID string, initiatorUserID string, targetUserID string) ([]*types.PersonalAccessToken, error) - GetUsersFromAccount(ctx context.Context, accountID, userID string) ([]*types.UserInfo, error) + GetUsersFromAccount(ctx context.Context, accountID, userID string) (map[string]*types.UserInfo, error) GetGroup(ctx context.Context, accountId, groupID, userID string) (*types.Group, error) GetAllGroups(ctx context.Context, accountID, userID string) ([]*types.Group, error) GetGroupByName(ctx context.Context, groupName, accountID string) (*types.Group, error) @@ -149,6 +149,7 @@ type AccountManager interface { GetAccountSettings(ctx context.Context, accountID string, userID string) (*types.Settings, error) DeleteSetupKey(ctx context.Context, accountID, userID, keyID string) error UpdateAccountPeers(ctx context.Context, accountID string) + BuildUserInfosForAccount(ctx context.Context, accountID, initiatorUserID string, accountUsers []*types.User) (map[string]*types.UserInfo, error) } type DefaultAccountManager struct { @@ -617,6 +618,12 @@ func (am *DefaultAccountManager) DeleteAccount(ctx context.Context, accountID, u if user.Role != types.UserRoleOwner { return status.Errorf(status.PermissionDenied, "user is not allowed to delete account. Only account owner can delete account") } + + userInfosMap, err := am.BuildUserInfosForAccount(ctx, accountID, userID, maps.Values(account.Users)) + if err != nil { + return status.Errorf(status.Internal, "failed to build user infos for account %s: %v", accountID, err) + } + for _, otherUser := range account.Users { if otherUser.IsServiceUser { continue @@ -626,13 +633,23 @@ func (am *DefaultAccountManager) DeleteAccount(ctx context.Context, accountID, u continue } - _, deleteUserErr := am.deleteRegularUser(ctx, accountID, userID, otherUser.Id) + userInfo, ok := userInfosMap[otherUser.Id] + if !ok { + return status.Errorf(status.NotFound, "user info not found for user %s", otherUser.Id) + } + + _, deleteUserErr := am.deleteRegularUser(ctx, accountID, userID, userInfo) if deleteUserErr != nil { return deleteUserErr } } - _, err = am.deleteRegularUser(ctx, accountID, userID, userID) + userInfo, ok := userInfosMap[userID] + if !ok { + return status.Errorf(status.NotFound, "user info not found for user %s", userID) + } + + _, err = am.deleteRegularUser(ctx, accountID, userID, userInfo) if err != nil { log.WithContext(ctx).Errorf("failed deleting user %s. error: %s", userID, err) return err diff --git a/management/server/activity/sqlite/sqlite.go b/management/server/activity/sqlite/sqlite.go index 823e0b4ac2c..ffb863de920 100644 --- a/management/server/activity/sqlite/sqlite.go +++ b/management/server/activity/sqlite/sqlite.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "path/filepath" + "runtime" "time" _ "github.com/mattn/go-sqlite3" @@ -95,6 +96,7 @@ func NewSQLiteStore(ctx context.Context, dataDir string, encryptionKey string) ( if err != nil { return nil, err } + db.SetMaxOpenConns(runtime.NumCPU()) crypt, err := NewFieldEncrypt(encryptionKey) if err != nil { diff --git a/management/server/http/handlers/events/events_handler_test.go b/management/server/http/handlers/events/events_handler_test.go index 17478aba351..fd603f289e0 100644 --- a/management/server/http/handlers/events/events_handler_test.go +++ b/management/server/http/handlers/events/events_handler_test.go @@ -32,8 +32,8 @@ func initEventsTestData(account string, events ...*activity.Event) *handler { GetAccountIDFromTokenFunc: func(_ context.Context, claims jwtclaims.AuthorizationClaims) (string, string, error) { return claims.AccountId, claims.UserId, nil }, - GetUsersFromAccountFunc: func(_ context.Context, accountID, userID string) ([]*types.UserInfo, error) { - return make([]*types.UserInfo, 0), nil + GetUsersFromAccountFunc: func(_ context.Context, accountID, userID string) (map[string]*types.UserInfo, error) { + return make(map[string]*types.UserInfo), nil }, }, claimsExtractor: jwtclaims.NewClaimsExtractor( diff --git a/management/server/http/handlers/users/users_handler_test.go b/management/server/http/handlers/users/users_handler_test.go index 90081830a0d..ff77cedfff0 100644 --- a/management/server/http/handlers/users/users_handler_test.go +++ b/management/server/http/handlers/users/users_handler_test.go @@ -52,7 +52,7 @@ var usersTestAccount = &types.Account{ Issued: types.UserIssuedAPI, }, nonDeletableServiceUserID: { - Id: serviceUserID, + Id: nonDeletableServiceUserID, Role: "admin", IsServiceUser: true, NonDeletable: true, @@ -70,10 +70,10 @@ func initUsersTestData() *handler { GetUserByIDFunc: func(ctx context.Context, id string) (*types.User, error) { return usersTestAccount.Users[id], nil }, - GetUsersFromAccountFunc: func(_ context.Context, accountID, userID string) ([]*types.UserInfo, error) { - users := make([]*types.UserInfo, 0) + GetUsersFromAccountFunc: func(_ context.Context, accountID, userID string) (map[string]*types.UserInfo, error) { + usersInfos := make(map[string]*types.UserInfo) for _, v := range usersTestAccount.Users { - users = append(users, &types.UserInfo{ + usersInfos[v.Id] = &types.UserInfo{ ID: v.Id, Role: string(v.Role), Name: "", @@ -81,9 +81,9 @@ func initUsersTestData() *handler { IsServiceUser: v.IsServiceUser, NonDeletable: v.NonDeletable, Issued: v.Issued, - }) + } } - return users, nil + return usersInfos, nil }, CreateUserFunc: func(_ context.Context, accountID, userID string, key *types.UserInfo) (*types.UserInfo, error) { if userID != existingUserID { diff --git a/management/server/http/testing/benchmarks/users_handler_benchmark_test.go b/management/server/http/testing/benchmarks/users_handler_benchmark_test.go index 452b21a576c..0baf7632832 100644 --- a/management/server/http/testing/benchmarks/users_handler_benchmark_test.go +++ b/management/server/http/testing/benchmarks/users_handler_benchmark_test.go @@ -37,12 +37,12 @@ func BenchmarkUpdateUser(b *testing.B) { var expectedMetrics = map[string]testing_tools.PerformanceMetrics{ "Users - XS": {MinMsPerOpLocal: 100, MaxMsPerOpLocal: 160, MinMsPerOpCICD: 100, MaxMsPerOpCICD: 310}, "Users - S": {MinMsPerOpLocal: 0.3, MaxMsPerOpLocal: 3, MinMsPerOpCICD: 1, MaxMsPerOpCICD: 15}, - "Users - M": {MinMsPerOpLocal: 1, MaxMsPerOpLocal: 3, MinMsPerOpCICD: 3, MaxMsPerOpCICD: 15}, - "Users - L": {MinMsPerOpLocal: 0.3, MaxMsPerOpLocal: 10, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 20}, + "Users - M": {MinMsPerOpLocal: 1, MaxMsPerOpLocal: 10, MinMsPerOpCICD: 3, MaxMsPerOpCICD: 20}, + "Users - L": {MinMsPerOpLocal: 5, MaxMsPerOpLocal: 20, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 50}, "Peers - L": {MinMsPerOpLocal: 80, MaxMsPerOpLocal: 150, MinMsPerOpCICD: 80, MaxMsPerOpCICD: 310}, - "Groups - L": {MinMsPerOpLocal: 10, MaxMsPerOpLocal: 30, MinMsPerOpCICD: 20, MaxMsPerOpCICD: 60}, - "Setup Keys - L": {MinMsPerOpLocal: 0.3, MaxMsPerOpLocal: 3, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, - "Users - XL": {MinMsPerOpLocal: 5, MaxMsPerOpLocal: 10, MinMsPerOpCICD: 10, MaxMsPerOpCICD: 30}, + "Groups - L": {MinMsPerOpLocal: 10, MaxMsPerOpLocal: 50, MinMsPerOpCICD: 20, MaxMsPerOpCICD: 120}, + "Setup Keys - L": {MinMsPerOpLocal: 5, MaxMsPerOpLocal: 20, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 50}, + "Users - XL": {MinMsPerOpLocal: 30, MaxMsPerOpLocal: 100, MinMsPerOpCICD: 60, MaxMsPerOpCICD: 280}, } log.SetOutput(io.Discard) @@ -152,14 +152,14 @@ func BenchmarkGetAllUsers(b *testing.B) { func BenchmarkDeleteUsers(b *testing.B) { var expectedMetrics = map[string]testing_tools.PerformanceMetrics{ - "Users - XS": {MinMsPerOpLocal: 3, MaxMsPerOpLocal: 10, MinMsPerOpCICD: 5, MaxMsPerOpCICD: 25}, - "Users - S": {MinMsPerOpLocal: 2, MaxMsPerOpLocal: 10, MinMsPerOpCICD: 5, MaxMsPerOpCICD: 25}, - "Users - M": {MinMsPerOpLocal: 2, MaxMsPerOpLocal: 10, MinMsPerOpCICD: 5, MaxMsPerOpCICD: 25}, - "Users - L": {MinMsPerOpLocal: 2, MaxMsPerOpLocal: 10, MinMsPerOpCICD: 5, MaxMsPerOpCICD: 25}, - "Peers - L": {MinMsPerOpLocal: 3, MaxMsPerOpLocal: 10, MinMsPerOpCICD: 5, MaxMsPerOpCICD: 25}, - "Groups - L": {MinMsPerOpLocal: 2, MaxMsPerOpLocal: 10, MinMsPerOpCICD: 5, MaxMsPerOpCICD: 25}, - "Setup Keys - L": {MinMsPerOpLocal: 3, MaxMsPerOpLocal: 10, MinMsPerOpCICD: 5, MaxMsPerOpCICD: 25}, - "Users - XL": {MinMsPerOpLocal: 3, MaxMsPerOpLocal: 10, MinMsPerOpCICD: 5, MaxMsPerOpCICD: 25}, + "Users - XS": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 5, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, + "Users - S": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 5, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, + "Users - M": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 5, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, + "Users - L": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 5, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, + "Peers - L": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 5, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, + "Groups - L": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 5, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, + "Setup Keys - L": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 5, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, + "Users - XL": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 5, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, } log.SetOutput(io.Discard) diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index e1f8e270983..b20eb87bb4f 100644 --- a/management/server/mock_server/account_mock.go +++ b/management/server/mock_server/account_mock.go @@ -53,7 +53,7 @@ type MockAccountManager struct { SavePolicyFunc func(ctx context.Context, accountID, userID string, policy *types.Policy) (*types.Policy, error) DeletePolicyFunc func(ctx context.Context, accountID, policyID, userID string) error ListPoliciesFunc func(ctx context.Context, accountID, userID string) ([]*types.Policy, error) - GetUsersFromAccountFunc func(ctx context.Context, accountID, userID string) ([]*types.UserInfo, error) + GetUsersFromAccountFunc func(ctx context.Context, accountID, userID string) (map[string]*types.UserInfo, error) GetPATInfoFunc func(ctx context.Context, token string) (*types.User, *types.PersonalAccessToken, string, string, error) MarkPATUsedFunc func(ctx context.Context, pat string) error UpdatePeerMetaFunc func(ctx context.Context, peerID string, meta nbpeer.PeerSystemMeta) error @@ -69,7 +69,7 @@ type MockAccountManager struct { SaveOrAddUserFunc func(ctx context.Context, accountID, userID string, user *types.User, addIfNotExists bool) (*types.UserInfo, error) SaveOrAddUsersFunc func(ctx context.Context, accountID, initiatorUserID string, update []*types.User, addIfNotExists bool) ([]*types.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 + DeleteRegularUsersFunc func(ctx context.Context, accountID, initiatorUserID string, targetUserIDs []string, userInfos map[string]*types.UserInfo) error CreatePATFunc func(ctx context.Context, accountID string, initiatorUserID string, targetUserId string, tokenName string, expiresIn int) (*types.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) (*types.PersonalAccessToken, error) @@ -110,6 +110,7 @@ type MockAccountManager struct { GetUserByIDFunc func(ctx context.Context, id string) (*types.User, error) GetAccountSettingsFunc func(ctx context.Context, accountID string, userID string) (*types.Settings, error) DeleteSetupKeyFunc func(ctx context.Context, accountID, userID, keyID string) error + BuildUserInfosForAccountFunc func(ctx context.Context, accountID, initiatorUserID string, accountUsers []*types.User) (map[string]*types.UserInfo, error) } func (am *MockAccountManager) UpdateAccountPeers(ctx context.Context, accountID string) { @@ -165,7 +166,7 @@ func (am *MockAccountManager) GetAllGroups(ctx context.Context, accountID, userI } // GetUsersFromAccount mock implementation of GetUsersFromAccount from server.AccountManager interface -func (am *MockAccountManager) GetUsersFromAccount(ctx context.Context, accountID string, userID string) ([]*types.UserInfo, error) { +func (am *MockAccountManager) GetUsersFromAccount(ctx context.Context, accountID string, userID string) (map[string]*types.UserInfo, error) { if am.GetUsersFromAccountFunc != nil { return am.GetUsersFromAccountFunc(ctx, accountID, userID) } @@ -550,9 +551,9 @@ func (am *MockAccountManager) DeleteUser(ctx context.Context, accountID string, } // DeleteRegularUsers mocks DeleteRegularUsers of the AccountManager interface -func (am *MockAccountManager) DeleteRegularUsers(ctx context.Context, accountID string, initiatorUserID string, targetUserIDs []string) error { +func (am *MockAccountManager) DeleteRegularUsers(ctx context.Context, accountID, initiatorUserID string, targetUserIDs []string, userInfos map[string]*types.UserInfo) error { if am.DeleteRegularUsersFunc != nil { - return am.DeleteRegularUsersFunc(ctx, accountID, initiatorUserID, targetUserIDs) + return am.DeleteRegularUsersFunc(ctx, accountID, initiatorUserID, targetUserIDs, userInfos) } return status.Errorf(codes.Unimplemented, "method DeleteRegularUsers is not implemented") } @@ -849,3 +850,11 @@ func (am *MockAccountManager) GetPeerGroups(ctx context.Context, accountID, peer } return nil, status.Errorf(codes.Unimplemented, "method GetPeerGroups is not implemented") } + +// BuildUserInfosForAccount mocks BuildUserInfosForAccount of the AccountManager interface +func (am *MockAccountManager) BuildUserInfosForAccount(ctx context.Context, accountID, initiatorUserID string, accountUsers []*types.User) (map[string]*types.UserInfo, error) { + if am.BuildUserInfosForAccountFunc != nil { + return am.BuildUserInfosForAccountFunc(ctx, accountID, initiatorUserID, accountUsers) + } + return nil, status.Errorf(codes.Unimplemented, "method BuildUserInfosForAccount is not implemented") +} diff --git a/management/server/user.go b/management/server/user.go index 5efbd2efafe..6ba9b68d3ac 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -264,7 +264,12 @@ func (am *DefaultAccountManager) DeleteUser(ctx context.Context, accountID, init return am.deleteServiceUser(ctx, accountID, initiatorUserID, targetUser) } - updateAccountPeers, err := am.deleteRegularUser(ctx, accountID, initiatorUserID, targetUserID) + userInfo, err := am.getUserInfo(ctx, targetUser, accountID) + if err != nil { + return err + } + + updateAccountPeers, err := am.deleteRegularUser(ctx, accountID, initiatorUserID, userInfo) if err != nil { return err } @@ -531,10 +536,16 @@ func (am *DefaultAccountManager) SaveOrAddUsers(ctx context.Context, accountID, } var updatedUsersInfo = make([]*types.UserInfo, 0, len(updates)) + + userInfos, err := am.GetUsersFromAccount(ctx, accountID, initiatorUserID) + if err != nil { + return nil, err + } + for _, updatedUser := range usersToSave { - updatedUserInfo, err := am.getUserInfo(ctx, updatedUser, accountID) - if err != nil { - return nil, fmt.Errorf("failed to get user info: %w", err) + updatedUserInfo, ok := userInfos[updatedUser.Id] + if !ok || updatedUserInfo == nil { + return nil, fmt.Errorf("failed to get user: %s updated user info", updatedUser.Id) } updatedUsersInfo = append(updatedUsersInfo, updatedUserInfo) } @@ -773,22 +784,34 @@ func (am *DefaultAccountManager) GetOrCreateAccountByUser(ctx context.Context, u // GetUsersFromAccount performs a batched request for users from IDP by account ID apply filter on what data to return // based on provided user role. -func (am *DefaultAccountManager) GetUsersFromAccount(ctx context.Context, accountID, userID string) ([]*types.UserInfo, error) { +func (am *DefaultAccountManager) GetUsersFromAccount(ctx context.Context, accountID, initiatorUserID string) (map[string]*types.UserInfo, error) { accountUsers, err := am.Store.GetAccountUsers(ctx, store.LockingStrengthShare, accountID) if err != nil { return nil, err } - user, err := am.Store.GetUserByUserID(ctx, store.LockingStrengthShare, userID) + initiatorUser, err := am.Store.GetUserByUserID(ctx, store.LockingStrengthShare, initiatorUserID) if err != nil { return nil, err } - if user.AccountID != accountID { + if initiatorUser.AccountID != accountID { return nil, status.NewUserNotPartOfAccountError() } - queriedUsers := make([]*idp.UserData, 0) + return am.BuildUserInfosForAccount(ctx, accountID, initiatorUserID, accountUsers) +} + +// BuildUserInfosForAccount builds user info for the given account. +func (am *DefaultAccountManager) BuildUserInfosForAccount(ctx context.Context, accountID, initiatorUserID string, accountUsers []*types.User) (map[string]*types.UserInfo, error) { + var queriedUsers []*idp.UserData + var err error + + initiatorUser, err := am.Store.GetUserByUserID(ctx, store.LockingStrengthShare, initiatorUserID) + if err != nil { + return nil, err + } + if !isNil(am.idpManager) { users := make(map[string]userLoggedInOnce, len(accountUsers)) usersFromIntegration := make([]*idp.UserData, 0) @@ -817,31 +840,33 @@ func (am *DefaultAccountManager) GetUsersFromAccount(ctx context.Context, accoun queriedUsers = append(queriedUsers, usersFromIntegration...) } - userInfos := make([]*types.UserInfo, 0) - settings, err := am.Store.GetAccountSettings(ctx, store.LockingStrengthShare, accountID) if err != nil { return nil, err } + userInfosMap := make(map[string]*types.UserInfo) + // in case of self-hosted, or IDP doesn't return anything, we will return the locally stored userInfo if len(queriedUsers) == 0 { for _, accountUser := range accountUsers { - if user.IsRegularUser() && user.Id != accountUser.Id { + if initiatorUser.IsRegularUser() && initiatorUser.Id != accountUser.Id { // if user is not an admin then show only current user and do not show other users continue } + info, err := accountUser.ToUserInfo(nil, settings) if err != nil { return nil, err } - userInfos = append(userInfos, info) + userInfosMap[accountUser.Id] = info } - return userInfos, nil + + return userInfosMap, nil } for _, localUser := range accountUsers { - if user.IsRegularUser() && user.Id != localUser.Id { + if initiatorUser.IsRegularUser() && initiatorUser.Id != localUser.Id { // if user is not an admin then show only current user and do not show other users continue } @@ -878,10 +903,10 @@ func (am *DefaultAccountManager) GetUsersFromAccount(ctx context.Context, accoun Permissions: types.UserPermissions{DashboardView: dashboardViewPermissions}, } } - userInfos = append(userInfos, info) + userInfosMap[info.ID] = info } - return userInfos, nil + return userInfosMap, nil } // expireAndUpdatePeers expires all peers of the given user and updates them in the account @@ -935,27 +960,13 @@ func (am *DefaultAccountManager) deleteUserFromIDP(ctx context.Context, targetUs return nil } -func (am *DefaultAccountManager) getEmailAndNameOfTargetUser(ctx context.Context, accountId, initiatorId, targetId string) (string, string, error) { - userInfos, err := am.GetUsersFromAccount(ctx, accountId, initiatorId) - if err != nil { - return "", "", err - } - for _, ui := range userInfos { - if ui.ID == targetId { - return ui.Email, ui.Name, nil - } - } - - 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 { +func (am *DefaultAccountManager) DeleteRegularUsers(ctx context.Context, accountID, initiatorUserID string, targetUserIDs []string, userInfos map[string]*types.UserInfo) error { initiatorUser, err := am.Store.GetUserByUserID(ctx, store.LockingStrengthShare, initiatorUserID) if err != nil { return err @@ -991,7 +1002,13 @@ func (am *DefaultAccountManager) DeleteRegularUsers(ctx context.Context, account continue } - userHadPeers, err := am.deleteRegularUser(ctx, accountID, initiatorUserID, targetUserID) + userInfo, ok := userInfos[targetUserID] + if !ok || userInfo == nil { + allErrors = errors.Join(allErrors, fmt.Errorf("user info not found for user: %s", targetUserID)) + continue + } + + userHadPeers, err := am.deleteRegularUser(ctx, accountID, initiatorUserID, userInfo) if err != nil { allErrors = errors.Join(allErrors, err) continue @@ -1010,53 +1027,48 @@ func (am *DefaultAccountManager) DeleteRegularUsers(ctx context.Context, account } // deleteRegularUser deletes a specified user and their related peers from the account. -func (am *DefaultAccountManager) deleteRegularUser(ctx context.Context, accountID, initiatorUserID, targetUserID string) (bool, error) { - tuEmail, tuName, err := am.getEmailAndNameOfTargetUser(ctx, accountID, initiatorUserID, targetUserID) - if err != nil { - log.WithContext(ctx).Errorf("failed to resolve email address: %s", err) - return false, err - } - +func (am *DefaultAccountManager) deleteRegularUser(ctx context.Context, accountID, initiatorUserID string, targetUserInfo *types.UserInfo) (bool, error) { 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: accountID}) + _, err := am.idpManager.GetUserDataByID(ctx, targetUserInfo.ID, idp.AppMetadata{WTAccountID: accountID}) if err == nil { - err = am.deleteUserFromIDP(ctx, targetUserID, accountID) + err = am.deleteUserFromIDP(ctx, targetUserInfo.ID, accountID) if err != nil { - log.WithContext(ctx).Debugf("failed to delete user from IDP: %s", targetUserID) + log.WithContext(ctx).Debugf("failed to delete user from IDP: %s", targetUserInfo.ID) return false, err } } else { - log.WithContext(ctx).Debugf("skipped deleting user %s from IDP, error: %v", targetUserID, err) + log.WithContext(ctx).Debugf("skipped deleting user %s from IDP, error: %v", targetUserInfo.ID, err) } } var addPeerRemovedEvents []func() var updateAccountPeers bool var targetUser *types.User + var err error err = am.Store.ExecuteInTransaction(ctx, func(transaction store.Store) error { - targetUser, err = transaction.GetUserByUserID(ctx, store.LockingStrengthShare, targetUserID) + targetUser, err = transaction.GetUserByUserID(ctx, store.LockingStrengthShare, targetUserInfo.ID) if err != nil { return fmt.Errorf("failed to get user to delete: %w", err) } - userPeers, err := transaction.GetUserPeers(ctx, store.LockingStrengthShare, accountID, targetUserID) + userPeers, err := transaction.GetUserPeers(ctx, store.LockingStrengthShare, accountID, targetUserInfo.ID) if err != nil { return fmt.Errorf("failed to get user peers: %w", err) } if len(userPeers) > 0 { updateAccountPeers = true - addPeerRemovedEvents, err = deletePeers(ctx, am, transaction, accountID, targetUserID, userPeers) + addPeerRemovedEvents, err = deletePeers(ctx, am, transaction, accountID, targetUserInfo.ID, userPeers) if err != nil { return fmt.Errorf("failed to delete user peers: %w", err) } } - if err = transaction.DeleteUser(ctx, store.LockingStrengthUpdate, accountID, targetUserID); err != nil { - return fmt.Errorf("failed to delete user: %s %w", targetUserID, err) + if err = transaction.DeleteUser(ctx, store.LockingStrengthUpdate, accountID, targetUserInfo.ID); err != nil { + return fmt.Errorf("failed to delete user: %s %w", targetUserInfo.ID, err) } return nil @@ -1068,7 +1080,7 @@ func (am *DefaultAccountManager) deleteRegularUser(ctx context.Context, accountI for _, addPeerRemovedEvent := range addPeerRemovedEvents { addPeerRemovedEvent() } - meta := map[string]any{"name": tuName, "email": tuEmail, "created_at": targetUser.CreatedAt} + meta := map[string]any{"name": targetUserInfo.Name, "email": targetUserInfo.Email, "created_at": targetUser.CreatedAt} am.StoreEvent(ctx, initiatorUserID, targetUser.Id, accountID, activity.UserDeleted, meta) return updateAccountPeers, nil diff --git a/management/server/user_test.go b/management/server/user_test.go index 5c4b1e2cbef..4a532c8a6a7 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -11,6 +11,7 @@ import ( cacheStore "github.com/eko/gocache/v3/store" "github.com/google/go-cmp/cmp" "github.com/netbirdio/netbird/management/server/util" + "golang.org/x/exp/maps" nbpeer "github.com/netbirdio/netbird/management/server/peer" "github.com/netbirdio/netbird/management/server/store" @@ -867,7 +868,10 @@ func TestUser_DeleteUser_RegularUsers(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - err = am.DeleteRegularUsers(context.Background(), mockAccountID, mockUserID, tc.userIDs) + userInfos, err := am.BuildUserInfosForAccount(context.Background(), mockAccountID, mockUserID, maps.Values(account.Users)) + assert.NoError(t, err) + + err = am.DeleteRegularUsers(context.Background(), mockAccountID, mockUserID, tc.userIDs, userInfos) if len(tc.expectedReasons) > 0 { assert.Error(t, err) var foundExpectedErrors int