test(api): cover concurrent refresh-token race + rotation atomicity (#243) - #370
Conversation
…243) Close the high-severity mock-level gaps in the two AuthSessionServiceTests: - Rotation atomicity: assert the persisted TokenHash is deterministically the SHA-256 of the new refresh token handed to the client (not just "!= original"), in both the Infrastructure and Application rotate tests. - Concurrent refresh: model the race two ways. A DbUpdateConcurrencyException on save (stale #329 xmin token) is a controlled failure — INVALID_SESSION, DiscardChanges() runs, and no new token is issued. A loser replaying the already-consumed token is rejected while the session rotates exactly once to the winner's token (no double-rotation). These complement the DB-integration coverage in AuthSessionServiceConcurrentRefreshTests by asserting the service's own contract (deterministic storage, recovery call, token non-issuance) at the unit level. Refs thomasluizon/orbit-ui-mobile#243 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review: #370 — test(api): cover concurrent refresh-token race + rotation atomicity (#243)
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical (incl. |
0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 0 |
Scope
Test-only PR touching two files (tests/Orbit.Application.Tests/Services/AuthSessionServiceTests.cs, tests/Orbit.Infrastructure.Tests/Services/AuthSessionServiceTests.cs), no src/ changes. Adds:
- A deterministic
TokenHash == SHA256(returnedRefreshToken)assertion to the two existing rotate tests. - A "loser replays already-rotated token" test (Application.Tests) using the stateful in-memory
FindOneTrackedAsyncmock. - A "concurrency conflict on save →
INVALID_SESSION+DiscardChanges()+ no token issued" test in both files.
Verification
Read src/Orbit.Infrastructure/Services/AuthSessionService.cs (RefreshSessionAsync, the catch (DbUpdateConcurrencyException) block) and src/Orbit.Domain/Entities/UserSession.cs (Rotate, CanUse) line-by-line against every new assertion:
ErrorCodes.InvalidSession == "INVALID_SESSION"— confirmed insrc/Orbit.Application/Common/ErrorCodes.cs, matches the literal already used in that test file's existing style.unitOfWork.DiscardChanges()is exactly what the production catch block calls on concurrency conflict — confirmed.- After
session.Rotate(HashToken(newRefreshToken), ...), the returnedSessionTokens.RefreshToken == newRefreshToken, so assertingTokenHash == Hash(returnedToken)is a real (non-tautological) check — confirmed. - The loser/winner race test is sound: once the winner rotates
_storedSession.TokenHash, the loser'sFindOneTrackedAsyncpredicate (built from the original consumed token's hash) legitimately stops matching, driving thesession is nullbranch →INVALID_SESSION; the failed loser call never touches_storedSession, so the final assertion holds.
No security, contract/DTO, DESIGN.md, i18n, or parity surface is touched (backend-only, test-only diff) — those rubric dimensions are N/A for this PR.
Deferred / not applicable
- Dimensions 8/9/10 (DESIGN.md, Parity, i18n) — N/A, no
apps/*files touched. - Dimension 11 (Contract drift) — N/A, no DTO/route/shared-type change;
contract-alignernot invoked (no staged consumer-repo changes and no contract surface touched). - Dimension 12 (Security) — N/A, no
src/changed; manually confirmed no new secret/token exposure in the added assertions. - Dimension 14 (FEATURES.md) — N/A, test-only, no feature-surface change.
dotnet build/dotnet test— skipped per workflow instructions; Build / Unit Tests / SonarCloud run as separate required CI checks on this PR.
No prior reviews or review comments existed on this PR before this one.
|



What
Closes the two high-severity test gaps flagged by the
/prod-readinesstests audit on the concurrent refresh-token path (guarded by the #329xmintoken + #334 pre-rotation validation), at the mock/unit level in the twoAuthSessionServiceTests.Rotation atomicity
The existing rotate tests only asserted
RefreshToken != original. Now they also assert the persistedsession.TokenHashis deterministically the SHA-256 of the new refresh token returned to the client — i.e. what's stored is exactly what the caller received. Added to both:Orbit.Infrastructure.TestsRefreshSessionAsync_RotatesExistingSessionOrbit.Application.TestsRefreshSessionAsync_WithNonExpiringSession_RotatesRefreshTokenWithoutAddingExpiryConcurrent refresh (two race orderings)
DbUpdateConcurrencyExceptionon save (stalexmin) is a controlled failure: the loser getsINVALID_SESSION,IUnitOfWork.DiscardChanges()runs (the revert that leaves the row unrotated), the exception never propagates, and no access token is issued. Covered in both files.INVALID_SESSIONwhile the session rotates exactly once to the winner's token (no double-rotation). Covered inOrbit.Application.Testsusing the stateful in-memory session store.Why these complement the existing DB test
AuthSessionServiceConcurrentRefreshTests(added in #329) already proves the behaviour end-to-end through a real EF in-memory provider + save interceptor. These new tests assert the service's own contract in isolation of the provider: deterministic hash storage, theDiscardChanges()recovery call, single-rotation, and token non-issuance. Each test fails if the corresponding behaviour regresses.Scope
Test-only. No source changes — the current
RefreshSessionAsynccatch path already satisfies every new assertion. Files touched: the two ownedAuthSessionServiceTests.csonly.Refs thomasluizon/orbit-ui-mobile#243