diff --git a/memex/Memex.Portal.Shared/Authentication/OAuthConnectController.cs b/memex/Memex.Portal.Shared/Authentication/OAuthConnectController.cs index 99b386c304..a7847706a9 100644 --- a/memex/Memex.Portal.Shared/Authentication/OAuthConnectController.cs +++ b/memex/Memex.Portal.Shared/Authentication/OAuthConnectController.cs @@ -199,11 +199,22 @@ public Task Authorize( return Task.FromResult(BadRequest(new { error = "invalid_request", error_description = "client_id and redirect_uri are required" })); } - // If user is not authenticated, redirect to login with return URL + // If user is not authenticated, redirect to login carrying THIS request as the return URL. + // + // 🚨 The returnUrl is a LOCAL path — `{Path}{QueryString}`, never `{Scheme}://{Host}{Path}…`. + // Every returnUrl SINK in the portal validates local-only (ReturnUrlPolicy.Sanitize here, + // LocalUrl.IsLocal in the GUI), because an unvalidated one is an open redirect. Those rules + // are correct and must not be relaxed; what was wrong was this SOURCE minting an absolute + // URL that no sink can accept. The login page's LocalOrNull then dropped it silently, the + // provider link was built with no returnUrl at all, and the user landed on "/" after signing + // in with the /authorize request gone — so no authorization code was ever issued and the MCP + // client waited forever for an "authentication successful" that could not arrive (#5074). + // The absolute form was also needless: the login page and this endpoint are the same origin. + // OnboardingMiddleware carries the same request the same way, for the same reason. if (User?.Identity?.IsAuthenticated != true) { - var authorizeUrl = $"{Request.Scheme}://{Request.Host}{Request.Path}{Request.QueryString}"; - var loginUrl = $"/login?returnUrl={Uri.EscapeDataString(authorizeUrl)}"; + var authorizePath = $"{Request.Path}{Request.QueryString}"; + var loginUrl = $"/login?returnUrl={Uri.EscapeDataString(ReturnUrlPolicy.Sanitize(authorizePath))}"; logger.LogInformation("OAuth /authorize: redirecting unauthenticated caller to {LoginUrl}", loginUrl); return Task.FromResult(Redirect(loginUrl)); } diff --git a/src/MeshWeaver.Documentation/Data/Architecture.md b/src/MeshWeaver.Documentation/Data/Architecture.md index 15f57eec65..76b4c146d8 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture.md +++ b/src/MeshWeaver.Documentation/Data/Architecture.md @@ -197,6 +197,7 @@ Each theme starts with its introductory page, followed by related architecture t - [Owner Injection](OwnerInjection) - [Permission API](PermissionApi) - [Invitation-Only Onboarding](InvitationOnlyOnboarding) +- [The Redirect-Target Contract](RedirectTargetContract) — every `returnUrl` sink validates local-only, so every source must mint local; a wrong source is refused rather than followed, which costs the whole flow and logs nothing - [Logon Actions](LogonActions) — per-user work at logon, run as the user - [Unanchored Security Reads](UnanchoredSecurityReads) — why the permission fold reads mesh-wide, and why pinning it to the viewer's partition is a silent revocation-fails-open bug - [A Denial Is an Answer](DenialIsAnAnswer) — a check on a hub with no evaluator grants Permission.All, and a refusal the mesh decided is rendered, never raised diff --git a/src/MeshWeaver.Documentation/Data/Architecture/GrantingAccess.md b/src/MeshWeaver.Documentation/Data/Architecture/GrantingAccess.md index 376f95d366..60923d99bc 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture/GrantingAccess.md +++ b/src/MeshWeaver.Documentation/Data/Architecture/GrantingAccess.md @@ -187,7 +187,9 @@ mcp create --node '{ > **This is enforced at the write boundary.** `AccessAssignmentGuard.IsScopeInvalid` (`src/MeshWeaver.Mesh.Contract/Services/AccessAssignmentGuard.cs`) refuses any `AccessAssignment` whose `mainNode` disagrees with the scope its path encodes — so `Admin/_Access/{id}` with `mainNode: ""` is rejected with *"has an EMPTY MainNode … this grants ROOT (every partition), not 'Admin'"*. A deliberately consistent root grant (`_Access/{id}` **and** `mainNode: ""`) still passes, because the test harness uses that shape; the access-control UI never offers it (`AccessAssignmentGuard.CanGrantAt` returns false at root). -> 🚨 **A SYSTEM-OWNED partition grants nobody write — and a grant COMMITTED TO A REPO can never be one.** A partition with a `{partition}/_GitSync` is rewritten from its repo on every sync, so the only identity that may write it is `system-security`. `AccessAssignmentGuard.IsForbiddenOnSystemOwned` refuses any `Admin`/`Editor`/unknown-role grant there (`Viewer`/`Commenter` entitlements and every `Denied` assignment stay legal), and `SystemOwnedAccessRetractionHandler` deletes such grants the moment the `_GitSync` is wired — so the shape is unsatisfiable, not merely inconvenient. +> 🚨 **A SYSTEM-OWNED partition grants nobody write — and a grant COMMITTED TO A REPO can never be one.** A partition with a **one-way** `{partition}/_GitSync` is rewritten from its repo on every sync, so the only identity that may write it is `system-security`. Two enforcement points, one predicate (`AccessAssignmentGuard.IsSystemOwned`): `IsForbiddenOnSystemOwned` refuses any `Admin`/`Editor`/unknown-role grant there, and `SystemOwnedAccessRetractionHandler` retracts **every** such grant — including the partition's last administrator — when the `_GitSync` is wired. `Viewer`/`Commenter` entitlements, every `Denied` assignment and `system-security` itself stay. A **bijective** (`twoWay: true`) sync is a different contract and is **not** system-owned: there the mesh nodes are the working copy and the people editing them keep write access. +> +> 🚨 **It did not hold until #5140, and the way it failed is worth knowing** — because three separate symptoms were one mechanism, and the middle one was the platform working as documented. The retraction sweep used to SPARE a partition's last administrator, deliberately: `SpaceAdminInvariantValidator` ("a space must always have at least one admin") would have refused that delete, so the sweep detected it up front rather than logging a guaranteed failure. But the same guard refuses granting a *second* Admin on a system-owned partition, so the sweep's stated escape — "retracted next time, once another admin exists" — could never arrive. A repo-owned space therefore kept one human `Admin` for ever, conferring write over content whose edits the next sync reverts, and a reporter reasonably read a working sweep as one that had never run. Both halves are fixed: the invariant exempts a system-owned partition (its administrator is the System identity, which holds `Permission.All` with no assignment node), and the sweep no longer spares anything. Fixing only one would have left the other silently in charge. > > The practical consequence for anyone editing a node repo: **a privileged `_Access/*.json` in a synced data tree is dead data that logs a `fail:` line on every sync.** That is exactly what shipped in `samples/Graph/Data` until #1245 — 75 refusals in 0.36 s per sync, and the import permanently `ImportedWithErrors`. `ShippedAccessGrantsTest` (`test/MeshWeaver.Graph.Test`) now fails the build if one is re-added. Platform admin belongs in `Auth:GlobalAdmins` (`GlobalAdminSeed` writes the `Admin/_Access` grant at startup); per-space write is granted on the live mesh. @@ -259,5 +261,6 @@ After creating or updating an assignment, run through this checklist: | `src/MeshWeaver.Mesh.Contract/Security/PermissionEvaluator.cs` | Synced query that aggregates AccessAssignments per user | | `src/MeshWeaver.Mesh.Contract/Services/AccessAssignmentGuard.cs` | Write-boundary guard: `mainNode` must equal the scope the path encodes; no write-conferring grant on a system-owned (GitSynced) partition | | `src/MeshWeaver.GitSync/SystemOwnedAccessRetractionHandler.cs` | Same predicate, applied as a sweep when a `_GitSync` is wired — retracts privileged grants that predate the sync | +| `src/MeshWeaver.Graph/Configuration/SpaceAdminInvariantValidator.cs` | "At least one admin per partition" — exempts system-managed mirrors AND system-owned (one-way GitSynced) partitions, so the sweep above can converge | | `test/MeshWeaver.Graph.Test/ShippedAccessGrantsTest.cs` | Structural guard: no `_Access` file committed to this repo may confer write, or name an undefined role | | `MeshWeaver.Plugins/src/MeshWeaver.Hosting.PostgreSql/PostgreSqlSchemaInitializer.cs` | `access_changed` trigger that rebuilds `partition_access` and `user_effective_permissions` | diff --git a/src/MeshWeaver.Documentation/Data/Architecture/RedirectTargetContract.md b/src/MeshWeaver.Documentation/Data/Architecture/RedirectTargetContract.md new file mode 100644 index 0000000000..d80ffd1783 --- /dev/null +++ b/src/MeshWeaver.Documentation/Data/Architecture/RedirectTargetContract.md @@ -0,0 +1,102 @@ +--- +Name: The Redirect-Target Contract — Both Ends of a returnUrl +Category: Architecture +Description: Every returnUrl sink validates local-only, so every returnUrl source must mint local — the second half of a rule that only had a guard on one end, and the silent failure mode that costs a whole sign-in flow. +Icon: +--- + +# The Redirect-Target Contract + +A `?returnUrl=` (also spelled `returnPath`, `returnTo`) carries "where the user was going" across a +flow that has to interrupt them — a sign-in, an onboarding form, a consent dialog. The portal has one +rule for it, and it is short: + +> **A redirect target is honoured only if it is LOCAL — a path on this host. Anything else falls back +> to the root.** + +`ReturnUrlPolicy.Sanitize` is that rule for the portal's server surface; the GUI has one +implementation of the same rule for its client surface. Both are correct, both are tested, and +neither is negotiable — an unvalidated redirect target is an open redirect, which is how a link that +looks like the tail of our own login flow sends a freshly-signed-in user somewhere else. + +## The contract has two ends, and only one of them had a guard + +The rule above is stated as a property of the **sink**: the thing that consumes an incoming target +and redirects to it. `RedirectSinksUseOnePolicyGuard` enforces exactly that, and the invariant it +pins is not "the policy is right" but "there is only one policy, and every sink uses it" — because +the defect it was written for was three hand-written copies of the rule, two of them subtly weaker +than the shared one. + +What nothing enforced is the **source**: the thing that MINTS a target and hands it onward. And the +two ends are not symmetric in what they cost when wrong: + +| | Wrong sink | Wrong source | +|---|---|---| +| What happens | a non-local target is followed | a non-local target is **refused** | +| Who is harmed | the user, by an open redirect | the flow, silently | +| How it surfaces | a security finding | "the feature intermittently does not work" | + +A wrong source produces no error anywhere. The sink refuses the target — doing its job — and the +flow simply carries on without it. The GUI's helper for a target it will not keep returns *nothing* +rather than substituting a destination, precisely so that a refused target does not become a +redirect to a place the user did not ask for. So the parameter is omitted, the flow completes, and +the user lands on `/`. + +## What that looked like in production + +`/authorize` (the OAuth authorization endpoint MCP clients use) minted its return target as an +absolute URL — scheme, host, path and query — and redirected to `/login` with it. Every sink +downstream then refused it, correctly, because an absolute URL is not local. The consequence, end to +end: + +1. An MCP client sends the user to `/authorize?…`. +2. The user is not signed in, so `/authorize` redirects to `/login?returnUrl=`. +3. The login page will not keep a non-local target, so the provider link carries **no** `returnUrl`. +4. The user signs in with Microsoft — successfully — and lands on `/`. +5. The `/authorize` request is gone. **No authorization code is ever issued.** +6. The client waits for an "authentication successful" that cannot arrive. + +Nothing in that sequence logs an error. From the user's side a sign-in that *worked* did not connect +anything, and from the client's side the credential looks expired — which is how it was reported +(#5074: *"Re-authenticating no longer helps"*). + +The absolute form also bought nothing: `/authorize` and `/login` are the same origin, so the scheme +and host were pure cost. `OnboardingMiddleware`, two files away, carries the same request the same +way and gets it right — `{Request.Path}{Request.QueryString}`, with a comment saying why that is +inherently local. One deviation, no guard to catch it. + +## The rule, for anyone minting one + +**Mint `{Request.Path}{Request.QueryString}`. Never `{Request.Scheme}://{Request.Host}…`.** Then pass +it through the shared policy anyway, so the intent is legible at the call site and the value is one +the policy demonstrably keeps. + +`RedirectSourcesMintLocalTargetsGuard` (`test/Memex.Portal.Shared.Test`) enforces the source end +across `memex/Memex.Portal.Shared` and `src/`. For each minting site it resolves the interpolated +value to its assignment in the same file: + +- an assignment composing a scheme and host → **refused**, naming the file and the expression; +- an inbound parameter of the enclosing method → a **forward**, not a mint; that value's safety is + the sink guard's subject where it lands; +- anything else → **fails closed**, because a site the guard cannot classify is not a clean one. + +It also asserts it found at least one minting site. A guard whose subject is renamed or moved out +from under it otherwise passes having checked nothing, and that failure is indistinguishable from +success. + +> 🚨 **A false positive here is worth reading, not suppressing.** On its first run the guard reported +> a site in each connect endpoint — both of which DOCUMENT their route as +> `GET /connect/github?returnPath={path}` in an XML comment, where the brace is a placeholder for a +> human reader and names no value at all. The fail-closed branch was right to report something it +> could not resolve; the fix was to make prose not a subject, not to loosen the classification. + +## Source references + +| File | Purpose | +|---|---| +| `memex/Memex.Portal.Shared/Authentication/ReturnUrlPolicy.cs` | The one rule for the portal's server surface | +| `memex/Memex.Portal.Shared/Authentication/OAuthConnectController.cs` | `/authorize` — mints the login return target | +| `memex/Memex.Portal.Shared/Authentication/OnboardingMiddleware.cs` | The same mint, done right, with the reason written down | +| `test/Memex.Portal.Shared.Test/RedirectSinksUseOnePolicyGuard.cs` | The sink end: every consumer routes through the one policy | +| `test/Memex.Portal.Shared.Test/RedirectSourcesMintLocalTargetsGuard.cs` | The source end: every minted target is one the policy keeps | +| `test/Memex.Portal.Shared.Test/OAuthAuthorizeReturnUrlIsLocalTest.cs` | `/authorize`'s own target, with a control on each side of the change | diff --git a/src/MeshWeaver.GitSync/SystemOwnedAccessRetractionHandler.cs b/src/MeshWeaver.GitSync/SystemOwnedAccessRetractionHandler.cs index 92183f5cc9..8336f46fa4 100644 --- a/src/MeshWeaver.GitSync/SystemOwnedAccessRetractionHandler.cs +++ b/src/MeshWeaver.GitSync/SystemOwnedAccessRetractionHandler.cs @@ -1,4 +1,3 @@ -using System.Collections.Immutable; using System.Reactive; using System.Reactive.Linq; using MeshWeaver.Mesh; @@ -39,16 +38,19 @@ namespace MeshWeaver.GitSync; /// /// What survives, deliberately: Viewer/Commenter entitlements (a /// purchase, a redeemed coupon, an admin-issued grant — the only door into a gated plugin), every -/// Denied assignment (that IS the gating), system-security itself (the identity -/// the importer writes under — retracting it would make the space unwritable by anything), and -/// the space's LAST administrator (#1120): the last-admin invariant -/// (SpaceAdminInvariantValidator — "a space must always have at least one admin") -/// outranks the sweep, so when no grant outside the doomed set still confers Admin, one admin -/// grant is spared — the earliest-created, i.e. the space's original owner — and logged at -/// Warning. The delete pipeline would refuse exactly that delete anyway; detecting it up front -/// turns a guaranteed rejection (formerly a fail-level "FAILED to retract") into the expected, -/// stated outcome. The spared grant converges like the rest of the sweep: the next time the -/// sync is (re)wired after another admin exists, it is retracted. +/// Denied assignment (that IS the gating), and system-security itself (the identity +/// the importer writes under — retracting it would make the space unwritable by anything). +/// +/// 🚨 The space's last administrator does NOT survive, and the pre-spare that used to hold +/// one back is gone (#5140). It existed because the last-admin invariant +/// (SpaceAdminInvariantValidator — "a space must always have at least one admin") refused +/// exactly that delete (#1120), which made issuing it a guaranteed rejection; the invariant now +/// exempts a system-owned partition, because its administrator is the System identity, which holds +/// Permission.All WITHOUT an assignment node. Keeping the pre-spare alongside that exemption +/// would have been worse than either alone: the sweep would still never issue the delete, so the +/// grant survived with nothing reporting it, and the stated escape — "retracted the next time the +/// sync is wired once another admin exists" — could never occur, because the write boundary refuses +/// that second Admin. That circularity is what made the documented end state unreachable. /// /// Best-effort by design (FailsCreateOnError stays false): a partition that cannot be /// swept must not block its own sync from being configured. Every retraction is logged at Warning @@ -122,51 +124,27 @@ private IObservable RetractAll(string partition, IList grants) if (doomed.Length == 0) return Observable.Return(Unit.Default); - // 🚨 The last-admin invariant OUTRANKS the sweep (#1120). SpaceAdminInvariantValidator - // refuses to delete a partition's last non-denied Admin assignment — a space must always - // keep at least one admin who can manage it (the System identity holds Permission.All - // WITHOUT an assignment node, so it does not count). Attempting that delete is therefore - // a guaranteed rejection: a known-valid guard outcome, not a fault. Detect it up front — - // when no grant OUTSIDE the doomed set still confers Admin, spare exactly one doomed - // admin grant (deterministically the earliest-created: the space's original owner) and - // retract the rest. The spared grant self-heals like the rest of the sweep: it is - // retracted the next time the sync is (re)wired once another admin exists. - var doomedPaths = doomed - .Select(c => c.Node.Path) - .ToImmutableHashSet(StringComparer.OrdinalIgnoreCase); - var adminSurvives = candidates.Any(c => - !doomedPaths.Contains(c.Node.Path) - && string.Equals(c.Node.NodeType, AccessAssignmentGuard.AccessAssignmentNodeType, - StringComparison.OrdinalIgnoreCase) - && AccessAssignmentGuard.GrantsAdmin(c.Assignment)); - - var spared = adminSurvives - ? default - : doomed - .Where(c => AccessAssignmentGuard.GrantsAdmin(c.Assignment)) - .OrderBy(c => c.Node.CreatedDate) - .ThenBy(c => c.Node.Path, StringComparer.Ordinal) - .FirstOrDefault(); - - if (spared.Node is not null) - { - logger?.LogWarning( - "[SystemOwned] keeping '{Path}' — '{Subject}' is the last administrator of " - + "'{Partition}' and a space must always have at least one admin. The grant still " - + "confers write access to a system-owned space; it is retracted the next time the " - + "sync is wired once another admin exists.", - spared.Node.Path, spared.Assignment?.AccessObject ?? "?", partition); - } - + // 🚨 EVERY doomed grant goes, the partition's last administrator included — and the + // pre-spare that used to hold one back is GONE (#5140). + // + // It was there because the last-admin invariant refused exactly that delete + // (SpaceAdminInvariantValidator, #1120), so issuing it was a guaranteed rejection and + // detecting it up front turned a fail-level "FAILED to retract" into a stated outcome. + // That reasoning was sound while the refusal stood. It no longer does: the invariant now + // exempts a system-owned partition, whose administrator is the System identity — which + // holds Permission.All WITHOUT an assignment node, which is why no grant has to survive + // to keep the partition manageable. + // + // Leaving the pre-spare in place would have made the exemption INERT, and in the worst way: + // the sweep would still never issue the delete, so the grant would survive with nothing + // reporting it, and each pass would spare it again. "Self-heals the next time the sync is + // wired once another admin exists" could never happen either, because the write boundary + // refuses that second Admin. That circularity is what made the documented end state + // unreachable, and removing this is the half that actually opens it. var doomedList = doomed .Select(c => c.Node.Path) - .Where(p => spared.Node is null - || !string.Equals(p, spared.Node.Path, StringComparison.OrdinalIgnoreCase)) .ToArray(); - if (doomedList.Length == 0) - return Observable.Return(Unit.Default); - logger?.LogWarning( "[SystemOwned] retracting {Count} privileged grant(s) — the partition is now system-owned " + "and only {System} may write it: {Paths}", diff --git a/src/MeshWeaver.Graph/Configuration/SpaceAdminInvariantValidator.cs b/src/MeshWeaver.Graph/Configuration/SpaceAdminInvariantValidator.cs index cf37b55cb5..d5f4a7a052 100644 --- a/src/MeshWeaver.Graph/Configuration/SpaceAdminInvariantValidator.cs +++ b/src/MeshWeaver.Graph/Configuration/SpaceAdminInvariantValidator.cs @@ -25,6 +25,20 @@ namespace MeshWeaver.Graph; /// Scope: assignments at {partition}/_Access/... only. Root-scope global-admin /// assignments (_Access/{user}_Access, no partition prefix) are exempt. /// +/// 🚨 Two kinds of partition are exempt because they have no HUMAN administrator to keep, +/// and holding one there makes their documented end state unreachable: a system-managed mirror +/// (WellKnownPartitions.IsMirror), and a SYSTEM-OWNED partition — one with a ONE-WAY +/// _GitSync, whose content is rewritten from its repo on every sync so that the only writer +/// whose edits survive is the importer identity. For the second, this invariant used to meet +/// AccessAssignmentGuard.IsForbiddenOnSystemOwned head-on: that rule refuses GRANTING another +/// Admin there, this one refused REMOVING the existing one, so +/// SystemOwnedAccessRetractionHandler's sweep could only spare the last grant on every run +/// and the partition kept a human Admin over content nobody may usefully edit (#5140). This +/// exemption is HALF the fix — the sweep's own pre-spare had to go with it, or this one is inert and +/// nothing reports that it is. A BIJECTIVE +/// sync is deliberately NOT exempt — there the mesh nodes are somebody's working copy, editing them +/// is the point, and the people doing it must keep write access. +/// /// Deadlock-safe read. The remaining-admin count comes from /// (the read-side query provider — a direct /// store/DB read), NOT a workspace.GetQuery synced subscription: the validator runs @@ -101,6 +115,65 @@ public IObservable Validate(NodeValidationContext context) if (meshService is null) return Observable.Return(NodeValidationResult.Valid()); + // 🚨 A SYSTEM-OWNED partition has no user administrator to keep, and holding one there makes + // the documented end state UNREACHABLE. Once a partition has a one-way `_GitSync` its + // content is rewritten from the repo on every sync, so the only identity that may write it + // is the importer's (AccessAssignmentGuard.IsSystemOwned). Two rules then meet head-on: + // AccessAssignmentGuard.IsForbiddenOnSystemOwned refuses GRANTING another Admin there, and + // this invariant refused REMOVING the existing one — so + // SystemOwnedAccessRetractionHandler's sweep could only spare that last grant, every time it + // ran, and the partition kept a human Admin over content nobody may usefully edit (whose + // edits the next sync reverts). Exempting the invariant is HALF of what lets the sweep + // converge; the other half is the sweep's own pre-spare, removed in the same change, because + // this exemption is inert while that is still refusing to issue the delete. Both halves: + // the partition does keep an administrator, and it is the System identity. The existing + // mirror-partition exemption above is the same reasoning one step earlier. + // + // The read is the shared authoritative one (storage, then static/config providers) rather + // than a stream subscription — this runs inside the delete/update pipeline ON the owning + // partition hub, where a synced read that round-trips back to that hub deadlocks. It is + // paid only once this operation is already known to remove an admin, so an ordinary + // _Access write costs nothing. + return NodeTypeAccessRuleGate + .ReadSubjectNode(hub, AccessAssignmentGuard.SyncConfigPath(partition)) + .Take(1) + .SelectMany(sync => + AccessAssignmentGuard.IsSystemOwned(sync, hub.JsonSerializerOptions) + ? SystemOwnedIsExempt(partition, node.Id) + : LastAdminVerdict(meshService, partition, node, context.Operation)) + .Catch(ex => + { + // Guard-rail, not a security boundary (RLS already gates who may write here): + // a lookup failure/timeout falls through to Valid rather than wedging all + // admin churn. Logged so a genuinely broken read-side is visible. + logger?.LogWarning(ex, + "SpaceAdminInvariantValidator: admin-count query failed for '{Partition}' — allowing", partition); + return Observable.Return(NodeValidationResult.Valid()); + }); + } + + /// + /// The system-owned outcome: allowed, and said out loud. Access disappearing silently is its + /// own bug class, and this is the branch that lets the last human Admin of a repo-owned space + /// actually go — so it states which rule permitted it rather than looking like no rule ran. + /// + private IObservable SystemOwnedIsExempt(string partition, string id) + { + logger?.LogInformation( + "Allowed last-admin removal on SYSTEM-OWNED partition '{Partition}' (assignment " + + "'{Id}'): it has {Partition}/_GitSync and is rewritten from its repo on every sync, " + + "so its administrator is the importer identity, not a person.", + partition, id, partition); + return Observable.Return(NodeValidationResult.Valid()); + } + + /// + /// The ordinary verdict: block when no OTHER non-denied Admin assignment remains in + /// {partition}/_Access. + /// + private IObservable LastAdminVerdict( + IMeshService meshService, string partition, MeshNode node, NodeOperation operation) + { // Count the OTHER non-denied Admin assignments remaining in {partition}/_Access. return meshService .Query(MeshQueryRequest.FromQuery( @@ -119,21 +192,15 @@ public IObservable Validate(NodeValidationContext context) logger?.LogInformation( "Blocked last-admin removal on partition '{Partition}' (assignment '{Id}', op {Op})", - partition, node.Id, context.Operation); + partition, node.Id, operation); return NodeValidationResult.Invalid( $"Cannot remove the last administrator of '{partition}'. " + "A space must always have at least one admin — grant another user the Admin role first.", NodeRejectionReason.ValidationFailed); - }) - .Catch(ex => - { - // Guard-rail, not a security boundary (RLS already gates who may write here): - // a lookup failure/timeout falls through to Valid rather than wedging all - // admin churn. Logged so a genuinely broken read-side is visible. - logger?.LogWarning(ex, - "SpaceAdminInvariantValidator: admin-count query failed for '{Partition}' — allowing", partition); - return Observable.Return(NodeValidationResult.Valid()); }); + // No Catch here: the caller wraps BOTH reads — the system-owned probe and this count — in + // the one fall-through-to-Valid handler, so a fault in either takes the same documented + // path. A second handler here would swallow the probe's fault before the caller saw it. } /// diff --git a/test/Memex.Portal.Shared.Test/OAuthAuthorizeReturnUrlIsLocalTest.cs b/test/Memex.Portal.Shared.Test/OAuthAuthorizeReturnUrlIsLocalTest.cs new file mode 100644 index 0000000000..f443b6913a --- /dev/null +++ b/test/Memex.Portal.Shared.Test/OAuthAuthorizeReturnUrlIsLocalTest.cs @@ -0,0 +1,132 @@ +using System; +using System.Threading.Tasks; +using Memex.Portal.Shared.Authentication; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.Logging.Abstractions; +using Xunit; + +namespace Memex.Portal.Shared.Test; + +/// +/// The returnUrl that /authorize hands to /login must be a LOCAL path. +/// +/// The defect (#5074). Every returnUrl SINK in the portal validates +/// local-only — here, the GUI's own single copy of the +/// same rule on the login page — because an unvalidated redirect target is an open redirect. +/// Those rules are right. What was wrong was this SOURCE: /authorize minted +/// {Scheme}://{Host}{Path}{QueryString}, an absolute URL that no sink can accept. The +/// login page dropped it silently (its local-or-null helper substitutes nothing rather than a +/// destination), so the provider link carried no returnUrl at all, the user landed on +/// "/" after signing in, and the /authorize request was gone. No authorization +/// code was ever issued, so an MCP client waited forever for an "authentication successful" +/// that could not arrive — and the user, who HAD signed in successfully, read it as the token +/// having expired again. +/// +/// Why the assertion is stated as a round-trip through the policy rather than as a +/// second hand-written "is it local?" check: there is exactly one such rule per assembly, and a +/// re-implementation is the bug the sink guard exists to prevent — a hand-written copy that was +/// subtly weaker than the shared rule has already shipped once. Asking the real policy whether it +/// would keep this value is the same question every sink downstream asks. +/// +/// SHOULD-FAIL-IF the absolute form comes back: +/// goes red, because +/// pins that the +/// policy sends that shape to "/". The pair is the control on each side of the change: a +/// policy that accepted everything would fail the second, and one that accepted nothing would +/// fail the first, so no constant satisfies both. +/// +public class OAuthAuthorizeReturnUrlIsLocalTest +{ + /// + /// The unauthenticated branch of /authorize resolves neither the code store nor the + /// token service (both are lazily-read properties reached only after the identity check), so + /// the action needs no services at all to exercise it. + /// + private sealed class NoServices : IServiceProvider + { + public object? GetService(Type serviceType) => null; + } + + private const string ClientId = "aB3-derived-client-id"; + private const string RedirectUri = "http://localhost:8765/callback"; + + /// The query an MCP client actually sends, escaped as it arrives on the wire. + private const string AuthorizeQuery = + "?response_type=code" + + "&client_id=" + ClientId + + "&redirect_uri=http%3A%2F%2Flocalhost%3A8765%2Fcallback" + + "&state=6ff1c0de&code_challenge=E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" + + "&code_challenge_method=S256"; + + private static OAuthConnectController Controller() => + new(new NoServices(), NullLogger.Instance) + { + ControllerContext = new ControllerContext + { + // No authenticated identity — DefaultHttpContext's principal has an + // unauthenticated identity, which is the branch under test. + HttpContext = new DefaultHttpContext + { + Request = + { + Scheme = "https", + Host = new HostString("memex.systemorph.com"), + Path = "/authorize", + QueryString = new QueryString(AuthorizeQuery), + }, + }, + }, + }; + + private static async Task LoginRedirect() + { + var result = await Controller().Authorize( + response_type: "code", + client_id: ClientId, + redirect_uri: RedirectUri, + state: "6ff1c0de", + scope: null, + code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", + code_challenge_method: "S256"); + + var redirect = Assert.IsType(result); + return redirect.Url; + } + + /// The returnUrl value as the login page will read it. + private static async Task CapturedReturnUrl() + { + var url = await LoginRedirect(); + const string prefix = "/login?returnUrl="; + Assert.StartsWith(prefix, url, StringComparison.Ordinal); + return Uri.UnescapeDataString(url[prefix.Length..]); + } + + [Fact] + public async Task TheLoginReturnUrlSurvivesThePolicyEverySinkApplies() + { + var returnUrl = await CapturedReturnUrl(); + + Assert.Equal(returnUrl, ReturnUrlPolicy.Sanitize(returnUrl)); + } + + [Fact] + public async Task TheLoginReturnUrlCarriesTheWholeAuthorizeRequest() + { + var returnUrl = await CapturedReturnUrl(); + + // Path AND query: dropping the query would send the user back to a /authorize with no + // client_id, which 400s — a different way of never issuing a code. + Assert.Equal("/authorize" + AuthorizeQuery, returnUrl); + } + + [Theory] + [InlineData("https://memex.systemorph.com/authorize?response_type=code&client_id=x")] + [InlineData("http://memex.systemorph.com/authorize")] + public void TheAbsoluteFormThisEndpointUsedToMint_IsRefusedByTheSamePolicy(string absolute) + // The control: this is why an absolute returnUrl never reached the login provider. + // It is not that the policy is wrong — an absolute target IS how an open redirect is + // spelled — it is that the source had to mint something the policy can keep. + => Assert.Equal("/", ReturnUrlPolicy.Sanitize(absolute)); +} diff --git a/test/Memex.Portal.Shared.Test/RedirectSourcesMintLocalTargetsGuard.cs b/test/Memex.Portal.Shared.Test/RedirectSourcesMintLocalTargetsGuard.cs new file mode 100644 index 0000000000..45837cfd8a --- /dev/null +++ b/test/Memex.Portal.Shared.Test/RedirectSourcesMintLocalTargetsGuard.cs @@ -0,0 +1,211 @@ +#pragma warning disable CS1591 + +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.IO; +using System.Linq; +using System.Text.RegularExpressions; +using Xunit; + +namespace Memex.Portal.Shared.Test; + +/// +/// The other half of the redirect contract: every place that MINTS a ?returnUrl= must mint a +/// LOCAL path. +/// +/// Why a second guard. RedirectSinksUseOnePolicyGuard pins the SINK side — a +/// redirect that consumes a request-supplied target routes it through the one shared policy. That +/// guard was green throughout #5074, and correctly so: nothing consumed an unvalidated target. The +/// defect was at the SOURCE. /authorize minted its own return target as +/// {Scheme}://{Host}{Path}{QueryString}, and because every sink downstream refuses a +/// non-local target — which is the sinks doing their job — the value was DISCARDED rather than +/// followed. The login page substitutes nothing for a target it will not keep, so the sign-in +/// completed and landed on "/" with the original request gone: no authorization code, and an +/// MCP client waiting for a completion that could not arrive. +/// +/// So the two guards answer different questions. The sink guard asks "is an incoming target +/// validated?" — a security question. This one asks "is an OUTGOING target one that validation will +/// keep?" — a correctness question, whose failure mode is silent and looks like an expired +/// credential. Neither subsumes the other, and the first cannot see the second by construction. +/// +/// How the rule is decided. For each minting site the guard resolves the interpolated +/// value to its assignment IN THE SAME FILE and refuses one composed as an absolute URL (a scheme +/// separator, or the request's scheme read at all). A value that is an inbound parameter of the +/// enclosing method is a FORWARD, not a mint, and belongs to the sink guard. Anything the guard +/// cannot resolve to one of those two fails closed and says so — an unresolvable site is not a +/// clean one. +/// +public class RedirectSourcesMintLocalTargetsGuard +{ + /// + /// The names a redirect target travels under — the sink guard's list, unchanged. + /// ImmutableArray, not string[]: an array is a mutable collection whatever the + /// field's modifiers say, and the collections policy admits a static readonly only for a + /// constant lookup that cannot be written at runtime. + /// + private static readonly ImmutableArray TargetNames = + ["returnUrl", "returnTo", "returnPath", "redirectUrl", "redirectTo"]; + + /// + /// A MINT: the parameter name inside a string, immediately followed by an interpolation hole. + /// "?returnUrl={x}" mints; "?returnPath=/" is a constant and mints nothing. + /// + private static readonly Regex MintSite = new( + @"[?&](" + string.Join("|", TargetNames) + @")=\{([^{}]{1,200})\}", + RegexOptions.Compiled | RegexOptions.IgnoreCase); + + /// Identifiers in an interpolated expression, innermost-first is not needed — all are checked. + private static readonly Regex Identifier = new(@"[A-Za-z_][A-Za-z0-9_]*", RegexOptions.Compiled); + + /// Names that are calls/types in the expression rather than the value being carried. + private static readonly ImmutableArray NotAValue = + ["Uri", "EscapeDataString", "ReturnUrlPolicy", "Sanitize", "string", "Join", "Format", + "LocalUrl", "LocalOrRoot", "LocalOrNull", "InstanceConnectFlow"]; + + private static IEnumerable Roots() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "MeshWeaver.slnx"))) + dir = dir.Parent; + Assert.NotNull(dir); + // Both trees that compose portal URLs: the auth/portal surface and the framework. + ImmutableArray relatives = [Path.Combine("memex", "Memex.Portal.Shared"), "src"]; + foreach (var relative in relatives) + { + var root = Path.Combine(dir!.FullName, relative); + Assert.True(Directory.Exists(root), $"{root} not found — update this guard's roots."); + yield return root; + } + } + + [Fact] + public void Every_minted_return_target_is_a_local_path() + { + // One pass, no accumulator: the sites are a SEQUENCE and both readings below are derived + // from it, so the vacuity check and the verdicts cannot disagree about what was scanned. + var sites = Roots() + .SelectMany(root => Directory.EnumerateFiles(root, "*.cs", SearchOption.AllDirectories)) + .SelectMany(file => + { + var text = File.ReadAllText(file); + return MintSite.Matches(text) + // 🚨 Prose, not code. Both connect endpoints DOCUMENT their routes as + // `GET /connect/github?returnPath={path}` in an XML comment, where `{path}` is a + // placeholder for the reader and names no C# value at all — the guard's + // fail-closed branch reported it, correctly, as something it could not resolve. + // A comment mints nothing, so the subject is the code line. + .Where(m => !IsInAComment(text, m.Index)) + .Select(m => (Name: Path.GetFileName(file), Text: text, Match: m)); + }) + .ToImmutableArray(); + + var offenders = sites + .Select(site => Offence(site.Text, site.Name, site.Match)) + .Where(o => o is not null) + .ToImmutableArray(); + + // 🚨 The guard must have a subject. A refactor that renames these parameters, or moves the + // portal surface out of this repo, would otherwise leave it green having checked nothing. + Assert.True(!sites.IsEmpty, + "No returnUrl/returnPath minting site was found in either root. Either the roots above " + + "are stale or the parameter names changed — a guard with no subject passes vacuously."); + + Assert.True( + offenders.IsEmpty, + "These sites hand a redirect target onward that the shared local-only policy will NOT " + + "keep. Every sink refuses a non-local target (correctly — that is the open-redirect " + + "defence), so such a value is DISCARDED rather than followed: the user completes the " + + "flow and lands on \"/\" with the original request gone, which reads as the flow " + + "silently not working rather than as a rejected redirect (#5074). Mint " + + "{Request.Path}{Request.QueryString}, never {Request.Scheme}://{Request.Host}… — the " + + "login page and the endpoint are the same origin, so the absolute form carries no " + + "information and costs the whole flow.\n " + + string.Join("\n ", offenders)); + } + + /// + /// The finding for one minting site, or null when the site is clean. + /// + /// 🚨 The unresolvable branch tests Any, not All. All(v => !v.Resolved) + /// reports only when NOTHING resolved, so an expression combining a known local with one + /// unclassifiable member would pass — which contradicts the fail-closed contract this guard is + /// built on, and in the one branch whose whole purpose is not to assume. If any carried value + /// cannot be classified, the site is not clean. + /// + private static string? Offence(string text, string name, Match m) + { + var carried = Identifier.Matches(m.Groups[2].Value) + .Select(i => i.Value) + .Where(i => !NotAValue.Contains(i)) + .Distinct(StringComparer.Ordinal) + .ToImmutableArray(); + + if (carried.IsEmpty) + return $"{name}: {m.Value} — the guard could not tell what value this carries."; + + var verdicts = carried.Select(id => Classify(text, id, m.Index)).ToImmutableArray(); + + if (verdicts.Any(v => v.Absolute)) + return $"{name}: {m.Value} — carries {verdicts.First(v => v.Absolute).Detail}, " + + "which is an ABSOLUTE url."; + + var unresolved = verdicts.Where(v => !v.Resolved).ToImmutableArray(); + return unresolved.IsEmpty + ? null + : $"{name}: {m.Value} — no assignment or parameter found for " + + $"{string.Join("/", unresolved.Select(v => v.Detail))}; the guard fails closed " + + "rather than assuming it is local."; + } + + /// + /// True when the offset sits on a line whose code has not started — a // / /// + /// line comment, or a line inside a block comment. Deliberately line-based rather than a real + /// lexer: the only thing it has to separate is a documented ROUTE from a composed URL. + /// + private static bool IsInAComment(string text, int index) + { + var lineStart = text.LastIndexOf('\n', Math.Max(0, index - 1)) + 1; + var prefix = text[lineStart..index].TrimStart(); + return prefix.StartsWith("//", StringComparison.Ordinal) + || prefix.StartsWith("*", StringComparison.Ordinal) + || prefix.StartsWith("/*", StringComparison.Ordinal); + } + + /// What a carried identifier resolves to, looking only at the file it is used in. + private readonly record struct Verdict(bool Resolved, bool Absolute, string Detail); + + /// + /// Resolves to its nearest preceding assignment, or to an inbound + /// parameter of the enclosing method. An assignment whose right-hand side composes a scheme and + /// host is ABSOLUTE; a parameter is a forwarded inbound value (the sink guard's subject). + /// + private static Verdict Classify(string text, string id, int useIndex) + { + var before = text[..useIndex]; + + var assignment = new Regex( + @"\b(?:var|string\??)\s+" + Regex.Escape(id) + @"\s*=\s*([^;]{0,400});", + RegexOptions.Compiled); + var assignments = assignment.Matches(before); + if (assignments.Count > 0) + { + var rhs = assignments[^1].Groups[1].Value; + // Either spelling of "this is a whole URL": the separator itself, or the request's + // scheme, which in this codebase is only ever read to build one. + var absolute = rhs.Contains("://", StringComparison.Ordinal) + || rhs.Contains("Request.Scheme", StringComparison.Ordinal); + return new Verdict(true, absolute, absolute ? $"`{id}` = {rhs.Trim()}" : $"`{id}`"); + } + + // A parameter of the enclosing declaration — a forwarded inbound value, governed by the + // sink guard where it lands. Looked for in the nearest preceding parameter list. + var parameter = new Regex( + @"\(\s*(?:[^()]{0,600}?[,\s])?(?:\[[^\]]{0,80}\]\s*)?[A-Za-z_][A-Za-z0-9_.<>?\[\]]*\s+" + + Regex.Escape(id) + @"\s*[,)=]", + RegexOptions.Compiled); + return parameter.IsMatch(before) + ? new Verdict(true, false, $"`{id}` (inbound parameter)") + : new Verdict(false, false, $"`{id}`"); + } +} diff --git a/test/Memex.Portal.Shared.Test/TokenMintedOnOneReplicaValidatesOnAnotherTest.cs b/test/Memex.Portal.Shared.Test/TokenMintedOnOneReplicaValidatesOnAnotherTest.cs new file mode 100644 index 0000000000..ec210ee243 --- /dev/null +++ b/test/Memex.Portal.Shared.Test/TokenMintedOnOneReplicaValidatesOnAnotherTest.cs @@ -0,0 +1,89 @@ +using System.Reactive.Linq; +using Memex.Portal.Shared.Authentication; +using MeshWeaver.Fixture; +using MeshWeaver.Hosting.Monolith.TestBase; +using MeshWeaver.Mesh; +using MeshWeaver.Mesh.Services; +using MeshWeaver.Messaging; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Xunit; + +namespace Memex.Portal.Shared.Test; + +/// +/// A bearer token minted by one replica is accepted by a replica that never saw it minted. +/// +/// Why this exists as a test and not as an argument. #5074 reported an MCP session that +/// alternated between working and answering 401 "token expired", and reasoned from outside the +/// cluster that the token must live in the issuing process's memory. It no longer does — +/// ApiTokenService reads the index row and the token row STRAIGHT off the shared storage +/// adapter, and CreateToken confirms both are committed before the raw token leaves the +/// server. But "it no longer does" is a claim about code, and the two shapes it replaced +/// (an in-memory dictionary, then a read that rode the mesh's cross-silo per-node-hub path) were +/// both invisible to every single-instance test in the suite. A claim that only a second replica +/// could falsify needs a second replica, or it is not being tested at all. +/// +/// Two ApiTokenService instances over one storage adapter is the same relationship two +/// pods have to one Postgres: independent service state, one authoritative store. The companion +/// case for authorization codes is OAuthCodeStoreTest's two-store exchange. +/// +/// SHOULD-FAIL-IF validation ever reads instance-local state again: the mint happens on +/// one instance and every assertion is made on the other. +/// is the control in the other +/// direction — a validator hardwired to Valid would pass the first test and fail this one, and it +/// also pins the distinction the 401/503 split rests on: an unknown token is +/// , never . +/// +public class TokenMintedOnOneReplicaValidatesOnAnotherTest(ITestOutputHelper output) + : MonolithMeshTestBase(output) +{ + private const string UserId = "replica-split-user"; + private const string UserEmail = "replica-split@example.com"; + + /// + /// One "replica": its own service instance, the SHARED store. Nothing is passed between the two + /// instances except the raw token, exactly as a client passes it. + /// + private ApiTokenService Replica() => + new( + Mesh.ServiceProvider.GetRequiredService(), + Mesh, + Mesh.ServiceProvider.GetRequiredService(), + Mesh.ServiceProvider.GetRequiredService>()); + + [Fact] + public async Task AFreshTokenIsValidOnTheReplicaThatDidNotMintIt() + { + var minting = Replica(); + var validating = Replica(); + + var created = await minting + .CreateToken(UserId, "Replica Split", UserEmail, "MintedOnA") + .Should().Emit(); + + var verdict = await validating.Validate(created.RawToken).Should().Emit(); + + verdict.Status.Should().Be( + TokenValidationStatus.Valid, + "the token's rows are in the shared store before the raw token is handed out, so the " + + "replica that receives the next request resolves it without having minted it — " + + "reason given: {0}", + verdict.Reason ?? "(none)"); + verdict.Token!.UserId.Should().Be(UserId); + } + + [Fact] + public async Task AnUnknownTokenIsADefinitiveNegativeOnEitherReplica() + { + var verdict = await Replica() + .Validate("mw_this-token-was-never-minted-anywhere") + .Should().Emit(); + + verdict.Status.Should().Be( + TokenValidationStatus.Invalid, + "an absent row is a verdict, not a failure to reach one — Unavailable is reserved for a " + + "store that could not be read, and answering 401 for that is what makes a client " + + "discard a credential that was never wrong"); + } +} diff --git a/test/MeshWeaver.Graph.Test/LastAdminInvariantExemptsSystemOwnedPartitionsTest.cs b/test/MeshWeaver.Graph.Test/LastAdminInvariantExemptsSystemOwnedPartitionsTest.cs new file mode 100644 index 0000000000..ff9f1dae8a --- /dev/null +++ b/test/MeshWeaver.Graph.Test/LastAdminInvariantExemptsSystemOwnedPartitionsTest.cs @@ -0,0 +1,150 @@ +using System.Reactive.Linq; +using System.Text.Json.Nodes; +using MeshWeaver.Fixture; +using MeshWeaver.Graph.Configuration; +using MeshWeaver.Hosting.Monolith.TestBase; +using MeshWeaver.Mesh; +using MeshWeaver.Mesh.Security; +using MeshWeaver.Mesh.Services; +using Microsoft.Extensions.DependencyInjection; +using Xunit; + +namespace MeshWeaver.Graph.Test; + +/// +/// The last-admin invariant does not apply to a SYSTEM-OWNED partition — and still applies to every +/// other one. +/// +/// The defect (#5140), which arrived wearing three faces. A partition with a one-way +/// _GitSync is rewritten from its repo on every sync, so the only identity that may usefully +/// write it is the importer's. Two rules then met head-on: +/// AccessAssignmentGuard.IsForbiddenOnSystemOwned refuses GRANTING another Admin there, and +/// refused REMOVING the one that is already present. So +/// SystemOwnedAccessRetractionHandler's sweep could only ever SPARE that last grant — which +/// its own documentation says it does, deliberately, rather than issuing a delete the pipeline would +/// refuse. The three reported symptoms (a retraction that "never fired", a removal that is blocked, +/// a direct write that is accepted) are one mechanism: the documented end state was unreachable +/// by construction. That, not any of the three, is the defect. +/// +/// Why the exemption is sound rather than a relaxation. The invariant exists so a space +/// always has somebody who can manage it. A system-owned space HAS one — the importer identity, +/// which is also the only writer whose edits survive a sync. The mirror-partition exemption already +/// in the validator is the same reasoning one step earlier. A partition that is neither mirror nor +/// system-owned always has a human admin (its creator is granted one at create), and this invariant +/// only ever fires while REMOVING an admin, so it cannot lock out a partition that has none. +/// +/// SHOULD-FAIL-IF the exemption is widened into "the invariant is off". There is a case +/// on each side, over the same partition shape and the same delete: +/// must pass and +/// must still refuse. The third, +/// , pins the discriminator that a +/// coarser fix would lose: a BIJECTIVE sync is not system-owned, because there the mesh nodes are +/// the working copy and the people editing them must keep write access. A fix keyed on "has a +/// _GitSync" would pass the first two and fail that one. +/// +public class LastAdminInvariantExemptsSystemOwnedPartitionsTest(ITestOutputHelper output) + : MonolithMeshTestBase(output) +{ + private const string AdminUser = "the-only-admin"; + + private IStorageAdapter Storage => Mesh.ServiceProvider.GetRequiredService(); + + private SpaceAdminInvariantValidator Validator => new(Mesh); + + /// + /// The partition's single Admin grant, written to the store so the validator's remaining-admin + /// count can see it and find nothing else. + /// + private async Task TheOnlyAdminGrant(string partition) + { + var node = new MeshNode(AdminUser + "_Access", $"{partition}/_Access") + { + Name = "Admin grant", + NodeType = "AccessAssignment", + State = MeshNodeState.Active, + Content = new AccessAssignment + { + AccessObject = AdminUser, + Roles = [new RoleAssignment { Role = Role.Admin.Id }], + }, + }; + await Storage.Write(node, Mesh.JsonSerializerOptions).Should().Emit(); + return node; + } + + /// Wires {partition}/_GitSync with the given direction. + private Task WireSync(string partition, bool twoWay) => + Storage.Write( + new MeshNode(AccessAssignmentGuard.SyncConfigId, partition) + { + Name = "Sync", + NodeType = "GitHubSyncConfig", + State = MeshNodeState.Active, + // A JsonObject is one of the three shapes IsTwoWay reads, and the one the node + // builders produce — so this exercises the same tolerance production does. + Content = new JsonObject + { + ["repository"] = "Systemorph/Memex", + ["twoWay"] = twoWay, + }, + }, + Mesh.JsonSerializerOptions) + .Should().Emit(); + + /// The verdict for deleting on its own (the partition stays). + private Task DeleteVerdict(MeshNode grant) => + Validator.Validate(new NodeValidationContext + { + Operation = NodeOperation.Delete, + Node = grant, + // The grant alone is the delete root: the partition is NOT going away, which is the + // only case the invariant guards (a partition-rooted cascade is already exempt). + DeleteCascadeRootPath = grant.Path, + }) + .Should().Emit(); + + [Fact] + public async Task OnASystemOwnedPartition_TheLastAdminMayGo() + { + const string Partition = "systemownedspace"; + var grant = await TheOnlyAdminGrant(Partition); + await WireSync(Partition, twoWay: false); + + var verdict = await DeleteVerdict(grant); + + verdict.IsValid.Should().BeTrue( + "the repo owns this partition, so its administrator is the importer identity and not a " + + "person — holding the last human grant here is what made the retraction sweep unable " + + "to converge, reason given: {0}", verdict.ErrorMessage ?? "(none)"); + } + + [Fact] + public async Task OnAnOrdinaryPartition_TheLastAdminIsStillRefused() + { + const string Partition = "ordinaryspace"; + var grant = await TheOnlyAdminGrant(Partition); + // No _GitSync at all — the ordinary Space this invariant exists for. + + var verdict = await DeleteVerdict(grant); + + verdict.IsValid.Should().BeFalse( + "the control on the other side: an ordinary space left with no administrator cannot be " + + "managed by anybody, and an exemption that also covered this one would be the " + + "invariant switched off rather than scoped"); + } + + [Fact] + public async Task OnATwoWaySyncedPartition_TheLastAdminIsStillRefused() + { + const string Partition = "bijectivespace"; + var grant = await TheOnlyAdminGrant(Partition); + await WireSync(Partition, twoWay: true); + + var verdict = await DeleteVerdict(grant); + + verdict.IsValid.Should().BeFalse( + "a bijective sync preserves and commits back server-side edits, so its mesh nodes are " + + "somebody's working copy and it is NOT system-owned — the exemption keys on the " + + "DIRECTION, exactly as the write boundary does, not on the presence of a sync"); + } +} diff --git a/test/MeshWeaver.Hosting.Test/SystemOwnedSweepRetractsTheLastAdminTest.cs b/test/MeshWeaver.Hosting.Test/SystemOwnedSweepRetractsTheLastAdminTest.cs new file mode 100644 index 0000000000..a554441941 --- /dev/null +++ b/test/MeshWeaver.Hosting.Test/SystemOwnedSweepRetractsTheLastAdminTest.cs @@ -0,0 +1,167 @@ +using System; +using System.Linq; +using System.Reactive.Linq; +using System.Threading.Tasks; +using MeshWeaver.Fixture; +using MeshWeaver.GitSync; +using MeshWeaver.Graph; +using MeshWeaver.Hosting.Monolith.TestBase; +using MeshWeaver.Mesh; +using MeshWeaver.Mesh.Security; +using MeshWeaver.Mesh.Services; +using MeshWeaver.Messaging; +using Microsoft.Extensions.DependencyInjection; +using Xunit; + +namespace MeshWeaver.Hosting.Test; + +/// +/// 🚨 Wiring a ONE-WAY _GitSync retracts EVERY privileged grant on the partition — the last +/// administrator included. Wiring a BIJECTIVE one retracts nothing. +/// +/// Why this test exists, and what it would have caught. #5140 reported three symptoms of +/// one mechanism: a system-owned Space that kept a human Admin grant through two syncs, a +/// direct write by that human being accepted, and the grant being impossible to delete. Two rules +/// were holding each other up. AccessAssignmentGuard.IsForbiddenOnSystemOwned refuses granting +/// a SECOND Admin on such a partition; SpaceAdminInvariantValidator refused removing the +/// FIRST; so SystemOwnedAccessRetractionHandler pre-detected the guaranteed rejection and +/// SPARED that grant — on every pass, for ever, because the escape it documented ("retracted next +/// time, once another admin exists") could never arrive. +/// +/// Exempting the invariant is only half the fix, and on its own it is the WORSE half: the +/// handler's pre-spare would still stop the delete from ever being issued, so the exemption would be +/// inert and nothing would report it. A test that drives the validator directly cannot see that — it +/// measures the half that changed. This one drives the SWEEP, which is the thing whose outcome the +/// issue is about. +/// +/// Control on each side, over the same Space and the same grants: +/// must converge to an EMPTY set of +/// write-conferring grants and must not. A sweep that +/// retracted unconditionally would pass the first and fail the second; one that spares anything +/// fails the first. Neither is satisfiable by a constant. +/// +/// 🚨 The assertion is about the SET, and the first version of this test — which named one +/// grant's path — passed against the unfixed sweep. A Space's post-creation handler grants its +/// creator Admin, so two write-conferring grants exist; the pre-spare kept the earliest (the +/// creator's) and deleted the other, making "my grant is gone" true either way. That is the same +/// defect this whole change set is about, in the test written to catch it, and only running the +/// control against the unfixed source exposed it. +/// +public class SystemOwnedSweepRetractsTheLastAdminTest(ITestOutputHelper output) + : MonolithMeshTestBase(output) +{ + private const string SpaceOwner = "space-owner"; + private const string RepoUrl = "https://github.com/test/sweep-last-admin"; + + private static TimeSpan Budget => TestTimeouts.Convergence; + + private GitHubSyncService Sync => Mesh.ServiceProvider.GetRequiredService(); + + protected override MeshBuilder ConfigureMesh(MeshBuilder builder) + => base.ConfigureMesh(builder) + .AddGitHubSyncTypes() + .ConfigureServices(services => + { + services.AddGitHubSyncServices(); + return services; + }); + + /// + /// A Space carrying a human Admin grant written BEFORE the sync is wired — the whole window + /// #5140 is about: legal when written, wrong one second later. + /// + /// 🚨 There are TWO admin grants here, not one, and that is deliberate — the Space's + /// post-creation handler grants its CREATOR Admin as well. Asserting on one path was how the + /// first version of this test passed against the unfixed sweep: the pre-spare kept the + /// earliest-created grant (the creator's) and deleted the other, so "my grant is gone" was true + /// either way. The property is about the SET, so the assertions below are about the set. + /// + private async Task ASpaceWithAHumanAdmin() + { + var space = "Swept" + Guid.NewGuid().ToString("N")[..8]; + await NodeFactory.CreateNode(new MeshNode(space) + { + NodeType = "Space", Name = "Swept space", State = MeshNodeState.Active, + Content = new Space(), + }).Should().Within(Budget).Emit("the Space fixture must be created", + cancellationToken: TestContext.Current.CancellationToken); + + await NodeFactory.CreateNode(AssignmentNodeFactory.UserRole(SpaceOwner, "Admin", space)) + .Should().Within(Budget).Emit("a human Admin grant must exist BEFORE the sync is wired", + cancellationToken: TestContext.Current.CancellationToken); + + await WriteGrantsRemain(space).Should().Within(Budget).Emit( + "at least one write-conferring grant must be readable before the sweep runs, or every " + + "assertion below is vacuous", + cancellationToken: TestContext.Current.CancellationToken); + + return space; + } + + /// + /// Emits once NO write-conferring AccessAssignment remains under {space}/_Access — + /// the property the sweep claims. Re-queried on an interval because a query is the read-side + /// index and trails the store: waiting on the CONDITION, never on a delay. + /// + private IObservable NoWriteGrantRemains(string space) => Poll(space, anyRemain: false); + + /// Emits once at least one write-conferring grant is readable there. + private IObservable WriteGrantsRemain(string space) => Poll(space, anyRemain: true); + + private IObservable Poll(string space, bool anyRemain) + => Observable.Interval(TestTimeouts.Quick / 10) + .StartWith(0L) + .SelectMany(_ => NodeFactory + .Query(MeshQueryRequest.FromQuery( + $"namespace:{space}/_Access nodeType:{AccessAssignmentGuard.AccessAssignmentNodeType}")) + .Take(1) + .Select(change => change.Items.Any(n => + AccessAssignmentGuard.ConfersWriteAccess( + n.ContentAs(Mesh.JsonSerializerOptions))))) + .Where(remain => remain == anyRemain) + .Take(1); + + private async Task WireSync(string space, bool twoWay) + { + var config = await Sync.SaveConfig( + space, RepoUrl, "main", null, false, false, + direction: twoWay ? SyncDirection.Bidirectional : SyncDirection.ImportOnly, + twoWay: twoWay) + .Should().Within(Budget).Emit("the sync config must be written", + cancellationToken: TestContext.Current.CancellationToken); + + Assert.Equal(GitHubSyncService.ConfigPath(space), config.Path); + Assert.Equal( + !twoWay, + AccessAssignmentGuard.IsSystemOwned(config, Mesh.JsonSerializerOptions)); + } + + [Fact] + public async Task AOneWaySync_LeavesNoWriteConferringGrantAtAll() + { + var space = await ASpaceWithAHumanAdmin(); + + await WireSync(space, twoWay: false); + + await NoWriteGrantRemains(space).Should().Within(Budget).Emit( + "the repo owns this Space now, so nobody else does — INCLUDING its last administrator. " + + "There is no second admin to wait for, because the write boundary refuses one, so " + + "sparing any of them is what made the documented state unreachable (#5140)", + cancellationToken: TestContext.Current.CancellationToken); + } + + [Fact] + public async Task ABijectiveSync_RetractsNothing() + { + var space = await ASpaceWithAHumanAdmin(); + + await WireSync(space, twoWay: true); + + // The negative shape: the set must NOT empty out. A bijective sync preserves and commits + // back server-side edits, so the mesh nodes are these admins' working copy and retracting + // their write access would make the feature unusable by everyone but the importer. + await NoWriteGrantRemains(space).Should().NotEmit(TestTimeouts.Quick, + "a bijective sync does not make a Space system-owned, so its administrators keep write " + + "access — the sweep keys on the DIRECTION, not on the presence of a sync"); + } +}