Merge branch 'main' into ps/PM-15127-tighten-scope-secrets

Author: dani-garcia

.github/workflows/build.yml

@@ -324,6 +324,8 @@ jobs:
         uses: github/codeql-action/upload-sarif@dd746615b3b9d728a6a37ca2045b68ca76d4841a # v3.28.8
         with:
           sarif_file: ${{ steps.container-scan.outputs.sarif }}
+          sha: ${{ contains(github.event_name, 'pull_request') && github.event.pull_request.head.sha || github.sha }}
+          ref: ${{ contains(github.event_name, 'pull_request') && format('refs/pull/{0}/head', github.event.pull_request.number) || github.ref }}
 
   upload:
     name: Upload

.github/workflows/scan.yml

@@ -49,6 +49,8 @@ jobs:
         uses: github/codeql-action/upload-sarif@dd746615b3b9d728a6a37ca2045b68ca76d4841a # v3.28.8
         with:
           sarif_file: cx_result.sarif
+          sha: ${{ contains(github.event_name, 'pull_request') && github.event.pull_request.head.sha || github.sha }}
+          ref: ${{ contains(github.event_name, 'pull_request') && format('refs/pull/{0}/head', github.event.pull_request.number) || github.ref }}
 
   quality:
     name: Quality scan

Directory.Build.props

@@ -3,7 +3,7 @@
   <PropertyGroup>
     <TargetFramework>net8.0</TargetFramework>
 
-    <Version>2025.3.6</Version>
+    <Version>2025.3.3</Version>
 
     <RootNamespace>Bit.$(MSBuildProjectName)</RootNamespace>
     <ImplicitUsings>enable</ImplicitUsings>

bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs

@@ -1,10 +1,8 @@
-using Bit.Core;
-using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces;
+using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces;
 using Bit.Core.AdminConsole.Repositories;
 using Bit.Core.Enums;
 using Bit.Core.Exceptions;
 using Bit.Core.Repositories;
-using Bit.Core.Services;
 using Bit.Scim.Groups.Interfaces;
 using Bit.Scim.Models;
 using Bit.Scim.Utilities;
@@ -24,32 +22,26 @@ public class GroupsController : Controller
     private readonly IGetGroupsListQuery _getGroupsListQuery;
     private readonly IDeleteGroupCommand _deleteGroupCommand;
     private readonly IPatchGroupCommand _patchGroupCommand;
-    private readonly IPatchGroupCommandvNext _patchGroupCommandvNext;
     private readonly IPostGroupCommand _postGroupCommand;
     private readonly IPutGroupCommand _putGroupCommand;
-    private readonly IFeatureService _featureService;
 
     public GroupsController(
         IGroupRepository groupRepository,
         IOrganizationRepository organizationRepository,
         IGetGroupsListQuery getGroupsListQuery,
         IDeleteGroupCommand deleteGroupCommand,
         IPatchGroupCommand patchGroupCommand,
-        IPatchGroupCommandvNext patchGroupCommandvNext,
         IPostGroupCommand postGroupCommand,
-        IPutGroupCommand putGroupCommand,
-        IFeatureService featureService
+        IPutGroupCommand putGroupCommand
         )
     {
         _groupRepository = groupRepository;
         _organizationRepository = organizationRepository;
         _getGroupsListQuery = getGroupsListQuery;
         _deleteGroupCommand = deleteGroupCommand;
         _patchGroupCommand = patchGroupCommand;
-        _patchGroupCommandvNext = patchGroupCommandvNext;
         _postGroupCommand = postGroupCommand;
         _putGroupCommand = putGroupCommand;
-        _featureService = featureService;
     }
 
     [HttpGet("{id}")]
