fix(api): Transact admin user writes, and stop the per-request Argon2 hash

src/Anichron.API.Tests.Unit/Security/PasswordHasherTests.cs

@@ -1,4 +1,5 @@
 using Anichron.API.Security;
+using System.Diagnostics;
 
 namespace Anichron.API.Tests.Unit.Security;
 
@@ -81,4 +82,55 @@ public void Verify_TamperedStoredHash_ReturnsFalse()
 
         result.Should().BeFalse();
     }
+
+    // ==========================================================================
+    // Equal work for a user that does not exist — issue #173
+    //
+    // A null storedHash means "no such account". Verifying it costs the same Argon2 pass as
+    // verifying a real one, so this method leaks nothing about account existence.
+    //
+    // ⚠️ That is a claim about the HASHER, not about login. AuthService.LoginAsync still writes
+    // a failed-attempt record for a known user and skips it for an unknown one, so end-to-end
+    // login timing remains distinguishable. Scoped deliberately — overstating it here is how the
+    // residual leak gets forgotten.
+    //
+    // The hasher owns the invariant because it owns the cost. It previously lived in an
+    // AuthService field initializer, which — AuthService being registered Scoped — spent 64 MiB
+    // and three Argon2 passes on *every request that resolved IAuthService*, including /refresh
+    // and /logout, which never read the value.
+    // ==========================================================================
+
+    [Fact]
+    public void Verify_NullStoredHash_ReturnsFalse()
+    {
+        var testee = new Argon2PasswordHasher();
+
+        var result = testee.Verify("password", null);
+
+        result.Should().BeFalse();
+    }
+
+    // ⚠️ Named for what it actually pins: that the null path is not a cheap early return. It
+    // does NOT pin "equal work" — it would still pass if the throwaway pass used weaker Argon2
+    // parameters, or were replaced by a sleep. Pinning true equality needs a comparison against
+    // the real path, and that is the wall-clock-ratio shape that goes flaky on a loaded runner.
+    // The guard against weakened parameters is that both paths call the same RunArgon2idSecure.
+    [Fact]
+    public void Verify_NullStoredHash_DoesNotShortCircuit()
+    {
+        var testee = new Argon2PasswordHasher();
+
+        var stopwatch = Stopwatch.StartNew();
+        testee.Verify("password", null);
+        stopwatch.Stop();
+
+        // ⭐ A LOWER bound, deliberately. "At least this slow" cannot fail on a loaded or slow
+        // machine — only on an impossibly fast one, and 64 MiB of Argon2id at three iterations
+        // does not complete in single-digit milliseconds on any hardware this runs on (measured
+        // ~30–80 ms locally). An upper bound, or a ratio against the real-hash path, is the
+        // shape that goes flaky on a busy CI runner.
+        stopwatch.ElapsedMilliseconds.Should().BeGreaterThan(10,
+            "a null hash must still cost an Argon2 pass; returning early would make "
+            + "verification time reveal that the account does not exist");
+    }
 }

src/Anichron.API.Tests.Unit/Services/AdminUserServiceTests.cs

@@ -2,6 +2,7 @@
 using Anichron.Core.Data;
 using Anichron.Core.Data.Repository;
 using Anichron.Core.Domain;
+using NSubstitute.ExceptionExtensions;
 
 namespace Anichron.API.Tests.Unit.Services;
 
@@ -16,7 +17,44 @@ private sealed class TestFixture
         public ITokenService TokenService { get; } = Substitute.For<ITokenService>();
         private readonly IClock _clock = Substitute.For<IClock>();
 
