fix: send a token on change-password and logout, and six other audit defects

src/SubVora.Api/Controllers/UsersController.cs

@@ -23,15 +23,23 @@ public UsersController(IUserRepository userRepository, IValidator<UpdateUserProf
     }
 
     /// <summary>Gets the authenticated user's own profile.</summary>
+    /// <remarks>
+    /// 404 rather than a 200 carrying null when the row is gone - the same answer <c>PUT</c> gives
+    /// on this resource. Only reachable if the row vanished between authenticating and reading it,
+    /// but a 200 with an empty body is indistinguishable from a successful read, so a client has no
+    /// way to tell the two apart.
+    /// </remarks>
     /// <response code="200">Returns the caller's profile.</response>
     /// <response code="401">The caller is not authenticated.</response>
+    /// <response code="404">The caller's user row no longer exists.</response>
     [HttpGet("me")]
     [ProducesResponseType(typeof(UserProfileDto), StatusCodes.Status200OK)]
     [ProducesResponseType(StatusCodes.Status401Unauthorized)]
+    [ProducesResponseType(StatusCodes.Status404NotFound)]
     public async Task<IActionResult> GetMe(CancellationToken cancellationToken)
     {
         var profile = await _userRepository.GetProfileAsync(GetUserId(), cancellationToken);
-        return Ok(profile);
+        return profile is null ? NotFound() : Ok(profile);
     }
 
     /// <summary>Updates the authenticated user's own profile.</summary>

src/SubVora.Application/Dashboard/BurnRateCalculator.cs

