Skip to content

NSA-9931: Fix Entra registration race condition - #238

Open
AhmadiRamin wants to merge 10 commits into
mainfrom
feature/NSA-9931-fix-registration-race
Open

AhmadiRamin wants to merge 10 commits into
mainfrom
feature/NSA-9931-fix-registration-race

Conversation

@AhmadiRamin

@AhmadiRamin AhmadiRamin commented Aug 4, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes NSA-9931 (Unlinked Entra accounts): two concurrent Entra registration attempts for the same email could both pass CreateUserUseCase's duplicate check before either committed, creating orphaned duplicate [user] rows. Confirmed live in production (a real duplicate pair found during investigation).

  • Adds a unique index on [user].email (IDX__user__email__unique)
  • Makes CreateUserUseCase recover from the resulting DbUpdateException by resolving to the concurrent winner's user ID instead of failing.
  • Adds audit logging in TokenIssuanceStartHandler for permanent (non-self-healing) linking failures, with wording that distinguishes genuinely permanent failures from ones that may self-heal on retry.

Test plan

  • dotnet build dsi-platform.sln — clean, including the migrations project
  • Full dotnet test suite passes, including new tests for the race-recovery and audit-failure-isolation paths
  • DBA review of the migration baselining plan
  • Manual remediation of the 3 known duplicate groups before this migration is applied to any environment

Documents the baselining hazard for the first-ever Directories migration
in-repo (migration file comment + new migrations/README.md), fixes the
audit message on Entra link failure to not overstate CannotCreateNewUserException
as permanent (it can self-heal via AutoLinkEntraUserToDsiUseCase on retry),
guards the audit dispatch so it can never mask the original exception it's
meant to record, and wires migrations.csproj into dsi-platform.sln (with a
fresh project GUID to resolve a collision with Dfe.SignIn.Core.Public) so CI
actually builds it.
Copilot AI review requested due to automatic review settings August 4, 2026 09:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a production race in Entra-driven registration/linking by enforcing email uniqueness at the database level and making the registration flow resilient to concurrent creates, while adding audit visibility for link failures.

Changes:

  • Added a unique index on user.email via EF Core model configuration and a new Directories migration.
  • Updated CreateUserUseCase to recover from unique-constraint DbUpdateException by resolving the “race winner” user ID.
  • Added audit logging in TokenIssuanceStartHandler for non-success linking outcomes, plus unit tests covering the new paths.

Reviewed changes

Copilot reviewed 15 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/Dfe.SignIn.Gateways.EntityFramework.UnitTests/UserEntityConfigurationTests.cs Adds SQLite-backed test to prove the EF unique email index is enforced.
tests/Dfe.SignIn.Gateways.EntityFramework.UnitTests/Dfe.SignIn.Gateways.EntityFramework.UnitTests.csproj Adds EF Core SQLite provider dependency for relational constraint testing.
tests/Dfe.SignIn.Fn.AuthExtensions.UnitTests/TokenIssuanceStartHandlerTests.cs Adds tests verifying audit write/rethrow behavior and audit isolation on failure.
tests/Dfe.SignIn.Core.UseCases.UnitTests/Users/CreateUserUseCaseTests.cs Adds tests for “race winner” recovery and rethrow behavior when recovery isn’t possible.
src/Dfe.SignIn.Gateways.EntityFramework/Configuration/Directories/UserEntityConfiguration.cs Adds unique index on UserEntity.Email.
src/Dfe.SignIn.Fn.AuthExtensions/OnTokenIssuanceStart/TokenIssuanceStartHandler.cs Adds audit logging for specific linking failures and ensures audit errors don’t mask the original exception.
src/Dfe.SignIn.Core.UseCases/Users/CreateUserUseCase.cs Implements post-DbUpdateException recovery by querying for the now-existing user row.
src/Dfe.SignIn.Core.Contracts/Audit/AuditConstants.cs Introduces AuditAuthEventNames.LinkFailed.
migrations/README.md Documents baselining requirements for first migrations on pre-existing databases.
migrations/migrations.csproj Updates migrations project metadata (including ProjectGuid).
migrations/Directories/DbDirectoriesContextModelSnapshot.cs Adds email unique index to the Directories model snapshot.
migrations/Directories/20260803140316_AddUniqueIndexToUserEmail.Designer.cs Adds designer output for the unique email index migration.
migrations/Directories/20260803140316_AddUniqueIndexToUserEmail.cs Adds migration creating the unique email index.
migrations/Directories/20260803140253_InitialCreate.Designer.cs Adds baseline “InitialCreate” migration designer for Directories context.
migrations/Directories/20260803140253_InitialCreate.cs Adds baseline “InitialCreate” migration with safety warnings.
dsi-platform.sln Adds the migrations project to the solution.
Directory.Packages.props Pins Microsoft.EntityFrameworkCore.Sqlite for test usage.
Files not reviewed (2)
  • migrations/Directories/20260803140253_InitialCreate.Designer.cs: Generated file
  • migrations/Directories/20260803140316_AddUniqueIndexToUserEmail.Designer.cs: Generated file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Dfe.SignIn.Core.UseCases/Users/CreateUserUseCase.cs
AhmadiRamin and others added 3 commits August 10, 2026 09:26
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…nceStartHandler

