diff --git a/THREAT-MODEL.md b/THREAT-MODEL.md index 30dac08..773c0a7 100644 --- a/THREAT-MODEL.md +++ b/THREAT-MODEL.md @@ -148,7 +148,9 @@ Stated honestly so you can make an informed risk decision. `UpdateAsync` **cannot** be guarded there: ASP.NET Identity drives them through anonymous flows (registration, password reset, email confirmation, lockout), so their authorization remains the reset/confirmation **token** and the UI-layer guard. - A stale full-state `UpdateAsync` overwrite is tracked separately in **#70**. + `UpdateAsync` is now protected against silently reverting a concurrent change by an + optimistic-concurrency (content-version) check that rejects a stale write while still + merging the auto-managed lockout counters forward (#70). - **First-run setup is an unauthenticated bootstrap** by design — see the host obligation above. The actual `/Setup` page is host-provided and outside this library's control. diff --git a/src/AndreGoepel.Marten.Identity.Abstractions/Users/Events/UserUpdated.cs b/src/AndreGoepel.Marten.Identity.Abstractions/Users/Events/UserUpdated.cs index f650d91..76c7f76 100644 --- a/src/AndreGoepel.Marten.Identity.Abstractions/Users/Events/UserUpdated.cs +++ b/src/AndreGoepel.Marten.Identity.Abstractions/Users/Events/UserUpdated.cs @@ -22,4 +22,12 @@ public record UserUpdated(UserId UserId) /// cookies stop revalidating, signing the user out everywhere. /// public string? SecurityStamp { get; init; } + + /// + /// True when this update only carries auto-managed lockout state (failed-count / + /// lockout window) and no user-visible content change. The projection skips bumping + /// for these, so lockout increments do not + /// trigger optimistic-concurrency conflicts on the generic update path (#70). + /// + public bool LockoutOnly { get; init; } } diff --git a/src/AndreGoepel.Marten.Identity/Users/User.cs b/src/AndreGoepel.Marten.Identity/Users/User.cs index 317398a..895130d 100644 --- a/src/AndreGoepel.Marten.Identity/Users/User.cs +++ b/src/AndreGoepel.Marten.Identity/Users/User.cs @@ -20,6 +20,14 @@ public Guid StreamId public bool RootUser { get; set; } public bool Deletable { get; set; } = true; public bool Deleted { get; set; } + + /// + /// Optimistic-concurrency token for the user's non-lockout content (#70). Bumped by + /// the projection on every content-changing event (create excluded) but not by + /// the auto-managed lockout increments, so a stale generic update is detected while + /// concurrent failed-login counting never triggers a spurious conflict. + /// + public int ContentVersion { get; set; } public string? AuthenticatorKey { get; set; } public string? RecoveryCodes { get; set; } diff --git a/src/AndreGoepel.Marten.Identity/Users/UserProjection.cs b/src/AndreGoepel.Marten.Identity/Users/UserProjection.cs index 2eef510..7c50646 100644 --- a/src/AndreGoepel.Marten.Identity/Users/UserProjection.cs +++ b/src/AndreGoepel.Marten.Identity/Users/UserProjection.cs @@ -75,6 +75,9 @@ public void Apply(UserRestored @event, User user) if (@event.SecurityStamp is not null) user.SecurityStamp = @event.SecurityStamp; + + // Restoring changes content — advance the optimistic-concurrency token (#70). + user.ContentVersion++; } [SuppressMessage( @@ -126,6 +129,13 @@ public void Apply(UserUpdated @event, User user) user.ChangedBy = @event.UpdatedBy; user.ChangedAt = @event.UpdatedAt; + + // Advance the optimistic-concurrency token for genuine content changes only; + // lockout-only updates (failed-count / lockout window) must not bump it, or + // concurrent failed-login counting would spuriously conflict with a profile + // update (#70). + if (!@event.LockoutOnly) + user.ContentVersion++; } [SuppressMessage( diff --git a/src/AndreGoepel.Marten.Identity/Users/UserStore.cs b/src/AndreGoepel.Marten.Identity/Users/UserStore.cs index 8165582..2cc1c29 100644 --- a/src/AndreGoepel.Marten.Identity/Users/UserStore.cs +++ b/src/AndreGoepel.Marten.Identity/Users/UserStore.cs @@ -43,6 +43,15 @@ private static IdentityResult NotAuthorized(string description) => new IdentityError { Code = "NotAuthorized", Description = description } ); + private static IdentityResult ConcurrencyFailure() => + IdentityResult.Failed( + new IdentityError + { + Code = "ConcurrencyFailure", + Description = "The user was modified concurrently; reload and try again.", + } + ); + public IQueryable Users => querySession.Query(); public Task GetUserIdAsync(TUser user, CancellationToken cancellationToken) => @@ -220,9 +229,23 @@ public async Task UpdateAsync(TUser user, CancellationToken canc user.Deletable = false; } - // Nothing changed — release the lock without churning the stream. + // Nothing changed — release the lock without churning the stream. Keep the + // caller's content version in step with the projection so a follow-up update + // on this same instance is not spuriously rejected. if (existingUser != null && existingUser.AreEqual(user)) + { + user.ContentVersion = existingUser.ContentVersion; return IdentityResult.Success; + } + + // Optimistic concurrency (#70): the non-lockout content is versioned. If it + // advanced since the caller loaded, this full-state write is based on a stale + // snapshot and would silently revert the concurrent change (e.g. a 2FA toggle + // overwritten by a stale profile save) — reject so the caller reloads and + // retries. Lockout increments do not bump ContentVersion, so concurrent + // failed-login counting never triggers a spurious conflict here. + if (existingUser != null && user.ContentVersion != existingUser.ContentVersion) + return ConcurrencyFailure(); session.Events.Append( userId.Value, @@ -244,6 +267,11 @@ public async Task UpdateAsync(TUser user, CancellationToken canc } ); await session.SaveChangesAsync(cancellationToken); + + // The inline projection just advanced the persisted content version; mirror it + // so repeated updates on the same instance (some ASP.NET Identity flows call + // UpdateAsync more than once) keep matching instead of self-conflicting. + user.ContentVersion = (existingUser?.ContentVersion ?? 0) + 1; return IdentityResult.Success; } catch (Exception ex) @@ -893,6 +921,9 @@ CancellationToken cancellationToken LockoutEnabled = current.LockoutEnabled, LockoutEnd = lockoutEnd, AccessFailedCount = accessFailedCount, + // Lockout counting is auto-managed; don't advance the content-version + // token, or it would spuriously conflict with a concurrent update (#70). + LockoutOnly = true, } ); await session.SaveChangesAsync(cancellationToken); diff --git a/tests/AndreGoepel.Marten.Identity.IntegrationTests/Users/UserStoreTests.cs b/tests/AndreGoepel.Marten.Identity.IntegrationTests/Users/UserStoreTests.cs index 47a2f60..7f58d20 100644 --- a/tests/AndreGoepel.Marten.Identity.IntegrationTests/Users/UserStoreTests.cs +++ b/tests/AndreGoepel.Marten.Identity.IntegrationTests/Users/UserStoreTests.cs @@ -384,6 +384,113 @@ public async Task UpdateAsync_ConcurrentWithLockoutIncrements_LosesNoIncrements( #endregion + #region Optimistic concurrency (#70) + + [Fact] + public async Task UpdateAsync_StaleSnapshot_IsRejectedWithConcurrencyFailure() + { + // A stale full-state update must not silently revert a change committed after the + // caller loaded — it is rejected so the caller reloads and retries. + // Arrange + var store = UserStoreTestHelpers.BuildStore(fixture.Store); + var user = UserStoreTestHelpers.NewUser(); + await store.CreateAsync(user, Ct); + var first = await store.FindByIdAsync(user.Id, Ct); + var second = await store.FindByIdAsync(user.Id, Ct); // same content version + + // Act — the first update commits; the second is now stale. + first!.PhoneNumber = "+49 111 1111"; + Assert.True((await store.UpdateAsync(first, Ct)).Succeeded); + + second!.PhoneNumber = "+49 222 2222"; + var result = await store.UpdateAsync(second, Ct); + + // Assert — rejected, and the first writer's change survives. + Assert.False(result.Succeeded); + Assert.Contains(result.Errors, e => e.Code == "ConcurrencyFailure"); + var persisted = await store.FindByIdAsync(user.Id, Ct); + Assert.Equal("+49 111 1111", persisted!.PhoneNumber); + } + + [Fact] + public async Task UpdateAsync_StaleProfileEdit_DoesNotClobberConcurrent2faEnable() + { + // The headline scenario: a stale profile edit must not turn 2FA back off after it + // was enabled concurrently. + // Arrange + var store = UserStoreTestHelpers.BuildStore(fixture.Store); + var user = UserStoreTestHelpers.NewUser(); + await store.CreateAsync(user, Ct); + var editor = await store.FindByIdAsync(user.Id, Ct); + var securitySnapshot = await store.FindByIdAsync(user.Id, Ct); + + // Act — 2FA is enabled; then the stale profile edit lands. + securitySnapshot!.TwoFactorEnabled = true; + Assert.True((await store.UpdateAsync(securitySnapshot, Ct)).Succeeded); + + editor!.PhoneNumber = "+49 555 5555"; + var result = await store.UpdateAsync(editor, Ct); + + // Assert — the stale edit is rejected and 2FA stays enabled. + Assert.False(result.Succeeded); + var persisted = await store.FindByIdAsync(user.Id, Ct); + Assert.True(persisted!.TwoFactorEnabled); + } + + [Fact] + public async Task UpdateAsync_SequentialUpdatesOnSameInstance_BothSucceed() + { + // ASP.NET Identity flows (e.g. ResetAuthenticator) call UpdateAsync more than once + // on the same in-memory instance without reloading; the content version must be + // refreshed after each write so the second call is not self-rejected. + // Arrange + var store = UserStoreTestHelpers.BuildStore(fixture.Store); + var user = UserStoreTestHelpers.NewUser(); + await store.CreateAsync(user, Ct); + var loaded = await store.FindByIdAsync(user.Id, Ct); + + // Act + loaded!.TwoFactorEnabled = true; + Assert.True((await store.UpdateAsync(loaded, Ct)).Succeeded); + loaded.PhoneNumber = "+49 333 3333"; + Assert.True((await store.UpdateAsync(loaded, Ct)).Succeeded); + + // Assert + var persisted = await store.FindByIdAsync(user.Id, Ct); + Assert.True(persisted!.TwoFactorEnabled); + Assert.Equal("+49 333 3333", persisted.PhoneNumber); + } + + [Fact] + public async Task UpdateAsync_AfterConcurrentLockoutIncrement_StillSucceeds() + { + // A concurrent failed-login increment must not be seen as a content conflict: + // lockout-only updates do not advance the content version. + // Arrange + var store = UserStoreTestHelpers.BuildStore(fixture.Store); + var user = UserStoreTestHelpers.NewUser(); + await store.CreateAsync(user, Ct); + var loaded = await store.FindByIdAsync(user.Id, Ct); + + await using (var session = fixture.Store.QuerySession()) + { + var other = await session.LoadAsync(user.UserId.Value, Ct); + await store.IncrementAccessFailedCountAsync(other!, Ct); + } + + // Act + loaded!.PhoneNumber = "+49 444 4444"; + var result = await store.UpdateAsync(loaded, Ct); + + // Assert — the profile update commits, and the increment is preserved. + Assert.True(result.Succeeded); + var persisted = await store.FindByIdAsync(user.Id, Ct); + Assert.Equal("+49 444 4444", persisted!.PhoneNumber); + Assert.Equal(1, persisted.AccessFailedCount); + } + + #endregion + #region Security stamp (session invalidation) [Fact]