NSA-9931: Fix Entra registration race condition

Directory.Packages.props

@@ -125,5 +125,7 @@
     <!-- Mocking -->
     <PackageVersion Include="Moq" Version="4.20.72" />
     <PackageVersion Include="Moq.AutoMock" Version="3.5.0" />
+    <!-- Relational-constraint testing (EF Core InMemory provider does not enforce unique indexes) -->
+    <PackageVersion Include="Microsoft.EntityFrameworkCore.Sqlite" Version="9.0.9" />
   </ItemGroup>
 </Project>

src/Dfe.SignIn.Core.Contracts/Audit/AuditConstants.cs

@@ -68,6 +68,11 @@ public static class AuditAuthEventNames
     /// Indicates that a new user was created and linked.
     /// </summary>
     public const string LinkToNewUser = "link-to-new-user";
+
+    /// <summary>
+    /// Indicates that linking an Entra account to a DfE Sign-In user permanently failed.
+    /// </summary>
+    public const string LinkFailed = "link-failed";
 }
 
 /// <summary>

src/Dfe.SignIn.Core.UseCases/Users/CreateUserUseCase.cs

@@ -53,14 +53,41 @@
             FirstName = context.Request.FirstName,
             IsEntra = true,
             LastName = context.Request.LastName,
             Password = "none",
             Salt = string.Empty,
             Status = (int)AccountStatus.Active,
             Sub = Guid.NewGuid(),
         };
 
         await unitOfWork.AddAsync(newUser, cancellationToken);
-        await unitOfWork.SaveChangesAsync(cancellationToken);
+
+        try {
+            await unitOfWork.SaveChangesAsync(cancellationToken);
+        }
+        catch (DbUpdateException ex) when (IsUniqueConstraintViolation(ex)) {
+            // A concurrent request for the same Entra identity may have won the race to
+            // create this user (e.g. two simultaneous sign-ins for the same account).
+            // Email and EntraOid are each independently enforced as unique, so a row
+            // matching both values is the unambiguous race winner. A row matching only
+            // one of the two (e.g. the email is already used by a different Entra
+            // identity) is a genuine conflict, not a self-race - that must surface as
+            // the original exception rather than silently resolving to someone else's
+            // account.
+            var raceWinner = await unitOfWork.Repository<UserEntity>()
+                    .Where(x =>
+                        x.Email == context.Request.EmailAddress &&
+                        x.EntraOid == context.Request.EntraUserId)
+                    .Select(x => new { x.Sub })
+                    .SingleOrDefaultAsync(cancellationToken);
+
+            if (raceWinner is null) {
+                throw;
+            }
+
+            return new CreateUserResponse {
+                UserId = raceWinner.Sub
+            };
+        }
 
         await interaction.DispatchAsync(
             new UpdateUserInSearchIndexRequest {
@@ -72,4 +99,17 @@
             UserId = newUser.Sub
         };
     }
+
+    // SqlException's constructors are internal, so it can't be inspected by type/Number
+    // here without a hard dependency on Microsoft.Data.SqlClient in this layer. Matching
+    // on the well-known SQL Server unique-violation message text (2601: unique index,
+    // 2627: named unique constraint - this table has both, see UserEntityConfiguration)
+    // keeps this scoped to actual unique-constraint races, not any other DbUpdateException
+    // cause (e.g. FK violations, timeouts) that recovery-by-lookup would misinterpret.
+    private static bool IsUniqueConstraintViolation(DbUpdateException ex)
+    {
+        var message = ex.InnerException?.Message ?? ex.Message;
+        return message.Contains("UNIQUE KEY constraint", StringComparison.OrdinalIgnoreCase)
+            || message.Contains("duplicate key row", StringComparison.OrdinalIgnoreCase);
+    }
 }

src/Dfe.SignIn.Fn.AuthExtensions/OnTokenIssuanceStart/TokenIssuanceStartHandler.cs

@@ -1,4 +1,5 @@
 using Dfe.SignIn.Base.Framework;
+using Dfe.SignIn.Core.Contracts.Audit;
 using Dfe.SignIn.Core.Contracts.Users;
 using Dfe.SignIn.Core.Public;
 using Microsoft.AspNetCore.Http;
