diff --git a/Directory.Packages.props b/Directory.Packages.props index cae4c566..94116e45 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -125,5 +125,7 @@ + + diff --git a/src/Dfe.SignIn.Core.Contracts/Audit/AuditConstants.cs b/src/Dfe.SignIn.Core.Contracts/Audit/AuditConstants.cs index 24b39e96..4189a291 100644 --- a/src/Dfe.SignIn.Core.Contracts/Audit/AuditConstants.cs +++ b/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. /// public const string LinkToNewUser = "link-to-new-user"; + + /// + /// Indicates that linking an Entra account to a DfE Sign-In user permanently failed. + /// + public const string LinkFailed = "link-failed"; } /// diff --git a/src/Dfe.SignIn.Core.UseCases/Users/CreateUserUseCase.cs b/src/Dfe.SignIn.Core.UseCases/Users/CreateUserUseCase.cs index c342828c..d44d8505 100644 --- a/src/Dfe.SignIn.Core.UseCases/Users/CreateUserUseCase.cs +++ b/src/Dfe.SignIn.Core.UseCases/Users/CreateUserUseCase.cs @@ -60,7 +60,34 @@ public override async Task InvokeAsync( }; 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() + .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 @@ await interaction.DispatchAsync( 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); + } } diff --git a/src/Dfe.SignIn.Fn.AuthExtensions/OnTokenIssuanceStart/TokenIssuanceStartHandler.cs b/src/Dfe.SignIn.Fn.AuthExtensions/OnTokenIssuanceStart/TokenIssuanceStartHandler.cs index 6354556c..1303c1f0 100644 --- a/src/Dfe.SignIn.Fn.AuthExtensions/OnTokenIssuanceStart/TokenIssuanceStartHandler.cs +++ b/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 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(); + } + 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(); + + throw; + } return ResponseAction(new ProvideClaimsForTokenAction { Claims = new() { diff --git a/src/Dfe.SignIn.Gateways.EntityFramework/Configuration/Directories/UserEntityConfiguration.cs b/src/Dfe.SignIn.Gateways.EntityFramework/Configuration/Directories/UserEntityConfiguration.cs index a1aab983..05c30a9a 100644 --- a/src/Dfe.SignIn.Gateways.EntityFramework/Configuration/Directories/UserEntityConfiguration.cs +++ b/src/Dfe.SignIn.Gateways.EntityFramework/Configuration/Directories/UserEntityConfiguration.cs @@ -19,6 +19,9 @@ public void Configure(EntityTypeBuilder 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"); diff --git a/tests/Dfe.SignIn.Core.UseCases.UnitTests/Users/CreateUserUseCaseTests.cs b/tests/Dfe.SignIn.Core.UseCases.UnitTests/Users/CreateUserUseCaseTests.cs index cc573635..802816c4 100644 --- a/tests/Dfe.SignIn.Core.UseCases.UnitTests/Users/CreateUserUseCaseTests.cs +++ b/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() + .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(); + mockUnitOfWork.Setup(u => u.Repository()).Returns(ctx.Users); + mockUnitOfWork + .Setup(u => u.AddAsync(It.IsAny(), It.IsAny())) + .Returns(Task.CompletedTask); + mockUnitOfWork + .Setup(u => u.SaveChangesAsync(It.IsAny())) + .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(); + + 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() + .UseInMemoryDatabase(Guid.NewGuid().ToString()) + .Options; + using var ctx = new DbDirectoriesContext(options); + + var conflictingUserId = Guid.Parse("8f6a9b1e-9e3a-4b8e-9f1a-9b2c3d4e5f6a"); + + var mockUnitOfWork = autoMocker.GetMock(); + mockUnitOfWork.Setup(u => u.Repository()).Returns(ctx.Users); + mockUnitOfWork + .Setup(u => u.AddAsync(It.IsAny(), It.IsAny())) + .Returns(Task.CompletedTask); + mockUnitOfWork + .Setup(u => u.SaveChangesAsync(It.IsAny())) + .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(); + + await Assert.ThrowsExactlyAsync(() + => 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() + .UseInMemoryDatabase(Guid.NewGuid().ToString()) + .Options; + using var ctx = new DbDirectoriesContext(options); + + var mockUnitOfWork = autoMocker.GetMock(); + mockUnitOfWork.Setup(u => u.Repository()).Returns(ctx.Users); + mockUnitOfWork + .Setup(u => u.AddAsync(It.IsAny(), It.IsAny())) + .Returns(Task.CompletedTask); + mockUnitOfWork + .Setup(u => u.SaveChangesAsync(It.IsAny())) + .ThrowsAsync(new DbUpdateException("Some unrelated database error.")); + + var interactor = autoMocker.CreateInstance(); + + await Assert.ThrowsExactlyAsync(() + => 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() + .UseInMemoryDatabase(Guid.NewGuid().ToString()) + .Options; + using var ctx = new DbDirectoriesContext(options); + + var requestEntraOid = Guid.Parse("fa70e11c-f1eb-4bab-9fa0-ff36a9620066"); + + var mockUnitOfWork = autoMocker.GetMock(); + mockUnitOfWork.Setup(u => u.Repository()).Returns(ctx.Users); + mockUnitOfWork + .Setup(u => u.AddAsync(It.IsAny(), It.IsAny())) + .Returns(Task.CompletedTask); + mockUnitOfWork + .Setup(u => u.SaveChangesAsync(It.IsAny())) + .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(); + + await Assert.ThrowsExactlyAsync(() + => interactor.InvokeAsync( + new CreateUserRequest { + EmailAddress = "joe.brown@example.com", + FirstName = "joe", + LastName = "brown", + EntraUserId = requestEntraOid + })); + } } diff --git a/tests/Dfe.SignIn.Fn.AuthExtensions.UnitTests/TokenIssuanceStartHandlerTests.cs b/tests/Dfe.SignIn.Fn.AuthExtensions.UnitTests/TokenIssuanceStartHandlerTests.cs index 4255a50d..00846475 100644 --- a/tests/Dfe.SignIn.Fn.AuthExtensions.UnitTests/TokenIssuanceStartHandlerTests.cs +++ b/tests/Dfe.SignIn.Fn.AuthExtensions.UnitTests/TokenIssuanceStartHandlerTests.cs @@ -1,4 +1,5 @@ using System.ComponentModel.DataAnnotations; +using Dfe.SignIn.Core.Contracts.Audit; using Dfe.SignIn.Core.Contracts.Users; using Dfe.SignIn.Core.Public; using Dfe.SignIn.Fn.AuthExtensions.OnTokenIssuanceStart; @@ -205,4 +206,98 @@ public async Task AutomaticallyLinkEntraUserToDsi_TrimsUserAttributesBeforeDispa var provideClaimsAction = TypeAssert.IsType(data.Actions[0]); Assert.AreEqual("d5ba1f44-1400-4c98-b834-5d5ba5b98995", provideClaimsAction.Claims[DsiClaimTypes.UserId]); } + + [TestMethod] + public async Task WritesAuditAndRethrows_WhenUserCannotBeLinkedBecauseInactive() + { + var autoMocker = new AutoMocker(); + + autoMocker.MockThrows(new CannotLinkInactiveUserException()); + + var capturedWriteToAudit = new List(); + autoMocker.CaptureRequest(capturedWriteToAudit.Add); + + var handler = autoMocker.CreateInstance(); + + var fakeRequest = HttpServerMocking.CreateJsonRequest(FakeEvent); + + await Assert.ThrowsAsync(() + => handler.Run(fakeRequest)); + + Assert.HasCount(1, capturedWriteToAudit); + Assert.AreEqual(AuditEventCategoryNames.Auth, capturedWriteToAudit[0].EventCategory); + Assert.AreEqual(AuditAuthEventNames.LinkFailed, capturedWriteToAudit[0].EventName); + Assert.IsTrue(capturedWriteToAudit[0].WasFailure); + StringAssert.Contains(capturedWriteToAudit[0].Message, "permanent failure requiring account reactivation"); + } + + [TestMethod] + public async Task WritesAuditAndRethrows_WhenUserCannotBeCreated() + { + var autoMocker = new AutoMocker(); + + autoMocker.MockThrows( + CannotCreateNewUserException.FromEmailAddress("jo.bradford@example.com")); + + var capturedWriteToAudit = new List(); + autoMocker.CaptureRequest(capturedWriteToAudit.Add); + + var handler = autoMocker.CreateInstance(); + + var fakeRequest = HttpServerMocking.CreateJsonRequest(FakeEvent); + + await Assert.ThrowsAsync(() + => handler.Run(fakeRequest)); + + Assert.HasCount(1, capturedWriteToAudit); + Assert.AreEqual(AuditAuthEventNames.LinkFailed, capturedWriteToAudit[0].EventName); + StringAssert.Contains(capturedWriteToAudit[0].Message, "may self-heal on the user's next sign-in"); + } + + [TestMethod] + public async Task RethrowsOriginalException_WhenAuditDispatchItselfThrows() + { + var autoMocker = new AutoMocker(); + + autoMocker.MockThrows(new CannotLinkInactiveUserException()); + autoMocker.MockThrows(new InvalidOperationException("Audit service unavailable.")); + + var handler = autoMocker.CreateInstance(); + + var fakeRequest = HttpServerMocking.CreateJsonRequest(FakeEvent); + + // The original, diagnosable exception must still propagate even though writing + // the audit entry itself failed — an audit-infrastructure error must never mask it. + await Assert.ThrowsAsync(() + => handler.Run(fakeRequest)); + } + + [TestMethod] + public async Task DoesNotWriteAudit_WhenLinkingSucceeds() + { + var autoMocker = new AutoMocker(); + + autoMocker.MockResponse( + new AutoLinkEntraUserToDsiRequest { + EntraUserId = new Guid("21892c65-88df-4268-b025-d06f51c52404"), + EmailAddress = "jo.bradford@example.com", + FirstName = "Jo", + LastName = "Bradford", + }, + new AutoLinkEntraUserToDsiResponse { + UserId = new Guid("d5ba1f44-1400-4c98-b834-5d5ba5b98995"), + } + ); + + var capturedWriteToAudit = new List(); + autoMocker.CaptureRequest(capturedWriteToAudit.Add); + + var handler = autoMocker.CreateInstance(); + + var fakeRequest = HttpServerMocking.CreateJsonRequest(FakeEvent); + + await handler.Run(fakeRequest); + + Assert.IsEmpty(capturedWriteToAudit); + } } diff --git a/tests/Dfe.SignIn.Gateways.EntityFramework.UnitTests/Dfe.SignIn.Gateways.EntityFramework.UnitTests.csproj b/tests/Dfe.SignIn.Gateways.EntityFramework.UnitTests/Dfe.SignIn.Gateways.EntityFramework.UnitTests.csproj index 245c4a53..2577b69c 100644 --- a/tests/Dfe.SignIn.Gateways.EntityFramework.UnitTests/Dfe.SignIn.Gateways.EntityFramework.UnitTests.csproj +++ b/tests/Dfe.SignIn.Gateways.EntityFramework.UnitTests/Dfe.SignIn.Gateways.EntityFramework.UnitTests.csproj @@ -17,6 +17,7 @@ + diff --git a/tests/Dfe.SignIn.Gateways.EntityFramework.UnitTests/UserEntityConfigurationTests.cs b/tests/Dfe.SignIn.Gateways.EntityFramework.UnitTests/UserEntityConfigurationTests.cs new file mode 100644 index 00000000..e1b45a16 --- /dev/null +++ b/tests/Dfe.SignIn.Gateways.EntityFramework.UnitTests/UserEntityConfigurationTests.cs @@ -0,0 +1,62 @@ +using Dfe.SignIn.Core.Entities.Directories; +using Microsoft.Data.Sqlite; +using Microsoft.EntityFrameworkCore; + +namespace Dfe.SignIn.Gateways.EntityFramework.UnitTests; + +[TestClass] +public sealed class UserEntityConfigurationTests +{ + // Note: this test uses the SQLite in-memory provider rather than the EF Core + // InMemory provider. The InMemory provider does not enforce unique indexes + // (confirmed: Microsoft.EntityFrameworkCore.InMemory.Storage.Internal.InMemoryTable + // only raises a concurrency-token check, never a uniqueness check), so a duplicate + // insert would silently succeed there and this test would never be able to verify + // the constraint added in UserEntityConfiguration. SQLite enforces UNIQUE constraints + // for real, which is what's needed to prove the index behaves as intended. + // + // Scope limitation: this only proves exact-match uniqueness is enforced. SQLite's + // in-memory TEXT unique index uses BINARY (case-sensitive) collation by default, not + // SQL Server's SQL_Latin1_General_CP1_CI_AS (case-insensitive, accent-sensitive) used + // by the live [user].email column. So this test cannot (and, without configuring a + // SQLite collation in a way that wouldn't be a like-for-like check anyway, structurally + // shouldn't try to) independently verify case-insensitive duplicate detection e.g. + // "a@b.com" vs "A@B.com". That guarantee rests on the confirmed production column + // collation (verified separately), not on this test. + [TestMethod] + public async Task Email_MustBeUnique() + { + using var connection = new SqliteConnection("DataSource=:memory:"); + connection.Open(); + + var options = new DbContextOptionsBuilder() + .UseSqlite(connection) + .Options; + + using var ctx = new DbDirectoriesContext(options); + await ctx.Database.EnsureCreatedAsync(); + + ctx.Users.Add(new UserEntity { + Sub = Guid.NewGuid(), + Email = "duplicate@example.com", + FirstName = "Alex", + LastName = "Johnson", + Password = "", + Salt = "", + Status = 1, + }); + await ctx.SaveChangesAsync(); + + ctx.Users.Add(new UserEntity { + Sub = Guid.NewGuid(), + Email = "duplicate@example.com", + FirstName = "Bob", + LastName = "Simons", + Password = "", + Salt = "", + Status = 1, + }); + + await Assert.ThrowsExactlyAsync(() => ctx.SaveChangesAsync()); + } +}