The prior commit (Copilot Autofix suggestion applied via GitHub UI)
introduced a second outcomeGuidance declaration and a repeated comment
block, causing CS0128 (variable already defined) and breaking the build.
Kept the more complete switch expression, which handles the two new
exception types with a default case; merged the trailing audit
explanation into the single remaining comment.
DbDirectoriesContext had never had a migration generated before, so
adding one required baselining (InitialCreate capturing the current
schema) before the real change (AddUniqueIndexToUserEmail) could be
layered on top - a first-migration hazard needing DBA sign-off just to
land a single index.

Since Fn.AuthExtensions never actually wires up the local EF-backed
CreateUserUseCase for the live token-issuance path (it dispatches to
Node instead - see login.dfe.directories PR #383), the only thing this
migration needs to change for real is the physical index in the
Directories database. That's now a plain SQL script instead:
login.dfe.db-scripts/schema/dirs-db/NSA-9931_create_unique_index_user_email.sql

UserEntityConfiguration.cs's HasIndex(...).IsUnique() and its SQLite
unit test are kept as-is - they document the constraint for dsi-platform's
own EF model, for whenever CreateUserUseCase's path goes live.
@AhmadiRamin

Copy link
Copy Markdown
Contributor Author

Update: removed the EF Core migrations (InitialCreate baseline + AddUniqueIndexToUserEmail) from this PR.

They required baselining __EFMigrationsHistory for DbDirectoriesContext's first-ever migration just to land one index - a real deployment hazard needing DBA sign-off. Since Fn.AuthExtensions never actually wires up the local EF-backed CreateUserUseCase for the live token-issuance path (that dispatches to Node - see login.dfe.directories#383), the only thing that needs to change for real is the physical index in the Directories database. That's now a plain SQL script in login.dfe.db-scripts instead: schema/dirs-db/NSA-9931_create_unique_index_user_email.sql.

UserEntityConfiguration.cs's HasIndex(...).IsUnique() and its SQLite unit test are unchanged - kept as documentation of the constraint for dsi-platform's own EF model, for whenever CreateUserUseCase's path goes live.

Also fixed a build break: a Copilot Autofix suggestion applied directly to this branch (commit 0c90c77) left a duplicated outcomeGuidance declaration in TokenIssuanceStartHandler.cs (CS0128). Merged it into the single, more complete version.

…ace recovery

Two issues Copilot flagged in the DbUpdateException catch block:

1. It caught any DbUpdateException (FK violations, timeouts, etc.) and
   treated it as a race, not just unique-constraint violations. Now
   gated on IsUniqueConstraintViolation, matched against the well-known
   SQL Server unique-violation message text (can't inspect SqlException
   by type/Number without a hard Microsoft.Data.SqlClient dependency in
   this layer).

2. The winner lookup queried Email == X || EntraOid == Y with
   SingleOrDefaultAsync, which throws InvalidOperationException (masking
   the original exception) if a different row matches each condition -
   e.g. the email is already used by a different Entra identity. Changed
   to Email == X && EntraOid == Y: the only unambiguous race winner is a
   row matching both, since a genuine race is the same Entra identity's
   own concurrent request. A partial match is a real conflict and must
   surface as the original exception rather than silently resolving to
   someone else's account.

Updated RecoversWhenConcurrentRequestWinsTheRace to model a true race
(same email and entra_oid, not just email); added tests for the two
new safety cases.
@AhmadiRamin

Copy link
Copy Markdown
Contributor Author

Addressed the other Copilot finding too (CreateUserUseCase.cs race-recovery): the catch is now gated to actual unique-constraint violations only (not any DbUpdateException), and the winner lookup requires matching both email and entra_oid — a partial match (email reused by a different Entra identity) now surfaces the original exception instead of risking an ambiguous SingleOrDefaultAsync throw or a silent cross-identity resolution. Two new tests cover both cases; existing race test corrected to model a genuine same-identity race.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (2)

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

  • In the unique-constraint recovery path, newUser is still tracked as Added. If this DbContext is reused later in the same scope, a subsequent SaveChangesAsync will try to insert it again and can re-trigger the unique constraint violation. Detach/remove the pending entity before returning the race-winner.
            return new CreateUserResponse {
                UserId = raceWinner.Sub
            };
        }

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

  • AuditAuthEventNames.LinkFailed is used for both permanent linking conflicts and potentially self-healing failures (e.g., a registration race). The XML summary currently states it is a permanent failure, which is misleading for consumers of the audit taxonomy.
    /// <summary>
    /// Indicates that linking an Entra account to a DfE Sign-In user permanently failed.
    /// </summary>
    public const string LinkFailed = "link-failed";

@AhmadiRamin
AhmadiRamin marked this pull request as draft August 11, 2026 08:47
@sonarqubecloud

Copy link
Copy Markdown

@AhmadiRamin
AhmadiRamin marked this pull request as ready for review August 20, 2026 07:59
Comment thread src/Dfe.SignIn.Core.UseCases/Users/CreateUserUseCase.cs
Comment thread src/Dfe.SignIn.Core.UseCases/Users/CreateUserUseCase.cs

@stephan-williamson stephan-williamson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple of comments but its possible they might be disregarded

Comment thread src/Dfe.SignIn.Core.UseCases/Users/CreateUserUseCase.cs
Comment thread src/Dfe.SignIn.Core.UseCases/Users/CreateUserUseCase.cs
@AhmadiRamin
AhmadiRamin enabled auto-merge August 20, 2026 14:13
@AhmadiRamin
AhmadiRamin added this pull request to the merge queue Aug 20, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to Branch Protection failures Aug 20, 2026
You're not authorized to push to this branch. Visit "About protected branches" for more information.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants