1
0
mirror of https://github.com/bitwarden/server synced 2026-02-27 09:53:42 +00:00

[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>
This commit is contained in:
Jared
2026-02-26 17:44:11 -05:00
committed by GitHub
parent 25df3c381c
commit d17d43cf7b
4 changed files with 454 additions and 9 deletions

View File

@@ -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);
}
}

View File

@@ -19,4 +19,5 @@ public static class PatchPaths
{
public const string Members = "members";
public const string DisplayName = "displayname";
public const string ExternalId = "externalid";
}

View File

@@ -568,6 +568,118 @@ public class UsersControllerTests : IClassFixture<ScimApplicationFactory>, 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<ScimPatchModel.OperationModel>()
{
new ScimPatchModel.OperationModel
{
Op = "replace",
Path = "externalId",
Value = JsonDocument.Parse($"\"{newExternalId}\"").RootElement
},
},
Schemas = new List<string>()
};
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<ScimPatchModel.OperationModel>()
{
new ScimPatchModel.OperationModel
{
Op = "replace",
Value = JsonDocument.Parse($"{{\"externalId\":\"{newExternalId}\"}}").RootElement
},
},
Schemas = new List<string>()
};
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<ScimPatchModel.OperationModel>()
{
new ScimPatchModel.OperationModel
{
Op = "replace",
Path = "externalId",
Value = JsonDocument.Parse($"\"{duplicateExternalId}\"").RootElement
},
},
Schemas = new List<string>()
};
var expectedResponse = new ScimErrorResponseModel
{
Status = StatusCodes.Status409Conflict,
Detail = "ExternalId already exists for another user.",
Schemas = new List<string> { ScimConstants.Scim2SchemaError }
};
var context = await _factory.UsersPatchAsync(ScimApplicationFactory.TestOrganizationId1, organizationUserId, inputModel);
Assert.Equal(StatusCodes.Status409Conflict, context.Response.StatusCode);
var responseModel = JsonSerializer.Deserialize<ScimErrorResponseModel>(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<ScimPatchModel.OperationModel>()
{
new ScimPatchModel.OperationModel
{
Op = "add",
Path = "displayName",
Value = JsonDocument.Parse("\"John Doe\"").RootElement
},
},
Schemas = new List<string>()
};
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()
{

View File

@@ -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<NotFoundException>(async () => await sutProvider.Sut.PatchUserAsync(organizationId, organizationUserId, scimPatchModel));
}
[Theory]
[BitAutoData]
public async Task PatchUser_ExternalIdFromPath_Success(SutProvider<PatchUserCommand> sutProvider, OrganizationUser organizationUser)
{
var newExternalId = "new-external-id-123";
organizationUser.ExternalId = "old-external-id";
sutProvider.GetDependency<IOrganizationUserRepository>()
.GetByIdAsync(organizationUser.Id)
.Returns(organizationUser);
sutProvider.GetDependency<IOrganizationUserRepository>()
.GetManyDetailsByOrganizationAsync(organizationUser.OrganizationId)
.Returns(new List<OrganizationUserUserDetails>());
var scimPatchModel = new Models.ScimPatchModel
{
Operations = new List<ScimPatchModel.OperationModel>
{
new ScimPatchModel.OperationModel
{
Op = "replace",
Path = "externalId",
Value = JsonDocument.Parse($"\"{newExternalId}\"").RootElement
}
},
Schemas = new List<string> { ScimConstants.Scim2SchemaUser }
};
await sutProvider.Sut.PatchUserAsync(organizationUser.OrganizationId, organizationUser.Id, scimPatchModel);
await sutProvider.GetDependency<IOrganizationUserRepository>().Received(1).ReplaceAsync(
Arg.Is<OrganizationUser>(ou => ou.ExternalId == newExternalId));
}
[Theory]
[BitAutoData]
public async Task PatchUser_ExternalIdFromValue_Success(SutProvider<PatchUserCommand> sutProvider, OrganizationUser organizationUser)
{
var newExternalId = "new-external-id-456";
organizationUser.ExternalId = null;
sutProvider.GetDependency<IOrganizationUserRepository>()
.GetByIdAsync(organizationUser.Id)
.Returns(organizationUser);
sutProvider.GetDependency<IOrganizationUserRepository>()
.GetManyDetailsByOrganizationAsync(organizationUser.OrganizationId)
.Returns(new List<OrganizationUserUserDetails>());
var scimPatchModel = new Models.ScimPatchModel
{
Operations = new List<ScimPatchModel.OperationModel>
{
new ScimPatchModel.OperationModel
{
Op = "replace",
Value = JsonDocument.Parse($"{{\"externalId\":\"{newExternalId}\"}}").RootElement
}
},
Schemas = new List<string> { ScimConstants.Scim2SchemaUser }
};
await sutProvider.Sut.PatchUserAsync(organizationUser.OrganizationId, organizationUser.Id, scimPatchModel);
await sutProvider.GetDependency<IOrganizationUserRepository>().Received(1).ReplaceAsync(
Arg.Is<OrganizationUser>(ou => ou.ExternalId == newExternalId));
}
[Theory]
[BitAutoData]
public async Task PatchUser_ExternalIdDuplicate_ThrowsConflict(SutProvider<PatchUserCommand> sutProvider, OrganizationUser organizationUser, OrganizationUserUserDetails existingUser)
{
var duplicateExternalId = "duplicate-id";
organizationUser.ExternalId = "old-id";
existingUser.ExternalId = duplicateExternalId;
existingUser.Id = Guid.NewGuid(); // Different user
sutProvider.GetDependency<IOrganizationUserRepository>()
.GetByIdAsync(organizationUser.Id)
.Returns(organizationUser);
sutProvider.GetDependency<IOrganizationUserRepository>()
.GetManyDetailsByOrganizationAsync(organizationUser.OrganizationId)
.Returns(new List<OrganizationUserUserDetails> { existingUser });
var scimPatchModel = new Models.ScimPatchModel
{
Operations = new List<ScimPatchModel.OperationModel>
{
new ScimPatchModel.OperationModel
{
Op = "replace",
Path = "externalId",
Value = JsonDocument.Parse($"\"{duplicateExternalId}\"").RootElement
}
},
Schemas = new List<string> { ScimConstants.Scim2SchemaUser }
};
await Assert.ThrowsAsync<ConflictException>(async () =>
await sutProvider.Sut.PatchUserAsync(organizationUser.OrganizationId, organizationUser.Id, scimPatchModel));
}
[Theory]
[BitAutoData]
public async Task PatchUser_ExternalIdTooLong_ThrowsBadRequest(SutProvider<PatchUserCommand> sutProvider, OrganizationUser organizationUser)
{
var tooLongExternalId = new string('a', 301); // Exceeds 300 character limit
sutProvider.GetDependency<IOrganizationUserRepository>()
.GetByIdAsync(organizationUser.Id)
.Returns(organizationUser);
var scimPatchModel = new Models.ScimPatchModel
{
Operations = new List<ScimPatchModel.OperationModel>
{
new ScimPatchModel.OperationModel
{
Op = "replace",
Path = "externalId",
Value = JsonDocument.Parse($"\"{tooLongExternalId}\"").RootElement
}
},
Schemas = new List<string> { ScimConstants.Scim2SchemaUser }
};
await Assert.ThrowsAsync<BadRequestException>(async () =>
await sutProvider.Sut.PatchUserAsync(organizationUser.OrganizationId, organizationUser.Id, scimPatchModel));
}
[Theory]
[BitAutoData]
public async Task PatchUser_ExternalIdNull_Success(SutProvider<PatchUserCommand> sutProvider, OrganizationUser organizationUser)
{
organizationUser.ExternalId = "existing-id";
sutProvider.GetDependency<IOrganizationUserRepository>()
.GetByIdAsync(organizationUser.Id)
.Returns(organizationUser);
sutProvider.GetDependency<IOrganizationUserRepository>()
.GetManyDetailsByOrganizationAsync(organizationUser.OrganizationId)
.Returns(new List<OrganizationUserUserDetails>());
var scimPatchModel = new Models.ScimPatchModel
{
Operations = new List<ScimPatchModel.OperationModel>
{
new ScimPatchModel.OperationModel
{
Op = "replace",
Path = "externalId",
Value = JsonDocument.Parse("null").RootElement
}
},
Schemas = new List<string> { ScimConstants.Scim2SchemaUser }
};
await sutProvider.Sut.PatchUserAsync(organizationUser.OrganizationId, organizationUser.Id, scimPatchModel);
await sutProvider.GetDependency<IOrganizationUserRepository>().Received(1).ReplaceAsync(
Arg.Is<OrganizationUser>(ou => ou.ExternalId == null));
}
[Theory]
[BitAutoData]
public async Task PatchUser_UnsupportedOperation_LogsWarningAndSucceeds(SutProvider<PatchUserCommand> sutProvider, OrganizationUser organizationUser)
{
sutProvider.GetDependency<IOrganizationUserRepository>()
.GetByIdAsync(organizationUser.Id)
.Returns(organizationUser);
var scimPatchModel = new Models.ScimPatchModel
{
Operations = new List<ScimPatchModel.OperationModel>
{
new ScimPatchModel.OperationModel
{
Op = "add",
Path = "displayName",
Value = JsonDocument.Parse("\"John Doe\"").RootElement
}
},
Schemas = new List<string> { 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<IRestoreOrganizationUserCommand>().DidNotReceiveWithAnyArgs().RestoreUserAsync(default, EventSystemUser.SCIM);
await sutProvider.GetDependency<IRevokeOrganizationUserCommand>().DidNotReceiveWithAnyArgs().RevokeUserAsync(default, EventSystemUser.SCIM);
}
[Theory]
[BitAutoData]
public async Task PatchUser_ActiveAndExternalIdFromValue_Success(SutProvider<PatchUserCommand> sutProvider, OrganizationUser organizationUser)
{
var newExternalId = "combined-test-id";
organizationUser.Status = OrganizationUserStatusType.Confirmed;
organizationUser.ExternalId = "old-id";
sutProvider.GetDependency<IOrganizationUserRepository>()
.GetByIdAsync(organizationUser.Id)
.Returns(organizationUser);
sutProvider.GetDependency<IOrganizationUserRepository>()
.GetManyDetailsByOrganizationAsync(organizationUser.OrganizationId)
.Returns(new List<OrganizationUserUserDetails>());
var scimPatchModel = new Models.ScimPatchModel
{
Operations = new List<ScimPatchModel.OperationModel>
{
new ScimPatchModel.OperationModel
{
Op = "replace",
Value = JsonDocument.Parse($"{{\"active\":false,\"externalId\":\"{newExternalId}\"}}").RootElement
}
},
Schemas = new List<string> { ScimConstants.Scim2SchemaUser }
};
await sutProvider.Sut.PatchUserAsync(organizationUser.OrganizationId, organizationUser.Id, scimPatchModel);
// Verify both operations were processed
await sutProvider.GetDependency<IRevokeOrganizationUserCommand>().Received(1).RevokeUserAsync(organizationUser, EventSystemUser.SCIM);
await sutProvider.GetDependency<IOrganizationUserRepository>().Received(1).ReplaceAsync(
Arg.Is<OrganizationUser>(ou => ou.ExternalId == newExternalId));
}
[Theory]
[BitAutoData]
public async Task PatchUser_RestoreAndExternalIdFromValue_DoesNotRevertRestore(SutProvider<PatchUserCommand> 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<IOrganizationUserRepository>()
.GetByIdAsync(organizationUser.Id)
.Returns(organizationUser, restoredOrgUser);
sutProvider.GetDependency<IOrganizationUserRepository>()
.GetManyDetailsByOrganizationAsync(organizationUser.OrganizationId)
.Returns(new List<OrganizationUserUserDetails>());
var scimPatchModel = new Models.ScimPatchModel
{
Operations = new List<ScimPatchModel.OperationModel>
{
new ScimPatchModel.OperationModel
{
Op = "replace",
Value = JsonDocument.Parse($"{{\"active\":true,\"externalId\":\"{newExternalId}\"}}").RootElement
}
},
Schemas = new List<string> { ScimConstants.Scim2SchemaUser }
};
await sutProvider.Sut.PatchUserAsync(organizationUser.OrganizationId, organizationUser.Id, scimPatchModel);
await sutProvider.GetDependency<IRestoreOrganizationUserCommand>().Received(1).RestoreUserAsync(organizationUser, EventSystemUser.SCIM);
// ReplaceAsync must use the re-fetched (restored) user, not the stale revoked state
await sutProvider.GetDependency<IOrganizationUserRepository>().Received(1).ReplaceAsync(
Arg.Is<OrganizationUser>(ou => ou.ExternalId == newExternalId && ou.Status != OrganizationUserStatusType.Revoked));
}
}