@@ -106,6 +106,18 @@ public async Task<BurnRateResult> CalculateAsync(IEnumerable<SubscriptionDto> su
 
             var convertedCost = subscription.CostAmount * rate;
 
+            // Active free trials aren't being charged yet - excluded until IsFreeTrial flips
+            // to false, at which point the same subscription joins the totals automatically.
+            //
+            // Above the cadence branch, not below it: sitting after the OneTime `continue` meant a
+            // one-time purchase marked as a free trial was never tested against this at all, and
+            // landed in OneTimeThisYear despite nothing being charged for it. Whether a trial counts
+            // is not a question about its cadence, so no cadence gets to bypass the check.
+            if (subscription.IsFreeTrial)
+            {
+                continue;
+            }
+
             if (subscription.CycleCadence == BillingCycleType.OneTime)
             {
                 if (subscription.PurchaseDate.Year == currentYear)
@@ -116,13 +128,6 @@ public async Task<BurnRateResult> CalculateAsync(IEnumerable<SubscriptionDto> su
                 continue;
             }
 
-            // Active free trials aren't being charged yet - excluded until IsFreeTrial flips
-            // to false, at which point the same subscription joins the totals automatically.
-            if (subscription.IsFreeTrial)
-            {
-                continue;
-            }
-
             // What this subscription costs over a year: its price times how often it is billed.
             var subscriptionAnnual = convertedCost * ChargesPerYear(subscription.CycleCadence);
             annualSum += subscriptionAnnual;

src/SubVora.Infrastructure/Auth/AuthService.cs

@@ -242,10 +242,27 @@ public async Task ForgotPasswordAsync(string email, CancellationToken cancellati
     {
         var normalizedEmail = email.Trim().ToLowerInvariant();
 
+        // Generated and hashed before the lookup, and unconditionally - the same shape RegisterAsync
+        // uses for its BCrypt hash and LoginAsync for DummyPasswordHash. Both branches now pay for
+        // the CSPRNG draw and the SHA-256, so that much of the work no longer depends on whether the
+        // address exists.
+        var code = RandomNumberGenerator.GetInt32(0, 1_000_000).ToString("D6");
+        var codeHash = HashResetCode(code);
+
         var user = await _dbContext.Users.SingleOrDefaultAsync(u => u.Email == normalizedEmail, cancellationToken);
         if (user is null)
         {
-            // No enumeration - caller gets the same outcome either way.
+            // No enumeration by status or body - the caller gets the same 200 and empty response
+            // either way, and the work above is done regardless.
+            //
+            // Residual, accepted deliberately: a real account additionally performs a SELECT for
+            // outstanding codes, an UPDATE per code found, an INSERT and a SaveChanges, so a known
+            // address still costs more round trips than an unknown one. Closing that fully would
+            // mean issuing throwaway writes for addresses with no account - real write load driven
+            // by anyone who can reach the endpoint. Judged not worth it: the remaining signal is a
+            // few milliseconds of database time rather than the ~250ms BCrypt asymmetry the register
+            // and login paths exist to erase, and this endpoint is IP rate-limited (the "auth"
+            // policy, 10/min by default) which bounds how finely it can be sampled.
             return;
         }
 
@@ -260,12 +277,10 @@ public async Task ForgotPasswordAsync(string email, CancellationToken cancellati
             superseded.UsedAt = DateTimeOffset.UtcNow;
         }
 
-        var code = RandomNumberGenerator.GetInt32(0, 1_000_000).ToString("D6");
-
         _dbContext.PasswordResetCodes.Add(new PasswordResetCode
         {
             UserId = user.Id,
-            CodeHash = HashResetCode(code),
+            CodeHash = codeHash,
             ExpiresAt = DateTimeOffset.UtcNow.Add(PasswordResetCodeLifetime),
             CreatedAt = DateTimeOffset.UtcNow,
         });

src/SubVora.Infrastructure/Currency/FxRateRefreshBackgroundService.cs

@@ -82,6 +82,8 @@ public async Task RefreshOnceAsync(CancellationToken cancellationToken = default
         var baseCurrencies = subscriptionCurrencies.Union(preferredCurrencies).Distinct();
 
         var allRates = new List<ExchangeRate>();
+        var failedCurrencies = 0;
+
         foreach (var baseCurrency in baseCurrencies)
         {
             var targets = targetCurrencies.Where(t => t != baseCurrency).ToList();
@@ -90,13 +92,47 @@ public async Task RefreshOnceAsync(CancellationToken cancellationToken = default
                 continue;
             }
 
-            var rates = await _exchangeRateClient.GetLatestRatesAsync(baseCurrency, targets, cancellationToken);
-            allRates.AddRange(rates);
+            // Isolated per currency, deliberately: a partial pass beats none. These rates were
+            // accumulated and upserted only after the loop, so a single unsupported pair or one
+            // transient 5xx threw straight past the upsert and discarded every rate that had
+            // already been fetched successfully - every user's totals aged by a day because one
+            // user tracked something exotic. The next scheduled run retries whatever failed.
+            try
+            {
+                var rates = await _exchangeRateClient.GetLatestRatesAsync(baseCurrency, targets, cancellationToken);
+                allRates.AddRange(rates);
+            }
+            catch (Exception ex) when (ex is not OperationCanceledException)
+            {
+                failedCurrencies++;
+                _logger.LogWarning(ex, "FX rate fetch failed for base currency {BaseCurrency}; continuing with the rest of the pass.", baseCurrency);
+            }
         }
 
         if (allRates.Count > 0)
         {
             await fxRateService.UpsertRatesAsync(allRates, cancellationToken);
         }
+
+        // Otherwise a provider that has quietly dropped a currency is invisible until someone works
+        // backwards from a total that stopped moving. Escalated to Error when nothing at all came
+        // back: isolating failures per currency must not turn a total provider outage - which used
+        // to throw and log at Error - into a handful of warnings nobody pages on.
+        if (failedCurrencies > 0)
+        {
+            if (allRates.Count == 0)
+            {
+                _logger.LogError(
+                    "FX rate refresh fetched nothing: all {FailedCurrencyCount} base currency/currencies failed. Previously cached rates are unaffected.",
+                    failedCurrencies);
+            }
+            else
+            {
+                _logger.LogWarning(
+                    "FX rate refresh completed with {FailedCurrencyCount} base currency/currencies failing; {UpsertedRateCount} rate(s) were still refreshed.",
+                    failedCurrencies,
+                    allRates.Count);
+            }
+        }
     }
 }

src/SubVora.Infrastructure/Repositories/SubscriptionRepository.cs

@@ -70,6 +70,19 @@ public async Task<SubscriptionUpdateResult> UpdateAsync(Guid id, Guid userId, Cr
         subscription.CatalogId = request.CatalogId;
         subscription.IsFreeTrial = request.IsFreeTrial;
 
+        // Force the row to be written even when nothing above actually differs. Without this, an
+        // unchanged payload leaves the entity Unchanged, EF issues no UPDATE at all, and there is no
+        // statement for the xmin predicate to be part of - so SaveChangesAsync cannot raise a
+        // concurrency exception and the call returns 200 against a row that has moved on.
+        //
+        // That is not a corner case: the conflict this check exists for is an edit screen opened
+        // before a mark-paid and then saved. A user who changed nothing is exactly the user whose
+        // save would silently roll the billing date back.
+        if (request.Version is not null)
+        {
+            _dbContext.Entry(subscription).State = EntityState.Modified;
+        }
+
         try
         {
             await _dbContext.SaveChangesAsync(cancellationToken);

src/SubVora.Mobile/Api/IAccountApi.cs

@@ -0,0 +1,39 @@
+using Refit;
+using SubVora.Mobile.Api.Dtos;
+
+namespace SubVora.Mobile.Api;
+
+/// <summary>
+/// The auth endpoints that require a bearer token, split out from <see cref="IAuthApi"/> so they can
+/// be registered with <c>AuthDelegatingHandler</c> attached.
+/// <para>
+/// The split exists because <see cref="IAuthApi"/> must <em>not</em> chain that handler - it carries
+/// <c>/auth/refresh</c>, and a 401 during refresh would recurse straight back into refresh. Leaving
+/// these two calls there meant they went out with no <c>Authorization</c> header at all, against
+/// endpoints the API marks <c>[Authorize]</c>: change-password answered 401 every time and was
+/// reported to the user as an expired session, and logout's revoke silently never happened while the
+/// client cleared its tokens and moved on.
+/// </para>
+/// <para>
+/// Anything added here must be an endpoint that requires authentication and is not itself part of
+/// the refresh path. <c>RefitClientCompositionTests</c> asserts this interface is registered with the
+/// handler and that <see cref="IAuthApi"/> is not.
+/// </para>
+/// </summary>
+public interface IAccountApi
+{
+    /// <summary>
+    /// Changes the signed-in user's password. Returns a fresh token pair, because succeeding
+    /// revokes every refresh token the account holds - including this device's.
+    /// </summary>
+    [Post("/api/v1/auth/change-password")]
+    Task<IApiResponse<AuthTokenResponse>> ChangePasswordAsync([Body] ChangePasswordRequest request, CancellationToken cancellationToken = default);
+
+    /// <summary>
+    /// Revokes the caller's refresh token, ending that session. Always 204 when it lands - the
+    /// server is deliberately quiet about whether the presented token existed, belonged to someone
+    /// else, or was already revoked.
+    /// </summary>
+    [Post("/api/v1/auth/logout")]
+    Task<IApiResponse> LogoutAsync([Body] RefreshRequest request, CancellationToken cancellationToken = default);
+}

src/SubVora.Mobile/Api/IAuthApi.cs

@@ -3,6 +3,17 @@
 
 namespace SubVora.Mobile.Api;
 
+/// <summary>
+/// The auth endpoints that take no bearer token. Registered <em>without</em>
+/// <c>AuthDelegatingHandler</c> on purpose: this interface carries <c>/auth/refresh</c>, and chaining
+/// the handler here would let a 401 during refresh recurse back into refresh.
+/// <para>
+/// An endpoint that requires authentication does not belong here - it goes on
+/// <see cref="IAccountApi"/>, which is registered with the handler attached. Adding one here would
+/// ship a call with no <c>Authorization</c> header against an <c>[Authorize]</c> endpoint, which is
+/// exactly how change-password and logout came to be silently broken.
+/// </para>
+/// </summary>
 public interface IAuthApi
 {
     [Post("/api/v1/auth/register")]
@@ -14,9 +25,6 @@ public interface IAuthApi
     [Post("/api/v1/auth/refresh")]
     Task<IApiResponse<AuthTokenResponse>> RefreshAsync([Body] RefreshRequest request, CancellationToken cancellationToken = default);
 
-    [Post("/api/v1/auth/logout")]
-    Task<IApiResponse> LogoutAsync([Body] RefreshRequest request, CancellationToken cancellationToken = default);
-
     /// <summary>
     /// Requests a reset code. Answers 200 whether or not the address has an account - the server
     /// will not say which, so the client must not imply it either.
@@ -30,11 +38,4 @@ public interface IAuthApi
     /// </summary>
     [Post("/api/v1/auth/reset-password")]
     Task<IApiResponse> ResetPasswordAsync([Body] ResetPasswordRequest request, CancellationToken cancellationToken = default);
-
-    /// <summary>
-    /// Changes the signed-in user's password. Returns a fresh token pair, because succeeding
-    /// revokes every refresh token the account holds - including this device's.
-    /// </summary>
-    [Post("/api/v1/auth/change-password")]
-    Task<IApiResponse<AuthTokenResponse>> ChangePasswordAsync([Body] ChangePasswordRequest request, CancellationToken cancellationToken = default);
 }