[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 – 97631eec-c25f-410b-b07b-d7e62a2c938f

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

[claude]

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

---

Re-Review: PR #6998 - PM-19232 Implement externalId handling in PatchUserCommand with validation

Reviewed commits since last review: df7935f15, c61e31bda, 8651cb38e, 4a764e9c1

Previous Findings Status

Finding	Severity	Status
Stale state on restore + externalId combined operation	CRITICAL	Fixed in 8651cb38e -- re-fetch added after active operations
Missing indentation on closing brace	IMPORTANT	Fixed in c61e31bda
Missing test for combined value object	SUGGESTED	Addressed -- PatchUser_ActiveAndExternalIdFromValue_Success and PatchUser_RestoreAndExternalIdFromValue_DoesNotRevertRestore added

New Changes Analysis

The latest commits correctly address all previously identified issues:

1. Re-fetch logic (PatchUserCommand.cs lines 53-58, 77-82): After HandleActiveOperationAsync returns true, the entity is re-fetched from the repository. This prevents ReplaceAsync from overwriting database state with stale in-memory values. The fix is applied in both the path-based active branch and the value-object active branch.

2. Test coverage (PatchUserCommandTests.cs lines 386-467): Two new tests verify the combined active + externalId scenario. The restore test (PatchUser_RestoreAndExternalIdFromValue_DoesNotRevertRestore) specifically validates that the re-fetched entity has non-revoked status, which would have caught the original bug.

Verdict

No new issues found. All previously identified issues have been resolved. The PR is in good shape.

Overall: Approve -- The implementation correctly handles externalId SCIM PATCH operations with proper validation (length, uniqueness), supports both path-based and value-object formats, and handles combined operations safely with the re-fetch pattern.

[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.

[codecov]

Codecov Report

❌ Patch coverage is 93.47826% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.69%. Comparing base (25df3c3) to head (4a764e9).
⚠️ Report is 1 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6998      +/-   ##
==========================================
+ Coverage   56.67%   56.69%   +0.01%     
==========================================
  Files        2013     2013              
  Lines       88131    88171      +40     
  Branches     7846     7856      +10     
==========================================
+ Hits        49949    49987      +38     
- Misses      36362    36363       +1     
- Partials     1820     1821       +1     

☔ 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.

[JimmyVo16]

The code is approved, but please make sure to add test coverage for all paths since it’s a complex method with many nested conditions.

[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