From 02b3453cd5c3a99cd53a4216ad6a9316c65d5bef Mon Sep 17 00:00:00 2001 From: Vincent Salucci <26154748+vincentsalucci@users.noreply.github.com> Date: Fri, 12 Jul 2024 12:25:04 -0500 Subject: [PATCH] [AC-2646] Remove FC MVP dead code from Core (#4281) * chore: remove fc refs in CreateGroup and UpdateGroup commands, refs AC-2646 * chore: remove fc refs and update interface to represent usage/get rid of double enumeration warnings, refs AC-2646 * chore: remove org/provider service fc callers, refs AC-2646 * chore: remove collection service fc callers, refs AC-2646 * chore: remove cipher service import ciphers fc callers, refs AC-2646 * fix: UpdateOrganizationUserCommandTests collections to list, refs AC-2646 * fix: update CreateGroupCommandTests, refs AC-2646 * fix: adjust UpdateGroupCommandTests, refs AC-2646 * fix: adjust UpdateOrganizationUserCommandTests for FC always true, refs AC-2646 * fix: update CollectionServiceTests, refs AC-2646 * fix: remove unnecessary test with fc disabled, refs AC-2646 * fix: update tests to account for AccessAll removal and Manager removal, refs AC-2646 * chore: remove dependence on FC flag for tests, refs AC-2646 --- .../AdminConsole/Services/ProviderService.cs | 8 +-- .../src/Scim/Models/ScimUserRequestModel.cs | 1 - .../Services/ProviderServiceTests.cs | 11 ++-- .../Scim.Test/Users/PostUserCommandTests.cs | 2 - .../AdminConsole/Entities/OrganizationUser.cs | 6 +- .../Models/Business/OrganizationUserInvite.cs | 2 - .../OrganizationUserInviteData.cs | 1 - .../Groups/CreateGroupCommand.cs | 17 +++--- .../Groups/UpdateGroupCommand.cs | 17 +++--- .../IUpdateOrganizationUserCommand.cs | 2 +- .../UpdateOrganizationUserCommand.cs | 15 ++--- .../Implementations/OrganizationService.cs | 57 +++++++----------- .../Implementations/CollectionService.cs | 29 +++++----- .../Services/Implementations/CipherService.cs | 13 ++--- .../AutoFixture/OrganizationFixtures.cs | 3 + .../Groups/CreateGroupCommandTests.cs | 38 +++++++++--- .../Groups/UpdateGroupCommandTests.cs | 33 ++++++++--- .../UpdateOrganizationUserCommandTests.cs | 22 +++++-- .../Services/OrganizationServiceTests.cs | 40 +++++-------- .../CollectionAccessSelectionFixtures.cs | 8 ++- .../Services/CollectionServiceTests.cs | 2 +- .../Vault/Services/CipherServiceTests.cs | 58 +------------------ 22 files changed, 167 insertions(+), 218 deletions(-) diff --git a/bitwarden_license/src/Commercial.Core/AdminConsole/Services/ProviderService.cs b/bitwarden_license/src/Commercial.Core/AdminConsole/Services/ProviderService.cs index 380d404b2..a9592dfcc 100644 --- a/bitwarden_license/src/Commercial.Core/AdminConsole/Services/ProviderService.cs +++ b/bitwarden_license/src/Commercial.Core/AdminConsole/Services/ProviderService.cs @@ -544,9 +544,9 @@ public class ProviderService : IProviderService await _providerOrganizationRepository.CreateAsync(providerOrganization); await _eventService.LogProviderOrganizationEventAsync(providerOrganization, EventType.ProviderOrganization_Created); - // If using Flexible Collections, give the owner Can Manage access over the default collection + // Give the owner Can Manage access over the default collection // The orgUser is not available when the org is created so we have to do it here as part of the invite - var defaultOwnerAccess = organization.FlexibleCollections && defaultCollection != null + var defaultOwnerAccess = defaultCollection != null ? [ new CollectionAccessSelection @@ -566,10 +566,6 @@ public class ProviderService : IProviderService new OrganizationUserInvite { Emails = new[] { clientOwnerEmail }, - - // If using Flexible Collections, AccessAll is deprecated and set to false. - // If not using Flexible Collections, set AccessAll to true (previous behavior) - AccessAll = !organization.FlexibleCollections, Type = OrganizationUserType.Owner, Permissions = null, Collections = defaultOwnerAccess, diff --git a/bitwarden_license/src/Scim/Models/ScimUserRequestModel.cs b/bitwarden_license/src/Scim/Models/ScimUserRequestModel.cs index e95e197db..990ac27ec 100644 --- a/bitwarden_license/src/Scim/Models/ScimUserRequestModel.cs +++ b/bitwarden_license/src/Scim/Models/ScimUserRequestModel.cs @@ -20,7 +20,6 @@ public class ScimUserRequestModel : BaseScimUserModel // Permissions cannot be set via SCIM so we use default values Type = OrganizationUserType.User, - AccessAll = false, Collections = new List(), Groups = new List() }; diff --git a/bitwarden_license/test/Commercial.Core.Test/AdminConsole/Services/ProviderServiceTests.cs b/bitwarden_license/test/Commercial.Core.Test/AdminConsole/Services/ProviderServiceTests.cs index 9a1c6c78d..51dfb7cea 100644 --- a/bitwarden_license/test/Commercial.Core.Test/AdminConsole/Services/ProviderServiceTests.cs +++ b/bitwarden_license/test/Commercial.Core.Test/AdminConsole/Services/ProviderServiceTests.cs @@ -637,8 +637,7 @@ public class ProviderServiceTests t.First().Item1.Emails.Count() == 1 && t.First().Item1.Emails.First() == clientOwnerEmail && t.First().Item1.Type == OrganizationUserType.Owner && - t.First().Item1.AccessAll && - !t.First().Item1.Collections.Any() && + t.First().Item1.Collections.Count() == 1 && t.First().Item2 == null)); } @@ -717,13 +716,12 @@ public class ProviderServiceTests t.First().Item1.Emails.Count() == 1 && t.First().Item1.Emails.First() == clientOwnerEmail && t.First().Item1.Type == OrganizationUserType.Owner && - t.First().Item1.AccessAll && - !t.First().Item1.Collections.Any() && + t.First().Item1.Collections.Count() == 1 && t.First().Item2 == null)); } - [Theory, OrganizationCustomize(FlexibleCollections = true), BitAutoData] - public async Task CreateOrganizationAsync_WithFlexibleCollections_SetsAccessAllToFalse + [Theory, OrganizationCustomize, BitAutoData] + public async Task CreateOrganizationAsync_SetsAccessAllToFalse (Provider provider, OrganizationSignup organizationSignup, Organization organization, string clientOwnerEmail, User user, SutProvider sutProvider, Collection defaultCollection) { @@ -747,7 +745,6 @@ public class ProviderServiceTests t.First().Item1.Emails.Count() == 1 && t.First().Item1.Emails.First() == clientOwnerEmail && t.First().Item1.Type == OrganizationUserType.Owner && - t.First().Item1.AccessAll == false && t.First().Item1.Collections.Single().Id == defaultCollection.Id && !t.First().Item1.Collections.Single().HidePasswords && !t.First().Item1.Collections.Single().ReadOnly && diff --git a/bitwarden_license/test/Scim.Test/Users/PostUserCommandTests.cs b/bitwarden_license/test/Scim.Test/Users/PostUserCommandTests.cs index cf1c33702..71ad6361b 100644 --- a/bitwarden_license/test/Scim.Test/Users/PostUserCommandTests.cs +++ b/bitwarden_license/test/Scim.Test/Users/PostUserCommandTests.cs @@ -43,7 +43,6 @@ public class PostUserCommandTests Arg.Is(i => i.Emails.Single().Equals(scimUserRequestModel.PrimaryEmail.ToLowerInvariant()) && i.Type == OrganizationUserType.User && - !i.AccessAll && !i.Collections.Any() && !i.Groups.Any() && i.AccessSecretsManager), externalId) @@ -56,7 +55,6 @@ public class PostUserCommandTests Arg.Is(i => i.Emails.Single().Equals(scimUserRequestModel.PrimaryEmail.ToLowerInvariant()) && i.Type == OrganizationUserType.User && - !i.AccessAll && !i.Collections.Any() && !i.Groups.Any() && i.AccessSecretsManager), externalId); diff --git a/src/Core/AdminConsole/Entities/OrganizationUser.cs b/src/Core/AdminConsole/Entities/OrganizationUser.cs index afc3dc490..5340e2255 100644 --- a/src/Core/AdminConsole/Entities/OrganizationUser.cs +++ b/src/Core/AdminConsole/Entities/OrganizationUser.cs @@ -19,7 +19,11 @@ public class OrganizationUser : ITableObject, IExternal public string? ResetPasswordKey { get; set; } public OrganizationUserStatusType Status { get; set; } public OrganizationUserType Type { get; set; } - public bool AccessAll { get; set; } + + /// + /// AccessAll is deprecated and should always be left as false. Scheduled for removal. + /// + public bool AccessAll { get; set; } = false; [MaxLength(300)] public string? ExternalId { get; set; } public DateTime CreationDate { get; internal set; } = DateTime.UtcNow; diff --git a/src/Core/AdminConsole/Models/Business/OrganizationUserInvite.cs b/src/Core/AdminConsole/Models/Business/OrganizationUserInvite.cs index 7102a5f9e..e177a5047 100644 --- a/src/Core/AdminConsole/Models/Business/OrganizationUserInvite.cs +++ b/src/Core/AdminConsole/Models/Business/OrganizationUserInvite.cs @@ -7,7 +7,6 @@ public class OrganizationUserInvite { public IEnumerable Emails { get; set; } public Enums.OrganizationUserType? Type { get; set; } - public bool AccessAll { get; set; } public bool AccessSecretsManager { get; set; } public Permissions Permissions { get; set; } public IEnumerable Collections { get; set; } @@ -19,7 +18,6 @@ public class OrganizationUserInvite { Emails = requestModel.Emails; Type = requestModel.Type; - AccessAll = requestModel.AccessAll; AccessSecretsManager = requestModel.AccessSecretsManager; Collections = requestModel.Collections; Groups = requestModel.Groups; diff --git a/src/Core/AdminConsole/Models/Data/Organizations/OrganizationUsers/OrganizationUserInviteData.cs b/src/Core/AdminConsole/Models/Data/Organizations/OrganizationUsers/OrganizationUserInviteData.cs index f8789fe5d..a48ee3a6c 100644 --- a/src/Core/AdminConsole/Models/Data/Organizations/OrganizationUsers/OrganizationUserInviteData.cs +++ b/src/Core/AdminConsole/Models/Data/Organizations/OrganizationUsers/OrganizationUserInviteData.cs @@ -6,7 +6,6 @@ public class OrganizationUserInviteData { public IEnumerable Emails { get; set; } public OrganizationUserType? Type { get; set; } - public bool AccessAll { get; set; } public bool AccessSecretsManager { get; set; } public IEnumerable Collections { get; set; } public IEnumerable Groups { get; set; } diff --git a/src/Core/AdminConsole/OrganizationFeatures/Groups/CreateGroupCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/Groups/CreateGroupCommand.cs index af95f9fb3..83d076475 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Groups/CreateGroupCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Groups/CreateGroupCommand.cs @@ -115,18 +115,15 @@ public class CreateGroupCommand : ICreateGroupCommand throw new BadRequestException("This organization cannot use groups."); } - if (organization.FlexibleCollections) + if (group.AccessAll) { - if (group.AccessAll) - { - throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the group to collections instead."); - } + throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the group to collections instead."); + } - var invalidAssociations = collections?.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); - if (invalidAssociations?.Any() ?? false) - { - throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); - } + var invalidAssociations = collections?.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); + if (invalidAssociations?.Any() ?? false) + { + throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); } } } diff --git a/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs index ba2aa2b8b..3a8c6afd8 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs @@ -109,18 +109,15 @@ public class UpdateGroupCommand : IUpdateGroupCommand throw new BadRequestException("This organization cannot use groups."); } - if (organization.FlexibleCollections) + if (group.AccessAll) { - if (group.AccessAll) - { - throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the group to collections instead."); - } + throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the group to collections instead."); + } - var invalidAssociations = collections?.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); - if (invalidAssociations?.Any() ?? false) - { - throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); - } + var invalidAssociations = collections?.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); + if (invalidAssociations?.Any() ?? false) + { + throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); } } } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs index 254a97b75..805b44002 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs @@ -6,5 +6,5 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interface public interface IUpdateOrganizationUserCommand { - Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, IEnumerable collections, IEnumerable? groups); + Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, List collections, IEnumerable? groups); } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs index 8dcac12dd..0abae991d 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs @@ -37,7 +37,7 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand } public async Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, - IEnumerable collections, IEnumerable? groups) + List? collections, IEnumerable? groups) { if (user.Id.Equals(default(Guid))) { @@ -59,14 +59,12 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand throw new BadRequestException("Organization must have at least one confirmed owner."); } - // If the organization is using Flexible Collections, prevent use of any deprecated permissions - var organization = await _organizationRepository.GetByIdAsync(user.OrganizationId); - if (organization.FlexibleCollections && user.AccessAll) + if (user.AccessAll) { throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the user to collections instead."); } - if (organization.FlexibleCollections && collections?.Any() == true) + if (collections?.Count > 0) { var invalidAssociations = collections.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); if (invalidAssociations.Any()) @@ -74,7 +72,6 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); } } - // End Flexible Collections // Only autoscale (if required) after all validation has passed so that we know it's a valid request before // updating Stripe @@ -83,17 +80,13 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand var additionalSmSeatsRequired = await _countNewSmSeatsRequiredQuery.CountNewSmSeatsRequiredAsync(user.OrganizationId, 1); if (additionalSmSeatsRequired > 0) { + var organization = await _organizationRepository.GetByIdAsync(user.OrganizationId); var update = new SecretsManagerSubscriptionUpdate(organization, true) .AdjustSeats(additionalSmSeatsRequired); await _updateSecretsManagerSubscriptionCommand.UpdateSubscriptionAsync(update); } } - if (user.AccessAll) - { - // We don't need any collections if we're flagged to have all access. - collections = new List(); - } await _organizationUserRepository.ReplaceAsync(user, collections); if (groups != null) diff --git a/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs b/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs index 5e66bf445..bce231fc0 100644 --- a/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs +++ b/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs @@ -743,10 +743,6 @@ public class OrganizationService : IOrganizationService AccessSecretsManager = organization.UseSecretsManager, Type = OrganizationUserType.Owner, Status = OrganizationUserStatusType.Confirmed, - - // If using Flexible Collections, AccessAll is deprecated and set to false. - // If not using Flexible Collections, set AccessAll to true (previous behavior) - AccessAll = !organization.FlexibleCollections, CreationDate = organization.CreationDate, RevisionDate = organization.CreationDate }; @@ -771,9 +767,9 @@ public class OrganizationService : IOrganizationService RevisionDate = organization.CreationDate }; - // If using Flexible Collections, give the owner Can Manage access over the default collection + // Give the owner Can Manage access over the default collection List defaultOwnerAccess = null; - if (orgUser != null && organization.FlexibleCollections) + if (orgUser != null) { defaultOwnerAccess = [new CollectionAccessSelection { Id = orgUser.Id, HidePasswords = false, ReadOnly = false, Manage = true }]; @@ -965,15 +961,11 @@ public class OrganizationService : IOrganizationService throw new BadRequestException("This method can only be used to invite a single user."); } - // Validate Collection associations if org is using latest collection enhancements - var organizationAbility = await _applicationCacheService.GetOrganizationAbilityAsync(organizationId); - if (organizationAbility?.FlexibleCollections ?? false) + // Validate Collection associations + var invalidAssociations = invite.Collections?.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); + if (invalidAssociations?.Any() ?? false) { - var invalidAssociations = invite.Collections?.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); - if (invalidAssociations?.Any() ?? false) - { - throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); - } + throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); } var results = await InviteUsersAsync(organizationId, invitingUserId, systemUser, @@ -1038,13 +1030,6 @@ public class OrganizationService : IOrganizationService throw new NotFoundException(); } - // If the organization is using Flexible Collections, prevent use of any deprecated permissions - if (organization.FlexibleCollections && invites.Any(i => i.invite.AccessAll)) - { - throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the user to collections instead."); - } - // End Flexible Collections - var existingEmails = new HashSet(await _organizationUserRepository.SelectKnownEmailsAsync( organizationId, invites.SelectMany(i => i.invite.Emails), false), StringComparer.InvariantCultureIgnoreCase); @@ -1087,8 +1072,8 @@ public class OrganizationService : IOrganizationService throw new BadRequestException("Organization must have at least one confirmed owner."); } - var orgUsers = new List(); - var limitedCollectionOrgUsers = new List<(OrganizationUser, IEnumerable)>(); + var orgUsersWithoutCollections = new List(); + var orgUsersWithCollections = new List<(OrganizationUser, IEnumerable)>(); var orgUserGroups = new List<(OrganizationUser, IEnumerable)>(); var orgUserInvitedCount = 0; var exceptions = new List(); @@ -1114,7 +1099,6 @@ public class OrganizationService : IOrganizationService Key = null, Type = invite.Type.Value, Status = OrganizationUserStatusType.Invited, - AccessAll = invite.AccessAll, AccessSecretsManager = invite.AccessSecretsManager, ExternalId = externalId, CreationDate = DateTime.UtcNow, @@ -1126,13 +1110,13 @@ public class OrganizationService : IOrganizationService orgUser.SetPermissions(invite.Permissions ?? new Permissions()); } - if (!orgUser.AccessAll && invite.Collections.Any()) + if (invite.Collections.Any()) { - limitedCollectionOrgUsers.Add((orgUser, invite.Collections)); + orgUsersWithCollections.Add((orgUser, invite.Collections)); } else { - orgUsers.Add(orgUser); + orgUsersWithoutCollections.Add(orgUser); } if (invite.Groups != null && invite.Groups.Any()) @@ -1155,10 +1139,14 @@ public class OrganizationService : IOrganizationService throw new AggregateException("One or more errors occurred while inviting users.", exceptions); } + var allOrgUsers = orgUsersWithoutCollections + .Concat(orgUsersWithCollections.Select(u => u.Item1)) + .ToList(); + try { - await _organizationUserRepository.CreateManyAsync(orgUsers); - foreach (var (orgUser, collections) in limitedCollectionOrgUsers) + await _organizationUserRepository.CreateManyAsync(orgUsersWithoutCollections); + foreach (var (orgUser, collections) in orgUsersWithCollections) { await _organizationUserRepository.CreateAsync(orgUser, collections); } @@ -1180,7 +1168,7 @@ public class OrganizationService : IOrganizationService await _updateSecretsManagerSubscriptionCommand.UpdateSubscriptionAsync(smSubscriptionUpdate); } - await SendInvitesAsync(orgUsers.Concat(limitedCollectionOrgUsers.Select(u => u.Item1)), organization); + await SendInvitesAsync(allOrgUsers, organization); await _referenceEventService.RaiseEventAsync( new ReferenceEvent(ReferenceEventType.InvitedUsers, organization, _currentContext) @@ -1191,7 +1179,7 @@ public class OrganizationService : IOrganizationService catch (Exception e) { // Revert any added users. - var invitedOrgUserIds = orgUsers.Select(u => u.Id).Concat(limitedCollectionOrgUsers.Select(u => u.Item1.Id)); + var invitedOrgUserIds = allOrgUsers.Select(ou => ou.Id); await _organizationUserRepository.DeleteManyAsync(invitedOrgUserIds); var currentOrganization = await _organizationRepository.GetByIdAsync(organization.Id); @@ -1220,7 +1208,7 @@ public class OrganizationService : IOrganizationService throw new AggregateException("One or more errors occurred while inviting users.", exceptions); } - return (orgUsers, events); + return (allOrgUsers, events); } public async Task>> ResendInvitesAsync(Guid organizationId, Guid? invitingUserId, @@ -1811,7 +1799,6 @@ public class OrganizationService : IOrganizationService { Emails = new List { user.Email }, Type = OrganizationUserType.User, - AccessAll = false, Collections = new List(), AccessSecretsManager = hasStandaloneSecretsManager }; @@ -2519,10 +2506,6 @@ public class OrganizationService : IOrganizationService Key = null, Type = OrganizationUserType.Owner, Status = OrganizationUserStatusType.Invited, - - // If using Flexible Collections, AccessAll is deprecated and set to false. - // If not using Flexible Collections, set AccessAll to true (previous behavior) - AccessAll = !organization.FlexibleCollections, }; await _organizationUserRepository.CreateAsync(ownerOrganizationUser); diff --git a/src/Core/Services/Implementations/CollectionService.cs b/src/Core/Services/Implementations/CollectionService.cs index 24f0cdf04..4b910470e 100644 --- a/src/Core/Services/Implementations/CollectionService.cs +++ b/src/Core/Services/Implementations/CollectionService.cs @@ -55,25 +55,22 @@ public class CollectionService : ICollectionService var groupsList = groups?.ToList(); var usersList = users?.ToList(); - if (org.FlexibleCollections) + // Cannot use Manage with ReadOnly/HidePasswords permissions + var invalidAssociations = groupsList?.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); + if (invalidAssociations?.Any() ?? false) { - // Cannot use Manage with ReadOnly/HidePasswords permissions - var invalidAssociations = groupsList?.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); - if (invalidAssociations?.Any() ?? false) - { - throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); - } + throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); + } - // If using Flexible Collections V1 - a collection should always have someone with Can Manage permissions - if (_featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1)) + // If using Flexible Collections V1 - a collection should always have someone with Can Manage permissions + if (_featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1)) + { + var groupHasManageAccess = groupsList?.Any(g => g.Manage) ?? false; + var userHasManageAccess = usersList?.Any(u => u.Manage) ?? false; + if (!groupHasManageAccess && !userHasManageAccess && !org.AllowAdminAccessToAllCollectionItems) { - var groupHasManageAccess = groupsList?.Any(g => g.Manage) ?? false; - var userHasManageAccess = usersList?.Any(u => u.Manage) ?? false; - if (!groupHasManageAccess && !userHasManageAccess && !org.AllowAdminAccessToAllCollectionItems) - { - throw new BadRequestException( - "At least one member or group must have can manage permission."); - } + throw new BadRequestException( + "At least one member or group must have can manage permission."); } } diff --git a/src/Core/Vault/Services/Implementations/CipherService.cs b/src/Core/Vault/Services/Implementations/CipherService.cs index 4f4905e2a..87a8343a3 100644 --- a/src/Core/Vault/Services/Implementations/CipherService.cs +++ b/src/Core/Vault/Services/Implementations/CipherService.cs @@ -788,15 +788,12 @@ public class CipherService : ICipherService { collection.SetNewId(); newCollections.Add(collection); - if (org.FlexibleCollections) + newCollectionUsers.Add(new CollectionUser { - newCollectionUsers.Add(new CollectionUser - { - CollectionId = collection.Id, - OrganizationUserId = importingOrgUser.Id, - Manage = true - }); - } + CollectionId = collection.Id, + OrganizationUserId = importingOrgUser.Id, + Manage = true + }); } } diff --git a/test/Core.Test/AdminConsole/AutoFixture/OrganizationFixtures.cs b/test/Core.Test/AdminConsole/AutoFixture/OrganizationFixtures.cs index 6ed7eb85f..efb4b1ad4 100644 --- a/test/Core.Test/AdminConsole/AutoFixture/OrganizationFixtures.cs +++ b/test/Core.Test/AdminConsole/AutoFixture/OrganizationFixtures.cs @@ -138,6 +138,9 @@ internal class OrganizationInvite : ICustomization .With(ou => ou.Permissions, PermissionsBlob)); fixture.Customize(composer => composer .With(oi => oi.Type, InviteeUserType)); + // Set Manage to false, this ensures it doesn't conflict with the other properties during validation + fixture.Customize(composer => composer + .With(c => c.Manage, false)); } } diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/CreateGroupCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/CreateGroupCommandTests.cs index bac2630ed..5bf1d0b90 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/CreateGroupCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/CreateGroupCommandTests.cs @@ -20,9 +20,12 @@ namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.Groups; [SutProviderCustomize] public class CreateGroupCommandTests { - [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = false), BitAutoData] + [Theory, OrganizationCustomize(UseGroups = true), BitAutoData] public async Task CreateGroup_Success(SutProvider sutProvider, Organization organization, Group group) { + // Deprecated with Flexible Collections + group.AccessAll = false; + await sutProvider.Sut.CreateGroupAsync(group, organization); await sutProvider.GetDependency().Received(1).CreateAsync(group); @@ -32,9 +35,21 @@ public class CreateGroupCommandTests AssertHelper.AssertRecent(group.RevisionDate); } - [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = false), BitAutoData] + [Theory, OrganizationCustomize(UseGroups = true), BitAutoData] public async Task CreateGroup_WithCollections_Success(SutProvider sutProvider, Organization organization, Group group, List collections) { + // Deprecated with Flexible Collections + group.AccessAll = false; + + // Arrange list of collections to make sure Manage is mutually exclusive + for (var i = 0; i < collections.Count; i++) + { + var cas = collections[i]; + cas.Manage = i != collections.Count - 1; + cas.HidePasswords = i == collections.Count - 1; + cas.ReadOnly = i == collections.Count - 1; + } + await sutProvider.Sut.CreateGroupAsync(group, organization, collections); await sutProvider.GetDependency().Received(1).CreateAsync(group, collections); @@ -44,9 +59,12 @@ public class CreateGroupCommandTests AssertHelper.AssertRecent(group.RevisionDate); } - [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = false), BitAutoData] + [Theory, OrganizationCustomize(UseGroups = true), BitAutoData] public async Task CreateGroup_WithEventSystemUser_Success(SutProvider sutProvider, Organization organization, Group group, EventSystemUser eventSystemUser) { + // Deprecated with Flexible Collections + group.AccessAll = false; + await sutProvider.Sut.CreateGroupAsync(group, organization, eventSystemUser); await sutProvider.GetDependency().Received(1).CreateAsync(group); @@ -56,9 +74,12 @@ public class CreateGroupCommandTests AssertHelper.AssertRecent(group.RevisionDate); } - [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = false), BitAutoData] + [Theory, OrganizationCustomize(UseGroups = true), BitAutoData] public async Task CreateGroup_WithNullOrganization_Throws(SutProvider sutProvider, Group group, EventSystemUser eventSystemUser) { + // Deprecated with Flexible Collections + group.AccessAll = false; + var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.CreateGroupAsync(group, null, eventSystemUser)); Assert.Contains("Organization not found", exception.Message); @@ -68,9 +89,12 @@ public class CreateGroupCommandTests await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().RaiseEventAsync(default); } - [Theory, OrganizationCustomize(UseGroups = false, FlexibleCollections = false), BitAutoData] + [Theory, OrganizationCustomize(UseGroups = false), BitAutoData] public async Task CreateGroup_WithUseGroupsAsFalse_Throws(SutProvider sutProvider, Organization organization, Group group, EventSystemUser eventSystemUser) { + // Deprecated with Flexible Collections + group.AccessAll = false; + var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.CreateGroupAsync(group, organization, eventSystemUser)); Assert.Contains("This organization cannot use groups", exception.Message); @@ -80,8 +104,8 @@ public class CreateGroupCommandTests await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().RaiseEventAsync(default); } - [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] - public async Task CreateGroup_WithFlexibleCollections_WithAccessAll_Throws( + [Theory, OrganizationCustomize(UseGroups = true), BitAutoData] + public async Task CreateGroup_WithAccessAll_Throws( SutProvider sutProvider, Organization organization, Group group) { group.AccessAll = true; diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs index 1b21574fd..45749782f 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs @@ -17,9 +17,12 @@ namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.Groups; [SutProviderCustomize] public class UpdateGroupCommandTests { - [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = false), BitAutoData] + [Theory, OrganizationCustomize(UseGroups = true), BitAutoData] public async Task UpdateGroup_Success(SutProvider sutProvider, Group group, Organization organization) { + // Deprecated with Flexible Collections + group.AccessAll = false; + await sutProvider.Sut.UpdateGroupAsync(group, organization); await sutProvider.GetDependency().Received(1).ReplaceAsync(group); @@ -27,9 +30,21 @@ public class UpdateGroupCommandTests AssertHelper.AssertRecent(group.RevisionDate); } - [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = false), BitAutoData] + [Theory, OrganizationCustomize(UseGroups = true), BitAutoData] public async Task UpdateGroup_WithCollections_Success(SutProvider sutProvider, Group group, Organization organization, List collections) { + // Deprecated with Flexible Collections + group.AccessAll = false; + + // Arrange list of collections to make sure Manage is mutually exclusive + for (var i = 0; i < collections.Count; i++) + { + var cas = collections[i]; + cas.Manage = i != collections.Count - 1; + cas.HidePasswords = i == collections.Count - 1; + cas.ReadOnly = i == collections.Count - 1; + } + await sutProvider.Sut.UpdateGroupAsync(group, organization, collections); await sutProvider.GetDependency().Received(1).ReplaceAsync(group, collections); @@ -37,9 +52,12 @@ public class UpdateGroupCommandTests AssertHelper.AssertRecent(group.RevisionDate); } - [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = false), BitAutoData] + [Theory, OrganizationCustomize(UseGroups = true), BitAutoData] public async Task UpdateGroup_WithEventSystemUser_Success(SutProvider sutProvider, Group group, Organization organization, EventSystemUser eventSystemUser) { + // Deprecated with Flexible Collections + group.AccessAll = false; + await sutProvider.Sut.UpdateGroupAsync(group, organization, eventSystemUser); await sutProvider.GetDependency().Received(1).ReplaceAsync(group); @@ -47,7 +65,7 @@ public class UpdateGroupCommandTests AssertHelper.AssertRecent(group.RevisionDate); } - [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = false), BitAutoData] + [Theory, OrganizationCustomize(UseGroups = true), BitAutoData] public async Task UpdateGroup_WithNullOrganization_Throws(SutProvider sutProvider, Group group, EventSystemUser eventSystemUser) { var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.UpdateGroupAsync(group, null, eventSystemUser)); @@ -58,7 +76,7 @@ public class UpdateGroupCommandTests await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().LogGroupEventAsync(default, default, default); } - [Theory, OrganizationCustomize(UseGroups = false, FlexibleCollections = false), BitAutoData] + [Theory, OrganizationCustomize(UseGroups = false), BitAutoData] public async Task UpdateGroup_WithUseGroupsAsFalse_Throws(SutProvider sutProvider, Organization organization, Group group, EventSystemUser eventSystemUser) { var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.UpdateGroupAsync(group, organization, eventSystemUser)); @@ -69,12 +87,11 @@ public class UpdateGroupCommandTests await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().LogGroupEventAsync(default, default, default); } - [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] - public async Task UpdateGroup_WithFlexibleCollections_WithAccessAll_Throws( + [Theory, OrganizationCustomize(UseGroups = true), BitAutoData] + public async Task UpdateGroup_WithAccessAll_Throws( SutProvider sutProvider, Organization organization, Group group) { group.AccessAll = true; - organization.FlexibleCollections = true; var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.UpdateGroupAsync(group, organization)); diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs index ce2981f35..2216b7896 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs @@ -21,7 +21,7 @@ public class UpdateOrganizationUserCommandTests { [Theory, BitAutoData] public async Task UpdateUserAsync_NoUserId_Throws(OrganizationUser user, Guid? savingUserId, - ICollection collections, IEnumerable groups, SutProvider sutProvider) + List collections, IEnumerable groups, SutProvider sutProvider) { user.Id = default(Guid); var exception = await Assert.ThrowsAsync( @@ -34,7 +34,7 @@ public class UpdateOrganizationUserCommandTests Organization organization, OrganizationUser oldUserData, OrganizationUser newUserData, - ICollection collections, + List collections, IEnumerable groups, Permissions permissions, [OrganizationUser(type: OrganizationUserType.Owner)] OrganizationUser savingUser, @@ -46,6 +46,19 @@ public class UpdateOrganizationUserCommandTests organizationRepository.GetByIdAsync(organization.Id).Returns(organization); + // Deprecated with Flexible Collections + oldUserData.AccessAll = false; + newUserData.AccessAll = false; + + // Arrange list of collections to make sure Manage is mutually exclusive + for (var i = 0; i < collections.Count; i++) + { + var cas = collections[i]; + cas.Manage = i != collections.Count - 1; + cas.HidePasswords = i == collections.Count - 1; + cas.ReadOnly = i == collections.Count - 1; + } + newUserData.Id = oldUserData.Id; newUserData.UserId = oldUserData.UserId; newUserData.OrganizationId = savingUser.OrganizationId = oldUserData.OrganizationId = organization.Id; @@ -78,16 +91,15 @@ public class UpdateOrganizationUserCommandTests } [Theory, BitAutoData] - public async Task UpdateUserAsync_WithFlexibleCollections_WithAccessAll_Throws( + public async Task UpdateUserAsync_WithAccessAll_Throws( Organization organization, [OrganizationUser(type: OrganizationUserType.User)] OrganizationUser oldUserData, [OrganizationUser(type: OrganizationUserType.User)] OrganizationUser newUserData, [OrganizationUser(type: OrganizationUserType.Owner, status: OrganizationUserStatusType.Confirmed)] OrganizationUser savingUser, - ICollection collections, + List collections, IEnumerable groups, SutProvider sutProvider) { - organization.FlexibleCollections = true; newUserData.Id = oldUserData.Id; newUserData.UserId = oldUserData.UserId; newUserData.OrganizationId = oldUserData.OrganizationId = savingUser.OrganizationId = organization.Id; diff --git a/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs b/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs index 398b88174..818b47b5a 100644 --- a/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs +++ b/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs @@ -730,7 +730,6 @@ OrganizationUserInvite invite, SutProvider sutProvider) } [Theory] - [OrganizationCustomize(FlexibleCollections = false)] [BitAutoData(OrganizationUserType.Admin)] [BitAutoData(OrganizationUserType.Owner)] [BitAutoData(OrganizationUserType.User)] @@ -843,7 +842,7 @@ OrganizationUserInvite invite, SutProvider sutProvider) [OrganizationInviteCustomize( InviteeUserType = OrganizationUserType.User, InvitorUserType = OrganizationUserType.Custom - ), OrganizationCustomize(FlexibleCollections = false), BitAutoData] + ), OrganizationCustomize, BitAutoData] public async Task InviteUser_Passes(Organization organization, OrganizationUserInvite invite, string externalId, OrganizationUser invitor, [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.Owner)] OrganizationUser owner, @@ -1152,9 +1151,11 @@ OrganizationUserInvite invite, SutProvider sutProvider) .ReturnsForAnyArgs(Task.FromResult(0)).AndDoes(x => organization.SmSeats += invitedSmUsers); // Throw error at the end of the try block - sutProvider.GetDependency().RaiseEventAsync(default).ThrowsForAnyArgs(); + sutProvider.GetDependency().RaiseEventAsync(default) + .ThrowsForAnyArgs(); - await Assert.ThrowsAsync(async () => await sutProvider.Sut.InviteUsersAsync(organization.Id, savingUser.Id, systemUser: null, invites)); + await Assert.ThrowsAsync(async () => + await sutProvider.Sut.InviteUsersAsync(organization.Id, savingUser.Id, systemUser: null, invites)); // OrgUser is reverted // Note: we don't know what their guids are so comparing length is the best we can do @@ -1182,28 +1183,6 @@ OrganizationUserInvite invite, SutProvider sutProvider) }); } - [Theory, OrganizationCustomize(FlexibleCollections = true), BitAutoData] - public async Task InviteUsers_WithFlexibleCollections_WithAccessAll_Throws(Organization organization, - OrganizationUserInvite invite, OrganizationUser invitor, SutProvider sutProvider) - { - invite.Type = OrganizationUserType.User; - invite.AccessAll = true; - - sutProvider.GetDependency() - .GetByIdAsync(organization.Id) - .Returns(organization); - - sutProvider.GetDependency() - .ManageUsers(organization.Id) - .Returns(true); - - var exception = await Assert.ThrowsAsync( - () => sutProvider.Sut.InviteUsersAsync(organization.Id, invitor.UserId, systemUser: null, - new (OrganizationUserInvite, string)[] { (invite, null) })); - - Assert.Contains("accessall property has been deprecated", exception.Message.ToLowerInvariant()); - } - private void InviteUserHelper_ArrangeValidPermissions(Organization organization, OrganizationUser savingUser, SutProvider sutProvider) { @@ -2336,6 +2315,15 @@ OrganizationUserInvite invite, SutProvider sutProvider) return Task.FromResult>(orgUsers.Select(u => u.Id).ToList()); } ); + + organizationUserRepository.CreateAsync(Arg.Any(), Arg.Any>()).Returns( + info => + { + var orgUser = info.Arg(); + orgUser.Id = Guid.NewGuid(); + return Task.FromResult(orgUser.Id); + } + ); } // Must set real guids in order for dictionary of guids to not throw aggregate exceptions diff --git a/test/Core.Test/AutoFixture/CollectionAccessSelectionFixtures.cs b/test/Core.Test/AutoFixture/CollectionAccessSelectionFixtures.cs index 54b7fb034..923939b47 100644 --- a/test/Core.Test/AutoFixture/CollectionAccessSelectionFixtures.cs +++ b/test/Core.Test/AutoFixture/CollectionAccessSelectionFixtures.cs @@ -8,16 +8,22 @@ namespace Bit.Core.Test.AutoFixture; public class CollectionAccessSelectionCustomization : ICustomization { public bool Manage { get; set; } + public bool ReadOnly { get; set; } + public bool HidePasswords { get; set; } public CollectionAccessSelectionCustomization(bool manage) { Manage = manage; + ReadOnly = !manage; + HidePasswords = !manage; } public void Customize(IFixture fixture) { fixture.Customize(composer => composer - .With(o => o.Manage, Manage)); + .With(o => o.Manage, Manage) + .With(o => o.ReadOnly, ReadOnly) + .With(o => o.HidePasswords, HidePasswords)); } } diff --git a/test/Core.Test/Services/CollectionServiceTests.cs b/test/Core.Test/Services/CollectionServiceTests.cs index d64e648f3..aa8097d2c 100644 --- a/test/Core.Test/Services/CollectionServiceTests.cs +++ b/test/Core.Test/Services/CollectionServiceTests.cs @@ -77,7 +77,7 @@ public class CollectionServiceTest [Theory, BitAutoData] public async Task SaveAsync_OrganizationNotUseGroup_CreateCollectionWithoutGroupsInRepository(Collection collection, - IEnumerable groups, [CollectionAccessSelectionCustomize(true)] IEnumerable users, + [CollectionAccessSelectionCustomize] IEnumerable groups, [CollectionAccessSelectionCustomize(true)] IEnumerable users, Organization organization, SutProvider sutProvider) { collection.Id = default; diff --git a/test/Core.Test/Vault/Services/CipherServiceTests.cs b/test/Core.Test/Vault/Services/CipherServiceTests.cs index 41bad8b00..a0623a6c7 100644 --- a/test/Core.Test/Vault/Services/CipherServiceTests.cs +++ b/test/Core.Test/Vault/Services/CipherServiceTests.cs @@ -26,63 +26,7 @@ namespace Bit.Core.Test.Services; public class CipherServiceTests { [Theory, BitAutoData] - public async Task ImportCiphersAsync_IntoOrganization_WithFlexibleCollectionsDisabled_Success( - Organization organization, - Guid importingUserId, - OrganizationUser importingOrganizationUser, - List collections, - List ciphers, - SutProvider sutProvider) - { - organization.MaxCollections = null; - organization.FlexibleCollections = false; - importingOrganizationUser.OrganizationId = organization.Id; - - foreach (var collection in collections) - { - collection.OrganizationId = organization.Id; - } - - foreach (var cipher in ciphers) - { - cipher.OrganizationId = organization.Id; - } - - KeyValuePair[] collectionRelationships = { - new(0, 0), - new(1, 1), - new(2, 2) - }; - - sutProvider.GetDependency() - .GetByIdAsync(organization.Id) - .Returns(organization); - - sutProvider.GetDependency() - .GetByOrganizationAsync(organization.Id, importingUserId) - .Returns(importingOrganizationUser); - - // Set up a collection that already exists in the organization - sutProvider.GetDependency() - .GetManyByOrganizationIdAsync(organization.Id) - .Returns(new List { collections[0] }); - - await sutProvider.Sut.ImportCiphersAsync(collections, ciphers, collectionRelationships, importingUserId); - - await sutProvider.GetDependency().Received(1).CreateAsync( - ciphers, - Arg.Is>(cols => cols.Count() == collections.Count - 1 && - !cols.Any(c => c.Id == collections[0].Id) && // Check that the collection that already existed in the organization was not added - cols.All(c => collections.Any(x => c.Name == x.Name))), - Arg.Is>(c => c.Count() == ciphers.Count), - Arg.Is>(i => !i.Any())); - await sutProvider.GetDependency().Received(1).PushSyncVaultAsync(importingUserId); - await sutProvider.GetDependency().Received(1).RaiseEventAsync( - Arg.Is(e => e.Type == ReferenceEventType.VaultImported)); - } - - [Theory, BitAutoData] - public async Task ImportCiphersAsync_IntoOrganization_WithFlexibleCollectionsEnabled_Success( + public async Task ImportCiphersAsync_IntoOrganization_Success( Organization organization, Guid importingUserId, OrganizationUser importingOrganizationUser,