From de294b8299f8964a621ca375bc83f2416171d5bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rui=20Tom=C3=A9?= <108268980+r-tome@users.noreply.github.com> Date: Fri, 9 Feb 2024 17:57:01 +0000 Subject: [PATCH] [AC-2154] Logging organization data before migrating for flexible collections (#3761) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [AC-2154] Logging organization data before migrating for flexible collections * [AC-2154] Refactored logging command to perform the data migration * [AC-2154] Moved validation inside the command * [AC-2154] PR feedback * [AC-2154] Changed logging level to warning * [AC-2154] Fixed unit test * [AC-2154] Removed logging unnecessary data * [AC-2154] Removed primary constructor * [AC-2154] Added comments --- .../Controllers/OrganizationsController.cs | 16 +-- ...tionEnableCollectionEnhancementsCommand.cs | 12 ++ ...tionEnableCollectionEnhancementsCommand.cs | 112 ++++++++++++++++++ ...OrganizationServiceCollectionExtensions.cs | 8 ++ .../OrganizationsControllerTests.cs | 30 ++--- ...nableCollectionEnhancementsCommandTests.cs | 46 +++++++ 6 files changed, 191 insertions(+), 33 deletions(-) create mode 100644 src/Core/AdminConsole/OrganizationFeatures/OrganizationCollectionEnhancements/Interfaces/IOrganizationEnableCollectionEnhancementsCommand.cs create mode 100644 src/Core/AdminConsole/OrganizationFeatures/OrganizationCollectionEnhancements/OrganizationEnableCollectionEnhancementsCommand.cs create mode 100644 test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationCollectionEnhancements/OrganizationEnableCollectionEnhancementsCommandTests.cs diff --git a/src/Api/AdminConsole/Controllers/OrganizationsController.cs b/src/Api/AdminConsole/Controllers/OrganizationsController.cs index 07b005bfc..c83fce762 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationsController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationsController.cs @@ -14,6 +14,7 @@ using Bit.Core; using Bit.Core.AdminConsole.Enums; using Bit.Core.AdminConsole.Models.Data.Organizations.Policies; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationApiKeys.Interfaces; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationCollectionEnhancements.Interfaces; using Bit.Core.AdminConsole.Repositories; using Bit.Core.Auth.Enums; using Bit.Core.Auth.Repositories; @@ -67,6 +68,7 @@ public class OrganizationsController : Controller private readonly ICancelSubscriptionCommand _cancelSubscriptionCommand; private readonly IGetSubscriptionQuery _getSubscriptionQuery; private readonly IReferenceEventService _referenceEventService; + private readonly IOrganizationEnableCollectionEnhancementsCommand _organizationEnableCollectionEnhancementsCommand; public OrganizationsController( IOrganizationRepository organizationRepository, @@ -92,7 +94,8 @@ public class OrganizationsController : Controller IPushNotificationService pushNotificationService, ICancelSubscriptionCommand cancelSubscriptionCommand, IGetSubscriptionQuery getSubscriptionQuery, - IReferenceEventService referenceEventService) + IReferenceEventService referenceEventService, + IOrganizationEnableCollectionEnhancementsCommand organizationEnableCollectionEnhancementsCommand) { _organizationRepository = organizationRepository; _organizationUserRepository = organizationUserRepository; @@ -118,6 +121,7 @@ public class OrganizationsController : Controller _cancelSubscriptionCommand = cancelSubscriptionCommand; _getSubscriptionQuery = getSubscriptionQuery; _referenceEventService = referenceEventService; + _organizationEnableCollectionEnhancementsCommand = organizationEnableCollectionEnhancementsCommand; } [HttpGet("{id}")] @@ -888,15 +892,7 @@ public class OrganizationsController : Controller throw new NotFoundException(); } - if (organization.FlexibleCollections) - { - throw new BadRequestException("Organization has already been migrated to the new collection enhancements"); - } - - await _organizationRepository.EnableCollectionEnhancements(id); - - organization.FlexibleCollections = true; - await _organizationService.ReplaceAndUpdateCacheAsync(organization); + await _organizationEnableCollectionEnhancementsCommand.EnableCollectionEnhancements(organization); // Force a vault sync for all owners and admins of the organization so that changes show immediately // Custom users are intentionally not handled as they are likely to be less impacted and we want to limit simultaneous syncs diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationCollectionEnhancements/Interfaces/IOrganizationEnableCollectionEnhancementsCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationCollectionEnhancements/Interfaces/IOrganizationEnableCollectionEnhancementsCommand.cs new file mode 100644 index 000000000..58a639c74 --- /dev/null +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationCollectionEnhancements/Interfaces/IOrganizationEnableCollectionEnhancementsCommand.cs @@ -0,0 +1,12 @@ +using Bit.Core.AdminConsole.Entities; + +namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationCollectionEnhancements.Interfaces; + +/// +/// Enable collection enhancements for an organization. +/// This command will be deprecated once all organizations have collection enhancements enabled. +/// +public interface IOrganizationEnableCollectionEnhancementsCommand +{ + Task EnableCollectionEnhancements(Organization organization); +} diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationCollectionEnhancements/OrganizationEnableCollectionEnhancementsCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationCollectionEnhancements/OrganizationEnableCollectionEnhancementsCommand.cs new file mode 100644 index 000000000..da32e9c51 --- /dev/null +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationCollectionEnhancements/OrganizationEnableCollectionEnhancementsCommand.cs @@ -0,0 +1,112 @@ +using Bit.Core.AdminConsole.Entities; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationCollectionEnhancements.Interfaces; +using Bit.Core.AdminConsole.Repositories; +using Bit.Core.Enums; +using Bit.Core.Exceptions; +using Bit.Core.Repositories; +using Bit.Core.Services; +using Microsoft.Extensions.Logging; + +namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationCollectionEnhancements; + +public class OrganizationEnableCollectionEnhancementsCommand : IOrganizationEnableCollectionEnhancementsCommand +{ + private readonly ICollectionRepository _collectionRepository; + private readonly IGroupRepository _groupRepository; + private readonly IOrganizationRepository _organizationRepository; + private readonly IOrganizationUserRepository _organizationUserRepository; + private readonly IOrganizationService _organizationService; + private readonly ILogger _logger; + + public OrganizationEnableCollectionEnhancementsCommand(ICollectionRepository collectionRepository, + IGroupRepository groupRepository, + IOrganizationRepository organizationRepository, + IOrganizationUserRepository organizationUserRepository, + IOrganizationService organizationService, + ILogger logger) + { + _collectionRepository = collectionRepository; + _groupRepository = groupRepository; + _organizationRepository = organizationRepository; + _organizationUserRepository = organizationUserRepository; + _organizationService = organizationService; + _logger = logger; + } + + public async Task EnableCollectionEnhancements(Organization organization) + { + if (organization.FlexibleCollections) + { + throw new BadRequestException("Organization has already been migrated to the new collection enhancements"); + } + + // Log the Organization data that will change when the migration is complete + await LogPreMigrationDataAsync(organization.Id); + + // Run the data migration script + await _organizationRepository.EnableCollectionEnhancements(organization.Id); + + organization.FlexibleCollections = true; + await _organizationService.ReplaceAndUpdateCacheAsync(organization); + } + + /// + /// This method logs the data that will be migrated to the new collection enhancements so that it can be restored if needed + /// + /// + private async Task LogPreMigrationDataAsync(Guid organizationId) + { + var groups = await _groupRepository.GetManyByOrganizationIdAsync(organizationId); + // Grab Group Ids that have AccessAll enabled as it will be removed in the data migration + var groupIdsWithAccessAllEnabled = groups + .Where(g => g.AccessAll) + .Select(g => g.Id) + .ToList(); + + var organizationUsers = await _organizationUserRepository.GetManyByOrganizationAsync(organizationId, type: null); + // Grab OrganizationUser Ids that have AccessAll enabled as it will be removed in the data migration + var organizationUserIdsWithAccessAllEnabled = organizationUsers + .Where(ou => ou.AccessAll) + .Select(ou => ou.Id) + .ToList(); + // Grab OrganizationUser Ids of Manager users as that will be downgraded to User in the data migration + var migratedManagers = organizationUsers + .Where(ou => ou.Type == OrganizationUserType.Manager) + .Select(ou => ou.Id) + .ToList(); + + var usersEligibleToManageCollections = organizationUsers + .Where(ou => + ou.Type == OrganizationUserType.Manager || + (ou.Type == OrganizationUserType.Custom && + !string.IsNullOrEmpty(ou.Permissions) && + ou.GetPermissions().EditAssignedCollections) + ) + .Select(ou => ou.Id) + .ToList(); + var collectionUsers = await _collectionRepository.GetManyByOrganizationIdWithAccessAsync(organizationId); + // Grab CollectionUser permissions that will change in the data migration + var collectionUsersData = collectionUsers.SelectMany(tuple => + tuple.Item2.Users.Select(user => + new + { + CollectionId = tuple.Item1.Id, + OrganizationUserId = user.Id, + user.ReadOnly, + user.HidePasswords + })) + .Where(cud => usersEligibleToManageCollections.Any(ou => ou == cud.OrganizationUserId)) + .ToList(); + + var logObject = new + { + OrganizationId = organizationId, + GroupAccessAll = groupIdsWithAccessAllEnabled, + UserAccessAll = organizationUserIdsWithAccessAllEnabled, + MigratedManagers = migratedManagers, + CollectionUsers = collectionUsersData + }; + + _logger.LogWarning("Flexible Collections data migration started. Backup data: {@LogObject}", logObject); + } +} diff --git a/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs b/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs index e70738e06..3f303e3a9 100644 --- a/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs +++ b/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs @@ -4,6 +4,8 @@ using Bit.Core.AdminConsole.OrganizationFeatures.Groups; using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationApiKeys; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationApiKeys.Interfaces; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationCollectionEnhancements; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationCollectionEnhancements.Interfaces; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationConnections; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationConnections.Interfaces; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationDomains; @@ -50,6 +52,7 @@ public static class OrganizationServiceCollectionExtensions services.AddOrganizationUserCommands(); services.AddOrganizationUserCommandsQueries(); services.AddBaseOrganizationSubscriptionCommandsQueries(); + services.AddOrganizationCollectionEnhancementsCommands(); } private static void AddOrganizationConnectionCommands(this IServiceCollection services) @@ -144,6 +147,11 @@ public static class OrganizationServiceCollectionExtensions services.AddScoped(); } + private static void AddOrganizationCollectionEnhancementsCommands(this IServiceCollection services) + { + services.AddScoped(); + } + private static void AddTokenizers(this IServiceCollection services) { services.AddSingleton>(serviceProvider => diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationsControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationsControllerTests.cs index 45b3d9af3..983470d6f 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationsControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationsControllerTests.cs @@ -5,6 +5,7 @@ using Bit.Api.AdminConsole.Models.Request.Organizations; using Bit.Api.Models.Request.Organizations; using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationApiKeys.Interfaces; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationCollectionEnhancements.Interfaces; using Bit.Core.AdminConsole.Repositories; using Bit.Core.Auth.Entities; using Bit.Core.Auth.Enums; @@ -57,6 +58,7 @@ public class OrganizationsControllerTests : IDisposable private readonly ICancelSubscriptionCommand _cancelSubscriptionCommand; private readonly IGetSubscriptionQuery _getSubscriptionQuery; private readonly IReferenceEventService _referenceEventService; + private readonly IOrganizationEnableCollectionEnhancementsCommand _organizationEnableCollectionEnhancementsCommand; private readonly OrganizationsController _sut; @@ -86,6 +88,7 @@ public class OrganizationsControllerTests : IDisposable _cancelSubscriptionCommand = Substitute.For(); _getSubscriptionQuery = Substitute.For(); _referenceEventService = Substitute.For(); + _organizationEnableCollectionEnhancementsCommand = Substitute.For(); _sut = new OrganizationsController( _organizationRepository, @@ -111,7 +114,8 @@ public class OrganizationsControllerTests : IDisposable _pushNotificationService, _cancelSubscriptionCommand, _getSubscriptionQuery, - _referenceEventService); + _referenceEventService, + _organizationEnableCollectionEnhancementsCommand); } public void Dispose() @@ -390,11 +394,7 @@ public class OrganizationsControllerTests : IDisposable await _sut.EnableCollectionEnhancements(organization.Id); - await _organizationRepository.Received(1).EnableCollectionEnhancements(organization.Id); - await _organizationService.Received(1).ReplaceAndUpdateCacheAsync( - Arg.Is(o => - o.Id == organization.Id && - o.FlexibleCollections)); + await _organizationEnableCollectionEnhancementsCommand.Received(1).EnableCollectionEnhancements(organization); await _pushNotificationService.Received(1).PushSyncVaultAsync(admin.UserId.Value); await _pushNotificationService.Received(1).PushSyncVaultAsync(owner.UserId.Value); await _pushNotificationService.DidNotReceive().PushSyncVaultAsync(user.UserId.Value); @@ -409,23 +409,7 @@ public class OrganizationsControllerTests : IDisposable await Assert.ThrowsAsync(async () => await _sut.EnableCollectionEnhancements(organization.Id)); - await _organizationRepository.DidNotReceiveWithAnyArgs().EnableCollectionEnhancements(Arg.Any()); - await _organizationService.DidNotReceiveWithAnyArgs().ReplaceAndUpdateCacheAsync(Arg.Any()); - await _pushNotificationService.DidNotReceiveWithAnyArgs().PushSyncVaultAsync(Arg.Any()); - } - - [Theory, AutoData] - public async Task EnableCollectionEnhancements_WhenAlreadyMigrated_Throws(Organization organization) - { - organization.FlexibleCollections = true; - _currentContext.OrganizationOwner(organization.Id).Returns(true); - _organizationRepository.GetByIdAsync(organization.Id).Returns(organization); - - var exception = await Assert.ThrowsAsync(async () => await _sut.EnableCollectionEnhancements(organization.Id)); - Assert.Contains("has already been migrated", exception.Message); - - await _organizationRepository.DidNotReceiveWithAnyArgs().EnableCollectionEnhancements(Arg.Any()); - await _organizationService.DidNotReceiveWithAnyArgs().ReplaceAndUpdateCacheAsync(Arg.Any()); + await _organizationEnableCollectionEnhancementsCommand.DidNotReceiveWithAnyArgs().EnableCollectionEnhancements(Arg.Any()); await _pushNotificationService.DidNotReceiveWithAnyArgs().PushSyncVaultAsync(Arg.Any()); } } diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationCollectionEnhancements/OrganizationEnableCollectionEnhancementsCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationCollectionEnhancements/OrganizationEnableCollectionEnhancementsCommandTests.cs new file mode 100644 index 000000000..c63c100ad --- /dev/null +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationCollectionEnhancements/OrganizationEnableCollectionEnhancementsCommandTests.cs @@ -0,0 +1,46 @@ +using Bit.Core.AdminConsole.Entities; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationCollectionEnhancements; +using Bit.Core.Exceptions; +using Bit.Core.Repositories; +using Bit.Core.Services; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using NSubstitute; +using Xunit; + +namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.OrganizationCollectionEnhancements; + +[SutProviderCustomize] +public class OrganizationEnableCollectionEnhancementsCommandTests +{ + [Theory] + [BitAutoData] + public async Task EnableCollectionEnhancements_Success( + SutProvider sutProvider, + Organization organization) + { + organization.FlexibleCollections = false; + + await sutProvider.Sut.EnableCollectionEnhancements(organization); + + await sutProvider.GetDependency().Received(1).EnableCollectionEnhancements(organization.Id); + await sutProvider.GetDependency().Received(1).ReplaceAndUpdateCacheAsync( + Arg.Is(o => + o.Id == organization.Id && + o.FlexibleCollections)); + } + + [Theory] + [BitAutoData] + public async Task EnableCollectionEnhancements_WhenAlreadyMigrated_Throws( + SutProvider sutProvider, + Organization organization) + { + organization.FlexibleCollections = true; + + var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.EnableCollectionEnhancements(organization)); + Assert.Contains("has already been migrated", exception.Message); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().EnableCollectionEnhancements(Arg.Any()); + } +}