@@ -26,14 +27,54 @@ public async Task<IActionResult> Run(
 
         @event.Validate();
 
-        var checkLinkedResponse = await interaction.DispatchAsync(
-            new AutoLinkEntraUserToDsiRequest {
-                EntraUserId = @event.Data.AuthenticationContext.User.Id,
-                EmailAddress = @event.Data.AuthenticationContext.User.Mail.Trim(),
-                FirstName = @event.Data.AuthenticationContext.User.GivenName.Trim(),
-                LastName = @event.Data.AuthenticationContext.User.Surname.Trim(),
+        AutoLinkEntraUserToDsiResponse checkLinkedResponse;
+        try {
+            checkLinkedResponse = await interaction.DispatchAsync(
+                new AutoLinkEntraUserToDsiRequest {
+                    EntraUserId = @event.Data.AuthenticationContext.User.Id,
+                    EmailAddress = @event.Data.AuthenticationContext.User.Mail.Trim(),
+                    FirstName = @event.Data.AuthenticationContext.User.GivenName.Trim(),
+                    LastName = @event.Data.AuthenticationContext.User.Surname.Trim(),
+                }
+            ).To<AutoLinkEntraUserToDsiResponse>();
+        }
+        catch (Exception ex) when (ex is CannotLinkInactiveUserException
+            or CannotCreateNewUserException
+            or UserAlreadyLinkedToEntraAccountException
+            or EntraAccountAlreadyLinkedToDifferentUserException) {
+            // CannotLinkInactiveUserException is a permanent failure requiring account
+            // reactivation: retrying (e.g. on the user's next sign-in attempt) will hit the
+            // same conflict every time. CannotCreateNewUserException is not reliably
+            // permanent — it may occur because of the registration race window in
+            // CreateUserUseCase's pre-check, in which case the user's next sign-in attempt
+            // will find the now-committed row by email via
+            // AutoLinkEntraUserToDsiUseCase.LinkToExistingDsiUserAsync and self-heal. Other
+            // linking-conflict exceptions indicate a genuine conflict that will not resolve
+            // without support intervention. Audit either way so support has visibility, then
+            // rethrow — this extension point has no way to return a custom error response to
+            // Entra, only claims for a successful token.
+            string outcomeGuidance = ex switch {
+                CannotLinkInactiveUserException => "permanent failure requiring account reactivation",
+                CannotCreateNewUserException => "may self-heal on the user's next sign-in if this was a registration race, but investigate if recurring for the same account",
+                _ => "permanent failure requiring support investigation",
+            };
+
+            try {
+                await interaction.DispatchAsync(new WriteToAuditRequest {
+                    EventCategory = AuditEventCategoryNames.Auth,
+                    EventName = AuditAuthEventNames.LinkFailed,
+                    Message = $"Failed to link Entra account {@event.Data.AuthenticationContext.User.Id} ({@event.Data.AuthenticationContext.User.Mail.Trim()}) to a DfE Sign-In user: {ex.GetType().Name} ({outcomeGuidance}).",
+                    WasFailure = true,
+                });
+            }
+            catch (Exception auditEx) {
+                // Never let an audit-infrastructure failure mask the original exception
+                // being handled here — log it and continue to the rethrow below.
+                logger.LogError(auditEx, "Failed to write link-failure audit entry.");
             }
-        ).To<AutoLinkEntraUserToDsiResponse>();
+
+            throw;
+        }
 
         return ResponseAction(new ProvideClaimsForTokenAction {
             Claims = new() {

src/Dfe.SignIn.Gateways.EntityFramework/Configuration/Directories/UserEntityConfiguration.cs

@@ -19,6 +19,9 @@ public void Configure(EntityTypeBuilder<UserEntity> builder)
                 .IsUnique()
                 .HasFilter("([entra_oid] IS NOT NULL)");
 
+        builder.HasIndex(e => e.Email, "IDX__user__email__unique")
+                .IsUnique();
+
         builder.Property(e => e.Sub)
                 .ValueGeneratedNever()
                 .HasColumnName("sub");

tests/Dfe.SignIn.Core.UseCases.UnitTests/Users/CreateUserUseCaseTests.cs

@@ -1,8 +1,11 @@
 using Dfe.SignIn.Core.Contracts.Search;
 using Dfe.SignIn.Core.Contracts.Users;
 using Dfe.SignIn.Core.Entities.Directories;
+using Dfe.SignIn.Core.Interfaces.DataAccess;
 using Dfe.SignIn.Core.UseCases.Users;
 using Dfe.SignIn.Gateways.EntityFramework;
+using Microsoft.EntityFrameworkCore;
+using Moq;
 using Moq.AutoMock;
 
 namespace Dfe.SignIn.Core.UseCases.UnitTests.Users;
@@ -144,4 +147,190 @@ public async Task UpdatesUserInSearchIndex()
         Assert.IsNotNull(capturedRequest);
         Assert.AreEqual(user.UserId, capturedRequest.UserId);
     }
+
+    [TestMethod]
+    public async Task RecoversWhenConcurrentRequestWinsTheRace()
+    {
+        var autoMocker = new AutoMocker();
+
+        var options = new DbContextOptionsBuilder<DbDirectoriesContext>()
+            .UseInMemoryDatabase(Guid.NewGuid().ToString())
+            .Options;
+        using var ctx = new DbDirectoriesContext(options);
+
+        var raceWinnerId = Guid.Parse("8f6a9b1e-9e3a-4b8e-9f1a-9b2c3d4e5f6a");
+        var raceEntraOid = Guid.Parse("2222e22e-2222-4222-8222-222222222222");
+
+        var mockUnitOfWork = autoMocker.GetMock<IUnitOfWorkDirectories>();
+        mockUnitOfWork.Setup(u => u.Repository<UserEntity>()).Returns(ctx.Users);
+        mockUnitOfWork
+            .Setup(u => u.AddAsync(It.IsAny<UserEntity>(), It.IsAny<CancellationToken>()))
+            .Returns(Task.CompletedTask);
+        mockUnitOfWork
+            .Setup(u => u.SaveChangesAsync(It.IsAny<CancellationToken>()))
+            .Callback(() => {
+                // Same email AND same entra_oid as the losing request below - this is
+                // the genuine race: the same Entra identity signing in twice
+                // concurrently, and the other request's insert committed first.
+                ctx.Users.Add(new UserEntity {
+                    Sub = raceWinnerId,
+                    Email = "race@example.com",
+                    FirstName = "Race",
+                    LastName = "Winner",
+                    Password = "",
+                    Salt = "",
+                    Status = 1,
+                    IsEntra = true,
+                    EntraOid = raceEntraOid,
+                });
+                ctx.SaveChanges();
+            })
+            .ThrowsAsync(new DbUpdateException("Violation of UNIQUE KEY constraint 'IDX__user__email__unique'."));
+
+        var interactor = autoMocker.CreateInstance<CreateUserUseCase>();
+
+        var response = await interactor.InvokeAsync(
+            new CreateUserRequest {
+                EmailAddress = "race@example.com",
+                FirstName = "joe",
+                LastName = "brown",
+                EntraUserId = raceEntraOid
+            });
+
+        Assert.AreEqual(raceWinnerId, response.UserId);
+    }
+
+    [TestMethod]
+    public async Task RethrowsWhen_EmailAlreadyUsedByADifferentEntraIdentity()
+    {
+        // Not a self-race: a row already exists with the same email but a DIFFERENT
+        // entra_oid. Recovering here would silently return someone else's account -
+        // this must surface as the original exception instead.
+        var autoMocker = new AutoMocker();
+
+        var options = new DbContextOptionsBuilder<DbDirectoriesContext>()
+            .UseInMemoryDatabase(Guid.NewGuid().ToString())
+            .Options;
+        using var ctx = new DbDirectoriesContext(options);
+
+        var conflictingUserId = Guid.Parse("8f6a9b1e-9e3a-4b8e-9f1a-9b2c3d4e5f6a");
+
+        var mockUnitOfWork = autoMocker.GetMock<IUnitOfWorkDirectories>();
+        mockUnitOfWork.Setup(u => u.Repository<UserEntity>()).Returns(ctx.Users);
+        mockUnitOfWork
+            .Setup(u => u.AddAsync(It.IsAny<UserEntity>(), It.IsAny<CancellationToken>()))
+            .Returns(Task.CompletedTask);
+        mockUnitOfWork
+            .Setup(u => u.SaveChangesAsync(It.IsAny<CancellationToken>()))
+            .Callback(() => {
+                ctx.Users.Add(new UserEntity {
+                    Sub = conflictingUserId,
+                    Email = "race@example.com",
+                    FirstName = "Someone",
+                    LastName = "Else",
+                    Password = "",
+                    Salt = "",
+                    Status = 1,
+                    IsEntra = true,
+                    EntraOid = Guid.Parse("1111e11e-1111-4111-8111-111111111111"),
+                });
+                ctx.SaveChanges();
+            })
+            .ThrowsAsync(new DbUpdateException("Violation of UNIQUE KEY constraint 'IDX__user__email__unique'."));
+
+        var interactor = autoMocker.CreateInstance<CreateUserUseCase>();
+
+        await Assert.ThrowsExactlyAsync<DbUpdateException>(()
+            => interactor.InvokeAsync(
+                new CreateUserRequest {
+                    EmailAddress = "race@example.com",
+                    FirstName = "joe",
+                    LastName = "brown",
+                    EntraUserId = Guid.Parse("2222e22e-2222-4222-8222-222222222222")
+                }));
+    }
+
+    [TestMethod]
+    public async Task RethrowsWhen_SaveFailsButNoMatchingUserIsFound()
+    {
+        var autoMocker = new AutoMocker();
+
+        var options = new DbContextOptionsBuilder<DbDirectoriesContext>()
+            .UseInMemoryDatabase(Guid.NewGuid().ToString())
+            .Options;
+        using var ctx = new DbDirectoriesContext(options);
+
+        var mockUnitOfWork = autoMocker.GetMock<IUnitOfWorkDirectories>();
+        mockUnitOfWork.Setup(u => u.Repository<UserEntity>()).Returns(ctx.Users);
+        mockUnitOfWork
+            .Setup(u => u.AddAsync(It.IsAny<UserEntity>(), It.IsAny<CancellationToken>()))
+            .Returns(Task.CompletedTask);
+        mockUnitOfWork
+            .Setup(u => u.SaveChangesAsync(It.IsAny<CancellationToken>()))
+            .ThrowsAsync(new DbUpdateException("Some unrelated database error."));
+
+        var interactor = autoMocker.CreateInstance<CreateUserUseCase>();
+
+        await Assert.ThrowsExactlyAsync<DbUpdateException>(()
+            => interactor.InvokeAsync(
+                new CreateUserRequest {
+                    EmailAddress = "joe.brown@example.com",
+                    FirstName = "joe",
+                    LastName = "brown",
+                    EntraUserId = Guid.Parse("fa70e11c-f1eb-4bab-9fa0-ff36a9620066")
+                }));
+    }
+
+    [TestMethod]
+    public async Task DoesNotAttemptRecovery_WhenSaveFailureIsNotAUniqueConstraintViolation()
+    {
+        // A DbUpdateException from an unrelated cause (FK violation, timeout, etc.)
+        // must not be reinterpreted as a race. A row matching this request by both
+        // email and entra_oid is inserted at the moment SaveChangesAsync is called
+        // (simulating a genuine concurrent race, the same way
+        // RecoversWhenConcurrentRequestWinsTheRace does) - if the `when` filter were
+        // missing or too broad, this would incorrectly recover instead of rethrowing.
+        var autoMocker = new AutoMocker();
+
+        var options = new DbContextOptionsBuilder<DbDirectoriesContext>()
+            .UseInMemoryDatabase(Guid.NewGuid().ToString())
+            .Options;
+        using var ctx = new DbDirectoriesContext(options);
+
+        var requestEntraOid = Guid.Parse("fa70e11c-f1eb-4bab-9fa0-ff36a9620066");
+
+        var mockUnitOfWork = autoMocker.GetMock<IUnitOfWorkDirectories>();
+        mockUnitOfWork.Setup(u => u.Repository<UserEntity>()).Returns(ctx.Users);
+        mockUnitOfWork
+            .Setup(u => u.AddAsync(It.IsAny<UserEntity>(), It.IsAny<CancellationToken>()))
+            .Returns(Task.CompletedTask);
+        mockUnitOfWork
+            .Setup(u => u.SaveChangesAsync(It.IsAny<CancellationToken>()))
+            .Callback(() => {
+                ctx.Users.Add(new UserEntity {
+                    Sub = Guid.Parse("8f6a9b1e-9e3a-4b8e-9f1a-9b2c3d4e5f6a"),
+                    Email = "joe.brown@example.com",
+                    FirstName = "joe",
+                    LastName = "brown",
+                    Password = "",
+                    Salt = "",
+                    Status = 1,
+                    IsEntra = true,
+                    EntraOid = requestEntraOid,
+                });
+                ctx.SaveChanges();
+            })
+            .ThrowsAsync(new DbUpdateException("The INSERT statement conflicted with the FOREIGN KEY constraint."));
+
+        var interactor = autoMocker.CreateInstance<CreateUserUseCase>();
+
+        await Assert.ThrowsExactlyAsync<DbUpdateException>(()
+            => interactor.InvokeAsync(
+                new CreateUserRequest {
+                    EmailAddress = "joe.brown@example.com",
+                    FirstName = "joe",
+                    LastName = "brown",
+                    EntraUserId = requestEntraOid
+                }));
+    }
 }