diff --git a/docs/architecture.md b/docs/architecture.md index 240f6c4..9f67573 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -191,6 +191,10 @@ Components/ Pages/Login.razor — The sign-in form (posts to /auth/login) Pages/Error.razor — Unhandled-error page, localized Pages/NotFound.razor — /not-found, localized +Security/ + RevalidatingUserAuthenticationStateProvider.cs + — Re-checks a circuit's principal on a timer and signs it out when the + account is gone; the circuit-side half of OnValidatePrincipal wwwroot/ theme.css — Semantic tokens, the muted-ink ramp and the gradients (see docs/theming.md) app.css — Global styles: MudBlazor overrides, badges, .action-btn, .stacked-table, diff --git a/docs/deployment.md b/docs/deployment.md index 496c882..e980fa1 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -15,7 +15,11 @@ custom domains at the lowest price point (~$3–5/month, less with scale-to-zero | `Program.cs` | `APP_DATA_DIR` env var overrides the data folder (DB, logs, data-protection keys); maps `/health` | On Fly, `APP_DATA_DIR=/data` points at a 1 GB persistent volume, so the SQLite DB, -Serilog logs, and data-protection keys all survive deploys and restarts. +Serilog logs, and data-protection keys all survive deploys and restarts. Surviving on disk is only +half of what a key ring needs, though: `AddDataProtection().SetApplicationName("FootballFormation")` +pins the purpose the keys are derived for, which otherwise defaults to the content root path and +would change with the Dockerfile's `WORKDIR`. Both halves have to hold or a deploy signs everyone +out with nothing in the log to say why. `ASPNETCORE_FORWARDEDHEADERS_ENABLED=true` (set in the Dockerfile) makes the app trust Fly's `X-Forwarded-Proto` header — without it `UseHttpsRedirection` would loop, because Fly terminates TLS at the edge and forwards plain HTTP to port 8080. diff --git a/docs/known_issues.md b/docs/known_issues.md index 3020583..86d56fc 100644 --- a/docs/known_issues.md +++ b/docs/known_issues.md @@ -381,6 +381,49 @@ Avoid repeating these mistakes: not backfill, because nothing left in the row says whether a 37 was stoppage time or typed in by hand. +## Authentication +- **`ExpireTimeSpan` does not keep anyone signed in — `IsPersistent` does.** `SignInAsync` without + `AuthenticationProperties` sets a *session* cookie: no `Expires` on the header, so the browser is + free to drop it whenever it decides the session ended. An eight-hour `ExpireTimeSpan` sat right + above it and looked like the answer, but it bounds the ticket *inside* the cookie and has no say + in whether the browser keeps the container. The symptom is phone-shaped and so reads as flaky + rather than broken: a desktop tab holds the cookie for days, while iOS Safari and an installed PWA + drop it every time the OS reclaims the backgrounded tab — which is a coach putting their phone + away at half time. Both sign-in routes now pass `PersistentSession()`, and it returns a fresh + instance per call because the cookie handler writes `IssuedUtc`/`ExpiresUtc` onto the object it is + handed; one shared static would pin every later sign-in to the first one's expiry. +- **`SameSite=Strict` makes an ordinary link look like a logged-out session.** Strict withholds the + cookie on *every* cross-site navigation, a plain top-level link click included — so opening the + site from WhatsApp, an email or a search result arrives anonymous and bounces to `/login`, and + then a reload puts it right because that navigation is same-site. Coming back on its own is what + makes it hard to report and easy to dismiss. `Lax` is the setting; it still withholds the cookie + on the cross-site POST that CSRF actually needs, and nothing here is reached by one. +- **Persisting data-protection keys is only half of surviving a deploy.** The keys are on the + volume, but the purpose they are derived for defaults to the content root path — `/app` only + because the Dockerfile says `WORKDIR /app`. Keys present on disk and derived for a different + string open nothing, and the failure is silent: no exception, no log line, just every cookie + rejected at once after a deploy that changed nothing about authentication. + `SetApplicationName("FootballFormation")` is what stops it. +- **These three are browser decisions, so no C# test can see them.** All three are pinned in + `tests/ui/specs/session.spec.js`, which reads the cookie's attributes after a real form sign-in + and follows a link into the app from another site. +- **`OnValidatePrincipal` is not what revokes a Blazor Server session.** It runs per HTTP request, + and a circuit makes almost none after its first page load — the rest of the session is SignalR. + The stock `ServerAuthenticationStateProvider` reads the principal once when the circuit is created + and never asks again, so deleting an account left the owner's open tab fully working. Measured, + not assumed: with revalidation off, an account deleted while its owner sat idle on `/users` still + rendered the Add User button. And this is not only a markup problem — `CircuitCurrentUser` reads + that same provider, so `RunAdminAsync` was consulting the stale principal too. + `RevalidatingUserAuthenticationStateProvider` closes it on a timer. +- **A rejoin does not carry stale authority through, so the retained-circuit window does not widen + the gap.** Worth knowing before reasoning about `DisconnectedCircuitRetentionPeriod` as if it did. + With a revoked cookie, a dropped circuit does not come back: the reconnect fails and Blazor's + client falls back to a full page reload, which is an HTTP request, which is + `OnValidatePrincipal` — landing on `/login`. Probed both ways round, blocking `_blazor/negotiate` + to force the give-up path and leaving it open for a clean rejoin; both reloaded, while + `reconnect.spec.js` shows a *valid* cookie rejoining cleanly and staying live. So the stale window + is the revalidation interval on a connected idle circuit, and nothing more. + ## General - **Port already in use**: Kill orphaned process with `taskkill //PID //F`. - **File locked during build**: Stop the running app before rebuilding. diff --git a/docs/models.md b/docs/models.md index f0e33bc..271b811 100644 --- a/docs/models.md +++ b/docs/models.md @@ -336,14 +336,21 @@ is `SetNull`, so deleting a user leaves their comments in place, unattributed. enum member name — so renaming a `UserRole` member breaks the build rather than quietly unauthorizing everyone. Anonymous (not signed in) is not a role and needs no member. -**SecurityStamp is what makes a change take effect now.** The cookie lasts eight hours and is +**SecurityStamp is what makes a change take effect now.** The cookie lasts fourteen days and is sliding, so without it, deleting an account or changing its role would leave the old session working until it lapsed. The stamp is copied into the cookie at sign-in and re-checked on every authenticated request by `OnValidatePrincipal` (Program.cs) via `UserService.FindForSessionAsync`; a mismatch rejects the principal and signs the browser out. `UserService` regenerates it on password change and role change — but deliberately **not** on a rename, which changes nothing about what the account may -do. Note that a live Blazor circuit is not re-validated per SignalR message: revocation lands on the -next HTTP request. +do. + +A live Blazor circuit is still not re-validated per SignalR message — that would be a database read +per keystroke. It is re-validated **on a timer** instead, by +`RevalidatingUserAuthenticationStateProvider` (Web/Security), which asks `FindForSessionAsync` the +same question `OnValidatePrincipal` asks and signs the circuit out when the answer is no. Five +minutes by default, `Auth:RevalidationIntervalSeconds` to change it. Without it a tab open since +before the change kept its authority until someone reloaded — and because `CircuitCurrentUser` reads +that same provider, so did the write guard on every service. `UserService.DeleteAsync` and `UpdateAsync` both refuse to remove or demote the **last** Admin — the one operation with no way back short of editing the database by hand. `EnsureAdminSeededAsync` diff --git a/docs/patterns.md b/docs/patterns.md index 4d9c1ff..d8da82f 100644 --- a/docs/patterns.md +++ b/docs/patterns.md @@ -369,6 +369,49 @@ appears, it belongs in this paragraph. `CircuitCurrentUser` answers false for an account still on its seeded password, so the first-login gate is a real restriction rather than a redirect that could be navigated around. +## The sign-in cookie has three settings that are easy to get wrong +All three live in `Program.cs`, and each one failed in a way that looks like "it logged me out +again" rather than like a bug with a cause. + +- **`IsPersistent`, on the sign-in, is what makes the cookie outlive the browser session.** Both + routes that sign anyone in — `/auth/login` and the dev-only `/dev/login` — pass + `PersistentSession()`, which is a *new* `AuthenticationProperties` each time because the cookie + handler writes `IssuedUtc`/`ExpiresUtc` onto the instance it is given. `ExpireTimeSpan` does not + substitute for it: that bounds the ticket the cookie carries, while `IsPersistent` is the only + thing that puts an `Expires` on the header at all. Without it the browser holds a session cookie + and drops it whenever it decides the session ended — on a phone, every time the OS reclaims the + backgrounded tab. +- **`SameSite` is `Lax`, and must not go back to `Strict`.** Strict withholds the cookie on every + cross-site navigation including an ordinary link click, so arriving from WhatsApp, an email or a + search result renders the page signed out until a reload. Lax still withholds it on the cross-site + POST that CSRF needs. +- **Data protection sets an application name.** Left unset the purpose string defaults to the + content root path — `/app` only because the Dockerfile says so — so pinning it is what stops a + change to where the app is unpacked from invalidating every issued cookie at once. + +`tests/ui/specs/session.spec.js` holds all three, by reading the cookie's own attributes and by +following a link into the app from another site. + +## Revoking authority takes two halves, because a circuit barely makes requests +`OnValidatePrincipal` re-checks the security stamp on every HTTP request — and a Blazor Server tab +makes almost none after the page loads. `RevalidatingUserAuthenticationStateProvider` +(Web/Security) is the other half: it re-asks `UserService.FindForSessionAsync` on a timer for the +life of the circuit and signs the circuit out when the account is gone or its stamp has moved. +Five minutes by default; `Auth:RevalidationIntervalSeconds` sets it, and `0` leaves the stock +provider in place so the UI test can be run against the old behaviour. + +Both halves call the same `FindForSessionAsync(ClaimsPrincipal)` overload on purpose — two places +deciding separately what a valid session looks like is how they drift. + +The provider takes an `IServiceScopeFactory` rather than a `UserService`, and **not** for the usual +short-lived-context reason. It *is* the circuit's `AuthenticationStateProvider`; `UserService` +depends on `ICurrentUser`, which depends on the `AuthenticationStateProvider`. Injecting it directly +closes the loop and the container refuses to build. + +A failed check makes the circuit anonymous. It cannot clear the cookie — a circuit has no HTTP +response to set a header on — so `[Authorize]` renders `NotAuthorized`, `RedirectToLogin` +force-loads, and *that* request is where `OnValidatePrincipal` finally drops the cookie. + ## Blazor Rendering - Entire app is Interactive Server (set on `` and `` in App.razor) - UI assembly discovered via `AddAdditionalAssemblies(typeof(FootballFormation.UI._Imports).Assembly)` diff --git a/docs/testing.md b/docs/testing.md index 06fb576..36ac64d 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -167,6 +167,7 @@ real SignalR circuit. | `localization.spec.js` | Dutch by default, the switcher moving the whole app to English, and the choice surviving a navigation | | `mobile.touchline.spec.js` | The phone layout — the drawer, the full-screen match sheet, the stacked squad — in the `mobile` project on a Pixel 7 | | `reconnect.spec.js` | Losing the circuit and getting it back: the retry schedule a suspended phone rejoins on, and the rejoined page still being interactive | +| `session.spec.js` | Staying signed in: the auth cookie carrying a real expiry rather than being a session cookie, surviving a link followed in from another site, and a deleted account losing its authority on an open circuit without anyone reloading | | `selectors.spec.js` | A test for the tests — see below | ### The test that guards the tests diff --git a/src/FootballFormation.Core/Services/UserService.cs b/src/FootballFormation.Core/Services/UserService.cs index 49d622f..cf7aab2 100644 --- a/src/FootballFormation.Core/Services/UserService.cs +++ b/src/FootballFormation.Core/Services/UserService.cs @@ -1,3 +1,4 @@ +using System.Security.Claims; using FootballFormation.Core.Data; using FootballFormation.Core.Models; using FootballFormation.Core.Security; @@ -36,10 +37,29 @@ public class UserService( return await VerifyAsync(db, username, password, cancellationToken); } + /// + /// The account behind a signed-in principal, or null when the session it stands for is no longer + /// good. Two callers need exactly this question answered and must not drift on the answer: + /// OnValidatePrincipal, on every authenticated HTTP request, and the circuit's + /// revalidation loop, which is the only thing asking it between one page load and the next. + /// + public Task FindForSessionAsync( + ClaimsPrincipal? principal, CancellationToken cancellationToken = default) + { + var stamp = principal?.FindFirst(AppClaims.SecurityStamp)?.Value; + var userId = principal?.FindFirst(AppClaims.UserId)?.Value; + + // Cookies issued before the security stamp shipped carry neither claim. Rejected rather + // than trusted — the only cost is one extra sign-in. + return stamp is not null && int.TryParse(userId, out var id) + ? FindForSessionAsync(id, stamp, cancellationToken) + : Task.FromResult(null); + } + /// /// The account behind a live cookie, or null when it has been deleted or its authority changed - /// since the cookie was issued. Called on every authenticated request — see OnValidatePrincipal - /// in Program.cs — so it reads no-tracking and touches one row by key. + /// since the cookie was issued. Called on every authenticated request, so it reads no-tracking + /// and touches one row by key. /// public async Task FindForSessionAsync( int userId, string securityStamp, CancellationToken cancellationToken = default) diff --git a/src/FootballFormation.Web/Program.cs b/src/FootballFormation.Web/Program.cs index 31121e3..4288498 100644 --- a/src/FootballFormation.Web/Program.cs +++ b/src/FootballFormation.Web/Program.cs @@ -11,8 +11,10 @@ using FootballFormation.UI.Security; using FootballFormation.UI.State; using FootballFormation.Web.Components; +using FootballFormation.Web.Security; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication.Cookies; +using Microsoft.AspNetCore.Components.Authorization; using Microsoft.AspNetCore.Localization; using Microsoft.AspNetCore.RateLimiting; using Microsoft.AspNetCore.ResponseCompression; @@ -74,8 +76,16 @@ builder.Services.AddLocalization(); - // Keys on disk so antiforgery/auth cookies survive container restarts + // Keys on disk so antiforgery/auth cookies survive container restarts — appDataFolder is the + // mounted volume when hosted, so a deploy reads back the key ring the last one wrote. + // + // The application name is pinned rather than left to default, because the default is the content + // root path: stable at /app only because the Dockerfile says WORKDIR /app, and silently + // different the moment that changes. Keys that are still on disk but derived for another + // purpose string are keys that cannot open a single cookie already issued, with nothing in the + // log to say why everyone was signed out at once. builder.Services.AddDataProtection() + .SetApplicationName("FootballFormation") .PersistKeysToFileSystem(new DirectoryInfo(Path.Combine(appDataFolder, "keys"))); // Compress SignalR WebSocket traffic (render diffs, events) @@ -125,32 +135,40 @@ { options.LoginPath = "/login"; options.LogoutPath = "/auth/logout"; - options.ExpireTimeSpan = TimeSpan.FromHours(8); + + // Long enough to span the gap between one match day and the next: signing in on + // Saturday morning should not mean typing a password again before Sunday's kickoff. + // Sliding, so regular use through a season never lapses while an abandoned session + // still does. This governs the ticket *inside* the cookie; how long the browser keeps + // the cookie at all is IsPersistent's job — see PersistentSession. + options.ExpireTimeSpan = TimeSpan.FromDays(14); options.SlidingExpiration = true; options.Cookie.Name = "ff.auth"; options.Cookie.HttpOnly = true; - options.Cookie.SameSite = SameSiteMode.Strict; + + // Lax rather than Strict. Strict withholds the cookie on *every* cross-site navigation + // including a plain link click, so arriving from WhatsApp, an email or a search result + // rendered the page signed out and only a reload — same-site by then — put it right. + // Lax still withholds it on the cross-site POST that CSRF needs, and no flow here is + // reached by one. + options.Cookie.SameSite = SameSiteMode.Lax; options.Cookie.SecurePolicy = builder.Environment.IsDevelopment() ? CookieSecurePolicy.SameAsRequest : CookieSecurePolicy.Always; - // A cookie is good for eight hours, but the authority it carries is not: deleting an - // account or changing its role has to take effect now, not whenever the cookie lapses. + // A cookie is good for a fortnight, but the authority it carries is not: deleting an + // account or resetting its password has to take effect now, not whenever the cookie + // lapses. The longer the cookie lives, the more this is the thing holding the line. // Every account carries a security stamp that changes when its authority does; the // cookie carries the stamp as it was at sign-in, and this compares the two. + // + // This runs per HTTP request, which a Blazor Server tab makes very few of — see + // RevalidatingUserAuthenticationStateProvider for the half that covers the circuit. options.Events.OnValidatePrincipal = async context => { - var principal = context.Principal; - var stamp = principal?.FindFirst(AppClaims.SecurityStamp)?.Value; - var userId = principal?.FindFirst(AppClaims.UserId)?.Value; - - // Cookies issued before this feature shipped carry neither claim. Reject them - // rather than trusting them — the only cost is one extra sign-in. - if (stamp is not null && int.TryParse(userId, out var id)) - { - var users = context.HttpContext.RequestServices.GetRequiredService(); - if (await users.FindForSessionAsync(id, stamp, context.HttpContext.RequestAborted) is not null) return; - } + var users = context.HttpContext.RequestServices.GetRequiredService(); + if (await users.FindForSessionAsync(context.Principal, context.HttpContext.RequestAborted) is not null) + return; context.RejectPrincipal(); await context.HttpContext.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme); @@ -159,6 +177,29 @@ builder.Services.AddAuthorization(); builder.Services.AddCascadingAuthenticationState(); + // Replaces the stock ServerAuthenticationStateProvider, which reads the principal once when the + // circuit is created and never asks again. Registered after AddInteractiveServerComponents so + // this wins; it derives from ServerAuthenticationStateProvider, which is what lets the circuit + // still hand it the initial state. + // + // Five minutes by default: one indexed read by primary key per signed-in circuit, and only for + // signed-in ones — the loop does not start for an anonymous visitor, which is most of this + // app's traffic. Configurable because the UI tests need it to fire inside a test's lifetime, + // and because "how stale may authority be" is an operational question, not a constant. + // + // Zero leaves the stock provider in place, which is the pre-revalidation behaviour. It exists + // so the UI test for this can be run against an app without it and actually go red — a test + // that cannot fail is not evidence. It is not a setting to reach for in production. + var revalidationInterval = TimeSpan.FromSeconds( + builder.Configuration.GetValue("Auth:RevalidationIntervalSeconds", 300)); + + if (revalidationInterval > TimeSpan.Zero) + builder.Services.AddScoped(sp => + new RevalidatingUserAuthenticationStateProvider( + sp.GetRequiredService(), + sp.GetRequiredService(), + revalidationInterval)); + // Rate limit login attempts: 5 per minute per IP, then queue/reject builder.Services.AddRateLimiter(options => { @@ -302,7 +343,8 @@ await context.SignInAsync( CookieAuthenticationDefaults.AuthenticationScheme, - PrincipalFor(user)); + PrincipalFor(user), + PersistentSession()); return Results.Redirect(IsLocalUrl(returnUrl) ? returnUrl : "/"); }) @@ -345,7 +387,8 @@ await context.SignInAsync( await context.SignInAsync( CookieAuthenticationDefaults.AuthenticationScheme, - PrincipalFor(admin)); + PrincipalFor(admin), + PersistentSession()); return Results.Redirect("/"); }); @@ -418,6 +461,24 @@ static ClaimsPrincipal PrincipalFor(AppUser user) new ClaimsIdentity(claims, CookieAuthenticationDefaults.AuthenticationScheme)); } +/// +/// What makes a sign-in outlive the browser session, and the other half of the pair with +/// — both sign-in routes use both, so neither can drift. +/// +/// Without IsPersistent the cookie goes out with no Expires at all, and a browser is +/// free to drop a session cookie whenever it decides the session ended. On a phone that is every +/// time the OS reclaims a backgrounded tab, and on the installed PWA every relaunch after one — +/// which is a coach putting their phone away at half time. No ExpireTimeSpan can rescue +/// that: it bounds the ticket the cookie carries, not the browser's willingness to keep the cookie. +/// +/// +/// A new instance per sign-in rather than one shared static: the cookie handler writes +/// IssuedUtc and ExpiresUtc onto the object it is handed, so a shared one would pin +/// every later sign-in to the expiry stamped on the first since boot. +/// +/// +static AuthenticationProperties PersistentSession() => new() { IsPersistent = true }; + static bool IsLocalUrl(string? url) { if (string.IsNullOrEmpty(url)) return false; diff --git a/src/FootballFormation.Web/Security/RevalidatingUserAuthenticationStateProvider.cs b/src/FootballFormation.Web/Security/RevalidatingUserAuthenticationStateProvider.cs new file mode 100644 index 0000000..bf9e5d8 --- /dev/null +++ b/src/FootballFormation.Web/Security/RevalidatingUserAuthenticationStateProvider.cs @@ -0,0 +1,47 @@ +using FootballFormation.Core.Services; +using Microsoft.AspNetCore.Components.Authorization; +using Microsoft.AspNetCore.Components.Server; + +namespace FootballFormation.Web.Security; + +/// +/// Re-checks a circuit's signed-in principal on a timer, and signs the circuit out when the account +/// behind it is gone or its authority has changed. +/// +/// Without this a circuit's principal is whatever the HTTP request that created it carried, for as +/// long as the tab stays open — the stock provider captures it once and never asks again. +/// OnValidatePrincipal in Program.cs runs per *HTTP request*, and a Blazor Server tab makes +/// almost none: the whole session after the first page load is SignalR messages. So deleting an +/// account or resetting its password left the open tab working until someone reloaded it, and that +/// is not a markup problem — CircuitCurrentUser reads this same provider, so the write guard +/// on every service was reading the stale principal too. +/// +/// +/// A failed check makes the circuit anonymous; it cannot clear the cookie, because a circuit has no +/// HTTP response to set headers on. The cookie is dropped on the browser's next real request, where +/// OnValidatePrincipal rejects it. Between the two the user sees the anonymous app, which is +/// the point — the authority is gone the moment this notices. +/// +/// +public sealed class RevalidatingUserAuthenticationStateProvider( + ILoggerFactory loggerFactory, + IServiceScopeFactory scopeFactory, + TimeSpan revalidationInterval) + : RevalidatingServerAuthenticationStateProvider(loggerFactory) +{ + protected override TimeSpan RevalidationInterval => revalidationInterval; + + protected override async Task ValidateAuthenticationStateAsync( + AuthenticationState authenticationState, CancellationToken cancellationToken) + { + // A scope of its own rather than an injected UserService, and not for the usual DbContext + // reason — UserService already opens its own short-lived context per call. It is that this + // provider *is* the circuit's AuthenticationStateProvider, and UserService depends on + // ICurrentUser, which depends on the AuthenticationStateProvider. Injecting it directly + // closes that loop and the container refuses to build. Do not "simplify" this. + await using var scope = scopeFactory.CreateAsyncScope(); + var users = scope.ServiceProvider.GetRequiredService(); + + return await users.FindForSessionAsync(authenticationState.User, cancellationToken) is not null; + } +} diff --git a/tests/FootballFormation.Core.Tests/UserServiceTests.cs b/tests/FootballFormation.Core.Tests/UserServiceTests.cs index 1ea2fbb..3158263 100644 --- a/tests/FootballFormation.Core.Tests/UserServiceTests.cs +++ b/tests/FootballFormation.Core.Tests/UserServiceTests.cs @@ -1,4 +1,6 @@ +using System.Security.Claims; using FootballFormation.Core.Models; +using FootballFormation.Core.Security; using Microsoft.EntityFrameworkCore; namespace FootballFormation.Core.Tests; @@ -210,6 +212,64 @@ public async Task Users_are_listed_by_name() Assert.Equal(["Anna", "Mila", "Zoe"], all.Value!.Select(u => u.DisplayName)); } + // ---------------------------------------------------------------- sessions read from a principal + + // Two callers ask "is this session still good" from a ClaimsPrincipal rather than an id and a + // stamp: the cookie handler on every HTTP request, and the circuit's revalidation loop. They + // share this overload so they cannot answer it differently. + + [Fact] + public async Task A_principal_carrying_a_live_accounts_claims_finds_that_account() + { + var created = await Users.CreateAsync("Jasper", "jasper", GoodPassword, UserRole.Admin); + var user = created.Value!; + + var found = await Users.FindForSessionAsync(PrincipalFor(user.Id, user.SecurityStamp)); + + Assert.Equal(user.Id, found?.Id); + } + + [Fact] + public async Task A_principal_whose_stamp_has_moved_on_finds_nothing() + { + var created = await Users.CreateAsync("Jasper", "jasper", GoodPassword, UserRole.Admin); + var user = created.Value!; + var principal = PrincipalFor(user.Id, user.SecurityStamp); + + await Users.SetPasswordAsync(user.Id, "brand-new-one"); + + Assert.Null(await Users.FindForSessionAsync(principal)); + } + + [Fact] + public async Task A_principal_missing_its_claims_finds_nothing_rather_than_throwing() + { + // A cookie issued before the security stamp shipped carries neither claim, and an anonymous + // circuit's principal carries nothing at all. Both are rejected, not trusted. + Assert.Null(await Users.FindForSessionAsync(new ClaimsPrincipal(new ClaimsIdentity()))); + Assert.Null(await Users.FindForSessionAsync((ClaimsPrincipal?)null)); + } + + [Fact] + public async Task A_principal_with_an_unreadable_user_id_finds_nothing() + { + var created = await Users.CreateAsync("Jasper", "jasper", GoodPassword, UserRole.Admin); + + var principal = new ClaimsPrincipal(new ClaimsIdentity([ + new Claim(AppClaims.UserId, "not-a-number"), + new Claim(AppClaims.SecurityStamp, created.Value!.SecurityStamp) + ])); + + Assert.Null(await Users.FindForSessionAsync(principal)); + } + + /// The claims the sign-in routes put on a cookie, as far as a session check reads them. + private static ClaimsPrincipal PrincipalFor(int userId, string securityStamp) => + new(new ClaimsIdentity([ + new Claim(AppClaims.UserId, userId.ToString()), + new Claim(AppClaims.SecurityStamp, securityStamp) + ])); + private const Core.Services.UserService.PasswordChangeResult PasswordChangeOk = Core.Services.UserService.PasswordChangeResult.Success; } diff --git a/tests/ui/global-setup.js b/tests/ui/global-setup.js index 35c5a69..fbcd6f4 100644 --- a/tests/ui/global-setup.js +++ b/tests/ui/global-setup.js @@ -21,7 +21,12 @@ import { ADMIN_STATE, BASE_URL, CHROMIUM_PATH, VISITOR_STATE } from './playwrigh import { addPlayer, createMatch, goto, waitForHandlers } from './helpers.js'; const SEED_PASSWORD = 'admin'; -const NEW_PASSWORD = 'uitest-admin-1'; + +// Exported because session.spec.js signs in through the real form rather than /dev/login — the +// cookie's own attributes are what that spec is about, and only /auth/login issues the real one. +export const ADMIN_USERNAME = 'admin'; +export const ADMIN_PASSWORD = 'uitest-admin-1'; +const NEW_PASSWORD = ADMIN_PASSWORD; // Named so a failure screenshot says where they came from. Shirt numbers are high to stay clear of // anything a spec invents. diff --git a/tests/ui/playwright.config.js b/tests/ui/playwright.config.js index 4a458a3..f8df24b 100644 --- a/tests/ui/playwright.config.js +++ b/tests/ui/playwright.config.js @@ -94,6 +94,10 @@ export default defineConfig({ APP_DATA_DIR: DATA_DIR, DOTNET_NOLOGO: '1', DOTNET_CLI_TELEMETRY_OPTOUT: '1', + // The circuit re-checks its principal on this interval; five minutes in production, which no + // test can wait for. Two seconds, not milliseconds: it is a database read per signed-in + // circuit, and every test here runs signed in. + Auth__RevalidationIntervalSeconds: '2', }, // A cold `dotnet build` on a first run is most of this. A published app skips that entirely and // is up in a couple of seconds, so the long timeout only ever applies to the local shape. diff --git a/tests/ui/specs/session.spec.js b/tests/ui/specs/session.spec.js new file mode 100644 index 0000000..f676082 --- /dev/null +++ b/tests/ui/specs/session.spec.js @@ -0,0 +1,156 @@ +// Staying signed in — the parts of authentication only a real browser can answer for. +// +// Two things live here. The cookie's own attributes, which decide whether the browser keeps it and +// whether it agrees to send it back; and the circuit's revalidation loop, which is what takes +// authority away from someone who is already looking at the app. No C# test can see either: the +// first is the browser reading Set-Cookie, and the second only happens over a live SignalR circuit. +import { test, expect } from '../fixtures.js'; +import { BASE_URL, VISITOR_STATE } from '../playwright.config.js'; +import { ADMIN_PASSWORD, ADMIN_USERNAME } from '../global-setup.js'; +import { clickFor, confirmDialog, fillField, goto, openDialog, submitDialog } from '../helpers.js'; + +const AUTH_COOKIE = 'ff.auth'; + +/** One account's row in the users table. */ +const userRow = (page, username) => + page.locator('.users-table .mud-table-body .mud-table-row', { hasText: username }).first(); + +/** + * Signs in through the form a person actually uses — /dev/login mints the same principal, but only + * /auth/login issues the cookie this file is about. + * + * `goto`, not `page.goto`, even though the form itself is plain `method="post"` HTML that needs no + * circuit to submit: the page *around* it is a Blazor component, and the render that arrives when + * the circuit connects resets the inputs. Filling the prerender and submitting therefore posts two + * empty strings, the app answers `/login?error=true`, and the sign-in silently does nothing — + * intermittently, because whether the re-render lands between the fill and the click depends on how + * busy the machine is. Waiting for handlers puts the typing after that render instead. + */ +async function signInThroughTheForm(page, username = ADMIN_USERNAME, password = ADMIN_PASSWORD) { + await goto(page, '/login'); + await page.fill('input[name="username"]', username); + await page.fill('input[name="password"]', password); + await page.click('button[type="submit"]'); + + // Asserted rather than waited on: a refused sign-in comes back to /login?error=true, which is + // still /login, so `waitForURL` would spend its whole timeout and then report a navigation that + // never happened instead of the credentials that were rejected. + await expect(page).not.toHaveURL(/\/login/, { timeout: 20_000 }); +} + +test.describe('signing in', () => { + test.use({ storageState: VISITOR_STATE }); + + test('leaves a cookie the browser will keep after it closes', async ({ page, context }) => { + await signInThroughTheForm(page); + + const cookie = (await context.cookies(BASE_URL)).find(c => c.name === AUTH_COOKIE); + expect(cookie, 'signing in should set the auth cookie').toBeDefined(); + + // -1 is Playwright for "session cookie" — no Expires on the header, so the browser is entitled + // to drop it the moment it decides the session ended. That is what it used to be, and on a + // phone reclaiming a backgrounded tab it meant signing in again on the touchline. + expect(cookie.expires, 'a session cookie does not survive the browser closing').toBeGreaterThan(0); + + const daysFromNow = (cookie.expires * 1000 - Date.now()) / 86_400_000; + expect(daysFromNow).toBeGreaterThan(13); + expect(daysFromNow).toBeLessThan(15); + + expect(cookie.httpOnly, 'script must not be able to read it').toBe(true); + }); + + test('leaves a cookie that survives arriving from another site', async ({ page, context }) => { + await signInThroughTheForm(page); + + // Strict is the value that fails this: it withholds the cookie on any cross-site navigation, + // including the plain link click below. + const cookie = (await context.cookies(BASE_URL)).find(c => c.name === AUTH_COOKIE); + expect(cookie.sameSite).toBe('Lax'); + }); +}); + +test.describe('an admin who is already signed in', () => { + test('is still signed in when following a link from another site', async ({ page }) => { + // A stand-in for WhatsApp, an email or a search result: a real top-level navigation from a + // different site, which is the case SameSite governs. The origin is fulfilled in the browser, + // so nothing leaves it and no DNS name has to exist. + await page.route('http://link.example/', route => route.fulfill({ + contentType: 'text/html', + body: `Match preferences`, + })); + + await page.goto('http://link.example/', { waitUntil: 'domcontentloaded' }); + await page.click('a'); + + // /settings is admin-only, so landing on it *is* the assertion that the cookie came along. With + // the cookie withheld this redirects to /login instead. + await expect(page).toHaveURL(/\/settings$/); + await expect(page.getByRole('heading', { name: 'Match Preferences', exact: false }).first()) + .toBeVisible(); + }); +}); + +// The circuit half of revocation. A Blazor Server tab makes almost no HTTP requests after its first +// page load, so `OnValidatePrincipal` — which runs per request — is not what takes authority away +// from someone already looking at the app. That is the revalidation loop, and this is the only place +// it is exercised: `Auth__RevalidationIntervalSeconds` is two seconds here against five minutes in +// production (see playwright.config.js). +test.describe('an account revoked while its owner is looking at the app', () => { + test('loses its authority without anyone reloading anything', async ({ page, browser }) => { + // Named per attempt, not per test: Playwright's CI retry re-runs this against the database the + // failed attempt left behind, and a username is unique — a fixed one would fail the retry on + // "already exists" rather than on whatever went wrong. See known_issues.md. + const username = `revoked-${Date.now()}`; + const password = 'revoked-admin-1'; + + await goto(page, '/users'); + await clickFor( + page.getByRole('button', { name: 'Add User' }), + () => expect(page.locator('.mud-dialog')).toBeVisible()); + + const dialog = await openDialog(page); + await fillField(dialog, 'Name', 'Revoked Admin'); + await fillField(dialog, 'Username', username); + await dialog.getByLabel('Password', { exact: false }).first().fill(password); + await dialog.getByLabel('Confirm password', { exact: false }).first().fill(password); + await submitDialog(page); + await expect(userRow(page, username)).toBeVisible(); + + // A second browser, signed in as that account and sitting on an admin page. + const theirContext = await browser.newContext({ storageState: VISITOR_STATE, baseURL: BASE_URL }); + try { + const theirPage = await theirContext.newPage(); + await signInThroughTheForm(theirPage, username, password); + + await goto(theirPage, '/users'); + await expect(theirPage.getByRole('heading', { name: 'Users', exact: false }).first()).toBeVisible(); + + // Delete the account from the first browser. Nothing in the second one makes a request + // through any of this — its circuit is open and idle, which is the whole scenario. + await goto(page, '/users'); + const menu = userRow(page, username).locator('.mud-menu button').first(); + const entry = page.locator('.mud-popover-open').getByText('Delete User', { exact: true }); + await clickFor(menu, () => expect(entry).toBeVisible()); + await entry.click(); + await confirmDialog(page, 'Delete'); + await expect(userRow(page, username)).toHaveCount(0); + + // The circuit notices on its own and RedirectToLogin force-loads — which is also the request + // that finally clears the cookie, since a circuit has no response to clear it on. + await theirPage.waitForURL(/\/login/, { timeout: 30_000 }); + } finally { + await theirContext.close(); + } + }); +}); + +test.describe('an anonymous visitor', () => { + test.use({ storageState: VISITOR_STATE }); + + test('carries no auth cookie at all', async ({ page, context }) => { + await goto(page, '/players'); + + const cookie = (await context.cookies(BASE_URL)).find(c => c.name === AUTH_COOKIE); + expect(cookie, 'reading is public and should mint nothing').toBeUndefined(); + }); +});