-        public TestFixture() => _clock.GetCurrentInstant().Returns(FixedNow);
+        // Record whether each write ran *while the transaction lambda was executing*, not merely
+        // that a transaction was opened somewhere: `Received().ExecuteInTransactionAsync(...)`
+        // alone would also pass on code that opened a transaction and wrote outside it.
+        public bool RevokedInsideTransaction { get; private set; }
+        public bool SavedInsideTransaction { get; private set; }
+        private bool insideTransaction;
+
+        public TestFixture()
+        {
+            _clock.GetCurrentInstant().Returns(FixedNow);
+
+            // The non-generic Func<Task> overload — the one AdminUserService uses. NSubstitute
+            // stubs per overload, so stubbing Func<Task<T>> here would leave the lambda
+            // silently un-invoked.
+            UnitOfWork
+                .ExecuteInTransactionAsync(Arg.Any<Func<Task>>(), Arg.Any<CancellationToken>())
+                .Returns(async callInfo =>
+                {
+                    insideTransaction = true;
+                    try
+                    {
+                        await callInfo.Arg<Func<Task>>()();
+                    }
+                    finally
+                    {
+                        insideTransaction = false;
+                    }
+                });
+
+            TokenService
+                .When(t => t.MarkAllSessionsRevokedAsync(
+                    Arg.Any<Guid>(), Arg.Any<Instant>(), Arg.Any<CancellationToken>()))
+                .Do(_ => RevokedInsideTransaction = insideTransaction);
+
+            UnitOfWork
+                .When(u => u.SaveChangesAsync(Arg.Any<CancellationToken>()))
+                .Do(_ => SavedInsideTransaction = insideTransaction);
+        }
 
         public AdminUserService CreateTestee() => new(Users, UnitOfWork, _clock, TokenService);
     }
@@ -82,6 +120,8 @@ public async Task UpdateAsync_BothParamsNull_ReturnsUserWithoutSaving()
         result.IsSuccess.Should().BeTrue();
         result.Value.Should().BeSameAs(user);
         await fixture.UnitOfWork.DidNotReceive().SaveChangesAsync(Arg.Any<CancellationToken>());
+        // Nothing is written, so opening a transaction would be cost without purpose.
+        await fixture.UnitOfWork.DidNotReceive().ExecuteInTransactionAsync(Arg.Any<Func<Task>>(), Arg.Any<CancellationToken>());
     }
 
     [Fact]
@@ -109,6 +149,7 @@ public async Task UpdateAsync_UserNotFound_ReturnsUserNotFound()
         result.IsSuccess.Should().BeFalse();
         result.Error.Should().Be(AuthError.UserNotFound);
         await fixture.UnitOfWork.DidNotReceive().SaveChangesAsync(Arg.Any<CancellationToken>());
+        await fixture.UnitOfWork.DidNotReceive().ExecuteInTransactionAsync(Arg.Any<Func<Task>>(), Arg.Any<CancellationToken>());
     }
 
     [Fact]
@@ -136,6 +177,11 @@ public async Task UpdateAsync_SetIsDisabledTrue_RevokesSessionsAndSaves()
 
         await fixture.TokenService.Received(1).MarkAllSessionsRevokedAsync(user.Id, FixedNow, Arg.Any<CancellationToken>());
         await fixture.UnitOfWork.Received(1).SaveChangesAsync(Arg.Any<CancellationToken>());
+        await fixture.UnitOfWork.Received(1).ExecuteInTransactionAsync(Arg.Any<Func<Task>>(), Arg.Any<CancellationToken>());
+        fixture.RevokedInsideTransaction.Should().BeTrue(
+            "revoking sessions outside the transaction cannot be rolled back when the save fails");
+        fixture.SavedInsideTransaction.Should().BeTrue(
+            "the user mutation must commit atomically with the revocation");
     }
 
     [Fact]
@@ -195,6 +241,7 @@ public async Task DeleteAsync_UserNotFound_ReturnsUserNotFound()
         result.Error.Should().Be(AuthError.UserNotFound);
         fixture.Users.DidNotReceive().Remove(Arg.Any<User>());
         await fixture.UnitOfWork.DidNotReceive().SaveChangesAsync(Arg.Any<CancellationToken>());
+        await fixture.UnitOfWork.DidNotReceive().ExecuteInTransactionAsync(Arg.Any<Func<Task>>(), Arg.Any<CancellationToken>());
     }
 
     [Fact]
