[PM-25520] - Adding a transaction for mutliple repo calls

.github/workflows/test-database.yml

@@ -9,6 +9,8 @@ on:
       - "hotfix-rc"
     paths:
       - ".github/workflows/test-database.yml" # This file
+      - "bitwarden_license/src/Scim/**" # SCIM source — concurrency tests cover it
+      - "bitwarden_license/test/Scim.IntegrationTest/**" # SCIM integration tests
       - "src/Sql/**" # SQL Server Database Changes
       - "util/Migrator/**" # New SQL Server Migrations
       - "util/MySqlMigrations/**" # Changes to MySQL
@@ -17,10 +19,13 @@ on:
       - "src/Infrastructure.Dapper/**" # Changes to SQL Server Dapper Repository Layer
       - "src/Infrastructure.EntityFramework/**" # Changes to Entity Framework Repository Layer
       - "test/Infrastructure.IntegrationTest/**" # Any changes to the tests
+      - "test/IntegrationTestCommon/**" # Shared test database infrastructure
       - "src/**/Entities/**/*.cs" # Database entity definitions
   pull_request:
     paths:
       - ".github/workflows/test-database.yml" # This file
+      - "bitwarden_license/src/Scim/**" # SCIM source — concurrency tests cover it
+      - "bitwarden_license/test/Scim.IntegrationTest/**" # SCIM integration tests
       - "src/Sql/**" # SQL Server Database Changes
       - "util/Migrator/**" # New SQL Server Migrations
       - "util/MySqlMigrations/**" # Changes to MySQL
@@ -29,6 +34,7 @@ on:
       - "src/Infrastructure.Dapper/**" # Changes to SQL Server Dapper Repository Layer
       - "src/Infrastructure.EntityFramework/**" # Changes to Entity Framework Repository Layer
       - "test/Infrastructure.IntegrationTest/**" # Any changes to the tests
+      - "test/IntegrationTestCommon/**" # Shared test database infrastructure
       - "src/**/Entities/**/*.cs" # Database entity definitions
 
 permissions:
@@ -139,6 +145,21 @@ jobs:
         run: dotnet test --logger "trx;LogFileName=infrastructure-test-results.trx" /p:CoverletOutputFormatter="cobertura" --collect:"XPlat Code Coverage"
         shell: pwsh
 
+      - name: Run SCIM concurrency tests
+        working-directory: "bitwarden_license/test/Scim.IntegrationTest"
+        env:
+          # Each provider's Database= points at a system DB that always exists; the
+          # *TestDatabase classes create their own per-run vault_test_<guid> and use
+          # the admin endpoint for CREATE/DROP DATABASE.
+          BW_TEST_DATABASES__0__TYPE: "Postgres"
+          BW_TEST_DATABASES__0__CONNECTIONSTRING: "Host=localhost;Username=postgres;Password=SET_A_PASSWORD_HERE_123;Database=postgres"
+          BW_TEST_DATABASES__1__TYPE: "MySql"
+          BW_TEST_DATABASES__1__CONNECTIONSTRING: "server=localhost;uid=root;pwd=SET_A_PASSWORD_HERE_123;database=mysql"
+          BW_TEST_DATABASES__2__TYPE: "SqlServer"
+          BW_TEST_DATABASES__2__CONNECTIONSTRING: "Server=localhost;Database=master;User Id=SA;Password=SET_A_PASSWORD_HERE_123;Encrypt=True;TrustServerCertificate=True;"
+        run: dotnet test --filter "FullyQualifiedName~UsersControllerConcurrencyTests" --logger "trx;LogFileName=scim-concurrency-test-results.trx"
+        shell: pwsh
+
       - name: Print MySQL Logs
         if: failure()
         run: 'docker logs "$(docker ps --quiet --filter "name=mysql")"'

bitwarden_license/src/Scim/Users/PostUserCommand.cs

@@ -1,5 +1,6 @@
 #nullable enable
 
+using System.Data;
 using Bit.Core;
 using Bit.Core.AdminConsole.Enums;
 using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.InviteUsers;
@@ -10,6 +11,7 @@
 using Bit.Core.Enums;
 using Bit.Core.Exceptions;
 using Bit.Core.Models.Data.Organizations.OrganizationUsers;
+using Bit.Core.Platform.Data;
 using Bit.Core.Repositories;
 using Bit.Core.Services;
 using Bit.Scim.Context;
