From 54f4ba945ef9194e9131bdf2667de78e544d14b7 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Fri, 28 Mar 2025 00:13:56 +1000 Subject: [PATCH] [PM-17558] Remove ShortcutDuplicatePatchRequests feature flag (#5551) * Delete old command and feature flag switch * Rename vNext command * Remove feature flag --- .../Scim/Controllers/v2/GroupsController.cs | 28 +- .../Groups/Interfaces/IPatchGroupCommand.cs | 2 +- .../Interfaces/IPatchGroupCommandvNext.cs | 9 - .../src/Scim/Groups/PatchGroupCommand.cs | 177 ++++---- .../src/Scim/Groups/PatchGroupCommandvNext.cs | 170 -------- .../ScimServiceCollectionExtensions.cs | 1 - .../v2/GroupsControllerPatchTests.cs | 1 + .../v2/GroupsControllerPatchTestsvNext.cs | 251 ------------ .../Groups/PatchGroupCommandTests.cs | 356 +++++++++++----- .../Groups/PatchGroupCommandvNextTests.cs | 381 ------------------ src/Core/Constants.cs | 1 - 11 files changed, 368 insertions(+), 1009 deletions(-) delete mode 100644 bitwarden_license/src/Scim/Groups/Interfaces/IPatchGroupCommandvNext.cs delete mode 100644 bitwarden_license/src/Scim/Groups/PatchGroupCommandvNext.cs delete mode 100644 bitwarden_license/test/Scim.IntegrationTest/Controllers/v2/GroupsControllerPatchTestsvNext.cs delete mode 100644 bitwarden_license/test/Scim.Test/Groups/PatchGroupCommandvNextTests.cs diff --git a/bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs b/bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs index 8c21793a9d..6da4001753 100644 --- a/bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs +++ b/bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs @@ -1,10 +1,8 @@ -using Bit.Core; -using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces; +using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces; using Bit.Core.AdminConsole.Repositories; using Bit.Core.Enums; using Bit.Core.Exceptions; using Bit.Core.Repositories; -using Bit.Core.Services; using Bit.Scim.Groups.Interfaces; using Bit.Scim.Models; using Bit.Scim.Utilities; @@ -24,10 +22,8 @@ public class GroupsController : Controller private readonly IGetGroupsListQuery _getGroupsListQuery; private readonly IDeleteGroupCommand _deleteGroupCommand; private readonly IPatchGroupCommand _patchGroupCommand; - private readonly IPatchGroupCommandvNext _patchGroupCommandvNext; private readonly IPostGroupCommand _postGroupCommand; private readonly IPutGroupCommand _putGroupCommand; - private readonly IFeatureService _featureService; public GroupsController( IGroupRepository groupRepository, @@ -35,10 +31,8 @@ public class GroupsController : Controller IGetGroupsListQuery getGroupsListQuery, IDeleteGroupCommand deleteGroupCommand, IPatchGroupCommand patchGroupCommand, - IPatchGroupCommandvNext patchGroupCommandvNext, IPostGroupCommand postGroupCommand, - IPutGroupCommand putGroupCommand, - IFeatureService featureService + IPutGroupCommand putGroupCommand ) { _groupRepository = groupRepository; @@ -46,10 +40,8 @@ public class GroupsController : Controller _getGroupsListQuery = getGroupsListQuery; _deleteGroupCommand = deleteGroupCommand; _patchGroupCommand = patchGroupCommand; - _patchGroupCommandvNext = patchGroupCommandvNext; _postGroupCommand = postGroupCommand; _putGroupCommand = putGroupCommand; - _featureService = featureService; } [HttpGet("{id}")] @@ -103,21 +95,13 @@ public class GroupsController : Controller [HttpPatch("{id}")] public async Task Patch(Guid organizationId, Guid id, [FromBody] ScimPatchModel model) { - if (_featureService.IsEnabled(FeatureFlagKeys.ShortcutDuplicatePatchRequests)) + var group = await _groupRepository.GetByIdAsync(id); + if (group == null || group.OrganizationId != organizationId) { - var group = await _groupRepository.GetByIdAsync(id); - if (group == null || group.OrganizationId != organizationId) - { - throw new NotFoundException("Group not found."); - } - - await _patchGroupCommandvNext.PatchGroupAsync(group, model); - return new NoContentResult(); + throw new NotFoundException("Group not found."); } - var organization = await _organizationRepository.GetByIdAsync(organizationId); - await _patchGroupCommand.PatchGroupAsync(organization, id, model); - + await _patchGroupCommand.PatchGroupAsync(group, model); return new NoContentResult(); } diff --git a/bitwarden_license/src/Scim/Groups/Interfaces/IPatchGroupCommand.cs b/bitwarden_license/src/Scim/Groups/Interfaces/IPatchGroupCommand.cs index b9516cf706..2856eaa860 100644 --- a/bitwarden_license/src/Scim/Groups/Interfaces/IPatchGroupCommand.cs +++ b/bitwarden_license/src/Scim/Groups/Interfaces/IPatchGroupCommand.cs @@ -5,5 +5,5 @@ namespace Bit.Scim.Groups.Interfaces; public interface IPatchGroupCommand { - Task PatchGroupAsync(Organization organization, Guid id, ScimPatchModel model); + Task PatchGroupAsync(Group group, ScimPatchModel model); } diff --git a/bitwarden_license/src/Scim/Groups/Interfaces/IPatchGroupCommandvNext.cs b/bitwarden_license/src/Scim/Groups/Interfaces/IPatchGroupCommandvNext.cs deleted file mode 100644 index f51cc54079..0000000000 --- a/bitwarden_license/src/Scim/Groups/Interfaces/IPatchGroupCommandvNext.cs +++ /dev/null @@ -1,9 +0,0 @@ -using Bit.Core.AdminConsole.Entities; -using Bit.Scim.Models; - -namespace Bit.Scim.Groups.Interfaces; - -public interface IPatchGroupCommandvNext -{ - Task PatchGroupAsync(Group group, ScimPatchModel model); -} diff --git a/bitwarden_license/src/Scim/Groups/PatchGroupCommand.cs b/bitwarden_license/src/Scim/Groups/PatchGroupCommand.cs index 94d9b7a4c2..ab082fc2a6 100644 --- a/bitwarden_license/src/Scim/Groups/PatchGroupCommand.cs +++ b/bitwarden_license/src/Scim/Groups/PatchGroupCommand.cs @@ -5,8 +5,10 @@ using Bit.Core.AdminConsole.Repositories; using Bit.Core.AdminConsole.Services; using Bit.Core.Enums; using Bit.Core.Exceptions; +using Bit.Core.Repositories; using Bit.Scim.Groups.Interfaces; using Bit.Scim.Models; +using Bit.Scim.Utilities; namespace Bit.Scim.Groups; @@ -16,118 +18,137 @@ public class PatchGroupCommand : IPatchGroupCommand private readonly IGroupService _groupService; private readonly IUpdateGroupCommand _updateGroupCommand; private readonly ILogger _logger; + private readonly IOrganizationRepository _organizationRepository; public PatchGroupCommand( IGroupRepository groupRepository, IGroupService groupService, IUpdateGroupCommand updateGroupCommand, - ILogger logger) + ILogger logger, + IOrganizationRepository organizationRepository) { _groupRepository = groupRepository; _groupService = groupService; _updateGroupCommand = updateGroupCommand; _logger = logger; + _organizationRepository = organizationRepository; } - public async Task PatchGroupAsync(Organization organization, Guid id, ScimPatchModel model) + public async Task PatchGroupAsync(Group group, ScimPatchModel model) { - var group = await _groupRepository.GetByIdAsync(id); - if (group == null || group.OrganizationId != organization.Id) - { - throw new NotFoundException("Group not found."); - } - - var operationHandled = false; foreach (var operation in model.Operations) { - // Replace operations - if (operation.Op?.ToLowerInvariant() == "replace") - { - // Replace a list of members - if (operation.Path?.ToLowerInvariant() == "members") + await HandleOperationAsync(group, operation); + } + } + + private async Task HandleOperationAsync(Group group, ScimPatchModel.OperationModel operation) + { + switch (operation.Op?.ToLowerInvariant()) + { + // Replace a list of members + case PatchOps.Replace when operation.Path?.ToLowerInvariant() == PatchPaths.Members: { var ids = GetOperationValueIds(operation.Value); await _groupRepository.UpdateUsersAsync(group.Id, ids); - operationHandled = true; + break; } - // Replace group name from path - else if (operation.Path?.ToLowerInvariant() == "displayname") + + // Replace group name from path + case PatchOps.Replace when operation.Path?.ToLowerInvariant() == PatchPaths.DisplayName: { group.Name = operation.Value.GetString(); + var organization = await _organizationRepository.GetByIdAsync(group.OrganizationId); + if (organization == null) + { + throw new NotFoundException(); + } await _updateGroupCommand.UpdateGroupAsync(group, organization, EventSystemUser.SCIM); - operationHandled = true; + break; } - // Replace group name from value object - else if (string.IsNullOrWhiteSpace(operation.Path) && - operation.Value.TryGetProperty("displayName", out var displayNameProperty)) + + // Replace group name from value object + case PatchOps.Replace when + string.IsNullOrWhiteSpace(operation.Path) && + operation.Value.TryGetProperty("displayName", out var displayNameProperty): { group.Name = displayNameProperty.GetString(); + var organization = await _organizationRepository.GetByIdAsync(group.OrganizationId); + if (organization == null) + { + throw new NotFoundException(); + } await _updateGroupCommand.UpdateGroupAsync(group, organization, EventSystemUser.SCIM); - operationHandled = true; + break; } - } + // Add a single member - else if (operation.Op?.ToLowerInvariant() == "add" && + case PatchOps.Add when !string.IsNullOrWhiteSpace(operation.Path) && - operation.Path.ToLowerInvariant().StartsWith("members[value eq ")) - { - var addId = GetOperationPathId(operation.Path); - if (addId.HasValue) + operation.Path.StartsWith("members[value eq ", StringComparison.OrdinalIgnoreCase) && + TryGetOperationPathId(operation.Path, out var addId): + { + await AddMembersAsync(group, [addId]); + break; + } + + // Add a list of members + case PatchOps.Add when + operation.Path?.ToLowerInvariant() == PatchPaths.Members: + { + await AddMembersAsync(group, GetOperationValueIds(operation.Value)); + break; + } + + // Remove a single member + case PatchOps.Remove when + !string.IsNullOrWhiteSpace(operation.Path) && + operation.Path.StartsWith("members[value eq ", StringComparison.OrdinalIgnoreCase) && + TryGetOperationPathId(operation.Path, out var removeId): + { + await _groupService.DeleteUserAsync(group, removeId, EventSystemUser.SCIM); + break; + } + + // Remove a list of members + case PatchOps.Remove when + operation.Path?.ToLowerInvariant() == PatchPaths.Members: { var orgUserIds = (await _groupRepository.GetManyUserIdsByIdAsync(group.Id)).ToHashSet(); - orgUserIds.Add(addId.Value); + foreach (var v in GetOperationValueIds(operation.Value)) + { + orgUserIds.Remove(v); + } await _groupRepository.UpdateUsersAsync(group.Id, orgUserIds); - operationHandled = true; + break; } - } - // Add a list of members - else if (operation.Op?.ToLowerInvariant() == "add" && - operation.Path?.ToLowerInvariant() == "members") - { - var orgUserIds = (await _groupRepository.GetManyUserIdsByIdAsync(group.Id)).ToHashSet(); - foreach (var v in GetOperationValueIds(operation.Value)) - { - orgUserIds.Add(v); - } - await _groupRepository.UpdateUsersAsync(group.Id, orgUserIds); - operationHandled = true; - } - // Remove a single member - else if (operation.Op?.ToLowerInvariant() == "remove" && - !string.IsNullOrWhiteSpace(operation.Path) && - operation.Path.ToLowerInvariant().StartsWith("members[value eq ")) - { - var removeId = GetOperationPathId(operation.Path); - if (removeId.HasValue) - { - await _groupService.DeleteUserAsync(group, removeId.Value, EventSystemUser.SCIM); - operationHandled = true; - } - } - // Remove a list of members - else if (operation.Op?.ToLowerInvariant() == "remove" && - operation.Path?.ToLowerInvariant() == "members") - { - var orgUserIds = (await _groupRepository.GetManyUserIdsByIdAsync(group.Id)).ToHashSet(); - foreach (var v in GetOperationValueIds(operation.Value)) - { - orgUserIds.Remove(v); - } - await _groupRepository.UpdateUsersAsync(group.Id, orgUserIds); - operationHandled = true; - } - } - if (!operationHandled) - { - _logger.LogWarning("Group patch operation not handled: {0} : ", - string.Join(", ", model.Operations.Select(o => $"{o.Op}:{o.Path}"))); + default: + { + _logger.LogWarning("Group patch operation not handled: {OperationOp}:{OperationPath}", operation.Op, operation.Path); + break; + } } } - private List GetOperationValueIds(JsonElement objArray) + private async Task AddMembersAsync(Group group, HashSet usersToAdd) { - var ids = new List(); + // Azure Entra ID is known to send redundant "add" requests for each existing member every time any member + // is removed. To avoid excessive load on the database, we check against the high availability replica and + // return early if they already exist. + var groupMembers = await _groupRepository.GetManyUserIdsByIdAsync(group.Id, useReadOnlyReplica: true); + if (usersToAdd.IsSubsetOf(groupMembers)) + { + _logger.LogDebug("Ignoring duplicate SCIM request to add members {Members} to group {Group}", usersToAdd, group.Id); + return; + } + + await _groupRepository.AddGroupUsersByIdAsync(group.Id, usersToAdd); + } + + private static HashSet GetOperationValueIds(JsonElement objArray) + { + var ids = new HashSet(); foreach (var obj in objArray.EnumerateArray()) { if (obj.TryGetProperty("value", out var valueProperty)) @@ -141,13 +162,9 @@ public class PatchGroupCommand : IPatchGroupCommand return ids; } - private Guid? GetOperationPathId(string path) + private static bool TryGetOperationPathId(string path, out Guid pathId) { // Parse Guid from string like: members[value eq "{GUID}"}] - if (Guid.TryParse(path.Substring(18).Replace("\"]", string.Empty), out var id)) - { - return id; - } - return null; + return Guid.TryParse(path.Substring(18).Replace("\"]", string.Empty), out pathId); } } diff --git a/bitwarden_license/src/Scim/Groups/PatchGroupCommandvNext.cs b/bitwarden_license/src/Scim/Groups/PatchGroupCommandvNext.cs deleted file mode 100644 index 359df4bc94..0000000000 --- a/bitwarden_license/src/Scim/Groups/PatchGroupCommandvNext.cs +++ /dev/null @@ -1,170 +0,0 @@ -using System.Text.Json; -using Bit.Core.AdminConsole.Entities; -using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces; -using Bit.Core.AdminConsole.Repositories; -using Bit.Core.AdminConsole.Services; -using Bit.Core.Enums; -using Bit.Core.Exceptions; -using Bit.Core.Repositories; -using Bit.Scim.Groups.Interfaces; -using Bit.Scim.Models; -using Bit.Scim.Utilities; - -namespace Bit.Scim.Groups; - -public class PatchGroupCommandvNext : IPatchGroupCommandvNext -{ - private readonly IGroupRepository _groupRepository; - private readonly IGroupService _groupService; - private readonly IUpdateGroupCommand _updateGroupCommand; - private readonly ILogger _logger; - private readonly IOrganizationRepository _organizationRepository; - - public PatchGroupCommandvNext( - IGroupRepository groupRepository, - IGroupService groupService, - IUpdateGroupCommand updateGroupCommand, - ILogger logger, - IOrganizationRepository organizationRepository) - { - _groupRepository = groupRepository; - _groupService = groupService; - _updateGroupCommand = updateGroupCommand; - _logger = logger; - _organizationRepository = organizationRepository; - } - - public async Task PatchGroupAsync(Group group, ScimPatchModel model) - { - foreach (var operation in model.Operations) - { - await HandleOperationAsync(group, operation); - } - } - - private async Task HandleOperationAsync(Group group, ScimPatchModel.OperationModel operation) - { - switch (operation.Op?.ToLowerInvariant()) - { - // Replace a list of members - case PatchOps.Replace when operation.Path?.ToLowerInvariant() == PatchPaths.Members: - { - var ids = GetOperationValueIds(operation.Value); - await _groupRepository.UpdateUsersAsync(group.Id, ids); - break; - } - - // Replace group name from path - case PatchOps.Replace when operation.Path?.ToLowerInvariant() == PatchPaths.DisplayName: - { - group.Name = operation.Value.GetString(); - var organization = await _organizationRepository.GetByIdAsync(group.OrganizationId); - if (organization == null) - { - throw new NotFoundException(); - } - await _updateGroupCommand.UpdateGroupAsync(group, organization, EventSystemUser.SCIM); - break; - } - - // Replace group name from value object - case PatchOps.Replace when - string.IsNullOrWhiteSpace(operation.Path) && - operation.Value.TryGetProperty("displayName", out var displayNameProperty): - { - group.Name = displayNameProperty.GetString(); - var organization = await _organizationRepository.GetByIdAsync(group.OrganizationId); - if (organization == null) - { - throw new NotFoundException(); - } - await _updateGroupCommand.UpdateGroupAsync(group, organization, EventSystemUser.SCIM); - break; - } - - // Add a single member - case PatchOps.Add when - !string.IsNullOrWhiteSpace(operation.Path) && - operation.Path.StartsWith("members[value eq ", StringComparison.OrdinalIgnoreCase) && - TryGetOperationPathId(operation.Path, out var addId): - { - await AddMembersAsync(group, [addId]); - break; - } - - // Add a list of members - case PatchOps.Add when - operation.Path?.ToLowerInvariant() == PatchPaths.Members: - { - await AddMembersAsync(group, GetOperationValueIds(operation.Value)); - break; - } - - // Remove a single member - case PatchOps.Remove when - !string.IsNullOrWhiteSpace(operation.Path) && - operation.Path.StartsWith("members[value eq ", StringComparison.OrdinalIgnoreCase) && - TryGetOperationPathId(operation.Path, out var removeId): - { - await _groupService.DeleteUserAsync(group, removeId, EventSystemUser.SCIM); - break; - } - - // Remove a list of members - case PatchOps.Remove when - operation.Path?.ToLowerInvariant() == PatchPaths.Members: - { - var orgUserIds = (await _groupRepository.GetManyUserIdsByIdAsync(group.Id)).ToHashSet(); - foreach (var v in GetOperationValueIds(operation.Value)) - { - orgUserIds.Remove(v); - } - await _groupRepository.UpdateUsersAsync(group.Id, orgUserIds); - break; - } - - default: - { - _logger.LogWarning("Group patch operation not handled: {OperationOp}:{OperationPath}", operation.Op, operation.Path); - break; - } - } - } - - private async Task AddMembersAsync(Group group, HashSet usersToAdd) - { - // Azure Entra ID is known to send redundant "add" requests for each existing member every time any member - // is removed. To avoid excessive load on the database, we check against the high availability replica and - // return early if they already exist. - var groupMembers = await _groupRepository.GetManyUserIdsByIdAsync(group.Id, useReadOnlyReplica: true); - if (usersToAdd.IsSubsetOf(groupMembers)) - { - _logger.LogDebug("Ignoring duplicate SCIM request to add members {Members} to group {Group}", usersToAdd, group.Id); - return; - } - - await _groupRepository.AddGroupUsersByIdAsync(group.Id, usersToAdd); - } - - private static HashSet GetOperationValueIds(JsonElement objArray) - { - var ids = new HashSet(); - foreach (var obj in objArray.EnumerateArray()) - { - if (obj.TryGetProperty("value", out var valueProperty)) - { - if (valueProperty.TryGetGuid(out var guid)) - { - ids.Add(guid); - } - } - } - return ids; - } - - private static bool TryGetOperationPathId(string path, out Guid pathId) - { - // Parse Guid from string like: members[value eq "{GUID}"}] - return Guid.TryParse(path.Substring(18).Replace("\"]", string.Empty), out pathId); - } -} diff --git a/bitwarden_license/src/Scim/Utilities/ScimServiceCollectionExtensions.cs b/bitwarden_license/src/Scim/Utilities/ScimServiceCollectionExtensions.cs index b5d866524a..75b60a71fc 100644 --- a/bitwarden_license/src/Scim/Utilities/ScimServiceCollectionExtensions.cs +++ b/bitwarden_license/src/Scim/Utilities/ScimServiceCollectionExtensions.cs @@ -10,7 +10,6 @@ public static class ScimServiceCollectionExtensions public static void AddScimGroupCommands(this IServiceCollection services) { services.AddScoped(); - services.AddScoped(); services.AddScoped(); services.AddScoped(); } diff --git a/bitwarden_license/test/Scim.IntegrationTest/Controllers/v2/GroupsControllerPatchTests.cs b/bitwarden_license/test/Scim.IntegrationTest/Controllers/v2/GroupsControllerPatchTests.cs index eaa5b3dcd7..66ce386d07 100644 --- a/bitwarden_license/test/Scim.IntegrationTest/Controllers/v2/GroupsControllerPatchTests.cs +++ b/bitwarden_license/test/Scim.IntegrationTest/Controllers/v2/GroupsControllerPatchTests.cs @@ -20,6 +20,7 @@ public class GroupsControllerPatchTests : IClassFixture, { var databaseContext = _factory.GetDatabaseContext(); _factory.ReinitializeDbForTests(databaseContext); + return Task.CompletedTask; } diff --git a/bitwarden_license/test/Scim.IntegrationTest/Controllers/v2/GroupsControllerPatchTestsvNext.cs b/bitwarden_license/test/Scim.IntegrationTest/Controllers/v2/GroupsControllerPatchTestsvNext.cs deleted file mode 100644 index f66184a8a2..0000000000 --- a/bitwarden_license/test/Scim.IntegrationTest/Controllers/v2/GroupsControllerPatchTestsvNext.cs +++ /dev/null @@ -1,251 +0,0 @@ -using System.Text.Json; -using Bit.Core; -using Bit.Core.AdminConsole.Entities; -using Bit.Core.Services; -using Bit.Scim.Groups.Interfaces; -using Bit.Scim.IntegrationTest.Factories; -using Bit.Scim.Models; -using Bit.Scim.Utilities; -using Bit.Test.Common.Helpers; -using NSubstitute; -using NSubstitute.ExceptionExtensions; -using Xunit; - -namespace Bit.Scim.IntegrationTest.Controllers.v2; - -public class GroupsControllerPatchTestsvNext : IClassFixture, IAsyncLifetime -{ - private readonly ScimApplicationFactory _factory; - - public GroupsControllerPatchTestsvNext(ScimApplicationFactory factory) - { - _factory = factory; - - // Enable the feature flag for new PatchGroupsCommand and stub out the old command to be safe - _factory.SubstituteService((IFeatureService featureService) - => featureService.IsEnabled(FeatureFlagKeys.ShortcutDuplicatePatchRequests).Returns(true)); - _factory.SubstituteService((IPatchGroupCommand patchGroupCommand) - => patchGroupCommand.PatchGroupAsync(Arg.Any(), Arg.Any(), Arg.Any()) - .ThrowsAsync(new Exception("This test suite should be testing the vNext command, but the existing command was called."))); - } - - public Task InitializeAsync() - { - var databaseContext = _factory.GetDatabaseContext(); - _factory.ReinitializeDbForTests(databaseContext); - - return Task.CompletedTask; - } - - Task IAsyncLifetime.DisposeAsync() => Task.CompletedTask; - - [Fact] - public async Task Patch_ReplaceDisplayName_Success() - { - var organizationId = ScimApplicationFactory.TestOrganizationId1; - var groupId = ScimApplicationFactory.TestGroupId1; - var newDisplayName = "Patch Display Name"; - var inputModel = new ScimPatchModel - { - Operations = new List() - { - new ScimPatchModel.OperationModel - { - Op = "replace", - Value = JsonDocument.Parse($"{{\"displayName\":\"{newDisplayName}\"}}").RootElement - } - }, - Schemas = new List() { ScimConstants.Scim2SchemaGroup } - }; - - var context = await _factory.GroupsPatchAsync(organizationId, groupId, inputModel); - - Assert.Equal(StatusCodes.Status204NoContent, context.Response.StatusCode); - - var databaseContext = _factory.GetDatabaseContext(); - var group = databaseContext.Groups.FirstOrDefault(g => g.Id == groupId); - Assert.Equal(newDisplayName, group.Name); - - Assert.Equal(ScimApplicationFactory.InitialGroupUsersCount, databaseContext.GroupUsers.Count()); - Assert.True(databaseContext.GroupUsers.Any(gu => gu.OrganizationUserId == ScimApplicationFactory.TestOrganizationUserId1)); - Assert.True(databaseContext.GroupUsers.Any(gu => gu.OrganizationUserId == ScimApplicationFactory.TestOrganizationUserId4)); - } - - [Fact] - public async Task Patch_ReplaceMembers_Success() - { - var organizationId = ScimApplicationFactory.TestOrganizationId1; - var groupId = ScimApplicationFactory.TestGroupId1; - var inputModel = new ScimPatchModel - { - Operations = new List() - { - new ScimPatchModel.OperationModel - { - Op = "replace", - Path = "members", - Value = JsonDocument.Parse($"[{{\"value\":\"{ScimApplicationFactory.TestOrganizationUserId2}\"}}]").RootElement - } - }, - Schemas = new List() { ScimConstants.Scim2SchemaGroup } - }; - - var context = await _factory.GroupsPatchAsync(organizationId, groupId, inputModel); - - Assert.Equal(StatusCodes.Status204NoContent, context.Response.StatusCode); - - var databaseContext = _factory.GetDatabaseContext(); - Assert.Single(databaseContext.GroupUsers); - - Assert.Equal(ScimApplicationFactory.InitialGroupUsersCount - 1, databaseContext.GroupUsers.Count()); - var groupUser = databaseContext.GroupUsers.FirstOrDefault(); - Assert.Equal(ScimApplicationFactory.TestOrganizationUserId2, groupUser.OrganizationUserId); - } - - [Fact] - public async Task Patch_AddSingleMember_Success() - { - var organizationId = ScimApplicationFactory.TestOrganizationId1; - var groupId = ScimApplicationFactory.TestGroupId1; - var inputModel = new ScimPatchModel - { - Operations = new List() - { - new ScimPatchModel.OperationModel - { - Op = "add", - Path = $"members[value eq \"{ScimApplicationFactory.TestOrganizationUserId2}\"]", - Value = JsonDocument.Parse("{}").RootElement - } - }, - Schemas = new List() { ScimConstants.Scim2SchemaGroup } - }; - - var context = await _factory.GroupsPatchAsync(organizationId, groupId, inputModel); - - Assert.Equal(StatusCodes.Status204NoContent, context.Response.StatusCode); - - var databaseContext = _factory.GetDatabaseContext(); - Assert.Equal(ScimApplicationFactory.InitialGroupUsersCount + 1, databaseContext.GroupUsers.Count()); - Assert.True(databaseContext.GroupUsers.Any(gu => gu.GroupId == groupId && gu.OrganizationUserId == ScimApplicationFactory.TestOrganizationUserId1)); - Assert.True(databaseContext.GroupUsers.Any(gu => gu.GroupId == groupId && gu.OrganizationUserId == ScimApplicationFactory.TestOrganizationUserId2)); - Assert.True(databaseContext.GroupUsers.Any(gu => gu.GroupId == groupId && gu.OrganizationUserId == ScimApplicationFactory.TestOrganizationUserId4)); - } - - [Fact] - public async Task Patch_AddListMembers_Success() - { - var organizationId = ScimApplicationFactory.TestOrganizationId1; - var groupId = ScimApplicationFactory.TestGroupId2; - var inputModel = new ScimPatchModel - { - Operations = new List() - { - new ScimPatchModel.OperationModel - { - Op = "add", - Path = "members", - Value = JsonDocument.Parse($"[{{\"value\":\"{ScimApplicationFactory.TestOrganizationUserId2}\"}},{{\"value\":\"{ScimApplicationFactory.TestOrganizationUserId3}\"}}]").RootElement - } - }, - Schemas = new List() { ScimConstants.Scim2SchemaGroup } - }; - - var context = await _factory.GroupsPatchAsync(organizationId, groupId, inputModel); - - Assert.Equal(StatusCodes.Status204NoContent, context.Response.StatusCode); - - var databaseContext = _factory.GetDatabaseContext(); - Assert.True(databaseContext.GroupUsers.Any(gu => gu.GroupId == groupId && gu.OrganizationUserId == ScimApplicationFactory.TestOrganizationUserId2)); - Assert.True(databaseContext.GroupUsers.Any(gu => gu.GroupId == groupId && gu.OrganizationUserId == ScimApplicationFactory.TestOrganizationUserId3)); - } - - [Fact] - public async Task Patch_RemoveSingleMember_ReplaceDisplayName_Success() - { - var organizationId = ScimApplicationFactory.TestOrganizationId1; - var groupId = ScimApplicationFactory.TestGroupId1; - var newDisplayName = "Patch Display Name"; - var inputModel = new ScimPatchModel - { - Operations = new List() - { - new ScimPatchModel.OperationModel - { - Op = "remove", - Path = $"members[value eq \"{ScimApplicationFactory.TestOrganizationUserId1}\"]", - Value = JsonDocument.Parse("{}").RootElement - }, - new ScimPatchModel.OperationModel - { - Op = "replace", - Value = JsonDocument.Parse($"{{\"displayName\":\"{newDisplayName}\"}}").RootElement - } - }, - Schemas = new List() { ScimConstants.Scim2SchemaGroup } - }; - - var context = await _factory.GroupsPatchAsync(organizationId, groupId, inputModel); - - Assert.Equal(StatusCodes.Status204NoContent, context.Response.StatusCode); - - var databaseContext = _factory.GetDatabaseContext(); - Assert.Equal(ScimApplicationFactory.InitialGroupUsersCount - 1, databaseContext.GroupUsers.Count()); - Assert.Equal(ScimApplicationFactory.InitialGroupCount, databaseContext.Groups.Count()); - - var group = databaseContext.Groups.FirstOrDefault(g => g.Id == groupId); - Assert.Equal(newDisplayName, group.Name); - } - - [Fact] - public async Task Patch_RemoveListMembers_Success() - { - var organizationId = ScimApplicationFactory.TestOrganizationId1; - var groupId = ScimApplicationFactory.TestGroupId1; - var inputModel = new ScimPatchModel - { - Operations = new List() - { - new ScimPatchModel.OperationModel - { - Op = "remove", - Path = "members", - Value = JsonDocument.Parse($"[{{\"value\":\"{ScimApplicationFactory.TestOrganizationUserId1}\"}}, {{\"value\":\"{ScimApplicationFactory.TestOrganizationUserId4}\"}}]").RootElement - } - }, - Schemas = new List() { ScimConstants.Scim2SchemaGroup } - }; - - var context = await _factory.GroupsPatchAsync(organizationId, groupId, inputModel); - - Assert.Equal(StatusCodes.Status204NoContent, context.Response.StatusCode); - - var databaseContext = _factory.GetDatabaseContext(); - Assert.Empty(databaseContext.GroupUsers); - } - - [Fact] - public async Task Patch_NotFound() - { - var organizationId = ScimApplicationFactory.TestOrganizationId1; - var groupId = Guid.NewGuid(); - var inputModel = new Models.ScimPatchModel - { - Operations = new List(), - Schemas = new List() { ScimConstants.Scim2SchemaGroup } - }; - var expectedResponse = new ScimErrorResponseModel - { - Status = StatusCodes.Status404NotFound, - Detail = "Group not found.", - Schemas = new List { ScimConstants.Scim2SchemaError } - }; - - var context = await _factory.GroupsPatchAsync(organizationId, groupId, inputModel); - - Assert.Equal(StatusCodes.Status404NotFound, context.Response.StatusCode); - - var responseModel = JsonSerializer.Deserialize(context.Response.Body, new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }); - AssertHelper.AssertPropertyEqual(expectedResponse, responseModel); - } -} diff --git a/bitwarden_license/test/Scim.Test/Groups/PatchGroupCommandTests.cs b/bitwarden_license/test/Scim.Test/Groups/PatchGroupCommandTests.cs index ff8cb3b546..1b02e62970 100644 --- a/bitwarden_license/test/Scim.Test/Groups/PatchGroupCommandTests.cs +++ b/bitwarden_license/test/Scim.Test/Groups/PatchGroupCommandTests.cs @@ -1,15 +1,18 @@ using System.Text.Json; +using AutoFixture; using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces; using Bit.Core.AdminConsole.Repositories; using Bit.Core.AdminConsole.Services; using Bit.Core.Enums; using Bit.Core.Exceptions; +using Bit.Core.Repositories; using Bit.Scim.Groups; using Bit.Scim.Models; using Bit.Scim.Utilities; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.Extensions.Logging; using NSubstitute; using Xunit; @@ -20,19 +23,16 @@ public class PatchGroupCommandTests { [Theory] [BitAutoData] - public async Task PatchGroup_ReplaceListMembers_Success(SutProvider sutProvider, Organization organization, Group group, IEnumerable userIds) + public async Task PatchGroup_ReplaceListMembers_Success(SutProvider sutProvider, + Organization organization, Group group, IEnumerable userIds) { group.OrganizationId = organization.Id; - sutProvider.GetDependency() - .GetByIdAsync(group.Id) - .Returns(group); - - var scimPatchModel = new Models.ScimPatchModel + var scimPatchModel = new ScimPatchModel { Operations = new List { - new ScimPatchModel.OperationModel + new() { Op = "replace", Path = "members", @@ -42,26 +42,31 @@ public class PatchGroupCommandTests Schemas = new List { ScimConstants.Scim2SchemaUser } }; - await sutProvider.Sut.PatchGroupAsync(organization, group.Id, scimPatchModel); + await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); - await sutProvider.GetDependency().Received(1).UpdateUsersAsync(group.Id, Arg.Is>(arg => arg.All(id => userIds.Contains(id)))); + await sutProvider.GetDependency().Received(1).UpdateUsersAsync( + group.Id, + Arg.Is>(arg => + arg.Count() == userIds.Count() && + arg.ToHashSet().SetEquals(userIds))); } [Theory] [BitAutoData] - public async Task PatchGroup_ReplaceDisplayNameFromPath_Success(SutProvider sutProvider, Organization organization, Group group, string displayName) + public async Task PatchGroup_ReplaceDisplayNameFromPath_Success( + SutProvider sutProvider, Organization organization, Group group, string displayName) { group.OrganizationId = organization.Id; - sutProvider.GetDependency() - .GetByIdAsync(group.Id) - .Returns(group); + sutProvider.GetDependency() + .GetByIdAsync(organization.Id) + .Returns(organization); - var scimPatchModel = new Models.ScimPatchModel + var scimPatchModel = new ScimPatchModel { Operations = new List { - new ScimPatchModel.OperationModel + new() { Op = "replace", Path = "displayname", @@ -71,27 +76,55 @@ public class PatchGroupCommandTests Schemas = new List { ScimConstants.Scim2SchemaUser } }; - await sutProvider.Sut.PatchGroupAsync(organization, group.Id, scimPatchModel); + await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); await sutProvider.GetDependency().Received(1).UpdateGroupAsync(group, organization, EventSystemUser.SCIM); Assert.Equal(displayName, group.Name); } + [Theory] + [BitAutoData] + public async Task PatchGroup_ReplaceDisplayNameFromPath_MissingOrganization_Throws( + SutProvider sutProvider, Organization organization, Group group, string displayName) + { + group.OrganizationId = organization.Id; + + sutProvider.GetDependency() + .GetByIdAsync(organization.Id) + .Returns((Organization)null); + + var scimPatchModel = new ScimPatchModel + { + Operations = new List + { + new() + { + Op = "replace", + Path = "displayname", + Value = JsonDocument.Parse($"\"{displayName}\"").RootElement + } + }, + Schemas = new List { ScimConstants.Scim2SchemaUser } + }; + + await Assert.ThrowsAsync(() => sutProvider.Sut.PatchGroupAsync(group, scimPatchModel)); + } + [Theory] [BitAutoData] public async Task PatchGroup_ReplaceDisplayNameFromValueObject_Success(SutProvider sutProvider, Organization organization, Group group, string displayName) { group.OrganizationId = organization.Id; - sutProvider.GetDependency() - .GetByIdAsync(group.Id) - .Returns(group); + sutProvider.GetDependency() + .GetByIdAsync(organization.Id) + .Returns(organization); - var scimPatchModel = new Models.ScimPatchModel + var scimPatchModel = new ScimPatchModel { Operations = new List { - new ScimPatchModel.OperationModel + new() { Op = "replace", Value = JsonDocument.Parse($"{{\"displayName\":\"{displayName}\"}}").RootElement @@ -100,12 +133,39 @@ public class PatchGroupCommandTests Schemas = new List { ScimConstants.Scim2SchemaUser } }; - await sutProvider.Sut.PatchGroupAsync(organization, group.Id, scimPatchModel); + await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); await sutProvider.GetDependency().Received(1).UpdateGroupAsync(group, organization, EventSystemUser.SCIM); Assert.Equal(displayName, group.Name); } + [Theory] + [BitAutoData] + public async Task PatchGroup_ReplaceDisplayNameFromValueObject_MissingOrganization_Throws( + SutProvider sutProvider, Organization organization, Group group, string displayName) + { + group.OrganizationId = organization.Id; + + sutProvider.GetDependency() + .GetByIdAsync(organization.Id) + .Returns((Organization)null); + + var scimPatchModel = new ScimPatchModel + { + Operations = new List + { + new() + { + Op = "replace", + Value = JsonDocument.Parse($"{{\"displayName\":\"{displayName}\"}}").RootElement + } + }, + Schemas = new List { ScimConstants.Scim2SchemaUser } + }; + + await Assert.ThrowsAsync(() => sutProvider.Sut.PatchGroupAsync(group, scimPatchModel)); + } + [Theory] [BitAutoData] public async Task PatchGroup_AddSingleMember_Success(SutProvider sutProvider, Organization organization, Group group, ICollection existingMembers, Guid userId) @@ -113,18 +173,14 @@ public class PatchGroupCommandTests group.OrganizationId = organization.Id; sutProvider.GetDependency() - .GetByIdAsync(group.Id) - .Returns(group); - - sutProvider.GetDependency() - .GetManyUserIdsByIdAsync(group.Id) + .GetManyUserIdsByIdAsync(group.Id, true) .Returns(existingMembers); - var scimPatchModel = new Models.ScimPatchModel + var scimPatchModel = new ScimPatchModel { Operations = new List { - new ScimPatchModel.OperationModel + new() { Op = "add", Path = $"members[value eq \"{userId}\"]", @@ -133,9 +189,47 @@ public class PatchGroupCommandTests Schemas = new List { ScimConstants.Scim2SchemaUser } }; - await sutProvider.Sut.PatchGroupAsync(organization, group.Id, scimPatchModel); + await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); - await sutProvider.GetDependency().Received(1).UpdateUsersAsync(group.Id, Arg.Is>(arg => arg.All(id => existingMembers.Append(userId).Contains(id)))); + await sutProvider.GetDependency().Received(1).AddGroupUsersByIdAsync( + group.Id, + Arg.Is>(arg => arg.Single() == userId)); + } + + [Theory] + [BitAutoData] + public async Task PatchGroup_AddSingleMember_ReturnsEarlyIfAlreadyInGroup( + SutProvider sutProvider, + Organization organization, + Group group, + ICollection existingMembers) + { + // User being added is already in group + var userId = existingMembers.First(); + group.OrganizationId = organization.Id; + + sutProvider.GetDependency() + .GetManyUserIdsByIdAsync(group.Id, true) + .Returns(existingMembers); + + var scimPatchModel = new ScimPatchModel + { + Operations = new List + { + new() + { + Op = "add", + Path = $"members[value eq \"{userId}\"]", + } + }, + Schemas = new List { ScimConstants.Scim2SchemaUser } + }; + + await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); + + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .AddGroupUsersByIdAsync(default, default); } [Theory] @@ -145,18 +239,14 @@ public class PatchGroupCommandTests group.OrganizationId = organization.Id; sutProvider.GetDependency() - .GetByIdAsync(group.Id) - .Returns(group); - - sutProvider.GetDependency() - .GetManyUserIdsByIdAsync(group.Id) + .GetManyUserIdsByIdAsync(group.Id, true) .Returns(existingMembers); - var scimPatchModel = new Models.ScimPatchModel + var scimPatchModel = new ScimPatchModel { Operations = new List { - new ScimPatchModel.OperationModel + new() { Op = "add", Path = $"members", @@ -166,9 +256,101 @@ public class PatchGroupCommandTests Schemas = new List { ScimConstants.Scim2SchemaUser } }; - await sutProvider.Sut.PatchGroupAsync(organization, group.Id, scimPatchModel); + await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); - await sutProvider.GetDependency().Received(1).UpdateUsersAsync(group.Id, Arg.Is>(arg => arg.All(id => existingMembers.Concat(userIds).Contains(id)))); + await sutProvider.GetDependency().Received(1).AddGroupUsersByIdAsync( + group.Id, + Arg.Is>(arg => + arg.Count() == userIds.Count && + arg.ToHashSet().SetEquals(userIds))); + } + + [Theory] + [BitAutoData] + public async Task PatchGroup_AddListMembers_IgnoresDuplicatesInRequest( + SutProvider sutProvider, Organization organization, Group group, + ICollection existingMembers) + { + // Create 3 userIds + var fixture = new Fixture { RepeatCount = 3 }; + var userIds = fixture.CreateMany().ToList(); + + // Copy the list and add a duplicate + var userIdsWithDuplicate = userIds.Append(userIds.First()).ToList(); + Assert.Equal(4, userIdsWithDuplicate.Count); + + group.OrganizationId = organization.Id; + + sutProvider.GetDependency() + .GetManyUserIdsByIdAsync(group.Id, true) + .Returns(existingMembers); + + var scimPatchModel = new ScimPatchModel + { + Operations = new List + { + new() + { + Op = "add", + Path = $"members", + Value = JsonDocument.Parse(JsonSerializer + .Serialize(userIdsWithDuplicate + .Select(uid => new { value = uid }) + .ToArray())).RootElement + } + }, + Schemas = new List { ScimConstants.Scim2SchemaUser } + }; + + await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); + + await sutProvider.GetDependency().Received(1).AddGroupUsersByIdAsync( + group.Id, + Arg.Is>(arg => + arg.Count() == 3 && + arg.ToHashSet().SetEquals(userIds))); + } + + [Theory] + [BitAutoData] + public async Task PatchGroup_AddListMembers_SuccessIfOnlySomeUsersAreInGroup( + SutProvider sutProvider, + Organization organization, Group group, + ICollection existingMembers, + ICollection userIds) + { + // A user is already in the group, but some still need to be added + userIds.Add(existingMembers.First()); + + group.OrganizationId = organization.Id; + + sutProvider.GetDependency() + .GetManyUserIdsByIdAsync(group.Id, true) + .Returns(existingMembers); + + var scimPatchModel = new ScimPatchModel + { + Operations = new List + { + new() + { + Op = "add", + Path = $"members", + Value = JsonDocument.Parse(JsonSerializer.Serialize(userIds.Select(uid => new { value = uid }).ToArray())).RootElement + } + }, + Schemas = new List { ScimConstants.Scim2SchemaUser } + }; + + await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); + + await sutProvider.GetDependency() + .Received(1) + .AddGroupUsersByIdAsync( + group.Id, + Arg.Is>(arg => + arg.Count() == userIds.Count && + arg.ToHashSet().SetEquals(userIds))); } [Theory] @@ -177,10 +359,6 @@ public class PatchGroupCommandTests { group.OrganizationId = organization.Id; - sutProvider.GetDependency() - .GetByIdAsync(group.Id) - .Returns(group); - var scimPatchModel = new Models.ScimPatchModel { Operations = new List @@ -194,21 +372,19 @@ public class PatchGroupCommandTests Schemas = new List { ScimConstants.Scim2SchemaUser } }; - await sutProvider.Sut.PatchGroupAsync(organization, group.Id, scimPatchModel); + await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); await sutProvider.GetDependency().Received(1).DeleteUserAsync(group, userId, EventSystemUser.SCIM); } [Theory] [BitAutoData] - public async Task PatchGroup_RemoveListMembers_Success(SutProvider sutProvider, Organization organization, Group group, ICollection existingMembers) + public async Task PatchGroup_RemoveListMembers_Success(SutProvider sutProvider, + Organization organization, Group group, ICollection existingMembers) { + List usersToRemove = [existingMembers.First(), existingMembers.Skip(1).First()]; group.OrganizationId = organization.Id; - sutProvider.GetDependency() - .GetByIdAsync(group.Id) - .Returns(group); - sutProvider.GetDependency() .GetManyUserIdsByIdAsync(group.Id) .Returns(existingMembers); @@ -217,30 +393,58 @@ public class PatchGroupCommandTests { Operations = new List { - new ScimPatchModel.OperationModel + new() { Op = "remove", Path = $"members", - Value = JsonDocument.Parse(JsonSerializer.Serialize(existingMembers.Select(uid => new { value = uid }).ToArray())).RootElement + Value = JsonDocument.Parse(JsonSerializer.Serialize(usersToRemove.Select(uid => new { value = uid }).ToArray())).RootElement } }, Schemas = new List { ScimConstants.Scim2SchemaUser } }; - await sutProvider.Sut.PatchGroupAsync(organization, group.Id, scimPatchModel); + await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); - await sutProvider.GetDependency().Received(1).UpdateUsersAsync(group.Id, Arg.Is>(arg => arg.All(id => existingMembers.Contains(id)))); + var expectedRemainingUsers = existingMembers.Skip(2).ToList(); + await sutProvider.GetDependency() + .Received(1) + .UpdateUsersAsync( + group.Id, + Arg.Is>(arg => + arg.Count() == expectedRemainingUsers.Count && + arg.ToHashSet().SetEquals(expectedRemainingUsers))); } [Theory] [BitAutoData] - public async Task PatchGroup_NoAction_Success(SutProvider sutProvider, Organization organization, Group group) + public async Task PatchGroup_InvalidOperation_Success(SutProvider sutProvider, Organization organization, Group group) { group.OrganizationId = organization.Id; - sutProvider.GetDependency() - .GetByIdAsync(group.Id) - .Returns(group); + var scimPatchModel = new Models.ScimPatchModel + { + Operations = [new ScimPatchModel.OperationModel { Op = "invalid operation" }], + Schemas = [ScimConstants.Scim2SchemaUser] + }; + + await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); + + // Assert: no operation performed + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().UpdateUsersAsync(default, default); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().GetManyUserIdsByIdAsync(default); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().UpdateGroupAsync(default, default); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().DeleteUserAsync(default, default); + + // Assert: logging + sutProvider.GetDependency>().ReceivedWithAnyArgs().LogWarning(default); + } + + [Theory] + [BitAutoData] + public async Task PatchGroup_NoOperation_Success( + SutProvider sutProvider, Organization organization, Group group) + { + group.OrganizationId = organization.Id; var scimPatchModel = new Models.ScimPatchModel { @@ -248,45 +452,11 @@ public class PatchGroupCommandTests Schemas = new List { ScimConstants.Scim2SchemaUser } }; - await sutProvider.Sut.PatchGroupAsync(organization, group.Id, scimPatchModel); + await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().UpdateUsersAsync(default, default); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().GetManyUserIdsByIdAsync(default); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().UpdateGroupAsync(default, default); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().DeleteUserAsync(default, default); } - - [Theory] - [BitAutoData] - public async Task PatchGroup_NotFound_Throws(SutProvider sutProvider, Organization organization, Guid groupId) - { - var scimPatchModel = new Models.ScimPatchModel - { - Operations = new List(), - Schemas = new List { ScimConstants.Scim2SchemaUser } - }; - - await Assert.ThrowsAsync(async () => await sutProvider.Sut.PatchGroupAsync(organization, groupId, scimPatchModel)); - } - - [Theory] - [BitAutoData] - public async Task PatchGroup_MismatchingOrganizationId_Throws(SutProvider sutProvider, Organization organization, Guid groupId) - { - var scimPatchModel = new Models.ScimPatchModel - { - Operations = new List(), - Schemas = new List { ScimConstants.Scim2SchemaUser } - }; - - sutProvider.GetDependency() - .GetByIdAsync(groupId) - .Returns(new Group - { - Id = groupId, - OrganizationId = Guid.NewGuid() - }); - - await Assert.ThrowsAsync(async () => await sutProvider.Sut.PatchGroupAsync(organization, groupId, scimPatchModel)); - } } diff --git a/bitwarden_license/test/Scim.Test/Groups/PatchGroupCommandvNextTests.cs b/bitwarden_license/test/Scim.Test/Groups/PatchGroupCommandvNextTests.cs deleted file mode 100644 index b9877f0b71..0000000000 --- a/bitwarden_license/test/Scim.Test/Groups/PatchGroupCommandvNextTests.cs +++ /dev/null @@ -1,381 +0,0 @@ -using System.Text.Json; -using AutoFixture; -using Bit.Core.AdminConsole.Entities; -using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces; -using Bit.Core.AdminConsole.Repositories; -using Bit.Core.AdminConsole.Services; -using Bit.Core.Enums; -using Bit.Core.Repositories; -using Bit.Scim.Groups; -using Bit.Scim.Models; -using Bit.Scim.Utilities; -using Bit.Test.Common.AutoFixture; -using Bit.Test.Common.AutoFixture.Attributes; -using NSubstitute; -using Xunit; - -namespace Bit.Scim.Test.Groups; - -[SutProviderCustomize] -public class PatchGroupCommandvNextTests -{ - [Theory] - [BitAutoData] - public async Task PatchGroup_ReplaceListMembers_Success(SutProvider sutProvider, - Organization organization, Group group, IEnumerable userIds) - { - group.OrganizationId = organization.Id; - - var scimPatchModel = new ScimPatchModel - { - Operations = new List - { - new() - { - Op = "replace", - Path = "members", - Value = JsonDocument.Parse(JsonSerializer.Serialize(userIds.Select(uid => new { value = uid }).ToArray())).RootElement - } - }, - Schemas = new List { ScimConstants.Scim2SchemaUser } - }; - - await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); - - await sutProvider.GetDependency().Received(1).UpdateUsersAsync( - group.Id, - Arg.Is>(arg => - arg.Count() == userIds.Count() && - arg.ToHashSet().SetEquals(userIds))); - } - - [Theory] - [BitAutoData] - public async Task PatchGroup_ReplaceDisplayNameFromPath_Success( - SutProvider sutProvider, Organization organization, Group group, string displayName) - { - group.OrganizationId = organization.Id; - - sutProvider.GetDependency() - .GetByIdAsync(organization.Id) - .Returns(organization); - - var scimPatchModel = new ScimPatchModel - { - Operations = new List - { - new() - { - Op = "replace", - Path = "displayname", - Value = JsonDocument.Parse($"\"{displayName}\"").RootElement - } - }, - Schemas = new List { ScimConstants.Scim2SchemaUser } - }; - - await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); - - await sutProvider.GetDependency().Received(1).UpdateGroupAsync(group, organization, EventSystemUser.SCIM); - Assert.Equal(displayName, group.Name); - } - - [Theory] - [BitAutoData] - public async Task PatchGroup_ReplaceDisplayNameFromValueObject_Success(SutProvider sutProvider, Organization organization, Group group, string displayName) - { - group.OrganizationId = organization.Id; - - sutProvider.GetDependency() - .GetByIdAsync(organization.Id) - .Returns(organization); - - var scimPatchModel = new ScimPatchModel - { - Operations = new List - { - new() - { - Op = "replace", - Value = JsonDocument.Parse($"{{\"displayName\":\"{displayName}\"}}").RootElement - } - }, - Schemas = new List { ScimConstants.Scim2SchemaUser } - }; - - await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); - - await sutProvider.GetDependency().Received(1).UpdateGroupAsync(group, organization, EventSystemUser.SCIM); - Assert.Equal(displayName, group.Name); - } - - [Theory] - [BitAutoData] - public async Task PatchGroup_AddSingleMember_Success(SutProvider sutProvider, Organization organization, Group group, ICollection existingMembers, Guid userId) - { - group.OrganizationId = organization.Id; - - sutProvider.GetDependency() - .GetManyUserIdsByIdAsync(group.Id, true) - .Returns(existingMembers); - - var scimPatchModel = new ScimPatchModel - { - Operations = new List - { - new() - { - Op = "add", - Path = $"members[value eq \"{userId}\"]", - } - }, - Schemas = new List { ScimConstants.Scim2SchemaUser } - }; - - await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); - - await sutProvider.GetDependency().Received(1).AddGroupUsersByIdAsync( - group.Id, - Arg.Is>(arg => arg.Single() == userId)); - } - - [Theory] - [BitAutoData] - public async Task PatchGroup_AddSingleMember_ReturnsEarlyIfAlreadyInGroup( - SutProvider sutProvider, - Organization organization, - Group group, - ICollection existingMembers) - { - // User being added is already in group - var userId = existingMembers.First(); - group.OrganizationId = organization.Id; - - sutProvider.GetDependency() - .GetManyUserIdsByIdAsync(group.Id, true) - .Returns(existingMembers); - - var scimPatchModel = new ScimPatchModel - { - Operations = new List - { - new() - { - Op = "add", - Path = $"members[value eq \"{userId}\"]", - } - }, - Schemas = new List { ScimConstants.Scim2SchemaUser } - }; - - await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); - - await sutProvider.GetDependency() - .DidNotReceiveWithAnyArgs() - .AddGroupUsersByIdAsync(default, default); - } - - [Theory] - [BitAutoData] - public async Task PatchGroup_AddListMembers_Success(SutProvider sutProvider, Organization organization, Group group, ICollection existingMembers, ICollection userIds) - { - group.OrganizationId = organization.Id; - - sutProvider.GetDependency() - .GetManyUserIdsByIdAsync(group.Id, true) - .Returns(existingMembers); - - var scimPatchModel = new ScimPatchModel - { - Operations = new List - { - new() - { - Op = "add", - Path = $"members", - Value = JsonDocument.Parse(JsonSerializer.Serialize(userIds.Select(uid => new { value = uid }).ToArray())).RootElement - } - }, - Schemas = new List { ScimConstants.Scim2SchemaUser } - }; - - await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); - - await sutProvider.GetDependency().Received(1).AddGroupUsersByIdAsync( - group.Id, - Arg.Is>(arg => - arg.Count() == userIds.Count && - arg.ToHashSet().SetEquals(userIds))); - } - - [Theory] - [BitAutoData] - public async Task PatchGroup_AddListMembers_IgnoresDuplicatesInRequest( - SutProvider sutProvider, Organization organization, Group group, - ICollection existingMembers) - { - // Create 3 userIds - var fixture = new Fixture { RepeatCount = 3 }; - var userIds = fixture.CreateMany().ToList(); - - // Copy the list and add a duplicate - var userIdsWithDuplicate = userIds.Append(userIds.First()).ToList(); - Assert.Equal(4, userIdsWithDuplicate.Count); - - group.OrganizationId = organization.Id; - - sutProvider.GetDependency() - .GetManyUserIdsByIdAsync(group.Id, true) - .Returns(existingMembers); - - var scimPatchModel = new ScimPatchModel - { - Operations = new List - { - new() - { - Op = "add", - Path = $"members", - Value = JsonDocument.Parse(JsonSerializer - .Serialize(userIdsWithDuplicate - .Select(uid => new { value = uid }) - .ToArray())).RootElement - } - }, - Schemas = new List { ScimConstants.Scim2SchemaUser } - }; - - await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); - - await sutProvider.GetDependency().Received(1).AddGroupUsersByIdAsync( - group.Id, - Arg.Is>(arg => - arg.Count() == 3 && - arg.ToHashSet().SetEquals(userIds))); - } - - [Theory] - [BitAutoData] - public async Task PatchGroup_AddListMembers_SuccessIfOnlySomeUsersAreInGroup( - SutProvider sutProvider, - Organization organization, Group group, - ICollection existingMembers, - ICollection userIds) - { - // A user is already in the group, but some still need to be added - userIds.Add(existingMembers.First()); - - group.OrganizationId = organization.Id; - - sutProvider.GetDependency() - .GetManyUserIdsByIdAsync(group.Id, true) - .Returns(existingMembers); - - var scimPatchModel = new ScimPatchModel - { - Operations = new List - { - new() - { - Op = "add", - Path = $"members", - Value = JsonDocument.Parse(JsonSerializer.Serialize(userIds.Select(uid => new { value = uid }).ToArray())).RootElement - } - }, - Schemas = new List { ScimConstants.Scim2SchemaUser } - }; - - await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); - - await sutProvider.GetDependency() - .Received(1) - .AddGroupUsersByIdAsync( - group.Id, - Arg.Is>(arg => - arg.Count() == userIds.Count && - arg.ToHashSet().SetEquals(userIds))); - } - - [Theory] - [BitAutoData] - public async Task PatchGroup_RemoveSingleMember_Success(SutProvider sutProvider, Organization organization, Group group, Guid userId) - { - group.OrganizationId = organization.Id; - - var scimPatchModel = new Models.ScimPatchModel - { - Operations = new List - { - new ScimPatchModel.OperationModel - { - Op = "remove", - Path = $"members[value eq \"{userId}\"]", - } - }, - Schemas = new List { ScimConstants.Scim2SchemaUser } - }; - - await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); - - await sutProvider.GetDependency().Received(1).DeleteUserAsync(group, userId, EventSystemUser.SCIM); - } - - [Theory] - [BitAutoData] - public async Task PatchGroup_RemoveListMembers_Success(SutProvider sutProvider, - Organization organization, Group group, ICollection existingMembers) - { - List usersToRemove = [existingMembers.First(), existingMembers.Skip(1).First()]; - group.OrganizationId = organization.Id; - - sutProvider.GetDependency() - .GetManyUserIdsByIdAsync(group.Id) - .Returns(existingMembers); - - var scimPatchModel = new Models.ScimPatchModel - { - Operations = new List - { - new() - { - Op = "remove", - Path = $"members", - Value = JsonDocument.Parse(JsonSerializer.Serialize(usersToRemove.Select(uid => new { value = uid }).ToArray())).RootElement - } - }, - Schemas = new List { ScimConstants.Scim2SchemaUser } - }; - - await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); - - var expectedRemainingUsers = existingMembers.Skip(2).ToList(); - await sutProvider.GetDependency() - .Received(1) - .UpdateUsersAsync( - group.Id, - Arg.Is>(arg => - arg.Count() == expectedRemainingUsers.Count && - arg.ToHashSet().SetEquals(expectedRemainingUsers))); - } - - [Theory] - [BitAutoData] - public async Task PatchGroup_NoAction_Success( - SutProvider sutProvider, Organization organization, Group group) - { - group.OrganizationId = organization.Id; - - var scimPatchModel = new Models.ScimPatchModel - { - Operations = new List(), - Schemas = new List { ScimConstants.Scim2SchemaUser } - }; - - await sutProvider.Sut.PatchGroupAsync(group, scimPatchModel); - - await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().UpdateUsersAsync(default, default); - await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().GetManyUserIdsByIdAsync(default); - await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().UpdateGroupAsync(default, default); - await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().DeleteUserAsync(default, default); - } -} diff --git a/src/Core/Constants.cs b/src/Core/Constants.cs index c629881458..379915c37d 100644 --- a/src/Core/Constants.cs +++ b/src/Core/Constants.cs @@ -106,7 +106,6 @@ public static class FeatureFlagKeys public const string VerifiedSsoDomainEndpoint = "pm-12337-refactor-sso-details-endpoint"; public const string DeviceApprovalRequestAdminNotifications = "pm-15637-device-approval-request-admin-notifications"; public const string LimitItemDeletion = "pm-15493-restrict-item-deletion-to-can-manage-permission"; - public const string ShortcutDuplicatePatchRequests = "pm-16812-shortcut-duplicate-patch-requests"; public const string PushSyncOrgKeysOnRevokeRestore = "pm-17168-push-sync-org-keys-on-revoke-restore"; public const string PolicyRequirements = "pm-14439-policy-requirements";