@@ -103,21 +95,13 @@ public async Task<IActionResult> Put(Guid organizationId, Guid id, [FromBody] Sc
     [HttpPatch("{id}")]
     public async Task<IActionResult> Patch(Guid organizationId, Guid id, [FromBody] ScimPatchModel model)
     {
-        if (_featureService.IsEnabled(FeatureFlagKeys.ShortcutDuplicatePatchRequests))
+        var group = await _groupRepository.GetByIdAsync(id);
+        if (group == null || group.OrganizationId != organizationId)
         {
-            var group = await _groupRepository.GetByIdAsync(id);
-            if (group == null || group.OrganizationId != organizationId)
-            {
-                throw new NotFoundException("Group not found.");
-            }
-
-            await _patchGroupCommandvNext.PatchGroupAsync(group, model);
-            return new NoContentResult();
+            throw new NotFoundException("Group not found.");
         }
 
-        var organization = await _organizationRepository.GetByIdAsync(organizationId);
-        await _patchGroupCommand.PatchGroupAsync(organization, id, model);
-
+        await _patchGroupCommand.PatchGroupAsync(group, model);
         return new NoContentResult();
     }
 

bitwarden_license/src/Scim/Groups/Interfaces/IPatchGroupCommand.cs

@@ -5,5 +5,5 @@ namespace Bit.Scim.Groups.Interfaces;
 
 public interface IPatchGroupCommand
 {
-    Task PatchGroupAsync(Organization organization, Guid id, ScimPatchModel model);
+    Task PatchGroupAsync(Group group, ScimPatchModel model);
 }

bitwarden_license/src/Scim/Groups/Interfaces/IPatchGroupCommandvNext.cs

@@ -5,8 +5,10 @@
 using Bit.Core.AdminConsole.Services;
 using Bit.Core.Enums;
 using Bit.Core.Exceptions;
+using Bit.Core.Repositories;
 using Bit.Scim.Groups.Interfaces;
 using Bit.Scim.Models;
+using Bit.Scim.Utilities;
 
 namespace Bit.Scim.Groups;
 
@@ -16,118 +18,137 @@ public class PatchGroupCommand : IPatchGroupCommand
     private readonly IGroupService _groupService;
     private readonly IUpdateGroupCommand _updateGroupCommand;
     private readonly ILogger<PatchGroupCommand> _logger;
+    private readonly IOrganizationRepository _organizationRepository;
 
     public PatchGroupCommand(
         IGroupRepository groupRepository,
         IGroupService groupService,
         IUpdateGroupCommand updateGroupCommand,
-        ILogger<PatchGroupCommand> logger)
+        ILogger<PatchGroupCommand> logger,
+        IOrganizationRepository organizationRepository)
     {
         _groupRepository = groupRepository;
         _groupService = groupService;
         _updateGroupCommand = updateGroupCommand;
         _logger = logger;
+        _organizationRepository = organizationRepository;
     }
 
-    public async Task PatchGroupAsync(Organization organization, Guid id, ScimPatchModel model)
+    public async Task PatchGroupAsync(Group group, ScimPatchModel model)
     {
-        var group = await _groupRepository.GetByIdAsync(id);
-        if (group == null || group.OrganizationId != organization.Id)
+        foreach (var operation in model.Operations)
         {
-            throw new NotFoundException("Group not found.");
+            await HandleOperationAsync(group, operation);
         }
+    }
 
