diff --git a/.secrets.baseline b/.secrets.baseline index e0ee3d3..cbbdf64 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 } ], @@ -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-11T06:02:08Z" + "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/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/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/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/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/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/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/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.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() { 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() { 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() { 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() { 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); + } + } } 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/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); + } } 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(),