From cb8cc67c2e05cf3295705863b80ec84fd75040d4 Mon Sep 17 00:00:00 2001 From: RghvGrv Date: Sat, 15 Aug 2026 00:56:12 +0530 Subject: [PATCH 1/7] fix(mobile): send a token on change-password and logout Both endpoints are [Authorize], but both lived on IAuthApi, which is registered without AuthDelegatingHandler so it never attaches an Authorization header. Change-password therefore answered 401 every time. SettingsViewModel maps 401 to "Session expired, please log in again.", so the user re-logged in, retried, and got the same message forever - the feature has never worked from the app. Logout was worse: it returns IApiResponse, which does not throw on a non-success status, so the 401 sailed past the catch and read exactly like a successful revoke. The client cleared its local tokens while the refresh token stayed live server-side for its full 30-day lifetime. Splits the two authenticated calls onto a new IAccountApi registered with the handler attached. IAuthApi keeps the anonymous endpoints and stays handler-free, preserving the property that a 401 during refresh cannot recurse into refresh. Adds RefitClientCompositionTests, which builds the real container and sends a real request through each client into a capturing primary handler. It asserts every client with [Authorize] endpoints carries a bearer token and that IAuthApi does not. Verified to fail when a handler registration is removed - this is the check whose absence let both defects ship, since the mobile tests substitute fakes and the API tests supply a token directly. Closes #185 Closes #186 Closes #187 --- src/SubVora.Mobile/Api/IAccountApi.cs | 39 +++++ src/SubVora.Mobile/Api/IAuthApi.cs | 21 +-- src/SubVora.Mobile/MauiProgram.cs | 109 ++++++++----- .../ViewModels/SettingsViewModel.cs | 46 +++++- .../BurnRateBannerTests.cs | 4 +- .../ChangePasswordViewModelTests.cs | 39 +++-- .../CurrencyPickerTests.cs | 2 +- .../Fakes/FakeAccountApi.cs | 38 +++++ .../SubVora.Mobile.Tests/Fakes/FakeAuthApi.cs | 20 --- .../OfflineFailureTests.cs | 10 +- .../OfflineWriteGuardTests.cs | 2 +- .../RefitClientCompositionTests.cs | 152 ++++++++++++++++++ .../SettingsViewModelTests.cs | 81 +++++++++- .../TabSwitchReloadTests.cs | 2 +- .../SubVora.Mobile.Tests/ThemeChoiceTests.cs | 2 +- 15 files changed, 462 insertions(+), 105 deletions(-) create mode 100644 src/SubVora.Mobile/Api/IAccountApi.cs create mode 100644 tests/SubVora.Mobile.Tests/Fakes/FakeAccountApi.cs create mode 100644 tests/SubVora.Mobile.Tests/RefitClientCompositionTests.cs diff --git a/src/SubVora.Mobile/Api/IAccountApi.cs b/src/SubVora.Mobile/Api/IAccountApi.cs new file mode 100644 index 0000000..dbe4db8 --- /dev/null +++ b/src/SubVora.Mobile/Api/IAccountApi.cs @@ -0,0 +1,39 @@ +using Refit; +using SubVora.Mobile.Api.Dtos; + +namespace SubVora.Mobile.Api; + +/// +/// The auth endpoints that require a bearer token, split out from so they can +/// be registered with AuthDelegatingHandler attached. +/// +/// The split exists because must not chain that handler - it carries +/// /auth/refresh, and a 401 during refresh would recurse straight back into refresh. Leaving +/// these two calls there meant they went out with no Authorization header at all, against +/// endpoints the API marks [Authorize]: 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. +/// +/// +/// Anything added here must be an endpoint that requires authentication and is not itself part of +/// the refresh path. RefitClientCompositionTests asserts this interface is registered with the +/// handler and that is not. +/// +/// +public interface IAccountApi +{ + /// + /// 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. + /// + [Post("/api/v1/auth/change-password")] + Task> ChangePasswordAsync([Body] ChangePasswordRequest request, CancellationToken cancellationToken = default); + + /// + /// 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. + /// + [Post("/api/v1/auth/logout")] + Task LogoutAsync([Body] RefreshRequest request, CancellationToken cancellationToken = default); +} diff --git a/src/SubVora.Mobile/Api/IAuthApi.cs b/src/SubVora.Mobile/Api/IAuthApi.cs index 22ed517..8fb408a 100644 --- a/src/SubVora.Mobile/Api/IAuthApi.cs +++ b/src/SubVora.Mobile/Api/IAuthApi.cs @@ -3,6 +3,17 @@ namespace SubVora.Mobile.Api; +/// +/// The auth endpoints that take no bearer token. Registered without +/// AuthDelegatingHandler on purpose: this interface carries /auth/refresh, and chaining +/// the handler here would let a 401 during refresh recurse back into refresh. +/// +/// An endpoint that requires authentication does not belong here - it goes on +/// , which is registered with the handler attached. Adding one here would +/// ship a call with no Authorization header against an [Authorize] endpoint, which is +/// exactly how change-password and logout came to be silently broken. +/// +/// public interface IAuthApi { [Post("/api/v1/auth/register")] @@ -14,9 +25,6 @@ public interface IAuthApi [Post("/api/v1/auth/refresh")] Task> RefreshAsync([Body] RefreshRequest request, CancellationToken cancellationToken = default); - [Post("/api/v1/auth/logout")] - Task LogoutAsync([Body] RefreshRequest request, CancellationToken cancellationToken = default); - /// /// 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 /// [Post("/api/v1/auth/reset-password")] Task ResetPasswordAsync([Body] ResetPasswordRequest request, CancellationToken cancellationToken = default); - - /// - /// 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. - /// - [Post("/api/v1/auth/change-password")] - Task> ChangePasswordAsync([Body] ChangePasswordRequest request, CancellationToken cancellationToken = default); } diff --git a/src/SubVora.Mobile/MauiProgram.cs b/src/SubVora.Mobile/MauiProgram.cs index 99de784..2d81fe2 100644 --- a/src/SubVora.Mobile/MauiProgram.cs +++ b/src/SubVora.Mobile/MauiProgram.cs @@ -3,6 +3,7 @@ using CommunityToolkit.Maui; using CommunityToolkit.Mvvm.Messaging; using Plugin.LocalNotification; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Refit; using SubVora.Mobile.Api; @@ -39,21 +40,42 @@ public static MauiApp CreateMauiApp() builder.Logging.AddDebug(); #endif + AddSubVoraServices(builder.Services); + + return builder.Build(); + } + + /// + /// Every service registration the app makes, split from so it can be + /// composed into a plain without a MAUI host. + /// + /// That is what lets RefitClientCompositionTests assert how the Refit clients are wired - + /// specifically that each one whose endpoints require authentication carries + /// . Nothing else asserted that, which is how change-password + /// and logout shipped calling [Authorize] endpoints with no token at all. + /// + /// + /// A pure move: the order and content of the registrations below are exactly what + /// used to perform inline. + /// + /// + public static void AddSubVoraServices(IServiceCollection services) + { // One messenger for the whole app: the burn-rate banner listens for subscription changes // published by the detail, list and settings view models. - builder.Services.AddSingleton(WeakReferenceMessenger.Default); + services.AddSingleton(WeakReferenceMessenger.Default); - builder.Services.AddSingleton(); + services.AddSingleton(); - builder.Services.AddSingleton(); + services.AddSingleton(); - builder.Services.AddSingleton(); + services.AddSingleton(); - builder.Services.AddSingleton(); + services.AddSingleton(); - builder.Services.AddSingleton(); + services.AddSingleton(); - builder.Services.AddSingleton(_ => + services.AddSingleton(_ => new SqliteLocalCacheService(Path.Combine(FileSystem.AppDataDirectory, "subvora_cache.db3"))); var refitSettings = new RefitSettings @@ -67,73 +89,80 @@ public static MauiApp CreateMauiApp() // Plain HttpClient with no AuthDelegatingHandler attached, used only to call the // refresh endpoint so a 401 during refresh can never recurse back into the handler. - builder.Services.AddHttpClient("AuthRefresh", client => + services.AddHttpClient("AuthRefresh", client => { client.BaseAddress = new Uri(ApiConfig.BaseAddress); client.Timeout = ApiConfig.RefreshTimeout; }); // Singleton: one refresh lock and one SessionExpired event for the whole app. - builder.Services.AddSingleton(sp => new SessionRefresher( + services.AddSingleton(sp => new SessionRefresher( sp.GetRequiredService(), sp.GetRequiredService().CreateClient("AuthRefresh"), sp.GetRequiredService())); // Transient: HttpClientFactory sets InnerHandler on each instance it is given, so sharing // one across the Refit clients below throws as soon as the second client is built. - builder.Services.AddTransient(sp => new AuthDelegatingHandler( + services.AddTransient(sp => new AuthDelegatingHandler( sp.GetRequiredService(), sp.GetRequiredService())); - // IAuthApi must not chain AuthDelegatingHandler - login/register/refresh calls - // themselves would otherwise loop back through the 401-refresh logic. - builder.Services.AddRefitClient(refitSettings) + // IAuthApi carries only the endpoints that take no bearer token, and must not chain + // AuthDelegatingHandler - login/register/refresh calls themselves would otherwise loop back + // through the 401-refresh logic. + services.AddRefitClient(refitSettings) .ConfigureHttpClient(ConfigureApiClient); - builder.Services.AddRefitClient(refitSettings) + // The auth endpoints that *do* require a token. Safe to chain the handler because neither + // call is /auth/refresh, so the handler's 401 path cannot recurse into refresh. Without this + // registration the calls went out unauthenticated against [Authorize] endpoints - see + // IAccountApi. + services.AddRefitClient(refitSettings) + .ConfigureHttpClient(ConfigureApiClient) + .AddHttpMessageHandler(sp => sp.GetRequiredService()); + + services.AddRefitClient(refitSettings) .ConfigureHttpClient(ConfigureApiClient) .AddHttpMessageHandler(sp => sp.GetRequiredService()); - builder.Services.AddRefitClient(refitSettings) + services.AddRefitClient(refitSettings) .ConfigureHttpClient(ConfigureApiClient) .AddHttpMessageHandler(sp => sp.GetRequiredService()); - builder.Services.AddRefitClient(refitSettings) + services.AddRefitClient(refitSettings) .ConfigureHttpClient(ConfigureApiClient) .AddHttpMessageHandler(sp => sp.GetRequiredService()); - builder.Services.AddRefitClient(refitSettings) + services.AddRefitClient(refitSettings) .ConfigureHttpClient(ConfigureApiClient) .AddHttpMessageHandler(sp => sp.GetRequiredService()); - builder.Services.AddRefitClient(refitSettings) + services.AddRefitClient(refitSettings) .ConfigureHttpClient(ConfigureApiClient) .AddHttpMessageHandler(sp => sp.GetRequiredService()); - builder.Services.AddTransient(); - builder.Services.AddTransient(); - builder.Services.AddTransient(); - builder.Services.AddTransient(); - builder.Services.AddTransient(); - builder.Services.AddTransient(); - builder.Services.AddTransient(); + services.AddTransient(); + services.AddTransient(); + services.AddTransient(); + services.AddTransient(); + services.AddTransient(); + services.AddTransient(); + services.AddTransient(); // Singleton, unlike every other view model: AppShell's banner and DashboardPage bind to // the same instance, so one fetch feeds both and they cannot drift apart. - builder.Services.AddSingleton(); - builder.Services.AddTransient(); - builder.Services.AddTransient(); - builder.Services.AddTransient(); - builder.Services.AddTransient(); - builder.Services.AddTransient(); - builder.Services.AddTransient(); - builder.Services.AddTransient(); - builder.Services.AddTransient(); - builder.Services.AddTransient(); - builder.Services.AddTransient(); - builder.Services.AddTransient(); - builder.Services.AddTransient(); - - return builder.Build(); + services.AddSingleton(); + services.AddTransient(); + services.AddTransient(); + services.AddTransient(); + services.AddTransient(); + services.AddTransient(); + services.AddTransient(); + services.AddTransient(); + services.AddTransient(); + services.AddTransient(); + services.AddTransient(); + services.AddTransient(); + services.AddTransient(); } /// diff --git a/src/SubVora.Mobile/ViewModels/SettingsViewModel.cs b/src/SubVora.Mobile/ViewModels/SettingsViewModel.cs index 0bfe3a4..f04b080 100644 --- a/src/SubVora.Mobile/ViewModels/SettingsViewModel.cs +++ b/src/SubVora.Mobile/ViewModels/SettingsViewModel.cs @@ -1,4 +1,5 @@ -using CommunityToolkit.Mvvm.ComponentModel; +using System.Net; +using CommunityToolkit.Mvvm.ComponentModel; using CommunityToolkit.Mvvm.Input; using CommunityToolkit.Mvvm.Messaging; using Refit; @@ -13,7 +14,11 @@ namespace SubVora.Mobile.ViewModels; public partial class SettingsViewModel : ObservableObject { private readonly IUsersApi _usersApi; - private readonly IAuthApi _authApi; + + // Change-password and logout both need a bearer token, so they live on IAccountApi, which is + // registered with AuthDelegatingHandler attached. This view model has no anonymous auth calls + // left, so it does not take IAuthApi at all. + private readonly IAccountApi _accountApi; private readonly ITokenStore _tokenStore; private readonly ILocalCacheService _localCacheService; private readonly IUserPrompt _userPrompt; @@ -110,10 +115,23 @@ partial void OnPreferredCurrencyChanged(string value) /// Raised after the password changes, so the view can confirm it. public event EventHandler? PasswordChanged; - public SettingsViewModel(IUsersApi usersApi, IAuthApi authApi, ITokenStore tokenStore, ILocalCacheService localCacheService, IUserPrompt userPrompt, IMessenger messenger, IThemeService themeService, IConnectivityService connectivity) + /// + /// Raised when the server refused the sign-out revoke. The local session still ends - that is + /// unconditional - but this says the refresh token may still be live server-side, which is the + /// one part of signing out the device cannot do for itself. + /// + /// An event rather than a log line because this class has no logger and the two existing + /// outcomes here (, ) are already events. + /// Nothing subscribes yet; it exists so the failure is observable at all, since the whole defect + /// was that a refusal was indistinguishable from a success. + /// + /// + public event EventHandler? LogoutRevokeFailed; + + public SettingsViewModel(IUsersApi usersApi, IAccountApi accountApi, ITokenStore tokenStore, ILocalCacheService localCacheService, IUserPrompt userPrompt, IMessenger messenger, IThemeService themeService, IConnectivityService connectivity) { _usersApi = usersApi; - _authApi = authApi; + _accountApi = accountApi; _tokenStore = tokenStore; _localCacheService = localCacheService; _userPrompt = userPrompt; @@ -239,7 +257,7 @@ private async Task ChangePasswordAsync() IsBusy = true; try { - var response = await _authApi.ChangePasswordAsync(new ChangePasswordRequest + var response = await _accountApi.ChangePasswordAsync(new ChangePasswordRequest { CurrentPassword = CurrentPassword, NewPassword = NewPassword, @@ -249,6 +267,13 @@ private async Task ChangePasswordAsync() { // 400 covers both a wrong current password and a new one that fails validation; // the server's own message distinguishes them, so surface that rather than guess. + // + // A 401 maps to "session expired", which is only truthful because the call now goes + // through IAccountApi: the handler attaches the token and retries once on 401, and + // SessionRefresher ends the session if that refresh fails. Reaching here with a 401 + // therefore does mean the session is gone. On IAuthApi - no handler, no token - the + // endpoint answered 401 unconditionally and that same message sent every user round + // a re-login loop that could never succeed. PasswordErrorMessage = ApiErrorMapper.ToDisplayMessage(response); return; } @@ -288,7 +313,16 @@ private async Task SignOutAsync() { try { - await _authApi.LogoutAsync(new RefreshRequest { RefreshToken = refreshToken }); + var response = await _accountApi.LogoutAsync(new RefreshRequest { RefreshToken = refreshToken }); + + // Observed, not assumed. LogoutAsync returns IApiResponse, which does not throw on a + // non-success status, so a refusal used to sail past the catch below and read + // exactly like a successful revoke. That is how logout came to clear the local + // tokens while leaving the refresh token live on the server for its full 30 days. + if (!response.IsSuccessStatusCode) + { + LogoutRevokeFailed?.Invoke(this, response.StatusCode); + } } catch (Exception ex) when (ApiErrorMapper.IsApiFailure(ex)) { diff --git a/tests/SubVora.Mobile.Tests/BurnRateBannerTests.cs b/tests/SubVora.Mobile.Tests/BurnRateBannerTests.cs index 12b2b8b..3f69972 100644 --- a/tests/SubVora.Mobile.Tests/BurnRateBannerTests.cs +++ b/tests/SubVora.Mobile.Tests/BurnRateBannerTests.cs @@ -190,7 +190,7 @@ public async Task ChangingTheHomeCurrency_PublishesTheRefreshMessage() var viewModel = new SettingsViewModel( new FakeUsersApi(), - new FakeAuthApi(), + new FakeAccountApi(), new FakeTokenStore(), new FakeLocalCacheService(), new FakeUserPrompt(), @@ -215,7 +215,7 @@ public async Task SigningOut_PublishesTheSessionEndedMessage() var viewModel = new SettingsViewModel( new FakeUsersApi(), - new FakeAuthApi(), + new FakeAccountApi(), new FakeTokenStore(), new FakeLocalCacheService(), new FakeUserPrompt { ConfirmResult = true }, diff --git a/tests/SubVora.Mobile.Tests/ChangePasswordViewModelTests.cs b/tests/SubVora.Mobile.Tests/ChangePasswordViewModelTests.cs index 5ff8c82..92e6981 100644 --- a/tests/SubVora.Mobile.Tests/ChangePasswordViewModelTests.cs +++ b/tests/SubVora.Mobile.Tests/ChangePasswordViewModelTests.cs @@ -7,10 +7,10 @@ namespace SubVora.Mobile.Tests; public class ChangePasswordViewModelTests { - private static SettingsViewModel CreateViewModel(FakeAuthApi authApi, FakeTokenStore tokenStore, FakeConnectivityService? connectivity = null) => + private static SettingsViewModel CreateViewModel(FakeAccountApi accountApi, FakeTokenStore tokenStore, FakeConnectivityService? connectivity = null) => new( new FakeUsersApi(), - authApi, + accountApi, tokenStore, new FakeLocalCacheService(), new FakeUserPrompt(), @@ -25,7 +25,7 @@ public async Task ChangePassword_OnSuccess_StoresTheReplacementTokens() // storing the replacement pair the next 401 signs the user out of the phone they are // holding - the change looking like it broke the app. var tokenStore = new FakeTokenStore(); - var viewModel = CreateViewModel(new FakeAuthApi(), tokenStore); + var viewModel = CreateViewModel(new FakeAccountApi(), tokenStore); viewModel.CurrentPassword = "correct-horse-battery-staple"; // pragma: allowlist secret viewModel.NewPassword = "an-entirely-different-passphrase"; // pragma: allowlist secret @@ -35,10 +35,29 @@ public async Task ChangePassword_OnSuccess_StoresTheReplacementTokens() Assert.Equal(FakeAuthApi.SampleTokens().RefreshToken, tokenStore.RefreshToken); } + [Fact] + public async Task ChangePassword_GoesThroughTheClientThatCarriesABearerToken() + { + // The whole defect: change-password lived on IAuthApi, which is registered without + // AuthDelegatingHandler, so the call went out with no Authorization header against an + // [Authorize] endpoint and answered 401 every time. Asserting the call lands on IAccountApi + // is what pins it to the client that attaches the token. + var accountApi = new FakeAccountApi(); + var viewModel = CreateViewModel(accountApi, new FakeTokenStore()); + viewModel.CurrentPassword = "correct-horse-battery-staple"; // pragma: allowlist secret + viewModel.NewPassword = "an-entirely-different-passphrase"; // pragma: allowlist secret + + await viewModel.ChangePasswordCommand.ExecuteAsync(null); + + var call = Assert.Single(accountApi.ChangePasswordCalls); + Assert.Equal("correct-horse-battery-staple", call.CurrentPassword); // pragma: allowlist secret + Assert.Equal("an-entirely-different-passphrase", call.NewPassword); // pragma: allowlist secret + } + [Fact] public async Task ChangePassword_OnSuccess_ClearsTheFieldsAndConfirms() { - var viewModel = CreateViewModel(new FakeAuthApi(), new FakeTokenStore()); + var viewModel = CreateViewModel(new FakeAccountApi(), new FakeTokenStore()); viewModel.CurrentPassword = "correct-horse-battery-staple"; // pragma: allowlist secret viewModel.NewPassword = "an-entirely-different-passphrase"; // pragma: allowlist secret @@ -57,7 +76,7 @@ public async Task ChangePassword_OnSuccess_ClearsTheFieldsAndConfirms() [Fact] public async Task ChangePassword_WithTheWrongCurrentPassword_KeepsTheSessionAndExplains() { - var authApi = new FakeAuthApi + var accountApi = new FakeAccountApi { ChangePasswordHandler = _ => Task.FromResult(FakeAuthApi.CreateResponse( HttpStatusCode.BadRequest, @@ -68,7 +87,7 @@ public async Task ChangePassword_WithTheWrongCurrentPassword_KeepsTheSessionAndE await tokenStore.SaveTokensAsync(FakeAuthApi.SampleTokens()); var existingToken = tokenStore.AccessToken; - var viewModel = CreateViewModel(authApi, tokenStore); + var viewModel = CreateViewModel(accountApi, tokenStore); viewModel.CurrentPassword = "wrong"; // pragma: allowlist secret viewModel.NewPassword = "an-entirely-different-passphrase"; // pragma: allowlist secret @@ -84,11 +103,11 @@ public async Task ChangePassword_KeepsItsMessagesSeparateFromThePreferencesCard( { // Both live on the same screen. A failed password change must not paint the currency card // red, and vice versa. - var authApi = new FakeAuthApi + var accountApi = new FakeAccountApi { ChangePasswordHandler = _ => Task.FromResult(FakeAuthApi.CreateResponse(HttpStatusCode.BadRequest, content: null)), }; - var viewModel = CreateViewModel(authApi, new FakeTokenStore()); + var viewModel = CreateViewModel(accountApi, new FakeTokenStore()); viewModel.CurrentPassword = "wrong"; // pragma: allowlist secret await viewModel.ChangePasswordCommand.ExecuteAsync(null); @@ -100,8 +119,8 @@ public async Task ChangePassword_KeepsItsMessagesSeparateFromThePreferencesCard( [Fact] public async Task ChangePassword_WhenOffline_SaysTheChangeDidNotLand() { - var authApi = new FakeAuthApi { ChangePasswordHandler = _ => throw new HttpRequestException("Connection refused") }; - var viewModel = CreateViewModel(authApi, new FakeTokenStore(), new FakeConnectivityService { IsConnected = false }); + var accountApi = new FakeAccountApi { ChangePasswordHandler = _ => throw new HttpRequestException("Connection refused") }; + var viewModel = CreateViewModel(accountApi, new FakeTokenStore(), new FakeConnectivityService { IsConnected = false }); viewModel.CurrentPassword = "correct-horse-battery-staple"; // pragma: allowlist secret await viewModel.ChangePasswordCommand.ExecuteAsync(null); diff --git a/tests/SubVora.Mobile.Tests/CurrencyPickerTests.cs b/tests/SubVora.Mobile.Tests/CurrencyPickerTests.cs index def7f16..9500577 100644 --- a/tests/SubVora.Mobile.Tests/CurrencyPickerTests.cs +++ b/tests/SubVora.Mobile.Tests/CurrencyPickerTests.cs @@ -15,7 +15,7 @@ public class CurrencyPickerTests private static SettingsViewModel CreateViewModel(FakeUsersApi usersApi) => new( usersApi, - new FakeAuthApi(), + new FakeAccountApi(), new FakeTokenStore(), new FakeLocalCacheService(), new FakeUserPrompt(), diff --git a/tests/SubVora.Mobile.Tests/Fakes/FakeAccountApi.cs b/tests/SubVora.Mobile.Tests/Fakes/FakeAccountApi.cs new file mode 100644 index 0000000..9f60d3e --- /dev/null +++ b/tests/SubVora.Mobile.Tests/Fakes/FakeAccountApi.cs @@ -0,0 +1,38 @@ +using System.Net; +using Refit; +using SubVora.Mobile.Api; +using SubVora.Mobile.Api.Dtos; + +namespace SubVora.Mobile.Tests.Fakes; + +/// +/// Minimal fake IAccountApi for ViewModel tests - no real HTTP involved. +/// +/// Response builders are reused from rather than duplicated, so both fakes +/// construct ApiResponse the same way. +/// +/// +public class FakeAccountApi : IAccountApi +{ + public Func>> ChangePasswordHandler = + _ => Task.FromResult(FakeAuthApi.CreateResponse(HttpStatusCode.OK, FakeAuthApi.SampleTokens())); + + public Func> LogoutHandler = + _ => Task.FromResult(FakeAuthApi.CreateResponse(HttpStatusCode.NoContent)); + + public List ChangePasswordCalls { get; } = []; + + public List LogoutCalls { get; } = []; + + public Task> ChangePasswordAsync(ChangePasswordRequest request, CancellationToken cancellationToken = default) + { + ChangePasswordCalls.Add(request); + return ChangePasswordHandler(request); + } + + public Task LogoutAsync(RefreshRequest request, CancellationToken cancellationToken = default) + { + LogoutCalls.Add(request); + return LogoutHandler(request); + } +} diff --git a/tests/SubVora.Mobile.Tests/Fakes/FakeAuthApi.cs b/tests/SubVora.Mobile.Tests/Fakes/FakeAuthApi.cs index 98906f9..4ad28bf 100644 --- a/tests/SubVora.Mobile.Tests/Fakes/FakeAuthApi.cs +++ b/tests/SubVora.Mobile.Tests/Fakes/FakeAuthApi.cs @@ -23,18 +23,10 @@ public class FakeAuthApi : IAuthApi public Func> ResetPasswordHandler = _ => Task.FromResult(CreateResponse(HttpStatusCode.OK)); - public Func>> ChangePasswordHandler = - _ => Task.FromResult(CreateResponse(HttpStatusCode.OK, SampleTokens())); - - public Func> LogoutHandler = - _ => Task.FromResult(CreateResponse(HttpStatusCode.NoContent)); - public List RegisterCalls { get; } = []; public List LoginCalls { get; } = []; - public List LogoutCalls { get; } = []; public List ForgotPasswordCalls { get; } = []; public List ResetPasswordCalls { get; } = []; - public List ChangePasswordCalls { get; } = []; public Task RegisterAsync(RegisterRequest request, CancellationToken cancellationToken = default) { @@ -51,12 +43,6 @@ public Task> LoginAsync(LoginRequest request, Ca public Task> RefreshAsync(RefreshRequest request, CancellationToken cancellationToken = default) => RefreshHandler(request); - public Task LogoutAsync(RefreshRequest request, CancellationToken cancellationToken = default) - { - LogoutCalls.Add(request); - return LogoutHandler(request); - } - public Task ForgotPasswordAsync(ForgotPasswordRequest request, CancellationToken cancellationToken = default) { ForgotPasswordCalls.Add(request); @@ -69,12 +55,6 @@ public Task ResetPasswordAsync(ResetPasswordRequest request, Cance return ResetPasswordHandler(request); } - public Task> ChangePasswordAsync(ChangePasswordRequest request, CancellationToken cancellationToken = default) - { - ChangePasswordCalls.Add(request); - return ChangePasswordHandler(request); - } - public static AuthTokenResponse SampleTokens() => new() { AccessToken = "sample-access-token", diff --git a/tests/SubVora.Mobile.Tests/OfflineFailureTests.cs b/tests/SubVora.Mobile.Tests/OfflineFailureTests.cs index 8d57eca..d6d1a13 100644 --- a/tests/SubVora.Mobile.Tests/OfflineFailureTests.cs +++ b/tests/SubVora.Mobile.Tests/OfflineFailureTests.cs @@ -119,7 +119,7 @@ public async Task PaymentSources_WhenApiIsUnreachable_ShowsOfflineMessageInstead public async Task Settings_WhenApiIsUnreachable_ShowsOfflineMessageInsteadOfCrashing() { var usersApi = new FakeUsersApi { GetMeHandler = () => throw Unreachable() }; - var viewModel = BuildSettingsViewModel(usersApi, new FakeAuthApi(), new FakeTokenStore()); + var viewModel = BuildSettingsViewModel(usersApi, new FakeAccountApi(), new FakeTokenStore()); await viewModel.LoadCommand.ExecuteAsync(null); @@ -132,11 +132,11 @@ public async Task SignOut_WhenApiIsUnreachable_StillEndsTheLocalSession() { // Signing out with no connection is the most likely case of all: the server-side revoke is // best-effort, but the local session must go either way. - var authApi = new FakeAuthApi { LogoutHandler = _ => throw Unreachable() }; + var accountApi = new FakeAccountApi { LogoutHandler = _ => throw Unreachable() }; var tokenStore = new FakeTokenStore(); await tokenStore.SaveTokensAsync(FakeAuthApi.SampleTokens()); - var viewModel = BuildSettingsViewModel(new FakeUsersApi(), authApi, tokenStore); + var viewModel = BuildSettingsViewModel(new FakeUsersApi(), accountApi, tokenStore); var signedOut = false; viewModel.SignedOut += (_, _) => signedOut = true; @@ -150,11 +150,11 @@ public async Task SignOut_WhenApiIsUnreachable_StillEndsTheLocalSession() private static SettingsViewModel BuildSettingsViewModel( FakeUsersApi usersApi, - FakeAuthApi authApi, + FakeAccountApi accountApi, FakeTokenStore tokenStore) => new( usersApi, - authApi, + accountApi, tokenStore, new FakeLocalCacheService(), new FakeUserPrompt { ConfirmResult = true }, diff --git a/tests/SubVora.Mobile.Tests/OfflineWriteGuardTests.cs b/tests/SubVora.Mobile.Tests/OfflineWriteGuardTests.cs index 9c857d6..5f7d06b 100644 --- a/tests/SubVora.Mobile.Tests/OfflineWriteGuardTests.cs +++ b/tests/SubVora.Mobile.Tests/OfflineWriteGuardTests.cs @@ -16,7 +16,7 @@ public class OfflineWriteGuardTests private static SettingsViewModel Settings(FakeConnectivityService connectivity, FakeUsersApi? usersApi = null) => new( usersApi ?? new FakeUsersApi(), - new FakeAuthApi(), + new FakeAccountApi(), new FakeTokenStore(), new FakeLocalCacheService(), new FakeUserPrompt(), diff --git a/tests/SubVora.Mobile.Tests/RefitClientCompositionTests.cs b/tests/SubVora.Mobile.Tests/RefitClientCompositionTests.cs new file mode 100644 index 0000000..54169b7 --- /dev/null +++ b/tests/SubVora.Mobile.Tests/RefitClientCompositionTests.cs @@ -0,0 +1,152 @@ +using System.Net; +using Microsoft.Extensions.DependencyInjection; +using SubVora.Mobile; +using SubVora.Mobile.Api; +using SubVora.Mobile.Api.Dtos; +using SubVora.Mobile.Services; +using SubVora.Mobile.Tests.Fakes; + +namespace SubVora.Mobile.Tests; + +/// +/// How the Refit clients are composed, asserted against the real container rather than a fake. +/// +/// This is the check whose absence let two defects ship at once: change-password and +/// logout both lived on , which is registered without +/// , so both called [Authorize] endpoints with no +/// Authorization header. Change-password answered 401 every time and was reported to the user as an +/// expired session; logout's revoke silently never happened while the client cleared its tokens and +/// moved on, leaving the refresh token live server-side for its full 30 days. +/// +/// +/// Every other mobile test substitutes a fake API, and the API tests call the endpoints directly +/// with a token, so nothing exercised the wiring itself. These tests do, by sending a real request +/// through the real handler pipeline into a capturing primary handler. +/// +/// +public class RefitClientCompositionTests +{ + private const string StoredAccessToken = "stored-access-token"; + + /// Captures the outbound request and answers with an empty JSON body. + private sealed class CapturingPrimaryHandler : HttpMessageHandler + { + public HttpRequestMessage? LastRequest { get; private set; } + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + LastRequest = request; + + return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) + { + RequestMessage = request, + // Deserializes as an empty list or a null object, whichever the method returns. + Content = new StringContent("[]", System.Text.Encoding.UTF8, "application/json"), + }); + } + } + + /// + /// The app's own registrations, with the two platform-backed services swapped for fakes so the + /// container can be built off-device. Everything about the HTTP pipeline is left exactly as + /// composes it - that is what is under test. + /// + private static (ServiceProvider Provider, CapturingPrimaryHandler Handler) BuildProvider(string? accessToken = StoredAccessToken) + { + var services = new ServiceCollection(); + MauiProgram.AddSubVoraServices(services); + + // SecureStorage and FileSystem.AppDataDirectory both need a packaged app identity. Replacing + // them is not weakening the test: neither is part of how a client is wired. + services.AddSingleton(new FakeTokenStore { AccessToken = accessToken, RefreshToken = "stored-refresh-token" }); + services.AddSingleton(new FakeLocalCacheService()); + + var capturing = new CapturingPrimaryHandler(); + + // ConfigureAll applies to every named HttpClient, so this replaces the socket layer for all + // of them without naming any - and without touching the DelegatingHandler chain above it, + // which is the part being asserted. + services.ConfigureAll(options => + options.HttpMessageHandlerBuilderActions.Add(builder => builder.PrimaryHandler = capturing)); + + return (services.BuildServiceProvider(), capturing); + } + + public static TheoryData AuthenticatedClients() => + new(nameof(IAccountApi), nameof(IUsersApi), nameof(ISubscriptionsApi), nameof(ICategoriesApi), nameof(IPaymentSourcesApi), nameof(IDashboardApi)); + + /// + /// One call per client that talks to [Authorize] endpoints. Each must arrive carrying the + /// stored bearer token. + /// + [Theory] + [MemberData(nameof(AuthenticatedClients))] + public async Task EveryAuthenticatedClient_SendsTheStoredBearerToken(string clientName) + { + var (provider, capturing) = BuildProvider(); + await using var _ = provider; + + await CallAsync(provider, clientName); + + Assert.NotNull(capturing.LastRequest); + Assert.NotNull(capturing.LastRequest!.Headers.Authorization); + Assert.Equal("Bearer", capturing.LastRequest.Headers.Authorization!.Scheme); + Assert.Equal(StoredAccessToken, capturing.LastRequest.Headers.Authorization.Parameter); + } + + [Fact] + public async Task IAuthApi_SendsNoBearerToken() + { + // The negative half, and it is load-bearing: without it, "attach the handler to everything" + // would pass. IAuthApi carries /auth/refresh, and chaining AuthDelegatingHandler there would + // let a 401 during refresh recurse straight back into refresh. + var (provider, capturing) = BuildProvider(); + await using var _ = provider; + + await provider.GetRequiredService() + .LoginAsync(new LoginRequest { Email = "someone@example.com", Password = "irrelevant" }); // pragma: allowlist secret + + Assert.NotNull(capturing.LastRequest); + Assert.Null(capturing.LastRequest!.Headers.Authorization); + } + + [Fact] + public async Task AnAuthenticatedClient_WithNoStoredToken_SendsNoAuthorizationHeader() + { + // Signed out, so there is nothing to attach. Proves the assertion above is reading the token + // the store actually holds rather than a header that is always present. + var (provider, capturing) = BuildProvider(accessToken: null); + await using var _ = provider; + + await CallAsync(provider, nameof(IUsersApi)); + + Assert.NotNull(capturing.LastRequest); + Assert.Null(capturing.LastRequest!.Headers.Authorization); + } + + /// + /// Issues one real call per client. What comes back is deliberately ignored: the canned body + /// does not fit every return type, and the assertion is about the request that went out, not the + /// response that came back. The request is captured before any deserialization is attempted. + /// + private static async Task CallAsync(IServiceProvider provider, string clientName) + { + try + { + await (clientName switch + { + nameof(IAccountApi) => provider.GetRequiredService().LogoutAsync(new RefreshRequest { RefreshToken = "stored-refresh-token" }), + nameof(IUsersApi) => (Task)provider.GetRequiredService().GetMeAsync(), + nameof(ISubscriptionsApi) => provider.GetRequiredService().GetAllAsync(), + nameof(ICategoriesApi) => provider.GetRequiredService().GetAllAsync(CancellationToken.None), + nameof(IPaymentSourcesApi) => provider.GetRequiredService().GetAllAsync(CancellationToken.None), + nameof(IDashboardApi) => provider.GetRequiredService().GetBurnRateAsync(), + _ => throw new ArgumentOutOfRangeException(nameof(clientName), clientName, "Unknown client - add it to CallAsync and to AuthenticatedClients."), + }); + } + catch (Exception ex) when (ex is not ArgumentOutOfRangeException) + { + // Response-shape noise only. A wiring failure shows up as a missing header, not here. + } + } +} diff --git a/tests/SubVora.Mobile.Tests/SettingsViewModelTests.cs b/tests/SubVora.Mobile.Tests/SettingsViewModelTests.cs index 516bd01..9fe3d22 100644 --- a/tests/SubVora.Mobile.Tests/SettingsViewModelTests.cs +++ b/tests/SubVora.Mobile.Tests/SettingsViewModelTests.cs @@ -11,7 +11,7 @@ public class SettingsViewModelTests { private static SettingsViewModel CreateViewModel( FakeUsersApi? usersApi = null, - FakeAuthApi? authApi = null, + FakeAccountApi? accountApi = null, FakeTokenStore? tokenStore = null, FakeLocalCacheService? cache = null, FakeUserPrompt? userPrompt = null, @@ -19,7 +19,7 @@ private static SettingsViewModel CreateViewModel( FakeThemeService? themeService = null) => new( usersApi ?? new FakeUsersApi(), - authApi ?? new FakeAuthApi(), + accountApi ?? new FakeAccountApi(), tokenStore ?? new FakeTokenStore(), cache ?? new FakeLocalCacheService(), userPrompt ?? new FakeUserPrompt(), @@ -89,9 +89,9 @@ public async Task SignOutAsync_WhenConfirmed_ClearsTokenStoreAndCacheThenRaisesS var cache = new FakeLocalCacheService(); await cache.UpsertAsync(new CachedBurnRate { Weekly = 10 }); await cache.UpsertAsync(new CachedSubscription { Id = Guid.NewGuid(), CustomName = "Netflix" }); - var authApi = new FakeAuthApi(); + var accountApi = new FakeAccountApi(); var userPrompt = new FakeUserPrompt { ConfirmResult = true }; - var viewModel = CreateViewModel(authApi: authApi, tokenStore: tokenStore, cache: cache, userPrompt: userPrompt); + var viewModel = CreateViewModel(accountApi: accountApi, tokenStore: tokenStore, cache: cache, userPrompt: userPrompt); var raised = false; viewModel.SignedOut += (_, _) => raised = true; @@ -100,18 +100,83 @@ public async Task SignOutAsync_WhenConfirmed_ClearsTokenStoreAndCacheThenRaisesS Assert.True(raised); Assert.True(tokenStore.Cleared); - Assert.Single(authApi.LogoutCalls); + Assert.Single(accountApi.LogoutCalls); Assert.Empty(await cache.GetAllAsync()); Assert.Empty(await cache.GetAllAsync()); } + [Fact] + public async Task SignOutAsync_RevokesThroughTheClientThatCarriesABearerToken() + { + // /auth/logout is [Authorize]. On IAuthApi - registered without AuthDelegatingHandler - the + // call went out with no token, answered 401, and IApiResponse does not throw, so the refusal + // was indistinguishable from a successful revoke. The refresh token then stayed live server + // side for its full 30 days after the user had explicitly signed out. + var tokenStore = new FakeTokenStore { AccessToken = "access", RefreshToken = "refresh" }; + var accountApi = new FakeAccountApi(); + var viewModel = CreateViewModel( + accountApi: accountApi, + tokenStore: tokenStore, + userPrompt: new FakeUserPrompt { ConfirmResult = true }); + + await viewModel.SignOutCommand.ExecuteAsync(null); + + var call = Assert.Single(accountApi.LogoutCalls); + Assert.Equal("refresh", call.RefreshToken); + } + + [Fact] + public async Task SignOutAsync_WhenTheRevokeIsRefused_StillEndsTheLocalSessionAndSaysSo() + { + // The local session ends unconditionally - that part was never in doubt. What is new is that + // a refusal is observed rather than silently treated as success. + var tokenStore = new FakeTokenStore { AccessToken = "access", RefreshToken = "refresh" }; + var accountApi = new FakeAccountApi + { + LogoutHandler = _ => Task.FromResult(FakeAuthApi.CreateResponse(HttpStatusCode.Unauthorized)), + }; + var viewModel = CreateViewModel( + accountApi: accountApi, + tokenStore: tokenStore, + userPrompt: new FakeUserPrompt { ConfirmResult = true }); + + HttpStatusCode? refusedWith = null; + viewModel.LogoutRevokeFailed += (_, status) => refusedWith = status; + + var signedOut = false; + viewModel.SignedOut += (_, _) => signedOut = true; + + await viewModel.SignOutCommand.ExecuteAsync(null); + + Assert.Equal(HttpStatusCode.Unauthorized, refusedWith); + Assert.True(signedOut); + Assert.True(tokenStore.Cleared); + } + + [Fact] + public async Task SignOutAsync_WhenTheRevokeSucceeds_RaisesNoFailure() + { + var accountApi = new FakeAccountApi(); + var viewModel = CreateViewModel( + accountApi: accountApi, + tokenStore: new FakeTokenStore { AccessToken = "access", RefreshToken = "refresh" }, + userPrompt: new FakeUserPrompt { ConfirmResult = true }); + + var failed = false; + viewModel.LogoutRevokeFailed += (_, _) => failed = true; + + await viewModel.SignOutCommand.ExecuteAsync(null); + + Assert.False(failed); + } + [Fact] public async Task SignOutAsync_WhenDeclined_MakesNoChanges() { var tokenStore = new FakeTokenStore { AccessToken = "access", RefreshToken = "refresh" }; - var authApi = new FakeAuthApi(); + var accountApi = new FakeAccountApi(); var userPrompt = new FakeUserPrompt { ConfirmResult = false }; - var viewModel = CreateViewModel(authApi: authApi, tokenStore: tokenStore, userPrompt: userPrompt); + var viewModel = CreateViewModel(accountApi: accountApi, tokenStore: tokenStore, userPrompt: userPrompt); var raised = false; viewModel.SignedOut += (_, _) => raised = true; @@ -120,6 +185,6 @@ public async Task SignOutAsync_WhenDeclined_MakesNoChanges() Assert.False(raised); Assert.False(tokenStore.Cleared); - Assert.Empty(authApi.LogoutCalls); + Assert.Empty(accountApi.LogoutCalls); } } diff --git a/tests/SubVora.Mobile.Tests/TabSwitchReloadTests.cs b/tests/SubVora.Mobile.Tests/TabSwitchReloadTests.cs index 75c7b0a..a52f13d 100644 --- a/tests/SubVora.Mobile.Tests/TabSwitchReloadTests.cs +++ b/tests/SubVora.Mobile.Tests/TabSwitchReloadTests.cs @@ -273,7 +273,7 @@ public async Task Settings_SecondAppearance_DoesNotRefetchTheProfile() var messenger = new WeakReferenceMessenger(); var viewModel = new SettingsViewModel( usersApi, - new FakeAuthApi(), + new FakeAccountApi(), new FakeTokenStore(), new FakeLocalCacheService(), new FakeUserPrompt(), diff --git a/tests/SubVora.Mobile.Tests/ThemeChoiceTests.cs b/tests/SubVora.Mobile.Tests/ThemeChoiceTests.cs index 1a8b70f..dc59fbc 100644 --- a/tests/SubVora.Mobile.Tests/ThemeChoiceTests.cs +++ b/tests/SubVora.Mobile.Tests/ThemeChoiceTests.cs @@ -14,7 +14,7 @@ public class ThemeChoiceTests private static SettingsViewModel CreateViewModel(FakeThemeService themeService) => new( new FakeUsersApi(), - new FakeAuthApi(), + new FakeAccountApi(), new FakeTokenStore(), new FakeLocalCacheService(), new FakeUserPrompt(), From 6dc26b1ac2d889373bc8a039dccd259bd5a79384 Mon Sep 17 00:00:00 2001 From: RghvGrv Date: Sat, 15 Aug 2026 00:56:24 +0530 Subject: [PATCH 2/7] fix: isolate FX refresh failures per currency Rates accumulated into one list and were upserted only after the loop, so a single unsupported pair or one transient 5xx threw straight past the upsert and discarded every rate already fetched in that pass. One user tracking an exotic currency aged everybody else's totals by a day. Each base currency now fails on its own and the pass continues. A total outage still logs at Error rather than a handful of warnings, so isolating per-currency failures does not quietly downgrade the signal for the case that used to throw. Closes #188 --- .../FxRateRefreshBackgroundService.cs | 40 +++++++++- .../FxRateRefreshJobTests.cs | 75 ++++++++++++++++++- 2 files changed, 111 insertions(+), 4 deletions(-) diff --git a/src/SubVora.Infrastructure/Currency/FxRateRefreshBackgroundService.cs b/src/SubVora.Infrastructure/Currency/FxRateRefreshBackgroundService.cs index 5825e86..22b1817 100644 --- a/src/SubVora.Infrastructure/Currency/FxRateRefreshBackgroundService.cs +++ b/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(); + 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); + } + } } } diff --git a/tests/SubVora.Infrastructure.Tests/FxRateRefreshJobTests.cs b/tests/SubVora.Infrastructure.Tests/FxRateRefreshJobTests.cs index 196cbe6..2748ed6 100644 --- a/tests/SubVora.Infrastructure.Tests/FxRateRefreshJobTests.cs +++ b/tests/SubVora.Infrastructure.Tests/FxRateRefreshJobTests.cs @@ -113,14 +113,61 @@ public async Task FxRateRefreshJob_ClientThrows_LeavesPreviouslyCachedRatesUncha await BuildService(new FakeExchangeRateClient(0.90m)).RefreshOnceAsync(); - await Assert.ThrowsAsync( - () => BuildService(new ThrowingExchangeRateClient()).RefreshOnceAsync()); + // No longer throws: failures are isolated per base currency so one bad pair cannot discard + // the pass. A total outage still writes nothing, which is what this test has always been + // about - the previously cached rate must survive untouched either way. + await BuildService(new ThrowingExchangeRateClient()).RefreshOnceAsync(); var rate = await _dbContext.FxRates.AsNoTracking() .SingleAsync(r => r.BaseCurrency == "USD" && r.TargetCurrency == "EUR"); Assert.Equal(0.90m, rate.Rate); } + [Fact] + public async Task FxRateRefreshJob_WhenOneBaseCurrencyFails_StillUpsertsTheRest() + { + // The defect: rates accumulated into one list and were upserted only after the loop, so a + // single unsupported pair or one transient 5xx threw past the upsert and discarded every + // rate already fetched. One user tracking something exotic aged everybody's totals. + var user = new User + { + Email = $"fx-partial-{Guid.NewGuid()}@example.com", + PasswordHash = "not-a-real-hash", + PreferredCurrency = "EUR", + CreatedAt = DateTimeOffset.UtcNow, + }; + _dbContext.Users.Add(user); + await _dbContext.SaveChangesAsync(); + + foreach (var currency in new[] { "USD", "ZZZ" }) + { + _dbContext.UserSubscriptions.Add(new UserSubscription + { + UserId = user.Id, + CustomName = $"{currency} Subscription", + CostAmount = 10m, + Currency = currency, + CycleCadence = BillingCycleType.Monthly, + PurchaseDate = new DateOnly(2026, 1, 1), + NextBillingDate = new DateOnly(2026, 2, 1), + AlertDaysAdvance = 3, + CreatedAt = DateTimeOffset.UtcNow, + }); + } + + await _dbContext.SaveChangesAsync(); + + await BuildService(new SelectivelyFailingExchangeRateClient(failFor: "ZZZ", rate: 0.88m)).RefreshOnceAsync(); + + var usd = await _dbContext.FxRates.AsNoTracking() + .SingleOrDefaultAsync(r => r.BaseCurrency == "USD" && r.TargetCurrency == "EUR"); + Assert.NotNull(usd); + Assert.Equal(0.88m, usd!.Rate); + + Assert.False(await _dbContext.FxRates.AsNoTracking() + .AnyAsync(r => r.BaseCurrency == "ZZZ" && r.TargetCurrency == "EUR")); + } + [Fact] public async Task GetRate_ForAPairTheScheduledPassNeverFetched_FetchesItOnDemandAndCachesIt() { @@ -260,4 +307,28 @@ private class ThrowingExchangeRateClient : IExchangeRateClient public Task> GetLatestRatesAsync(string baseCurrency, IReadOnlyCollection targetCurrencies, CancellationToken cancellationToken = default) => throw new InvalidOperationException("Simulated exchangerate.host outage."); } + + /// Fails for one base currency and succeeds for every other - the partial-failure case. + private class SelectivelyFailingExchangeRateClient : IExchangeRateClient + { + private readonly string _failFor; + private readonly decimal _rate; + + public SelectivelyFailingExchangeRateClient(string failFor, decimal rate) + { + _failFor = failFor; + _rate = rate; + } + + public Task> GetLatestRatesAsync(string baseCurrency, IReadOnlyCollection targetCurrencies, CancellationToken cancellationToken = default) + { + if (baseCurrency == _failFor) + { + throw new InvalidOperationException($"Simulated provider refusal for {baseCurrency}."); + } + + IReadOnlyList rates = targetCurrencies.Select(target => new ExchangeRate(baseCurrency, target, _rate)).ToList(); + return Task.FromResult(rates); + } + } } From 63ee6a12acbcf153d3a9fe1bf8e3f6988a918189 Mon Sep 17 00:00:00 2001 From: RghvGrv Date: Sat, 15 Aug 2026 00:56:27 +0530 Subject: [PATCH 3/7] fix: detect a stale version on an update that changes nothing UpdateAsync substitutes the client's version into the tracked entity's xmin original value, so the generated UPDATE asserts nothing has moved. That only works if an UPDATE is generated at all: submit values identical to what is stored and EF marks nothing modified, issues no statement, and SaveChangesAsync cannot raise a concurrency exception. The call returned 200 against a row that had moved on, while the controller documents the 409 unconditionally. This is the shape the check exists for rather than a corner case - a user who opened the edit screen before a mark-paid, changed nothing and pressed Save is exactly the one whose write would silently roll the billing date back. Forces the row to be written when a version was supplied. Omitting the version still applies unconditionally, so older clients are unaffected. Closes #191 --- .../Repositories/SubscriptionRepository.cs | 13 ++++++ .../ConcurrentUpdateTests.cs | 45 +++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/src/SubVora.Infrastructure/Repositories/SubscriptionRepository.cs b/src/SubVora.Infrastructure/Repositories/SubscriptionRepository.cs index 08dd46e..816cd71 100644 --- a/src/SubVora.Infrastructure/Repositories/SubscriptionRepository.cs +++ b/src/SubVora.Infrastructure/Repositories/SubscriptionRepository.cs @@ -70,6 +70,19 @@ public async Task 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); diff --git a/tests/SubVora.Api.Tests/ConcurrentUpdateTests.cs b/tests/SubVora.Api.Tests/ConcurrentUpdateTests.cs index 363ae9d..9b2d2c7 100644 --- a/tests/SubVora.Api.Tests/ConcurrentUpdateTests.cs +++ b/tests/SubVora.Api.Tests/ConcurrentUpdateTests.cs @@ -140,6 +140,51 @@ public async Task Update_WithAStaleVersionAfterAnotherEdit_Returns409() Assert.Equal("Edited on device A", after!.CustomName); } + [Fact] + public async Task Update_WithAStaleVersionButNoActualChanges_StillReturns409() + { + // Every other conflict test alters a field, so EF generates an UPDATE and the xmin predicate + // rides along with it. Submit values byte-identical to what is stored and EF marks nothing + // modified, issues no statement at all, and SaveChangesAsync cannot raise a concurrency + // exception - the save answered 200 against a row that had moved on. + // + // This is the shape the check exists for, not a corner case: a user who opened the edit + // screen, changed nothing, and pressed Save is exactly the one whose write would silently + // roll back whatever happened underneath them. + var client = await CreateAuthenticatedClientAsync(); + var created = await CreateAsync(client); + + var firstEdit = ValidRequest(); + firstEdit.Version = created.Version; + firstEdit.CustomName = "Edited on device A"; + Assert.Equal(HttpStatusCode.OK, (await client.PutAsJsonAsync($"/api/v1/subscriptions/{created.Id}", firstEdit, JsonOptions)).StatusCode); + + // Exactly what is stored now, but carrying the version read before device A's edit. + var unchanged = ValidRequest(); + unchanged.Version = created.Version; + unchanged.CustomName = "Edited on device A"; + + var response = await client.PutAsJsonAsync($"/api/v1/subscriptions/{created.Id}", unchanged, JsonOptions); + + Assert.Equal(HttpStatusCode.Conflict, response.StatusCode); + } + + [Fact] + public async Task Update_WithTheCurrentVersionAndNoActualChanges_StillSucceeds() + { + // The other side of the same guard: forcing the write must not turn an unchanged save + // carrying a *current* version into a spurious conflict. + var client = await CreateAuthenticatedClientAsync(); + var created = await CreateAsync(client); + + var unchanged = ValidRequest(); + unchanged.Version = created.Version; + + var response = await client.PutAsJsonAsync($"/api/v1/subscriptions/{created.Id}", unchanged, JsonOptions); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + [Fact] public async Task Update_WithNoVersion_StillAppliesUnconditionally() { From 59191529b53c245eea1cbb62dadf971e266fff3e Mon Sep 17 00:00:00 2001 From: RghvGrv Date: Sat, 15 Aug 2026 00:57:10 +0530 Subject: [PATCH 4/7] fix: even out forgot-password's known and unknown branches An unknown address returned after a single SELECT; a known one generated a code, hashed it, queried outstanding codes, updated each, inserted and saved. Response time alone therefore said whether an address had an account - on the one endpoint whose entire contract is that it reveals nothing, and in a file that goes to real lengths elsewhere to erase exactly this signal (RegisterAsync hashes unconditionally, LoginAsync verifies against a dummy hash). Moves the code generation and SHA-256 above the user lookup so both branches pay for them. An unknown address still writes nothing, which the new test pins. Residual gap accepted and documented in the code: a real account still costs extra round trips. Closing that fully would mean issuing throwaway writes for addresses with no account - real write load driven by anyone who can reach the endpoint - to hide a few milliseconds of database time on an endpoint already IP rate-limited to 10/min. The baseline is re-staged for line-number drift only: inserting a test shifted five already-audited is_secret:false entries in PasswordResetControllerTests.cs. No new finding. Closes #190 --- .secrets.baseline | 12 +++++----- .../Auth/AuthService.cs | 23 +++++++++++++++---- .../PasswordResetControllerTests.cs | 23 +++++++++++++++++++ 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index e0ee3d3..ea64124 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -333,7 +333,7 @@ "filename": "tests\\SubVora.Api.Tests\\PasswordResetControllerTests.cs", "hashed_secret": "dd606cd49bbbd06b4c2606fc2449f8fb87975786", "is_verified": false, - "line_number": 57, + "line_number": 80, "is_secret": false }, { @@ -341,7 +341,7 @@ "filename": "tests\\SubVora.Api.Tests\\PasswordResetControllerTests.cs", "hashed_secret": "e400573056b556e8c6cde4da840f466980c6af0f", "is_verified": false, - "line_number": 75, + "line_number": 98, "is_secret": false }, { @@ -349,7 +349,7 @@ "filename": "tests\\SubVora.Api.Tests\\PasswordResetControllerTests.cs", "hashed_secret": "c3c6d08928310be981391bda1493419bcc98c4fd", "is_verified": false, - "line_number": 129, + "line_number": 152, "is_secret": false }, { @@ -357,7 +357,7 @@ "filename": "tests\\SubVora.Api.Tests\\PasswordResetControllerTests.cs", "hashed_secret": "0e1f3d0551dcb2581486fba9609162ce3dfbe09d", "is_verified": false, - "line_number": 216, + "line_number": 239, "is_secret": false }, { @@ -365,7 +365,7 @@ "filename": "tests\\SubVora.Api.Tests\\PasswordResetControllerTests.cs", "hashed_secret": "61db6e4f416772347879825e7338248f3e54db15", "is_verified": false, - "line_number": 221, + "line_number": 244, "is_secret": false } ], @@ -520,5 +520,5 @@ } ] }, - "generated_at": "2026-08-11T06:02:08Z" + "generated_at": "2026-08-14T19:26:43Z" } diff --git a/src/SubVora.Infrastructure/Auth/AuthService.cs b/src/SubVora.Infrastructure/Auth/AuthService.cs index 05d7cb9..1194dea 100644 --- a/src/SubVora.Infrastructure/Auth/AuthService.cs +++ b/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, }); diff --git a/tests/SubVora.Api.Tests/PasswordResetControllerTests.cs b/tests/SubVora.Api.Tests/PasswordResetControllerTests.cs index ce485c5..d86a93c 100644 --- a/tests/SubVora.Api.Tests/PasswordResetControllerTests.cs +++ b/tests/SubVora.Api.Tests/PasswordResetControllerTests.cs @@ -49,6 +49,29 @@ public async Task ForgotPassword_UnknownEmail_StillReturns200() Assert.DoesNotContain(emailSender.SentEmails, e => e.ToEmail == unknownEmail); } + [Fact] + public async Task ForgotPassword_UnknownEmail_WritesNoResetCodeRow() + { + // Both branches now generate and hash a code before the user lookup, so response time + // depends less on whether the address exists. The unknown branch must still persist + // nothing - the hashing is throwaway work, not a row. + // + // Timing parity itself is held by the structure of ForgotPasswordAsync (and its comment + // recording the accepted residual gap), not asserted here: a wall-clock assertion would be + // flaky in CI and would fail for reasons unrelated to the property it claims to check. + var client = _factory.CreateClient(); + var unknownEmail = $"unknown-norow-{Guid.NewGuid()}@example.com"; + + using var scope = _factory.Services.CreateScope(); + var dbContext = scope.ServiceProvider.GetRequiredService(); + var before = await dbContext.PasswordResetCodes.CountAsync(); + + var response = await client.PostAsJsonAsync("/api/v1/auth/forgot-password", new ForgotPasswordRequest { Email = unknownEmail }); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal(before, await dbContext.PasswordResetCodes.CountAsync()); + } + [Fact] public async Task ForgotPassword_KnownEmail_CreatesCodeAndSendsEmail() { From 06c974ccd13981c21ff8b09af35373aa88fa908c Mon Sep 17 00:00:00 2001 From: RghvGrv Date: Sat, 15 Aug 2026 00:57:23 +0530 Subject: [PATCH 5/7] fix(mobile): mirror Version and CatalogId in the SQLite cache CachedSubscription mirrored neither field. Not reachable today - the detail screen always fetches over the network and writes are gated when offline - but the moment any screen edits from cache, a Version of 0 matches no xmin and every save 409s forever, and a null CatalogId silently strips the record's catalog link. Version is stored nullable rather than uint: sqlite-net adds a new column to an existing table but leaves pre-upgrade rows at the default, and a cached 0 read as authoritative is worse than an absent one. Adds a reflection-driven round-trip test over every settable property of SubscriptionDto, so a field added later fails the build instead of being quietly dropped by the mirror. Closes #192 --- .../Models/CachedSubscription.cs | 31 ++++++- .../SqliteLocalCacheServiceTests.cs | 92 +++++++++++++++++++ 2 files changed, 122 insertions(+), 1 deletion(-) diff --git a/src/SubVora.Mobile/Models/CachedSubscription.cs b/src/SubVora.Mobile/Models/CachedSubscription.cs index 2e4010e..57d8f97 100644 --- a/src/SubVora.Mobile/Models/CachedSubscription.cs +++ b/src/SubVora.Mobile/Models/CachedSubscription.cs @@ -3,7 +3,18 @@ namespace SubVora.Mobile.Models; -/// sqlite-net-pcl mirror of SubscriptionDto, one row per subscription. +/// +/// sqlite-net-pcl mirror of SubscriptionDto, one row per subscription. +/// +/// Must stay field-complete against . A partial mirror is a trap the +/// moment anything reads an edit back out of it: Version was missing, so a save built from a +/// cached row would carry 0, match no xmin, and 409 forever; CatalogId was missing, so +/// the same save would silently strip the record's catalog link - the exact loss +/// SubscriptionDetailViewModel.ApplySubscription orders its assignments to avoid. +/// SqliteLocalCacheServiceTests walks the DTO's properties by reflection so a field added +/// later fails the build rather than being quietly dropped. +/// +/// public class CachedSubscription { [PrimaryKey] @@ -20,10 +31,21 @@ public class CachedSubscription public DateTime? LastPaidDate { get; set; } public int AlertDaysAdvance { get; set; } + + /// + /// Nullable, unlike the DTO's. sqlite-net adds a new column to an existing table on + /// CreateTableAsync but leaves rows written before it existed at the default, and a cached + /// version of 0 read as authoritative is worse than an absent one - it matches no xmin + /// and turns every save built from that row into a permanent 409. Null means "this mirror does + /// not know", which a caller can act on; 0 is a lie. + /// + public uint? Version { get; set; } + public Guid? CategoryId { get; set; } public string? CategoryName { get; set; } public Guid? PaymentSourceId { get; set; } public string? PaymentSourceLabel { get; set; } + public Guid? CatalogId { get; set; } public string? CatalogLogoUrl { get; set; } public bool IsFreeTrial { get; set; } public bool IsActive { get; set; } @@ -40,10 +62,12 @@ public class CachedSubscription NextBillingDate = dto.NextBillingDate.ToDateTime(TimeOnly.MinValue), LastPaidDate = dto.LastPaidDate?.ToDateTime(TimeOnly.MinValue), AlertDaysAdvance = dto.AlertDaysAdvance, + Version = dto.Version, CategoryId = dto.CategoryId, CategoryName = dto.CategoryName, PaymentSourceId = dto.PaymentSourceId, PaymentSourceLabel = dto.PaymentSourceLabel, + CatalogId = dto.CatalogId, CatalogLogoUrl = dto.CatalogLogoUrl, IsFreeTrial = dto.IsFreeTrial, IsActive = dto.IsActive, @@ -61,10 +85,15 @@ public class CachedSubscription NextBillingDate = DateOnly.FromDateTime(NextBillingDate), LastPaidDate = LastPaidDate is null ? null : DateOnly.FromDateTime(LastPaidDate.Value), AlertDaysAdvance = AlertDaysAdvance, + // 0 when the row predates the column. That is the same value the DTO would have had before + // this field was mirrored at all, and SubscriptionDetailViewModel only ever sends a version + // it read from the network, so nothing regresses - see the property's note above. + Version = Version ?? 0, CategoryId = CategoryId, CategoryName = CategoryName, PaymentSourceId = PaymentSourceId, PaymentSourceLabel = PaymentSourceLabel, + CatalogId = CatalogId, CatalogLogoUrl = CatalogLogoUrl, IsFreeTrial = IsFreeTrial, IsActive = IsActive, diff --git a/tests/SubVora.Mobile.Tests/SqliteLocalCacheServiceTests.cs b/tests/SubVora.Mobile.Tests/SqliteLocalCacheServiceTests.cs index 322f9e4..5b21016 100644 --- a/tests/SubVora.Mobile.Tests/SqliteLocalCacheServiceTests.cs +++ b/tests/SubVora.Mobile.Tests/SqliteLocalCacheServiceTests.cs @@ -1,4 +1,5 @@ using SQLite; +using SubVora.Mobile.Api.Dtos; using SubVora.Mobile.Models; using SubVora.Mobile.Services; @@ -86,4 +87,95 @@ public async Task ClearAllAsync_EmptiesEveryCachedType() Assert.Empty(await _cacheService.GetAllAsync()); Assert.Empty(await _cacheService.GetAllAsync()); } + + /// + /// Every settable property on , filled with a distinct non-default + /// value. Reflection then checks that each one survives the round trip, so a property added to + /// the DTO later fails here instead of being silently dropped by the mirror. + /// + private static SubscriptionDto FullyPopulatedDto() => new() + { + Id = Guid.NewGuid(), + CustomName = "Netflix Premium", + CostAmount = 649.50m, + Currency = "INR", + CycleCadence = BillingCycleType.Quarterly, + PurchaseDate = new DateOnly(2026, 1, 15), + NextBillingDate = new DateOnly(2026, 4, 15), + LastPaidDate = new DateOnly(2026, 1, 15), + AlertDaysAdvance = 7, + Version = 8_675_309u, + CategoryId = Guid.NewGuid(), + CategoryName = "Entertainment", + PaymentSourceId = Guid.NewGuid(), + PaymentSourceLabel = "HDFC Card", + CatalogId = Guid.NewGuid(), + CatalogLogoUrl = "https://cdn.simpleicons.org/netflix", + IsFreeTrial = true, + IsActive = true, + CreatedAt = new DateTimeOffset(2026, 1, 15, 9, 30, 0, TimeSpan.Zero), + }; + + [Fact] + public void CachedSubscription_MirrorsEveryPropertyOfTheDto() + { + // IsOverdue is computed from NextBillingDate and IsActive, so it has nothing to store. + var excluded = new[] { nameof(SubscriptionDto.IsOverdue) }; + + var dto = FullyPopulatedDto(); + var roundTripped = CachedSubscription.FromDto(dto).ToDto(); + + var properties = typeof(SubscriptionDto).GetProperties() + .Where(property => property.CanWrite && !excluded.Contains(property.Name)) + .ToList(); + + // Guards the guard: if the DTO's shape changes so that nothing is enumerated, this test + // would pass vacuously and stop protecting anything. + Assert.NotEmpty(properties); + + foreach (var property in properties) + { + var expected = property.GetValue(dto); + var actual = property.GetValue(roundTripped); + + Assert.False( + Equals(expected, property.PropertyType.IsValueType ? Activator.CreateInstance(property.PropertyType) : null), + $"{property.Name} was left at its default in FullyPopulatedDto, so the round trip is not actually tested."); + Assert.Equal(expected, actual); + } + } + + [Fact] + public async Task CachedSubscription_SurvivesTheDatabaseRoundTripWithVersionAndCatalogId() + { + // Through real sqlite-net rather than just FromDto/ToDto: the two columns added for this are + // a uint? and a Guid?, and it is the storage layer that has to accept them. + var dto = FullyPopulatedDto(); + + await _cacheService.UpsertAsync(CachedSubscription.FromDto(dto)); + var reloaded = Assert.Single(await _cacheService.GetAllAsync()).ToDto(); + + Assert.Equal(dto.Version, reloaded.Version); + Assert.Equal(dto.CatalogId, reloaded.CatalogId); + } + + [Fact] + public void CachedSubscription_WrittenBeforeTheVersionColumnExisted_ReadsBackAsZeroNotAsGarbage() + { + // sqlite-net adds the column on CreateTableAsync but leaves pre-upgrade rows at null. The + // mirror stores Version as uint? precisely so that state is representable. + var row = new CachedSubscription + { + Id = Guid.NewGuid(), + CustomName = "Written by an older build", + Currency = "INR", + Version = null, + CatalogId = null, + }; + + var dto = row.ToDto(); + + Assert.Equal(0u, dto.Version); + Assert.Null(dto.CatalogId); + } } From 5f44345a4bdff12b40a0c021cdd557f5b3d806de Mon Sep 17 00:00:00 2001 From: RghvGrv Date: Sat, 15 Aug 2026 00:57:24 +0530 Subject: [PATCH 6/7] fix: exclude a free-trial one-time purchase from the dashboard The IsFreeTrial guard sat after the OneTime branch returned, so a one-time purchase marked as a trial was never tested against it and counted in full toward OneTimeThisYear despite nothing being charged. Moves the guard above the cadence branch - whether a trial counts is not a question about its cadence, so no cadence gets to bypass the check. Left after rate resolution so unresolved-rate reporting is unchanged. Closes #193 --- .../Dashboard/BurnRateCalculator.cs | 19 +++++++---- .../BurnRateCalculatorTests.cs | 33 +++++++++++++++++-- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/src/SubVora.Application/Dashboard/BurnRateCalculator.cs b/src/SubVora.Application/Dashboard/BurnRateCalculator.cs index 882a67e..b60d178 100644 --- a/src/SubVora.Application/Dashboard/BurnRateCalculator.cs +++ b/src/SubVora.Application/Dashboard/BurnRateCalculator.cs @@ -106,6 +106,18 @@ public async Task CalculateAsync(IEnumerable 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 CalculateAsync(IEnumerable 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; diff --git a/tests/SubVora.Application.Tests/BurnRateCalculatorTests.cs b/tests/SubVora.Application.Tests/BurnRateCalculatorTests.cs index ea619b9..50fbe49 100644 --- a/tests/SubVora.Application.Tests/BurnRateCalculatorTests.cs +++ b/tests/SubVora.Application.Tests/BurnRateCalculatorTests.cs @@ -34,7 +34,7 @@ public BurnRateCalculatorTests() CreatedAt = DateTimeOffset.UtcNow, }; - private static SubscriptionDto OneTimeSubscription(decimal cost, DateOnly purchaseDate, bool isActive = true, string currency = "USD") => new() + private static SubscriptionDto OneTimeSubscription(decimal cost, DateOnly purchaseDate, bool isActive = true, string currency = "USD", bool isFreeTrial = false) => new() { Id = Guid.NewGuid(), CustomName = "One-Time Purchase", @@ -44,11 +44,40 @@ public BurnRateCalculatorTests() PurchaseDate = purchaseDate, NextBillingDate = purchaseDate, AlertDaysAdvance = 3, - IsFreeTrial = false, + IsFreeTrial = isFreeTrial, IsActive = isActive, CreatedAt = DateTimeOffset.UtcNow, }; + [Fact] + public async Task OneTimePurchase_OnAFreeTrial_IsExcludedFromOneTimeThisYear() + { + // The free-trial guard used to sit *after* the OneTime branch returned, so a one-time + // purchase marked as a trial was never tested against it and counted in full. + var subscriptions = new[] + { + OneTimeSubscription(99m, new DateOnly(DateTime.UtcNow.Year, 3, 1), isFreeTrial: true), + }; + + var result = await _calculator.CalculateAsync(subscriptions, "USD"); + + Assert.Equal(0m, result.OneTimeThisYear); + } + + [Fact] + public async Task OneTimePurchase_NotOnAFreeTrial_StillCounts() + { + // The other half of the guard: excluding trials must not quietly exclude real purchases. + var subscriptions = new[] + { + OneTimeSubscription(99m, new DateOnly(DateTime.UtcNow.Year, 3, 1), isFreeTrial: false), + }; + + var result = await _calculator.CalculateAsync(subscriptions, "USD"); + + Assert.Equal(99m, result.OneTimeThisYear); + } + [Fact] public async Task CalculatesWeeklyMonthlyYearly_ForMixOfCycles() { From 822f0742ce3494092c67b001319cdafd51eee5cd Mon Sep 17 00:00:00 2001 From: RghvGrv Date: Sat, 15 Aug 2026 00:57:37 +0530 Subject: [PATCH 7/7] fix: answer 404 from GET /users/me when the row is gone GET returned Ok(profile) unconditionally, so a vanished row produced a 200 with a null body - indistinguishable from a successful read - while PUT on the same resource has always answered 404. Baseline re-staged for line-number drift only: the new test shifted one already-audited is_secret:false entry in UsersControllerTests.cs by three lines. No new finding. Closes #194 --- .secrets.baseline | 4 ++-- .../Controllers/UsersController.cs | 10 +++++++- .../SubVora.Api.Tests/UsersControllerTests.cs | 23 +++++++++++++++++++ 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index ea64124..cbbdf64 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -405,7 +405,7 @@ "filename": "tests\\SubVora.Api.Tests\\UsersControllerTests.cs", "hashed_secret": "dd606cd49bbbd06b4c2606fc2449f8fb87975786", "is_verified": false, - "line_number": 21, + "line_number": 24, "is_secret": false } ], @@ -520,5 +520,5 @@ } ] }, - "generated_at": "2026-08-14T19:26:43Z" + "generated_at": "2026-08-14T19:27:26Z" } diff --git a/src/SubVora.Api/Controllers/UsersController.cs b/src/SubVora.Api/Controllers/UsersController.cs index e9f11f7..6896c43 100644 --- a/src/SubVora.Api/Controllers/UsersController.cs +++ b/src/SubVora.Api/Controllers/UsersController.cs @@ -23,15 +23,23 @@ public UsersController(IUserRepository userRepository, IValidatorGets the authenticated user's own profile. + /// + /// 404 rather than a 200 carrying null when the row is gone - the same answer PUT 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. + /// /// Returns the caller's profile. /// The caller is not authenticated. + /// The caller's user row no longer exists. [HttpGet("me")] [ProducesResponseType(typeof(UserProfileDto), StatusCodes.Status200OK)] [ProducesResponseType(StatusCodes.Status401Unauthorized)] + [ProducesResponseType(StatusCodes.Status404NotFound)] public async Task GetMe(CancellationToken cancellationToken) { var profile = await _userRepository.GetProfileAsync(GetUserId(), cancellationToken); - return Ok(profile); + return profile is null ? NotFound() : Ok(profile); } /// Updates the authenticated user's own profile. diff --git a/tests/SubVora.Api.Tests/UsersControllerTests.cs b/tests/SubVora.Api.Tests/UsersControllerTests.cs index 1961e3e..da28725 100644 --- a/tests/SubVora.Api.Tests/UsersControllerTests.cs +++ b/tests/SubVora.Api.Tests/UsersControllerTests.cs @@ -1,8 +1,11 @@ using System.Net; using System.Net.Http.Headers; using System.Net.Http.Json; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; using SubVora.Application.Auth; using SubVora.Application.Users; +using SubVora.Infrastructure.Data; namespace SubVora.Api.Tests; @@ -57,6 +60,26 @@ public async Task GetMe_WithoutAuth_Returns401() Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); } + [Fact] + public async Task GetMe_WhenTheRowIsGone_Returns404() + { + // The token is still valid - it is signed, not looked up - so the request authenticates and + // then finds nothing. Answering 200 with a null body made that indistinguishable from a + // successful read; PUT has always answered 404 here. + var email = $"users-getme-vanished-{Guid.NewGuid()}@example.com"; + var client = await CreateAuthenticatedClientAsync(email); + + using (var scope = _factory.Services.CreateScope()) + { + var dbContext = scope.ServiceProvider.GetRequiredService(); + await dbContext.Users.Where(u => u.Email == email).ExecuteDeleteAsync(); + } + + var response = await client.GetAsync("/api/v1/users/me"); + + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + [Fact] public async Task UpdateMe_PersistsChanges() {