From fe702c6535243b4fac88d2021c4abe771f72da94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rui=20Tom=C3=A9?= <108268980+r-tome@users.noreply.github.com> Date: Wed, 29 Nov 2023 17:02:43 +0000 Subject: [PATCH] [AC-1784] Lining up new Manage collection permissions for users with deprecated EditAssignedCollections permission (#3406) * [AC-1784] Setting up collections with permission 'Manage = true' if flexible collections feature flag is off and user has EditAssignedCollections * [AC-1784] Added unit tests * [AC-1784] Deleted duplicated variable --- .../Implementations/CollectionService.cs | 21 ++++- .../Implementations/OrganizationService.cs | 38 ++++++--- .../Services/CollectionServiceTests.cs | 38 +++++++++ .../Services/OrganizationServiceTests.cs | 84 +++++++++++++++++++ 4 files changed, 166 insertions(+), 15 deletions(-) diff --git a/src/Core/Services/Implementations/CollectionService.cs b/src/Core/Services/Implementations/CollectionService.cs index 0e703dced..b36e3b13a 100644 --- a/src/Core/Services/Implementations/CollectionService.cs +++ b/src/Core/Services/Implementations/CollectionService.cs @@ -53,21 +53,36 @@ public class CollectionService : ICollectionService } var groupsList = groups?.ToList(); - var usersList = users?.ToList(); + var usersList = users?.ToList() ?? new List(); // If using Flexible Collections - a collection should always have someone with Can Manage permissions if (_featureService.IsEnabled(FeatureFlagKeys.FlexibleCollections, _currentContext)) { var groupHasManageAccess = groupsList?.Any(g => g.Manage) ?? false; - var userHasManageAccess = usersList?.Any(u => u.Manage) ?? false; + var userHasManageAccess = usersList.Any(u => u.Manage); if (!groupHasManageAccess && !userHasManageAccess) { throw new BadRequestException( "At least one member or group must have can manage permission."); } } + else + { + // If not using Flexible Collections + // all Organization users with EditAssignedCollections permission should have Manage permission for the collection + var organizationUsers = await _organizationUserRepository + .GetManyByOrganizationAsync(collection.OrganizationId, null); + foreach (var orgUser in organizationUsers.Where(ou => ou.GetPermissions()?.EditAssignedCollections ?? false)) + { + var user = usersList.FirstOrDefault(u => u.Id == orgUser.Id); + if (user != null) + { + user.Manage = true; + } + } + } - if (collection.Id == default(Guid)) + if (collection.Id == default) { if (org.MaxCollections.HasValue) { diff --git a/src/Core/Services/Implementations/OrganizationService.cs b/src/Core/Services/Implementations/OrganizationService.cs index 2b3dece37..3184b969a 100644 --- a/src/Core/Services/Implementations/OrganizationService.cs +++ b/src/Core/Services/Implementations/OrganizationService.cs @@ -64,6 +64,9 @@ public class OrganizationService : IOrganizationService private readonly IDataProtectorTokenFactory _orgUserInviteTokenDataFactory; private readonly IFeatureService _featureService; + private bool FlexibleCollectionsIsEnabled => _featureService.IsEnabled(FeatureFlagKeys.FlexibleCollections, _currentContext); + private bool FlexibleCollectionsV1IsEnabled => _featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1, _currentContext); + public OrganizationService( IOrganizationRepository organizationRepository, IOrganizationUserRepository organizationUserRepository, @@ -437,11 +440,6 @@ public class OrganizationService : IOrganizationService await ValidateSignUpPoliciesAsync(signup.Owner.Id); } - var flexibleCollectionsIsEnabled = - _featureService.IsEnabled(FeatureFlagKeys.FlexibleCollections, _currentContext); - var flexibleCollectionsV1IsEnabled = - _featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1, _currentContext); - var organization = new Organization { // Pre-generate the org id so that we can save it with the Stripe subscription.. @@ -479,8 +477,8 @@ public class OrganizationService : IOrganizationService Status = OrganizationStatusType.Created, UsePasswordManager = true, UseSecretsManager = signup.UseSecretsManager, - LimitCollectionCreationDeletion = !flexibleCollectionsIsEnabled, - AllowAdminAccessToAllCollectionItems = !flexibleCollectionsV1IsEnabled + LimitCollectionCreationDeletion = !FlexibleCollectionsIsEnabled, + AllowAdminAccessToAllCollectionItems = !FlexibleCollectionsV1IsEnabled }; if (signup.UseSecretsManager) @@ -937,6 +935,10 @@ public class OrganizationService : IOrganizationService orgUser.Permissions = JsonSerializer.Serialize(invite.Permissions, JsonHelpers.CamelCase); } + // If Flexible Collections is disabled and the user has EditAssignedCollections permission + // grant Manage permission for all assigned collections + invite.Collections = ApplyManageCollectionPermissions(orgUser, invite.Collections); + if (!orgUser.AccessAll && invite.Collections.Any()) { limitedCollectionOrgUsers.Add((orgUser, invite.Collections)); @@ -1323,11 +1325,9 @@ public class OrganizationService : IOrganizationService } } - if (user.AccessAll) - { - // We don't need any collections if we're flagged to have all access. - collections = new List(); - } + // If Flexible Collections is disabled and the user has EditAssignedCollections permission + // grant Manage permission for all assigned collections + collections = ApplyManageCollectionPermissions(user, collections); await _organizationUserRepository.ReplaceAsync(user, collections); if (groups != null) @@ -2440,4 +2440,18 @@ public class OrganizationService : IOrganizationService await _collectionRepository.CreateAsync(defaultCollection); } } + + private IEnumerable ApplyManageCollectionPermissions(OrganizationUser orgUser, IEnumerable collections) + { + if (!FlexibleCollectionsIsEnabled && (orgUser.GetPermissions()?.EditAssignedCollections ?? false)) + { + return collections.Select(c => + { + c.Manage = true; + return c; + }).ToList(); + } + + return collections; + } } diff --git a/test/Core.Test/Services/CollectionServiceTests.cs b/test/Core.Test/Services/CollectionServiceTests.cs index 34384c631..f4b48b41b 100644 --- a/test/Core.Test/Services/CollectionServiceTests.cs +++ b/test/Core.Test/Services/CollectionServiceTests.cs @@ -8,6 +8,7 @@ using Bit.Core.Repositories; using Bit.Core.Services; using Bit.Core.Test.AutoFixture; using Bit.Core.Test.AutoFixture.OrganizationFixtures; +using Bit.Core.Utilities; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; using NSubstitute; @@ -37,6 +38,43 @@ public class CollectionServiceTest Assert.True(collection.RevisionDate - utcNow < TimeSpan.FromSeconds(1)); } + [Theory, BitAutoData] + public async Task SaveAsync_DefaultIdWithUsers_WithOneEditAssignedCollectionsUser_WhileFCFlagDisabled_CreatesCollectionInTheRepository( + Collection collection, Organization organization, + [CollectionAccessSelectionCustomize] IEnumerable users, + SutProvider sutProvider) + { + sutProvider.GetDependency() + .IsEnabled(FeatureFlagKeys.FlexibleCollections, Arg.Any(), Arg.Any()) + .Returns(false); + + collection.Id = default; + collection.OrganizationId = organization.Id; + sutProvider.GetDependency().GetByIdAsync(collection.OrganizationId).Returns(organization); + sutProvider.GetDependency() + .GetManyByOrganizationAsync(collection.OrganizationId, Arg.Any()) + .Returns(new List + { + users.Select(x => new OrganizationUser + { + Id = x.Id, + Type = OrganizationUserType.Custom, + Permissions = CoreHelpers.ClassToJsonData(new Permissions { EditAssignedCollections = true }) + }).First() + }); + var utcNow = DateTime.UtcNow; + + await sutProvider.Sut.SaveAsync(collection, null, users); + + await sutProvider.GetDependency().Received() + .CreateAsync(collection, Arg.Is>(l => l == null), + Arg.Is>(l => l.Count(i => i.Manage == true) == 1)); + await sutProvider.GetDependency().Received() + .LogCollectionEventAsync(collection, EventType.Collection_Created); + Assert.True(collection.CreationDate - utcNow < TimeSpan.FromSeconds(1)); + Assert.True(collection.RevisionDate - utcNow < TimeSpan.FromSeconds(1)); + } + [Theory, BitAutoData] public async Task SaveAsync_DefaultIdWithGroupsAndUsers_CreateCollectionWithGroupsAndUsersInRepository(Collection collection, [CollectionAccessSelectionCustomize(true)] IEnumerable groups, IEnumerable users, Organization organization, SutProvider sutProvider) diff --git a/test/Core.Test/Services/OrganizationServiceTests.cs b/test/Core.Test/Services/OrganizationServiceTests.cs index 147ecac1c..d2be53c11 100644 --- a/test/Core.Test/Services/OrganizationServiceTests.cs +++ b/test/Core.Test/Services/OrganizationServiceTests.cs @@ -23,6 +23,7 @@ using Bit.Core.Repositories; using Bit.Core.Services; using Bit.Core.Settings; using Bit.Core.Test.AdminConsole.AutoFixture; +using Bit.Core.Test.AutoFixture; using Bit.Core.Test.AutoFixture.OrganizationFixtures; using Bit.Core.Test.AutoFixture.OrganizationUserFixtures; using Bit.Core.Tokens; @@ -826,6 +827,53 @@ public class OrganizationServiceTests }); } + [Theory] + [OrganizationInviteCustomize( + InviteeUserType = OrganizationUserType.Custom, + InvitorUserType = OrganizationUserType.Owner + ), BitAutoData] + public async Task InviteUser_WithEditAssignedCollectionsTrue_WhileFCFlagDisabled_SetsCollectionsManageTrue(Organization organization, (OrganizationUserInvite invite, string externalId) invite, + OrganizationUser invitor, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.Owner)] OrganizationUser owner, + SutProvider sutProvider) + { + invite.invite.Permissions = new Permissions { EditAssignedCollections = true }; + + sutProvider.GetDependency() + .IsEnabled(FeatureFlagKeys.FlexibleCollections, Arg.Any(), Arg.Any()) + .Returns(false); + + var organizationRepository = sutProvider.GetDependency(); + var organizationUserRepository = sutProvider.GetDependency(); + var currentContext = sutProvider.GetDependency(); + + organizationRepository.GetByIdAsync(organization.Id).Returns(organization); + organizationUserRepository.GetManyByOrganizationAsync(organization.Id, OrganizationUserType.Owner) + .Returns(new[] { owner }); + currentContext.ManageUsers(organization.Id).Returns(true); + currentContext.EditAssignedCollections(organization.Id).Returns(true); + currentContext.GetOrganization(organization.Id) + .Returns(new CurrentContextOrganization + { + Permissions = new Permissions + { + CreateNewCollections = true, + DeleteAnyCollection = true + } + }); + + await sutProvider.Sut.InviteUsersAsync(organization.Id, invitor.UserId, new[] { invite }); + + await sutProvider.GetDependency() + .Received(invite.invite.Emails.Count()) + .CreateAsync(Arg.Is(ou => + ou.OrganizationId == organization.Id && + ou.Type == invite.invite.Type && + invite.invite.Emails.Contains(ou.Email)), + Arg.Is>(collections => + collections.All(c => c.Manage))); + } + private void InviteUserHelper_ArrangeValidPermissions(Organization organization, OrganizationUser savingUser, SutProvider sutProvider) { @@ -887,6 +935,42 @@ public class OrganizationServiceTests await sutProvider.Sut.SaveUserAsync(newUserData, savingUser.UserId, collections, groups); } + [Theory, BitAutoData] + public async Task SaveUser_WithEditAssignedCollections_WhileFCFlagDisabled_SetsCollectionsManageTrue( + Organization organization, + OrganizationUser oldUserData, + OrganizationUser newUserData, + [CollectionAccessSelectionCustomize] IEnumerable collections, + IEnumerable groups, + [OrganizationUser(type: OrganizationUserType.Owner)] OrganizationUser savingUser, + SutProvider sutProvider) + { + sutProvider.GetDependency() + .IsEnabled(FeatureFlagKeys.FlexibleCollections, Arg.Any(), Arg.Any()) + .Returns(false); + + var organizationRepository = sutProvider.GetDependency(); + var organizationUserRepository = sutProvider.GetDependency(); + var currentContext = sutProvider.GetDependency(); + + organizationRepository.GetByIdAsync(organization.Id).Returns(organization); + + newUserData.Id = oldUserData.Id; + newUserData.UserId = oldUserData.UserId; + newUserData.OrganizationId = savingUser.OrganizationId = oldUserData.OrganizationId = organization.Id; + newUserData.Permissions = CoreHelpers.ClassToJsonData(new Permissions { EditAssignedCollections = true }); + organizationUserRepository.GetByIdAsync(oldUserData.Id).Returns(oldUserData); + organizationUserRepository.GetManyByOrganizationAsync(savingUser.OrganizationId, OrganizationUserType.Owner) + .Returns(new List { savingUser }); + currentContext.OrganizationOwner(savingUser.OrganizationId).Returns(true); + + await sutProvider.Sut.SaveUserAsync(newUserData, savingUser.UserId, collections, groups); + + await sutProvider.GetDependency().Received(1).ReplaceAsync( + Arg.Is(ou => ou.Id == newUserData.Id), + Arg.Is>(i => i.All(c => c.Manage))); + } + [Theory, BitAutoData] public async Task SaveUser_WithCustomType_WhenUseCustomPermissionsIsFalse_Throws( Organization organization,