From d17d43cf7bed52df28206034ba2663f104cbf56e Mon Sep 17 00:00:00 2001 From: Jared Date: Thu, 26 Feb 2026 17:44:11 -0500 Subject: [PATCH] [PM-19232] Implement externalId handling in PatchUserCommand with validation (#6998) * Implement externalId handling in PatchUserCommand with validation and tests * Change back for testing because we don't want to potentially stop code flow... * Refactor PatchUserCommand and related tests to log warnings for unsupported operations instead of throwing exceptions. Update method names for clarity and adjust assertions in test cases accordingly. * Refactor PatchUserCommand to streamline handling of active and externalId properties from value objects, consolidating logic for improved clarity and maintainability. * Update bitwarden_license/test/Scim.Test/Users/PatchUserCommandTests.cs Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * Fix formatting issue in PatchUserCommandTests.cs by removing invisible characters and ensuring proper code structure. * Enhance PatchUserCommand to re-fetch user status after restore/revoke operations, ensuring accurate updates. Add corresponding test case to verify behavior when restoring users and updating externalId. --------- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> --- .../src/Scim/Users/PatchUserCommand.cs | 70 ++++- .../src/Scim/Utilities/ScimConstants.cs | 1 + .../Controllers/v2/UsersControllerTests.cs | 112 +++++++ .../Scim.Test/Users/PatchUserCommandTests.cs | 280 ++++++++++++++++++ 4 files changed, 454 insertions(+), 9 deletions(-) diff --git a/bitwarden_license/src/Scim/Users/PatchUserCommand.cs b/bitwarden_license/src/Scim/Users/PatchUserCommand.cs index 474557a9cb..535ba4ca22 100644 --- a/bitwarden_license/src/Scim/Users/PatchUserCommand.cs +++ b/bitwarden_license/src/Scim/Users/PatchUserCommand.cs @@ -5,6 +5,7 @@ using Bit.Core.Exceptions; using Bit.Core.Repositories; using Bit.Scim.Models; using Bit.Scim.Users.Interfaces; +using Bit.Scim.Utilities; namespace Bit.Scim.Users; @@ -38,7 +39,7 @@ public class PatchUserCommand : IPatchUserCommand foreach (var operation in model.Operations) { // Replace operations - if (operation.Op?.ToLowerInvariant() == "replace") + if (operation.Op?.ToLowerInvariant() == PatchOps.Replace) { // Active from path if (operation.Path?.ToLowerInvariant() == "active") @@ -49,15 +50,42 @@ public class PatchUserCommand : IPatchUserCommand { operationHandled = handled; } - } - // Active from value object - else if (string.IsNullOrWhiteSpace(operation.Path) && - operation.Value.TryGetProperty("active", out var activeProperty)) - { - var handled = await HandleActiveOperationAsync(orgUser, activeProperty.GetBoolean()); - if (!operationHandled) + // Re-fetch to pick up status changes persisted by restore/revoke + if (handled) { - operationHandled = handled; + orgUser = await _organizationUserRepository.GetByIdAsync(orgUser.Id) + ?? throw new NotFoundException("User not found."); + } + } + // ExternalId from path + else if (operation.Path?.ToLowerInvariant() == PatchPaths.ExternalId) + { + var newExternalId = operation.Value.GetString(); + await HandleExternalIdOperationAsync(orgUser, newExternalId); + operationHandled = true; + } + // Value object with no path — check for each supported property independently + else if (string.IsNullOrWhiteSpace(operation.Path)) + { + if (operation.Value.TryGetProperty("active", out var activeProperty)) + { + var handled = await HandleActiveOperationAsync(orgUser, activeProperty.GetBoolean()); + if (!operationHandled) + { + operationHandled = handled; + } + // Re-fetch to pick up status changes persisted by restore/revoke + if (handled) + { + orgUser = await _organizationUserRepository.GetByIdAsync(orgUser.Id) + ?? throw new NotFoundException("User not found."); + } + } + if (operation.Value.TryGetProperty("externalId", out var externalIdProperty)) + { + var newExternalId = externalIdProperty.GetString(); + await HandleExternalIdOperationAsync(orgUser, newExternalId); + operationHandled = true; } } } @@ -84,4 +112,28 @@ public class PatchUserCommand : IPatchUserCommand } return false; } + + private async Task HandleExternalIdOperationAsync(Core.Entities.OrganizationUser orgUser, string? newExternalId) + { + // Validate max length (300 chars per OrganizationUser.cs line 59) + if (!string.IsNullOrWhiteSpace(newExternalId) && newExternalId.Length > 300) + { + throw new BadRequestException("ExternalId cannot exceed 300 characters."); + } + + // Check for duplicate externalId (same validation as PostUserCommand.cs) + if (!string.IsNullOrWhiteSpace(newExternalId)) + { + var existingUsers = await _organizationUserRepository.GetManyDetailsByOrganizationAsync(orgUser.OrganizationId); + if (existingUsers.Any(u => u.Id != orgUser.Id && + !string.IsNullOrWhiteSpace(u.ExternalId) && + u.ExternalId.Equals(newExternalId, StringComparison.OrdinalIgnoreCase))) + { + throw new ConflictException("ExternalId already exists for another user."); + } + } + + orgUser.ExternalId = newExternalId; + await _organizationUserRepository.ReplaceAsync(orgUser); + } } diff --git a/bitwarden_license/src/Scim/Utilities/ScimConstants.cs b/bitwarden_license/src/Scim/Utilities/ScimConstants.cs index 0836a72c7f..01cb063d82 100644 --- a/bitwarden_license/src/Scim/Utilities/ScimConstants.cs +++ b/bitwarden_license/src/Scim/Utilities/ScimConstants.cs @@ -19,4 +19,5 @@ public static class PatchPaths { public const string Members = "members"; public const string DisplayName = "displayname"; + public const string ExternalId = "externalid"; } diff --git a/bitwarden_license/test/Scim.IntegrationTest/Controllers/v2/UsersControllerTests.cs b/bitwarden_license/test/Scim.IntegrationTest/Controllers/v2/UsersControllerTests.cs index b8dc1b1123..621d78c741 100644 --- a/bitwarden_license/test/Scim.IntegrationTest/Controllers/v2/UsersControllerTests.cs +++ b/bitwarden_license/test/Scim.IntegrationTest/Controllers/v2/UsersControllerTests.cs @@ -568,6 +568,118 @@ public class UsersControllerTests : IClassFixture, IAsyn AssertHelper.AssertPropertyEqual(expectedResponse, responseModel); } + [Fact] + public async Task Patch_ExternalIdFromPath_Success() + { + var organizationUserId = ScimApplicationFactory.TestOrganizationUserId1; + var newExternalId = "new-external-id-path"; + var inputModel = new ScimPatchModel + { + Operations = new List() + { + new ScimPatchModel.OperationModel + { + Op = "replace", + Path = "externalId", + Value = JsonDocument.Parse($"\"{newExternalId}\"").RootElement + }, + }, + Schemas = new List() + }; + + var context = await _factory.UsersPatchAsync(ScimApplicationFactory.TestOrganizationId1, organizationUserId, inputModel); + + Assert.Equal(StatusCodes.Status204NoContent, context.Response.StatusCode); + + var databaseContext = _factory.GetDatabaseContext(); + var organizationUser = databaseContext.OrganizationUsers.FirstOrDefault(g => g.Id == organizationUserId); + Assert.Equal(newExternalId, organizationUser.ExternalId); + } + + [Fact] + public async Task Patch_ExternalIdFromValue_Success() + { + var organizationUserId = ScimApplicationFactory.TestOrganizationUserId2; + var newExternalId = "new-external-id-value"; + var inputModel = new ScimPatchModel + { + Operations = new List() + { + new ScimPatchModel.OperationModel + { + Op = "replace", + Value = JsonDocument.Parse($"{{\"externalId\":\"{newExternalId}\"}}").RootElement + }, + }, + Schemas = new List() + }; + + var context = await _factory.UsersPatchAsync(ScimApplicationFactory.TestOrganizationId1, organizationUserId, inputModel); + + Assert.Equal(StatusCodes.Status204NoContent, context.Response.StatusCode); + + var databaseContext = _factory.GetDatabaseContext(); + var organizationUser = databaseContext.OrganizationUsers.FirstOrDefault(g => g.Id == organizationUserId); + Assert.Equal(newExternalId, organizationUser.ExternalId); + } + + [Fact] + public async Task Patch_ExternalIdDuplicate_ThrowsConflict() + { + var organizationUserId = ScimApplicationFactory.TestOrganizationUserId1; + var duplicateExternalId = "UB"; // This is the externalId of TestOrganizationUserId2 + var inputModel = new ScimPatchModel + { + Operations = new List() + { + new ScimPatchModel.OperationModel + { + Op = "replace", + Path = "externalId", + Value = JsonDocument.Parse($"\"{duplicateExternalId}\"").RootElement + }, + }, + Schemas = new List() + }; + var expectedResponse = new ScimErrorResponseModel + { + Status = StatusCodes.Status409Conflict, + Detail = "ExternalId already exists for another user.", + Schemas = new List { ScimConstants.Scim2SchemaError } + }; + + var context = await _factory.UsersPatchAsync(ScimApplicationFactory.TestOrganizationId1, organizationUserId, inputModel); + + Assert.Equal(StatusCodes.Status409Conflict, context.Response.StatusCode); + + var responseModel = JsonSerializer.Deserialize(context.Response.Body, new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }); + AssertHelper.AssertPropertyEqual(expectedResponse, responseModel); + } + + [Fact] + public async Task Patch_UnsupportedOperation_LogsWarningAndSucceeds() + { + var organizationUserId = ScimApplicationFactory.TestOrganizationUserId1; + var inputModel = new ScimPatchModel + { + Operations = new List() + { + new ScimPatchModel.OperationModel + { + Op = "add", + Path = "displayName", + Value = JsonDocument.Parse("\"John Doe\"").RootElement + }, + }, + Schemas = new List() + }; + + var context = await _factory.UsersPatchAsync(ScimApplicationFactory.TestOrganizationId1, organizationUserId, inputModel); + + // Unsupported operations are logged as warnings but don't fail the request + Assert.Equal(StatusCodes.Status204NoContent, context.Response.StatusCode); + } + [Fact] public async Task Delete_Success() { diff --git a/bitwarden_license/test/Scim.Test/Users/PatchUserCommandTests.cs b/bitwarden_license/test/Scim.Test/Users/PatchUserCommandTests.cs index 8b6c850c6f..28a9feec63 100644 --- a/bitwarden_license/test/Scim.Test/Users/PatchUserCommandTests.cs +++ b/bitwarden_license/test/Scim.Test/Users/PatchUserCommandTests.cs @@ -4,6 +4,7 @@ using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.RevokeUser.v1 using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; +using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.Repositories; using Bit.Scim.Models; using Bit.Scim.Users; @@ -185,4 +186,283 @@ public class PatchUserCommandTests await Assert.ThrowsAsync(async () => await sutProvider.Sut.PatchUserAsync(organizationId, organizationUserId, scimPatchModel)); } + + [Theory] + [BitAutoData] + public async Task PatchUser_ExternalIdFromPath_Success(SutProvider sutProvider, OrganizationUser organizationUser) + { + var newExternalId = "new-external-id-123"; + organizationUser.ExternalId = "old-external-id"; + + sutProvider.GetDependency() + .GetByIdAsync(organizationUser.Id) + .Returns(organizationUser); + + sutProvider.GetDependency() + .GetManyDetailsByOrganizationAsync(organizationUser.OrganizationId) + .Returns(new List()); + + var scimPatchModel = new Models.ScimPatchModel + { + Operations = new List + { + new ScimPatchModel.OperationModel + { + Op = "replace", + Path = "externalId", + Value = JsonDocument.Parse($"\"{newExternalId}\"").RootElement + } + }, + Schemas = new List { ScimConstants.Scim2SchemaUser } + }; + + await sutProvider.Sut.PatchUserAsync(organizationUser.OrganizationId, organizationUser.Id, scimPatchModel); + + await sutProvider.GetDependency().Received(1).ReplaceAsync( + Arg.Is(ou => ou.ExternalId == newExternalId)); + } + + [Theory] + [BitAutoData] + public async Task PatchUser_ExternalIdFromValue_Success(SutProvider sutProvider, OrganizationUser organizationUser) + { + var newExternalId = "new-external-id-456"; + organizationUser.ExternalId = null; + + sutProvider.GetDependency() + .GetByIdAsync(organizationUser.Id) + .Returns(organizationUser); + + sutProvider.GetDependency() + .GetManyDetailsByOrganizationAsync(organizationUser.OrganizationId) + .Returns(new List()); + + var scimPatchModel = new Models.ScimPatchModel + { + Operations = new List + { + new ScimPatchModel.OperationModel + { + Op = "replace", + Value = JsonDocument.Parse($"{{\"externalId\":\"{newExternalId}\"}}").RootElement + } + }, + Schemas = new List { ScimConstants.Scim2SchemaUser } + }; + + await sutProvider.Sut.PatchUserAsync(organizationUser.OrganizationId, organizationUser.Id, scimPatchModel); + + await sutProvider.GetDependency().Received(1).ReplaceAsync( + Arg.Is(ou => ou.ExternalId == newExternalId)); + } + + [Theory] + [BitAutoData] + public async Task PatchUser_ExternalIdDuplicate_ThrowsConflict(SutProvider sutProvider, OrganizationUser organizationUser, OrganizationUserUserDetails existingUser) + { + var duplicateExternalId = "duplicate-id"; + organizationUser.ExternalId = "old-id"; + existingUser.ExternalId = duplicateExternalId; + existingUser.Id = Guid.NewGuid(); // Different user + + sutProvider.GetDependency() + .GetByIdAsync(organizationUser.Id) + .Returns(organizationUser); + + sutProvider.GetDependency() + .GetManyDetailsByOrganizationAsync(organizationUser.OrganizationId) + .Returns(new List { existingUser }); + + var scimPatchModel = new Models.ScimPatchModel + { + Operations = new List + { + new ScimPatchModel.OperationModel + { + Op = "replace", + Path = "externalId", + Value = JsonDocument.Parse($"\"{duplicateExternalId}\"").RootElement + } + }, + Schemas = new List { ScimConstants.Scim2SchemaUser } + }; + + await Assert.ThrowsAsync(async () => + await sutProvider.Sut.PatchUserAsync(organizationUser.OrganizationId, organizationUser.Id, scimPatchModel)); + } + + [Theory] + [BitAutoData] + public async Task PatchUser_ExternalIdTooLong_ThrowsBadRequest(SutProvider sutProvider, OrganizationUser organizationUser) + { + var tooLongExternalId = new string('a', 301); // Exceeds 300 character limit + + sutProvider.GetDependency() + .GetByIdAsync(organizationUser.Id) + .Returns(organizationUser); + + var scimPatchModel = new Models.ScimPatchModel + { + Operations = new List + { + new ScimPatchModel.OperationModel + { + Op = "replace", + Path = "externalId", + Value = JsonDocument.Parse($"\"{tooLongExternalId}\"").RootElement + } + }, + Schemas = new List { ScimConstants.Scim2SchemaUser } + }; + + await Assert.ThrowsAsync(async () => + await sutProvider.Sut.PatchUserAsync(organizationUser.OrganizationId, organizationUser.Id, scimPatchModel)); + } + + [Theory] + [BitAutoData] + public async Task PatchUser_ExternalIdNull_Success(SutProvider sutProvider, OrganizationUser organizationUser) + { + organizationUser.ExternalId = "existing-id"; + + sutProvider.GetDependency() + .GetByIdAsync(organizationUser.Id) + .Returns(organizationUser); + + sutProvider.GetDependency() + .GetManyDetailsByOrganizationAsync(organizationUser.OrganizationId) + .Returns(new List()); + + var scimPatchModel = new Models.ScimPatchModel + { + Operations = new List + { + new ScimPatchModel.OperationModel + { + Op = "replace", + Path = "externalId", + Value = JsonDocument.Parse("null").RootElement + } + }, + Schemas = new List { ScimConstants.Scim2SchemaUser } + }; + + await sutProvider.Sut.PatchUserAsync(organizationUser.OrganizationId, organizationUser.Id, scimPatchModel); + + await sutProvider.GetDependency().Received(1).ReplaceAsync( + Arg.Is(ou => ou.ExternalId == null)); + } + + [Theory] + [BitAutoData] + public async Task PatchUser_UnsupportedOperation_LogsWarningAndSucceeds(SutProvider sutProvider, OrganizationUser organizationUser) + { + sutProvider.GetDependency() + .GetByIdAsync(organizationUser.Id) + .Returns(organizationUser); + + var scimPatchModel = new Models.ScimPatchModel + { + Operations = new List + { + new ScimPatchModel.OperationModel + { + Op = "add", + Path = "displayName", + Value = JsonDocument.Parse("\"John Doe\"").RootElement + } + }, + Schemas = new List { ScimConstants.Scim2SchemaUser } + }; + + // Should not throw - unsupported operations are logged as warnings but don't fail the request + await sutProvider.Sut.PatchUserAsync(organizationUser.OrganizationId, organizationUser.Id, scimPatchModel); + + // Verify no restore or revoke operations were called + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().RestoreUserAsync(default, EventSystemUser.SCIM); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().RevokeUserAsync(default, EventSystemUser.SCIM); + } + + [Theory] + [BitAutoData] + public async Task PatchUser_ActiveAndExternalIdFromValue_Success(SutProvider sutProvider, OrganizationUser organizationUser) + { + var newExternalId = "combined-test-id"; + organizationUser.Status = OrganizationUserStatusType.Confirmed; + organizationUser.ExternalId = "old-id"; + + sutProvider.GetDependency() + .GetByIdAsync(organizationUser.Id) + .Returns(organizationUser); + + sutProvider.GetDependency() + .GetManyDetailsByOrganizationAsync(organizationUser.OrganizationId) + .Returns(new List()); + + var scimPatchModel = new Models.ScimPatchModel + { + Operations = new List + { + new ScimPatchModel.OperationModel + { + Op = "replace", + Value = JsonDocument.Parse($"{{\"active\":false,\"externalId\":\"{newExternalId}\"}}").RootElement + } + }, + Schemas = new List { ScimConstants.Scim2SchemaUser } + }; + + await sutProvider.Sut.PatchUserAsync(organizationUser.OrganizationId, organizationUser.Id, scimPatchModel); + + // Verify both operations were processed + await sutProvider.GetDependency().Received(1).RevokeUserAsync(organizationUser, EventSystemUser.SCIM); + await sutProvider.GetDependency().Received(1).ReplaceAsync( + Arg.Is(ou => ou.ExternalId == newExternalId)); + } + + [Theory] + [BitAutoData] + public async Task PatchUser_RestoreAndExternalIdFromValue_DoesNotRevertRestore(SutProvider sutProvider, OrganizationUser organizationUser) + { + var newExternalId = "combined-restore-id"; + organizationUser.Status = OrganizationUserStatusType.Revoked; + organizationUser.ExternalId = "old-id"; + + // Simulate the re-fetch after restore returning a user with a non-revoked status + var restoredOrgUser = new OrganizationUser + { + Id = organizationUser.Id, + OrganizationId = organizationUser.OrganizationId, + Status = OrganizationUserStatusType.Confirmed, + ExternalId = organizationUser.ExternalId, + }; + + sutProvider.GetDependency() + .GetByIdAsync(organizationUser.Id) + .Returns(organizationUser, restoredOrgUser); + + sutProvider.GetDependency() + .GetManyDetailsByOrganizationAsync(organizationUser.OrganizationId) + .Returns(new List()); + + var scimPatchModel = new Models.ScimPatchModel + { + Operations = new List + { + new ScimPatchModel.OperationModel + { + Op = "replace", + Value = JsonDocument.Parse($"{{\"active\":true,\"externalId\":\"{newExternalId}\"}}").RootElement + } + }, + Schemas = new List { ScimConstants.Scim2SchemaUser } + }; + + await sutProvider.Sut.PatchUserAsync(organizationUser.OrganizationId, organizationUser.Id, scimPatchModel); + + await sutProvider.GetDependency().Received(1).RestoreUserAsync(organizationUser, EventSystemUser.SCIM); + // ReplaceAsync must use the re-fetched (restored) user, not the stale revoked state + await sutProvider.GetDependency().Received(1).ReplaceAsync( + Arg.Is(ou => ou.ExternalId == newExternalId && ou.Status != OrganizationUserStatusType.Revoked)); + } }