@@ -210,5 +257,40 @@ public async Task DeleteAsync_ValidUser_RevokesSessionsRemovesUserAndSaves()
         await fixture.TokenService.Received(1).MarkAllSessionsRevokedAsync(user.Id, FixedNow, Arg.Any<CancellationToken>());
         fixture.Users.Received(1).Remove(user);
         await fixture.UnitOfWork.Received(1).SaveChangesAsync(Arg.Any<CancellationToken>());
+        await fixture.UnitOfWork.Received(1).ExecuteInTransactionAsync(Arg.Any<Func<Task>>(), Arg.Any<CancellationToken>());
+        fixture.RevokedInsideTransaction.Should().BeTrue();
+        fixture.SavedInsideTransaction.Should().BeTrue();
+    }
+
+    // ==========================================================================
+    // Transactional integrity — issue #172
+    //
+    // Revoking sessions uses ExecuteUpdateAsync, which bypasses the change tracker and issues
+    // SQL immediately. Untransacted, a failing SaveChangesAsync therefore leaves the sessions
+    // revoked and the user mutation lost: an admin "disables" an account, the save fails, and
+    // the account is still enabled while its sessions are gone.
+    // ==========================================================================
+
+    [Fact]
+    public async Task UpdateAsync_SaveThrows_TheRevocationRanInsideTheFailedTransaction()
+    {
+        var fixture = new TestFixture();
+        var user = new User { Id = Guid.NewGuid(), IsDisabled = false, StorageConfigs = [] };
+        fixture.Users.FindByIdWithConfigsAsync(user.Id, Arg.Any<CancellationToken>()).Returns(user);
+        fixture.UnitOfWork
+            .SaveChangesAsync(Arg.Any<CancellationToken>())
+            .ThrowsAsync(new InvalidOperationException("constraint violation"));
+
+        var act = async () => await fixture.CreateTestee()
+            .UpdateAsync(Guid.NewGuid(), user.Id, isAdmin: null, isDisabled: true, CancellationToken.None);
+
+        // The throw propagating is not the interesting part — it did that before the fix too.
+        // What matters is that BOTH writes sat inside the transaction that failed, so the
+        // rollback covers them.
+        await act.Should().ThrowAsync<InvalidOperationException>();
+        fixture.RevokedInsideTransaction.Should().BeTrue(
+            "otherwise the sessions stay revoked while the user mutation is lost — the #172 defect");
+        fixture.SavedInsideTransaction.Should().BeTrue(
+            "the save that failed must itself have been inside the transaction");
     }
 }

src/Anichron.API.Tests.Unit/Services/AuthServiceTests.cs

@@ -18,7 +18,7 @@ private sealed class TestFixture
         private readonly IUnitOfWork _unitOfWork = Substitute.For<IUnitOfWork>();
         private readonly IClock _clock = Substitute.For<IClock>();
         private readonly IGuidFactory _guidFactory = Substitute.For<IGuidFactory>();
-        private readonly IPasswordHasher _passwordHasher = Substitute.For<IPasswordHasher>();
+        internal readonly IPasswordHasher PasswordHasher = Substitute.For<IPasswordHasher>();
         private readonly IRegistrationValidator _validator = Substitute.For<IRegistrationValidator>();
         internal readonly ILockoutService Lockout = Substitute.For<ILockoutService>();
 
