From 15cff6c50728c7887a37b8cad4fe23f9f0a2f1be Mon Sep 17 00:00:00 2001 From: Brandon Date: Fri, 9 May 2025 14:19:18 -0400 Subject: [PATCH] refactor groups logic --- .../OrganizationGroupImportData.cs | 12 ++ ...cData.cs => OrganizationUserImportData.cs} | 0 .../ImportOrganizationUserCommand.cs | 113 ++++++++++-------- .../IInviteOrganizationUsersCommand.cs | 2 +- .../InviteOrganizationUsersRequestTests.cs | 6 +- 5 files changed, 78 insertions(+), 55 deletions(-) create mode 100644 src/Core/AdminConsole/Models/Data/Organizations/OrganizationGroupImportData.cs rename src/Core/AdminConsole/Models/Data/Organizations/OrganizationUsers/{OrganizationUserSyncData.cs => OrganizationUserImportData.cs} (100%) diff --git a/src/Core/AdminConsole/Models/Data/Organizations/OrganizationGroupImportData.cs b/src/Core/AdminConsole/Models/Data/Organizations/OrganizationGroupImportData.cs new file mode 100644 index 0000000000..562fa7e766 --- /dev/null +++ b/src/Core/AdminConsole/Models/Data/Organizations/OrganizationGroupImportData.cs @@ -0,0 +1,12 @@ +using Bit.Core.AdminConsole.Entities; +using Bit.Core.AdminConsole.Models.Business; + +namespace Bit.Core.Models.Data.Organizations; + +public class OrganizationGroupImportData +{ + public IEnumerable Groups { get; set; } + public ICollection ExistingGroups { get; set; } + public IEnumerable ExistingExternalGroups { get; set; } + public IDictionary GroupsDict { get; set; } +} diff --git a/src/Core/AdminConsole/Models/Data/Organizations/OrganizationUsers/OrganizationUserSyncData.cs b/src/Core/AdminConsole/Models/Data/Organizations/OrganizationUsers/OrganizationUserImportData.cs similarity index 100% rename from src/Core/AdminConsole/Models/Data/Organizations/OrganizationUsers/OrganizationUserSyncData.cs rename to src/Core/AdminConsole/Models/Data/Organizations/OrganizationUsers/OrganizationUserImportData.cs diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ImportOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ImportOrganizationUserCommand.cs index 367e763f33..8307dc15a0 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ImportOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ImportOrganizationUserCommand.cs @@ -10,6 +10,7 @@ using Bit.Core.Enums; using Bit.Core.Exceptions; using Bit.Core.Models.Business; using Bit.Core.Models.Commands; +using Bit.Core.Models.Data.Organizations; using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.Repositories; @@ -60,8 +61,7 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand IEnumerable newUsers, IEnumerable removeUserExternalIds, bool overwriteExisting, - EventSystemUser eventSystemUser - ) + EventSystemUser eventSystemUser) { var organization = await GetOrgById(organizationId); if (organization == null) @@ -76,27 +76,27 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand var existingUsers = await _organizationUserRepository.GetManyDetailsByOrganizationAsync(organizationId); - var importData = new OrganizationUserImportData + var importUserData = new OrganizationUserImportData { NewUsersSet = new HashSet(newUsers?.Select(u => u.ExternalId) ?? new List()), ExistingUsers = existingUsers, ExistingExternalUsers = GetExistingExternalUsers(existingUsers), - ExistingExternalUsersIdDict = GetExistingExternalUsersIdDict(existingUsers) + ExistingExternalUsersIdDict = GetExistingExternalUsers(existingUsers).ToDictionary(u => u.ExternalId, u => u.Id) }; var events = new List<(OrganizationUserUserDetails ou, EventType e, DateTime? d)>(); // Users - await RemoveExistingExternalUsers(removeUserExternalIds, events, importData); + await RemoveExistingExternalUsers(removeUserExternalIds, events, importUserData); if (overwriteExisting) { - await OverwriteExisting(events, importData); + await OverwriteExisting(events, importUserData); } - await RemoveExistingUsers(existingUsers, newUsers, organization, importData); - await AddNewUsers(organization, newUsers, eventSystemUser, importData); + await RemoveExistingUsers(existingUsers, newUsers, organization, importUserData); + await AddNewUsers(organization, newUsers, eventSystemUser, importUserData); // Groups - await ImportGroups(organization, groups, eventSystemUser, importData); + await ImportGroups(organization, groups, eventSystemUser, importUserData); await _eventService.LogOrganizationUserEventsAsync(events.Select(e => (e.ou, e.e, eventSystemUser, e.d))); await _referenceEventService.RaiseEventAsync( @@ -116,20 +116,18 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand await _groupRepository.UpdateUsersAsync(group.Id, users); } - private async Task RemoveExistingExternalUsers( - IEnumerable removeUserExternalIds, + private async Task RemoveExistingExternalUsers(IEnumerable removeUserExternalIds, List<(OrganizationUserUserDetails ou, EventType e, DateTime? d)> events, - OrganizationUserImportData importData - ) + OrganizationUserImportData importUserData) { if (!removeUserExternalIds.Any()) { return; } - var existingUsersDict = importData.ExistingExternalUsers.ToDictionary(u => u.ExternalId); + var existingUsersDict = importUserData.ExistingExternalUsers.ToDictionary(u => u.ExternalId); var removeUsersSet = new HashSet(removeUserExternalIds) - .Except(importData.NewUsersSet) + .Except(importUserData.NewUsersSet) .Where(u => existingUsersDict.ContainsKey(u) && existingUsersDict[u].Type != OrganizationUserType.Owner) .Select(u => existingUsersDict[u]); @@ -142,12 +140,10 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand ); } - private async Task RemoveExistingUsers( - IEnumerable existingUsers, + private async Task RemoveExistingUsers(IEnumerable existingUsers, IEnumerable newUsers, Organization organization, - OrganizationUserImportData importData - ) + OrganizationUserImportData importUserData) { if (!newUsers.Any()) { @@ -169,7 +165,7 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand { orgUser.ExternalId = newUsersEmailsDict[user].ExternalId; usersToUpsert.Add(orgUser); - importData.ExistingExternalUsersIdDict.Add(orgUser.ExternalId, orgUser.Id); + importUserData.ExistingExternalUsersIdDict.Add(orgUser.ExternalId, orgUser.Id); } } await _organizationUserRepository.UpsertManyAsync(usersToUpsert); @@ -178,11 +174,11 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand private async Task AddNewUsers(Organization organization, IEnumerable newUsers, EventSystemUser eventSystemUser, - OrganizationUserImportData importData) + OrganizationUserImportData importUserData) { - var existingUsersSet = new HashSet(importData.ExistingExternalUsersIdDict.Keys); - var usersToAdd = importData.NewUsersSet.Except(existingUsersSet).ToList(); + var existingUsersSet = new HashSet(importUserData.ExistingExternalUsersIdDict.Keys); + var usersToAdd = importUserData.NewUsersSet.Except(existingUsersSet).ToList(); var seatsAvailable = int.MaxValue; var enoughSeatsAvailable = true; @@ -219,11 +215,10 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand switch (commandResult) { - case Success success: - var result = success.Value; - foreach (var u in result.InvitedUsers) + case Success result: + foreach (var u in result.Value.InvitedUsers) { - importData.ExistingExternalUsersIdDict.Add(u.ExternalId, u.Id); + importUserData.ExistingExternalUsersIdDict.Add(u.ExternalId, u.Id); } break; case Failure failure: @@ -244,14 +239,13 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand private async Task OverwriteExisting( List<(OrganizationUserUserDetails ou, EventType e, DateTime? d)> events, - OrganizationUserImportData importData - ) + OrganizationUserImportData importUserData) { // Remove existing external users that are not in new user set - var usersToDelete = importData.ExistingExternalUsers.Where(u => + var usersToDelete = importUserData.ExistingExternalUsers.Where(u => u.Type != OrganizationUserType.Owner && - !importData.NewUsersSet.Contains(u.ExternalId) && - importData.ExistingExternalUsersIdDict.ContainsKey(u.ExternalId)); + !importUserData.NewUsersSet.Contains(u.ExternalId) && + importUserData.ExistingExternalUsersIdDict.ContainsKey(u.ExternalId)); await _organizationUserRepository.DeleteManyAsync(usersToDelete.Select(u => u.Id)); events.AddRange(usersToDelete.Select(u => ( u, @@ -261,15 +255,14 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand ); foreach (var deletedUser in usersToDelete) { - importData.ExistingExternalUsersIdDict.Remove(deletedUser.ExternalId); + importUserData.ExistingExternalUsersIdDict.Remove(deletedUser.ExternalId); } } private async Task ImportGroups(Organization organization, IEnumerable groups, EventSystemUser eventSystemUser, - OrganizationUserImportData importData - ) + OrganizationUserImportData importUserData) { if (!groups.Any()) @@ -282,14 +275,25 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand throw new BadRequestException("Organization cannot use groups."); } - var groupsDict = groups.ToDictionary(g => g.Group.ExternalId); var existingGroups = await _groupRepository.GetManyByOrganizationIdAsync(organization.Id); - var existingExternalGroups = existingGroups - .Where(u => !string.IsNullOrWhiteSpace(u.ExternalId)).ToList(); - var existingExternalGroupsDict = existingExternalGroups.ToDictionary(g => g.ExternalId); + var importGroupData = new OrganizationGroupImportData + { + Groups = groups, + GroupsDict = groups.ToDictionary(g => g.Group.ExternalId), + ExistingGroups = existingGroups, + ExistingExternalGroups = GetExistingExternalGroups(existingGroups) + }; - var newGroups = groups - .Where(g => !existingExternalGroupsDict.ContainsKey(g.Group.ExternalId)) + await SaveNewGroups(importGroupData, importUserData, eventSystemUser); + await UpdateExistingGroups(importGroupData, importUserData, organization, eventSystemUser); + } + + private async Task SaveNewGroups(OrganizationGroupImportData importGroupData, + OrganizationUserImportData importUserData, + EventSystemUser eventSystemUser) + { + var newGroups = importGroupData.Groups + .Where(g => !importGroupData.ExistingExternalGroups.ToDictionary(g => g.ExternalId).ContainsKey(g.Group.ExternalId)) .Select(g => g.Group).ToList(); var savedGroups = new List(); @@ -298,15 +302,21 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand group.CreationDate = group.RevisionDate = DateTime.UtcNow; savedGroups.Add(await _groupRepository.CreateAsync(group)); - await UpdateUsersAsync(group, groupsDict[group.ExternalId].ExternalUserIds, - importData.ExistingExternalUsersIdDict); + await UpdateUsersAsync(group, importGroupData.GroupsDict[group.ExternalId].ExternalUserIds, + importUserData.ExistingExternalUsersIdDict); } await _eventService.LogGroupEventsAsync( savedGroups.Select(g => (g, EventType.Group_Created, (EventSystemUser?)eventSystemUser, (DateTime?)DateTime.UtcNow))); + } - var updateGroups = existingExternalGroups - .Where(g => groupsDict.ContainsKey(g.ExternalId)) + private async Task UpdateExistingGroups(OrganizationGroupImportData importGroupData, + OrganizationUserImportData importUserData, + Organization organization, + EventSystemUser eventSystemUser) + { + var updateGroups = importGroupData.ExistingExternalGroups + .Where(g => importGroupData.GroupsDict.ContainsKey(g.ExternalId)) .ToList(); if (updateGroups.Any()) @@ -318,7 +328,7 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand foreach (var group in updateGroups) { - var updatedGroup = groupsDict[group.ExternalId].Group; + var updatedGroup = importGroupData.GroupsDict[group.ExternalId].Group; if (group.Name != updatedGroup.Name) { group.RevisionDate = DateTime.UtcNow; @@ -327,8 +337,8 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand await _groupRepository.ReplaceAsync(group); } - await UpdateUsersAsync(group, groupsDict[group.ExternalId].ExternalUserIds, - importData.ExistingExternalUsersIdDict, + await UpdateUsersAsync(group, importGroupData.GroupsDict[group.ExternalId].ExternalUserIds, + importUserData.ExistingExternalUsersIdDict, existingGroupUsers.ContainsKey(group.Id) ? existingGroupUsers[group.Id] : null); } @@ -336,6 +346,7 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand await _eventService.LogGroupEventsAsync( updateGroups.Select(g => (g, EventType.Group_Updated, (EventSystemUser?)eventSystemUser, (DateTime?)DateTime.UtcNow))); } + } private IEnumerable GetExistingExternalUsers(ICollection existingUsers) @@ -345,11 +356,11 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand .ToList(); } - private Dictionary GetExistingExternalUsersIdDict(ICollection existingUsers) + private IEnumerable GetExistingExternalGroups(ICollection existingGroups) { - return existingUsers + return existingGroups .Where(u => !string.IsNullOrWhiteSpace(u.ExternalId)) - .ToDictionary(u => u.ExternalId, u => u.Id); + .ToList(); } private async Task GetOrgById(Guid id) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/InviteUsers/IInviteOrganizationUsersCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/InviteUsers/IInviteOrganizationUsersCommand.cs index 1322bb4423..766e39c683 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/InviteUsers/IInviteOrganizationUsersCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/InviteUsers/IInviteOrganizationUsersCommand.cs @@ -20,7 +20,7 @@ public interface IInviteOrganizationUsersCommand /// Response from InviteScimOrganiation Task> InviteScimOrganizationUserAsync(InviteOrganizationUsersRequest request); /// - /// Sends invitations to add imported organization users via directory connector or public API. + /// Sends invitations to add imported organization users via the public API. /// This can be a Success or a Failure. Failure will contain the Error along with a representation of the errored value. /// Success will be the successful return object. /// diff --git a/test/Core.Test/AdminConsole/Models/InviteOrganizationUsersRequestTests.cs b/test/Core.Test/AdminConsole/Models/InviteOrganizationUsersRequestTests.cs index 71b2b9766c..a9d1836a9f 100644 --- a/test/Core.Test/AdminConsole/Models/InviteOrganizationUsersRequestTests.cs +++ b/test/Core.Test/AdminConsole/Models/InviteOrganizationUsersRequestTests.cs @@ -16,7 +16,7 @@ public class InviteOrganizationUsersRequestTests public void Constructor_WhenPassedInvalidEmail_ThrowsException(string email, OrganizationUserType type, Permissions permissions, string externalId) { var exception = Assert.Throws(() => - new OrganizationUserInvite(email, [], [], type, permissions, externalId, false)); + new OrganizationUserInviteCommandModel(email, [], [], type, permissions, externalId, false)); Assert.Contains(InvalidEmailErrorMessage, exception.Message); } @@ -33,7 +33,7 @@ public class InviteOrganizationUsersRequestTests }; var exception = Assert.Throws(() => - new OrganizationUserInvite( + new OrganizationUserInviteCommandModel( email: validEmail, assignedCollections: [invalidCollectionConfiguration], groups: [], @@ -51,7 +51,7 @@ public class InviteOrganizationUsersRequestTests const string validEmail = "test@email.com"; var validCollectionConfiguration = new CollectionAccessSelection { Id = Guid.NewGuid(), Manage = true }; - var invite = new OrganizationUserInvite( + var invite = new OrganizationUserInviteCommandModel( email: validEmail, assignedCollections: [validCollectionConfiguration], groups: [],