Skip to content

[PM-19232] Implement externalId handling in PatchUserCommand with validation #6998

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JaredScar wants to merge 5 commits into
base: main
Choose a base branch
from
+351 −1
Open

[PM-19232] Implement externalId handling in PatchUserCommand with validation #6998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a62d5ef
Implement externalId handling in PatchUserCommand with validation and…
JaredScar Feb 12, 2026
255daa1
Merge branch 'main' into ac/pm-19232-entraID-reports-successful-PATCH…
JaredScar Feb 12, 2026
5905907
Change back for testing because we don't want to potentially stop cod…
JaredScar Feb 13, 2026
33ef792
Merge branch 'ac/pm-19232-entraID-reports-successful-PATCH-ops-fix' o…
JaredScar Feb 13, 2026
ca4d73d
Refactor PatchUserCommand and related tests to log warnings for unsup…
JaredScar Feb 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 41 additions & 1 deletion bitwarden_license/src/Scim/Users/PatchUserCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Bit.Core.Repositories;
using Bit.Scim.Models;
using Bit.Scim.Users.Interfaces;
using Bit.Scim.Utilities;

namespace Bit.Scim.Users;

Expand Down Expand Up @@ -38,7 +39,7 @@ public async Task PatchUserAsync(Guid organizationId, Guid id, ScimPatchModel mo
foreach (var operation in model.Operations)
{
// Replace operations
if (operation.Op?.ToLowerInvariant() == "replace")
if (operation.Op?.ToLowerInvariant() == PatchOps.Replace)
Copy link
Contributor

@JimmyVo16 JimmyVo16 Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocker: I know you're just following the existing pattern, so the code as is good, but consider abstracting this logic into its own method and using the early return pattern to reduce nested conditionals when possible.

            // Replace operations
            if (operation.Op?.ToLowerInvariant() == PatchOps.Replace)
            {
                // Active from path
                if (operation.Path?.ToLowerInvariant() == "active")
                {
                    var active = operation.Value.ToString()?.ToLowerInvariant();
                    var handled = await HandleActiveOperationAsync(orgUser, active == "true");
                    if (!operationHandled)
                    {
                        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)
                    {
                        operationHandled = handled;
                    }
                }
                // ExternalId from path
                else if (operation.Path?.ToLowerInvariant() == PatchPaths.ExternalId)
                {
                    var newExternalId = operation.Value.GetString();
                    await HandleExternalIdOperationAsync(orgUser, newExternalId);
                    operationHandled = true;
                }
                // ExternalId from value object
                else if (string.IsNullOrWhiteSpace(operation.Path) &&
                    operation.Value.TryGetProperty("externalId", out var externalIdProperty))
                {
                    var newExternalId = externalIdProperty.GetString();
                    await HandleExternalIdOperationAsync(orgUser, newExternalId);
                    operationHandled = true;
                }
            }

Methods are normally used for DRY, but I think the ability to keep the scope small and let the method’s name provide a quick summary of what this group of code is doing makes it easier to understand at a glance. Also, the early return pattern makes it easier to follow.

{
// Active from path
if (operation.Path?.ToLowerInvariant() == "active")
Expand All @@ -60,6 +61,21 @@ public async Task PatchUserAsync(Guid organizationId, Guid id, ScimPatchModel mo
operationHandled = handled;
}
}
// ExternalId from path
else if (operation.Path?.ToLowerInvariant() == PatchPaths.ExternalId)
{
var newExternalId = operation.Value.GetString();
await HandleExternalIdOperationAsync(orgUser, newExternalId);
operationHandled = true;
}
// ExternalId from value object
else if (string.IsNullOrWhiteSpace(operation.Path) &&
operation.Value.TryGetProperty("externalId", out var externalIdProperty))
{
var newExternalId = externalIdProperty.GetString();
await HandleExternalIdOperationAsync(orgUser, newExternalId);
operationHandled = true;
}
Comment on lines +64 to +78
Copy link
Contributor

@claude claude bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ IMPORTANT -- Correctness: combined value object silently drops externalId

The else if chain between the value-object branches (lines 55 and 72) means that when a SCIM provider sends a single replace operation with no path and a value object containing both active and externalId properties, only active is processed and the externalId update is silently dropped.

Example request that triggers this:

{
  "op": "replace",
  "value": { "active": false, "externalId": "new-id" }
}

This is valid per SCIM RFC 7644 Section 3.5.2.2 (replace with no path targets the resource itself with all sub-attributes in the value). SCIM providers like EntraID could reasonably send combined property updates in a single operation.

Suggested fix: For the value-object branches (no path specified), check for each property independently rather than using else if:

// Value object with no path - check for each supported property
if (string.IsNullOrWhiteSpace(operation.Path))
{
    if (operation.Value.TryGetProperty("active", out var activeProperty))
    {
        var handled = await HandleActiveOperationAsync(orgUser, activeProperty.GetBoolean());
        if (!operationHandled)
        {
            operationHandled = handled;
        }
    }
    if (operation.Value.TryGetProperty("externalId", out var externalIdProperty))
    {
        var newExternalId = externalIdProperty.GetString();
        await HandleExternalIdOperationAsync(orgUser, newExternalId);
        operationHandled = true;
    }
}

This preserves the existing behavior for path-based operations while correctly handling combined value objects.

}
}

Expand All @@ -84,4 +100,28 @@ private async Task<bool> HandleActiveOperationAsync(Core.Entities.OrganizationUs
}
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);
}
}
1 change: 1 addition & 0 deletions bitwarden_license/src/Scim/Utilities/ScimConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,5 @@ public static class PatchPaths
{
public const string Members = "members";
public const string DisplayName = "displayname";
public const string ExternalId = "externalid";
}
112 changes: 112 additions & 0 deletions bitwarden_license/test/Scim.IntegrationTest/Controllers/v2/UsersControllerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,118 @@ public async Task Patch_NotFound()
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()
{
Expand Down
Loading
Loading