@@ -27,7 +27,7 @@ private sealed class TestFixture
         public TestFixture()
         {
             _guidFactory.NewGuid().Returns(Guid.Empty);
-            _passwordHasher.Hash(Arg.Any<string>()).Returns("hashed_value");
+            PasswordHasher.Hash(Arg.Any<string>()).Returns("hashed_value");
             _clock.GetCurrentInstant().Returns(Instant.FromUtc(2026, 1, 1, 12, 0, 0));
             TokenService.IssueAsync(Arg.Any<User>(), Arg.Any<CancellationToken>())
                 .Returns(new AuthTokens("access_token", "refresh_token"));
@@ -58,7 +58,7 @@ public TestFixture WithValidInvite(Invite invite)
 
         public TestFixture WithPasswordValid()
         {
-            _passwordHasher.Verify(Arg.Any<string>(), Arg.Any<string>()).Returns(true);
+            PasswordHasher.Verify(Arg.Any<string>(), Arg.Any<string>()).Returns(true);
             return this;
         }
 
@@ -148,7 +148,7 @@ public TestFixture WithUserFoundForCredential(string normalizedCredential, User
         }
 
         public AuthService CreateTestee() => new(
-            Users, _invites, _unitOfWork, _clock, _guidFactory, _passwordHasher, _validator, TokenService, Lockout);
+            Users, _invites, _unitOfWork, _clock, _guidFactory, PasswordHasher, _validator, TokenService, Lockout);
     }
 
     // ConstraintName has no setter in Npgsql 10 — must use the full constructor.
@@ -465,6 +465,22 @@ public async Task LoginAsync_UnknownUser_ReturnsInvalidCredentials()
         });
     }
 
+    // Issue #173: the equal-work guarantee now lives in the hasher, which spends a full Argon2
+    // pass on a null hash. This pins the call site's half of that bargain — it must hand the
+    // hasher a null rather than short-circuiting, and must not resurrect a precomputed dummy
+    // hash (the old field initializer cost 64 MiB on *every* request that resolved IAuthService,
+    // including ones that never logged anyone in).
+    [Fact]
+    public async Task LoginAsync_UnknownUser_VerifiesAgainstANullHashExactlyOnce()
+    {
+        var fixture = new TestFixture();
+
+        await fixture.CreateTestee().LoginAsync("ghost", "password", CancellationToken.None);
+
+        fixture.PasswordHasher.Received(1).Verify("password", null);
+        fixture.PasswordHasher.DidNotReceive().Hash(Arg.Any<string>());
+    }
+
     [Fact]
     public async Task LoginAsync_WrongPassword_ReturnsInvalidCredentials()
     {

src/Anichron.API/Security/PasswordHasher.cs

@@ -8,7 +8,11 @@ namespace Anichron.API.Security;
 public interface IPasswordHasher
 {
     string Hash(string password);
-    bool Verify(string password, string storedHash);
+
+    // storedHash is null when no such account exists. A null hash still costs a full Argon2
+    // pass, so verification itself takes the same time either way. That removes the HASHING
+    // asymmetry only — it does not make a caller's whole code path constant-time.
+    bool Verify(string password, string? storedHash);
 }
 
 public sealed class Argon2PasswordHasher : IPasswordHasher
@@ -24,8 +28,29 @@ public string Hash(string password)
         return Convert.ToBase64String(combined);
     }
 