-        var operationHandled = false;
-        foreach (var operation in model.Operations)
+    private async Task HandleOperationAsync(Group group, ScimPatchModel.OperationModel operation)
+    {
+        switch (operation.Op?.ToLowerInvariant())
         {
-            // Replace operations
-            if (operation.Op?.ToLowerInvariant() == "replace")
-            {
-                // Replace a list of members
-                if (operation.Path?.ToLowerInvariant() == "members")
+            // Replace a list of members
+            case PatchOps.Replace when operation.Path?.ToLowerInvariant() == PatchPaths.Members:
                 {
                     var ids = GetOperationValueIds(operation.Value);
                     await _groupRepository.UpdateUsersAsync(group.Id, ids);
-                    operationHandled = true;
+                    break;
                 }
-                // Replace group name from path
-                else if (operation.Path?.ToLowerInvariant() == "displayname")
+
+            // Replace group name from path
+            case PatchOps.Replace when operation.Path?.ToLowerInvariant() == PatchPaths.DisplayName:
                 {
                     group.Name = operation.Value.GetString();
+                    var organization = await _organizationRepository.GetByIdAsync(group.OrganizationId);
+                    if (organization == null)
+                    {
+                        throw new NotFoundException();
+                    }
                     await _updateGroupCommand.UpdateGroupAsync(group, organization, EventSystemUser.SCIM);
-                    operationHandled = true;
+                    break;
                 }
-                // Replace group name from value object
-                else if (string.IsNullOrWhiteSpace(operation.Path) &&
-                    operation.Value.TryGetProperty("displayName", out var displayNameProperty))
+
+            // Replace group name from value object
+            case PatchOps.Replace when
+                string.IsNullOrWhiteSpace(operation.Path) &&
+                operation.Value.TryGetProperty("displayName", out var displayNameProperty):
                 {
                     group.Name = displayNameProperty.GetString();
+                    var organization = await _organizationRepository.GetByIdAsync(group.OrganizationId);
+                    if (organization == null)
+                    {
+                        throw new NotFoundException();
+                    }
                     await _updateGroupCommand.UpdateGroupAsync(group, organization, EventSystemUser.SCIM);
-                    operationHandled = true;
+                    break;
                 }
-            }
+
             // Add a single member
-            else if (operation.Op?.ToLowerInvariant() == "add" &&
+            case PatchOps.Add when
                 !string.IsNullOrWhiteSpace(operation.Path) &&
-                operation.Path.ToLowerInvariant().StartsWith("members[value eq "))
-            {
-                var addId = GetOperationPathId(operation.Path);
-                if (addId.HasValue)
+                operation.Path.StartsWith("members[value eq ", StringComparison.OrdinalIgnoreCase) &&
+                TryGetOperationPathId(operation.Path, out var addId):
                 {
-                    var orgUserIds = (await _groupRepository.GetManyUserIdsByIdAsync(group.Id)).ToHashSet();
-                    orgUserIds.Add(addId.Value);
-                    await _groupRepository.UpdateUsersAsync(group.Id, orgUserIds);
-                    operationHandled = true;
+                    await AddMembersAsync(group, [addId]);
+                    break;
                 }
-            }
+
             // Add a list of members
-            else if (operation.Op?.ToLowerInvariant() == "add" &&
-                operation.Path?.ToLowerInvariant() == "members")
-            {
-                var orgUserIds = (await _groupRepository.GetManyUserIdsByIdAsync(group.Id)).ToHashSet();
-                foreach (var v in GetOperationValueIds(operation.Value))
+            case PatchOps.Add when
+                operation.Path?.ToLowerInvariant() == PatchPaths.Members:
                 {
-                    orgUserIds.Add(v);
+                    await AddMembersAsync(group, GetOperationValueIds(operation.Value));
+                    break;
                 }
-                await _groupRepository.UpdateUsersAsync(group.Id, orgUserIds);
-                operationHandled = true;
-            }
+
             // Remove a single member
-            else if (operation.Op?.ToLowerInvariant() == "remove" &&
+            case PatchOps.Remove when
                 !string.IsNullOrWhiteSpace(operation.Path) &&
-                operation.Path.ToLowerInvariant().StartsWith("members[value eq "))
-            {
-                var removeId = GetOperationPathId(operation.Path);
-                if (removeId.HasValue)
+                operation.Path.StartsWith("members[value eq ", StringComparison.OrdinalIgnoreCase) &&
+                TryGetOperationPathId(operation.Path, out var removeId):
                 {
-                    await _groupService.DeleteUserAsync(group, removeId.Value, EventSystemUser.SCIM);
-                    operationHandled = true;
+                    await _groupService.DeleteUserAsync(group, removeId, EventSystemUser.SCIM);
+                    break;
                 }
-            }
+
             // Remove a list of members
-            else if (operation.Op?.ToLowerInvariant() == "remove" &&
-                operation.Path?.ToLowerInvariant() == "members")
-            {
-                var orgUserIds = (await _groupRepository.GetManyUserIdsByIdAsync(group.Id)).ToHashSet();
-                foreach (var v in GetOperationValueIds(operation.Value))
+            case PatchOps.Remove when
+                operation.Path?.ToLowerInvariant() == PatchPaths.Members:
                 {
-                    orgUserIds.Remove(v);
+                    var orgUserIds = (await _groupRepository.GetManyUserIdsByIdAsync(group.Id)).ToHashSet();
+                    foreach (var v in GetOperationValueIds(operation.Value))
+                    {
+                        orgUserIds.Remove(v);
+                    }
+                    await _groupRepository.UpdateUsersAsync(group.Id, orgUserIds);
+                    break;
+                }
+
+            default:
+                {
+                    _logger.LogWarning("Group patch operation not handled: {OperationOp}:{OperationPath}", operation.Op, operation.Path);
+                    break;
                 }
-                await _groupRepository.UpdateUsersAsync(group.Id, orgUserIds);
-                operationHandled = true;
-            }
         }
+    }
 
-        if (!operationHandled)
+    private async Task AddMembersAsync(Group group, HashSet<Guid> usersToAdd)
+    {
+        // Azure Entra ID is known to send redundant "add" requests for each existing member every time any member
+        // is removed. To avoid excessive load on the database, we check against the high availability replica and
+        // return early if they already exist.
+        var groupMembers = await _groupRepository.GetManyUserIdsByIdAsync(group.Id, useReadOnlyReplica: true);
+        if (usersToAdd.IsSubsetOf(groupMembers))
         {
-            _logger.LogWarning("Group patch operation not handled: {0} : ",
-                string.Join(", ", model.Operations.Select(o => $"{o.Op}:{o.Path}")));
+            _logger.LogDebug("Ignoring duplicate SCIM request to add members {Members} to group {Group}", usersToAdd, group.Id);
+            return;
         }
+
+        await _groupRepository.AddGroupUsersByIdAsync(group.Id, usersToAdd);
     }
 
-    private List<Guid> GetOperationValueIds(JsonElement objArray)
+    private static HashSet<Guid> GetOperationValueIds(JsonElement objArray)
     {
-        var ids = new List<Guid>();
+        var ids = new HashSet<Guid>();
         foreach (var obj in objArray.EnumerateArray())
         {
             if (obj.TryGetProperty("value", out var valueProperty))
@@ -141,13 +162,9 @@ private List<Guid> GetOperationValueIds(JsonElement objArray)
         return ids;
     }
 
-    private Guid? GetOperationPathId(string path)
+    private static bool TryGetOperationPathId(string path, out Guid pathId)
     {
         // Parse Guid from string like: members[value eq "{GUID}"}]
-        if (Guid.TryParse(path.Substring(18).Replace("\"]", string.Empty), out var id))
-        {
-            return id;
-        }
-        return null;
+        return Guid.TryParse(path.Substring(18).Replace("\"]", string.Empty), out pathId);
     }
 }