From f9941f5dfe4985063174417833a8eb3a02005cd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rui=20Tom=C3=A9?= <108268980+r-tome@users.noreply.github.com> Date: Mon, 4 Dec 2023 12:41:03 +0000 Subject: [PATCH] [AC-1784] Revert changes made on assigning Manage permission to collections (#3501) This reverts commit fe702c6535243b4fac88d2021c4abe771f72da94. --- .../Implementations/OrganizationService.cs | 38 +++------ .../Implementations/CollectionService.cs | 21 +---- .../Services/OrganizationServiceTests.cs | 84 ------------------- .../Services/CollectionServiceTests.cs | 38 --------- 4 files changed, 15 insertions(+), 166 deletions(-) diff --git a/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs b/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs index 3184b969a..2b3dece37 100644 --- a/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs +++ b/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs @@ -64,9 +64,6 @@ 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, @@ -440,6 +437,11 @@ 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.. @@ -477,8 +479,8 @@ public class OrganizationService : IOrganizationService Status = OrganizationStatusType.Created, UsePasswordManager = true, UseSecretsManager = signup.UseSecretsManager, - LimitCollectionCreationDeletion = !FlexibleCollectionsIsEnabled, - AllowAdminAccessToAllCollectionItems = !FlexibleCollectionsV1IsEnabled + LimitCollectionCreationDeletion = !flexibleCollectionsIsEnabled, + AllowAdminAccessToAllCollectionItems = !flexibleCollectionsV1IsEnabled }; if (signup.UseSecretsManager) @@ -935,10 +937,6 @@ 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)); @@ -1325,9 +1323,11 @@ public class OrganizationService : IOrganizationService } } - // If Flexible Collections is disabled and the user has EditAssignedCollections permission - // grant Manage permission for all assigned collections - collections = ApplyManageCollectionPermissions(user, collections); + 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) @@ -2440,18 +2440,4 @@ 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/src/Core/Services/Implementations/CollectionService.cs b/src/Core/Services/Implementations/CollectionService.cs index b36e3b13a..0e703dced 100644 --- a/src/Core/Services/Implementations/CollectionService.cs +++ b/src/Core/Services/Implementations/CollectionService.cs @@ -53,36 +53,21 @@ public class CollectionService : ICollectionService } var groupsList = groups?.ToList(); - var usersList = users?.ToList() ?? new List(); + var usersList = users?.ToList(); // 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); + var userHasManageAccess = usersList?.Any(u => u.Manage) ?? false; 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) + if (collection.Id == default(Guid)) { if (org.MaxCollections.HasValue) { diff --git a/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs b/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs index d2be53c11..147ecac1c 100644 --- a/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs +++ b/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs @@ -23,7 +23,6 @@ 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; @@ -827,53 +826,6 @@ 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) { @@ -935,42 +887,6 @@ 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, diff --git a/test/Core.Test/Services/CollectionServiceTests.cs b/test/Core.Test/Services/CollectionServiceTests.cs index f4b48b41b..34384c631 100644 --- a/test/Core.Test/Services/CollectionServiceTests.cs +++ b/test/Core.Test/Services/CollectionServiceTests.cs @@ -8,7 +8,6 @@ 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; @@ -38,43 +37,6 @@ 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)