-    public bool Verify(string password, string storedHash)
+    public bool Verify(string password, string? storedHash)
     {
+        // ⛔ Equal work, not an early return. Verifying a missing account must cost the same
+        // Argon2 pass as verifying a real one, so this method leaks nothing about account
+        // existence. ⚠️ Whether the CALLER leaks it is the caller's problem — AuthService.Login
+        // still does a database write for a known user that it skips for an unknown one.
+        //
+        // The invariant lives here because this class owns the cost. It used to live in an
+        // AuthService field initializer, which — AuthService being Scoped — burned 64 MiB and
+        // three Argon2 passes on every request that resolved IAuthService, including ones that
+        // never read the value. See #173.
+        //
+        // ⚠️ `return false` unconditionally, rather than comparing against random bytes:
+        // correctness must not rest on two random values differing. The FixedTimeEquals of the
+        // real path is deliberately not mirrored here — it is microseconds against ~100 ms of
+        // Argon2, so adding it back would buy nothing and reintroduce that dependency.
+        if (storedHash is null)
+        {
+            var throwawaySalt = RandomNumberGenerator.GetBytes(AppDefaults.Argon2.SaltLength);
+            CryptographicOperations.ZeroMemory(RunArgon2idSecure(password, throwawaySalt));
+            return false;
+        }
+
         var combined = Convert.FromBase64String(storedHash);
         var salt = combined[..AppDefaults.Argon2.SaltLength];
         var expected = combined[AppDefaults.Argon2.SaltLength..];

src/Anichron.API/Services/AdminUserService.cs

@@ -43,10 +43,17 @@ public async Task<AuthResult<User>> UpdateAsync(Guid callerId, Guid targetId, bo
         if (isDisabled.HasValue)
             user.IsDisabled = isDisabled.Value;
 
-        if (shouldRevokeSessions)
-            await tokenService.MarkAllSessionsRevokedAsync(targetId, clock.GetCurrentInstant(), ct);
+        // Both writes, or neither: revocation goes through ExecuteUpdateAsync, which bypasses the
+        // change tracker and commits immediately, so untransacted a failing save would leave the
+        // sessions revoked and the user mutation lost.
+        await unitOfWork.ExecuteInTransactionAsync(async () =>
+        {
+            if (shouldRevokeSessions)
+                await tokenService.MarkAllSessionsRevokedAsync(targetId, clock.GetCurrentInstant(), ct);
+
+            await unitOfWork.SaveChangesAsync(ct);
+        }, ct);
 
-        await unitOfWork.SaveChangesAsync(ct);
         return AuthResult.Ok(user);
     }
 
@@ -59,9 +66,15 @@ public async Task<AuthResult> DeleteAsync(Guid callerId, Guid targetId, Cancella
         if (user is null)
             return AuthResult.Fail(AuthError.UserNotFound);
 
-        await tokenService.MarkAllSessionsRevokedAsync(targetId, clock.GetCurrentInstant(), ct);
+        // Same revoke-then-save pairing as UpdateAsync, transacted for the same reason — here the
+        // loss would be a user whose sessions are all revoked but whose row was never deleted.
         users.Remove(user);
-        await unitOfWork.SaveChangesAsync(ct);
+        await unitOfWork.ExecuteInTransactionAsync(async () =>
+        {
+            await tokenService.MarkAllSessionsRevokedAsync(targetId, clock.GetCurrentInstant(), ct);
+            await unitOfWork.SaveChangesAsync(ct);
+        }, ct);
+
         return AuthResult.Ok();
     }
 }

src/Anichron.API/Services/AuthService.cs

@@ -62,8 +62,6 @@ public sealed class AuthService(
     ILockoutService lockout)
     : IAuthService
 {
-    private readonly string dummyPasswordHash = passwordHasher.Hash(guidFactory.NewGuid().ToString());
-
     public async Task<AuthResult<AuthTokens>> RegisterAsync(string username, string email, string password, string inviteToken, CancellationToken ct)
     {
         ArgumentNullException.ThrowIfNull(username);
@@ -130,8 +128,16 @@ public async Task<AuthResult<AuthTokens>> LoginAsync(string usernameOrEmail, str
         var normalized = usernameOrEmail.Trim().ToLowerInvariant();
         var user = await users.FindByCredentialAsync(normalized, ct);
 
-        // Prevent timing attack: Use dummy hash for non-existing accounts
-        var passwordValid = passwordHasher.Verify(password, user?.PasswordHash ?? dummyPasswordHash);
+        // A null hash means "no such account", and the hasher spends a full Argon2 pass on it
+        // rather than short-circuiting. That removes the hashing asymmetry; this call site's only
+        // job is not to defeat it by branching before the call.
+        //
+        // ⚠️ It does NOT make login constant-time end to end, and this comment deliberately does
+        // not claim that. A known user with a wrong password goes on to
+        // RecordFailedAttemptAsync below, which is a database write an unknown user never makes —
+        // milliseconds, far more than the hashing difference this removes. Username enumeration
+        // by timing is therefore still possible. Tracked separately; see the note on that call.
+        var passwordValid = passwordHasher.Verify(password, user?.PasswordHash);
 
         var now = clock.GetCurrentInstant();