@@ -27,7 +29,8 @@ public class PostUserCommand(
     IScimContext scimContext,
     IFeatureService featureService,
     IInviteOrganizationUsersCommand inviteOrganizationUsersCommand,
-    TimeProvider timeProvider)
+    TimeProvider timeProvider,
+    ITransactionManager transactionManager)
     : IPostUserCommand
 {
     public async Task<OrganizationUserUserDetails?> PostUserAsync(Guid organizationId, ScimUserRequestModel model)
@@ -45,6 +48,8 @@ public class PostUserCommand(
         Guid organizationId,
         ScimProviderType scimProvider)
     {
+        await using var transactionScope = await transactionManager.BeginTransactionAsync(IsolationLevel.Serializable);
+
         var organization = await organizationRepository.GetByIdAsync(organizationId);
 
         if (organization is null)
@@ -81,6 +86,7 @@ public class PostUserCommand(
             ? await organizationUserRepository.GetDetailsByIdAsync(invitedOrganizationUserId.Value)
             : null;
 
+        await transactionScope.CommitAsync();
         return organizationUser;
     }
 

bitwarden_license/test/Scim.IntegrationTest/Controllers/v2/UsersControllerConcurrencyTests.cs

@@ -0,0 +1,167 @@
+using Bit.Core;
+using Bit.Core.Billing.Enums;
+using Bit.Core.Enums;
+using Bit.Core.Services;
+using Bit.Infrastructure.EntityFramework.Repositories;
+using Bit.IntegrationTestCommon;
+using Bit.Scim.IntegrationTest.Factories;
+using Bit.Scim.Models;
+using Bit.Scim.Utilities;
+using NSubstitute;
+using Xunit;
+
+namespace Bit.Scim.IntegrationTest.Controllers.v2;
+
+/// <summary>
+/// Verifies seat-count integrity when SCIM invite requests run concurrently.
+/// Runs against every supported real RDBMS (SqlServer, Postgres, MySql) when its
+/// connection string is configured via BW_TEST_DATABASES__N__*. SQLite is excluded
+/// because it serializes writes globally and cannot reproduce the read-modify-write
+/// race on Organization.Seats.
+/// </summary>
+public class UsersControllerConcurrencyTests
+{
+    private static readonly Lazy<IReadOnlyDictionary<SupportedDatabaseProviders, string>> _configuredConnections =
+        new(LoadConfiguredConnections);
+
+    [SkippableTheory]
+    [MemberData(nameof(DatabaseProviders))]
+    public async Task Post_ConcurrentInvites_DoNotOvershootMaxAutoscaleSeats(
+        SupportedDatabaseProviders providerType)
+    {
+        var baseConnectionString = _configuredConnections.Value.GetValueOrDefault(providerType);
+        Skip.If(baseConnectionString is null,
+            $"{providerType}: not configured (set BW_TEST_DATABASES__N__TYPE/CONNECTIONSTRING).");
+
+        const short startingSeats = 3;
+        const int availableSeats = 2;
+        const int concurrentInvites = 6;
+
+        const string testDatabaseName = "vault_test_scim";
+        ITestDatabase testDatabase = providerType switch
+        {
+            SupportedDatabaseProviders.SqlServer => new SqlServerTestDatabase(baseConnectionString!, testDatabaseName),
+            SupportedDatabaseProviders.Postgres => new PostgresTestDatabase(baseConnectionString!, testDatabaseName),
+            SupportedDatabaseProviders.MySql => new MySqlTestDatabase(baseConnectionString!, testDatabaseName),
+            _ => throw new InvalidOperationException($"Unsupported provider: {providerType}"),
+        };
+
+        var factory = new ScimApplicationFactory
+        {
+            TestDatabase = testDatabase
+        };
+
+        factory.SubstituteService((IFeatureService f) => f.IsEnabled(FeatureFlagKeys.ScimInviteUserOptimization)
+            .Returns(true));
+
+        try
+        {
+            factory.ReinitializeDbForTests(factory.GetDatabaseContext());
+
+            using (var setupScope = factory.Services.CreateScope())
+            {
+                var setupContext = setupScope.ServiceProvider.GetRequiredService<DatabaseContext>();
+                var org = setupContext.Organizations.Single(o => o.Id == ScimApplicationFactory.TestOrganizationId1);
+                org.PlanType = PlanType.EnterpriseAnnually;
+                org.Plan = "Enterprise (Annually)";
+                org.Seats = startingSeats;
+                org.MaxAutoscaleSeats = startingSeats + availableSeats;
+                await setupContext.SaveChangesAsync();
+            }
+
+            var inputs = Enumerable.Range(0, concurrentInvites).Select(BuildInvite).ToArray();
+
+            var responses = await Task.WhenAll(
+                inputs.Select(input =>
+                    factory.UsersPostAsync(ScimApplicationFactory.TestOrganizationId1, input)));
+
+            var successfulInvites = responses.Count(r => r.Response.StatusCode == StatusCodes.Status201Created);
+
+            using var verifyScope = factory.Services.CreateScope();
+            var verifyContext = verifyScope.ServiceProvider.GetRequiredService<DatabaseContext>();
+            var finalOrg = verifyContext.Organizations
+                .Single(o => o.Id == ScimApplicationFactory.TestOrganizationId1);
+            var finalActiveUserCount = verifyContext.OrganizationUsers
+                .Count(ou => ou.OrganizationId == ScimApplicationFactory.TestOrganizationId1 && ou.Status >= 0);
+
+            Assert.All(responses, r => Assert.True(r.Response.StatusCode < 500,
+                $"Expected non-5xx status, got {r.Response.StatusCode}"));
+
+            Assert.Equal(startingSeats + successfulInvites, finalOrg.Seats);
+
+            Assert.Equal(startingSeats + successfulInvites, finalActiveUserCount);
+
+            Assert.True(finalOrg.Seats <= finalOrg.MaxAutoscaleSeats,
+                $"Seats {finalOrg.Seats} exceeded MaxAutoscaleSeats {finalOrg.MaxAutoscaleSeats}");
+        }
+        finally
+        {
+            await factory.DisposeAsync();
+        }
+    }
+
+    public static IEnumerable<object?[]> DatabaseProviders()
+    {
+        yield return [SupportedDatabaseProviders.SqlServer];
+        yield return [SupportedDatabaseProviders.Postgres];
+        yield return [SupportedDatabaseProviders.MySql];
+    }
+
+    private static Dictionary<SupportedDatabaseProviders, string> LoadConfiguredConnections()
+    {
+        var config = new ConfigurationBuilder()
+            .AddUserSecrets(typeof(Bit.Identity.Startup).Assembly, optional: true)
+            .AddEnvironmentVariables("BW_TEST_")
+            .Build();
+
+        var configured = new Dictionary<SupportedDatabaseProviders, string>();
+
+        // Preferred source: BW_TEST_DATABASES__N__* env vars (set by test-database.yml in CI)
+        for (var i = 0; ; i++)
+        {
+            var rawType = config[$"DATABASES:{i}:TYPE"];
+            var connectionString = config[$"DATABASES:{i}:CONNECTIONSTRING"];
+            if (rawType is null && connectionString is null)
+            {
+                break;
+            }
+            if (rawType is null || connectionString is null)
+            {
+                continue;
+            }
+            if (Enum.TryParse<SupportedDatabaseProviders>(rawType, ignoreCase: true, out var type))
+            {
+                configured.TryAdd(type, connectionString);
+            }
+        }
+
+        // Fallback for local dev: Identity user secrets (globalSettings:<provider>:connectionString)
+        TryAddFromUserSecrets(SupportedDatabaseProviders.SqlServer, "globalSettings:sqlServer:connectionString");
+        TryAddFromUserSecrets(SupportedDatabaseProviders.Postgres, "globalSettings:postgreSql:connectionString");
+        TryAddFromUserSecrets(SupportedDatabaseProviders.MySql, "globalSettings:mySql:connectionString");
+
+        return configured;
+
+        void TryAddFromUserSecrets(SupportedDatabaseProviders type, string key)
+        {
+            if (configured.ContainsKey(type))
+            {
+                return;
+            }
+            var value = config[key];
+            if (!string.IsNullOrWhiteSpace(value))
+            {
+                configured[type] = value;
+            }
+        }
+    }
+
+    private static ScimUserRequestModel BuildInvite(int i) => new()
+    {
+        DisplayName = $"Concurrent User {i}",
+        Emails = [new() { Primary = true, Type = "work", Value = $"concurrent-{i}@example.com" }],
+        ExternalId = $"CONC-{i}",
+        Active = true,
+        Schemas = [ScimConstants.Scim2SchemaUser]
+    };
+}

bitwarden_license/test/Scim.IntegrationTest/Scim.IntegrationTest.csproj

@@ -15,6 +15,7 @@
     <PackageReference Include="Microsoft.NET.Test.Sdk" Version="$(MicrosoftNetTestSdkVersion)" />
     <PackageReference Include="NSubstitute" Version="$(NSubstituteVersion)" />
     <PackageReference Include="xunit" Version="$(XUnitVersion)" />
+    <PackageReference Include="Xunit.SkippableFact" Version="1.4.13" />
     <PackageReference Include="xunit.runner.visualstudio" Version="$(XUnitRunnerVisualStudioVersion)">
       <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
       <PrivateAssets>all</PrivateAssets>

src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/InviteUsers/InviteOrganizationUsersCommand.cs

@@ -171,11 +171,10 @@ private async Task<CommandResult<InviteOrganizationUsersResponse>> InviteOrganiz
         {
             logger.LogError(ex, FailedToInviteUsersError.Code);
 
-            await organizationUserRepository.DeleteManyAsync(organizationUserToInviteEntities.Select(x => x.OrganizationUser.Id));
-
-            // Do this first so that SmSeats never exceed PM seats (due to current billing requirements)
+            // The ambient transaction rolls back DB writes; the calls below compensate for
+            // state that lives outside the transaction (Stripe subscription, application cache).
+            // SM revert runs first so SmSeats never exceed PM seats (current billing requirement).
             await RevertSecretsManagerChangesAsync(validatedRequest, organization, validatedRequest.Value.InviteOrganization.SmSeats);
-
             await RevertPasswordManagerChangesAsync(validatedRequest, organization);
 
             return new Failure<InviteOrganizationUsersResponse>(
@@ -206,7 +205,8 @@ private async Task RevertPasswordManagerChangesAsync(Valid<InviteOrganizationUse
         {
             organization.Seats = (short?)validatedResult.Value.PasswordManagerSubscriptionUpdate.Seats;
 
-            await organizationRepository.ReplaceAsync(organization);
+            // Compensating write: the application cache is not part of the ambient transaction,
+            // so we must explicitly restore the pre-increment seat count.
             await applicationCacheService.UpsertOrganizationAbilityAsync(organization);
         }
     }

src/Core/Platform/Data/ITransactionManager.cs

@@ -0,0 +1,22 @@
+using System.Data;
+
+namespace Bit.Core.Platform.Data;
+
+/// <summary>
+/// Manages ambient database transactions that span multiple repository calls.
+/// Implementations are singleton-safe; transaction state is stored per async flow.
+/// </summary>
+public interface ITransactionManager
+{
+    /// <summary>
+    /// Begins a new ambient transaction. All repository operations on the current
+    /// async flow will use the same connection and transaction until disposed.
+    /// Supports nesting: inner calls return a no-op scope that shares the outer
+    /// transaction. Only the outermost Commit/Dispose actually affects the database.
+    /// The isolation level on nested calls is ignored — inner scopes inherit the
+    /// outer transaction's isolation level.
+    /// </summary>
+    Task<ITransactionScope> BeginTransactionAsync(
+        IsolationLevel isolationLevel = IsolationLevel.ReadCommitted,
+        CancellationToken cancellationToken = default);
+}

src/Core/Platform/Data/ITransactionScope.cs

@@ -0,0 +1,11 @@
+namespace Bit.Core.Platform.Data;
+
+/// <summary>
+/// Represents an ambient transaction scope. Commit must be called explicitly;
+/// disposing without committing triggers rollback.
+/// </summary>
+public interface ITransactionScope : IAsyncDisposable
+{
+    Task CommitAsync(CancellationToken cancellationToken = default);
+    Task RollbackAsync(CancellationToken cancellationToken = default);
+}

src/Core/Platform/Data/NestedTransactionScope.cs

@@ -0,0 +1,36 @@
+namespace Bit.Core.Platform.Data;
+
+public sealed class NestedTransactionScope : ITransactionScope
+{
+    private readonly TransactionHolder _holder;
+    private bool _disposed;
+
+    public NestedTransactionScope(TransactionHolder holder)
+    {
+        _holder = holder;
+    }
+
+    public Task CommitAsync(CancellationToken cancellationToken = default)
+    {
+        // Nested scope commit is a no-op; only the root scope commits.
+        return Task.CompletedTask;
+    }
+
+    public Task RollbackAsync(CancellationToken cancellationToken = default)
+    {
+        // Mark the transaction as doomed so the root scope cannot commit.
+        _holder.MarkDoomed();
+        return Task.CompletedTask;
+    }
+
+    public ValueTask DisposeAsync()
+    {
+        if (_disposed)
+        {
+            return ValueTask.CompletedTask;
+        }
+
+        _disposed = true;
+        return ValueTask.CompletedTask;
+    }
+}