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

[JaredScar]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-19232

📔 Objective

Fit EntraID handling

[github-actions]

Checkmarx One – Scan Summary & Details – cacbc668-ceb5-4cf0-b3e1-c475c3f95393

Great job! No new security vulnerabilities introduced in this pull request

[claude]

Claude finished @JaredScar's task in 4m 34s —— View job

---

Review: PM-19232 Implement externalId handling in PatchUserCommand with validation

Overview

This PR adds externalId handling to the SCIM PatchUserCommand, supporting both path-based ("path": "externalId") and value-object-based ("value": {"externalId": "..."}) replace operations. It includes validation for max length (300 characters) and duplicate externalId checks, consistent with PostUserCommand.

Findings

Severity	File	Description
⚠️ IMPORTANT	PatchUserCommand.cs L55-78	The else if chain between value-object branches means a combined value object containing both active and externalId will only process active, silently dropping the externalId update. This is valid per SCIM RFC 7644 Section 3.5.2.2.

Previous Finding Status

Finding	Status
Breaking change: throw vs log for unhandled operations	Resolved -- Developer reverted to log-warning behavior and tests updated in latest commit (ca4d73d).
Test-code mismatch for unsupported operations	Resolved -- Tests now correctly assert log-warning behavior (no exception thrown, returns 204).

What Looks Good

• The HandleExternalIdOperationAsync method has appropriate validation: 300-char max length matching the entity constraint, case-insensitive duplicate check matching PostUserCommand patterns.
• New constant PatchPaths.ExternalId improves consistency over magic strings.
• Comprehensive test coverage with 6 unit tests and 4 integration tests covering happy paths, error cases (duplicate, too long), null/clearing, and unsupported operations.
• Setting externalId to null correctly clears the value without triggering validation, allowing identity providers to unset the field.

Checklist

• Security: No sensitive data exposed; validation prevents data integrity issues
• Breaking changes: Unsupported operation behavior resolved per developer decision
• Test coverage: Both unit and integration tests cover new functionality
• Error handling: Appropriate BadRequestException and ConflictException thrown

[JimmyVo16]

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.

[JimmyVo16]

Ping me again when you need a reapproval after fixing the tests.

[claude]

⚠️ 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.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[codecov]

Codecov Report

❌ Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.36%. Comparing base (94f7266) to head (ca4d73d).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...twarden_license/src/Scim/Users/PatchUserCommand.cs	93.33%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6998      +/-   ##
==========================================
+ Coverage   56.28%   56.36%   +0.07%     
==========================================
  Files        1986     1990       +4     
  Lines       87667    87861     +194     
  Branches     7816     7840      +24     
==========================================
+ Hits        49345    49524     +179     
- Misses      36491    36502      +11     
- Partials     1831     1835       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.