diff --git a/dev-docs/future-features/identity-lifecycle-untangle.md b/dev-docs/future-features/identity-lifecycle-untangle.md index aa3a7cb2..a1e449cc 100644 --- a/dev-docs/future-features/identity-lifecycle-untangle.md +++ b/dev-docs/future-features/identity-lifecycle-untangle.md @@ -2,9 +2,9 @@ Status: **partially implemented** — original analysis/decision-gate pass from 2026-05-28; phases 0–2 shipped 2026-05-29 (see status section below), phases 3–4 still open. -## Implementation status (updated 2026-05-29) +## Implementation status (updated 2026-06-02) -4 of the 7 themes are resolved; the remaining 3 (`UNLINK`, `EXTLOGIN`, `MEMBERSHIP`) are still open. Branch `feat/lifecycle-email-deletion` (6 commits, not yet pushed at time of writing) plus the already-merged Hotfix C (#21) and Federation v1 (#23/#24) cover the closed themes. +6 of the 7 themes are resolved; only the *durable leased external-membership* slice of `MEMBERSHIP` (Phase-4 large) remains. Phase 3 + the contained Phase-4 wiring shipped on branch `feat/lifecycle-phase3-4-unlink-rematch` (not yet pushed at time of writing), on top of the already-merged Hotfix C (#21), Federation v1 (#23/#24), and the email/deletion wave. | Theme | Status | Where | |---|---|---| @@ -12,34 +12,40 @@ Status: **partially implemented** — original analysis/decision-gate pass from | `EMAIL` | ✅ shipped | PR A — partial-unique index `WHERE NOT is_deleted` (declarative in Marten config since 2026-06-02; the temporary `EmailUniquenessMigration` was removed) | | `DELETE` | ✅ shipped | Hotfix C (#21) token/link-PII scrub + PR B recycle-bin/grace/GDPR-scrub convergence | | `SOFTDELETE` | ✅ shipped | Hotfix C (#21) access revocation + PR B grace/recycle-bin model | -| `UNLINK` | ❌ open | Phase 3 below | -| `EXTLOGIN` | ⚠️ partial | session-derived authz shipped (Federation v1); the unlink/re-link + passkey/last-method hardening of Phase 3 is open | -| `MEMBERSHIP` | ⚠️ partial | session-scoped derivation shipped (Federation v1); durable leased external-membership (Phase 4) is open | +| `UNLINK` | ✅ shipped | Phase 3 — Variant-C "unlink forgets the binding" (projection `ShouldDelete`), admin force-unlink, AuthLog | +| `EXTLOGIN` | ✅ shipped | Phase 3 hardening complete (passkey registration + last-auth-method guard already landed in #42) | +| `MEMBERSHIP` | ⚠️ partial | session-scoped derivation (Federation v1) + Phase-4 link/unlink recompute wiring shipped; durable leased external-membership (Phase-4 large) still open | **Phase status:** - **Phase 0 — Ratify positioning** ✅ — pinned hub-by-default + broker-opt-in; realised through Federation v1. - **Phase 1 — Email as a real invariant** ✅ — PR A. Follow-up ✅ DONE (2026-06-02): `EmailUniquenessMigration` removed, partial-unique index now declarative in the Marten config. - **Phase 2 — Converge delete paths + close the access-survival gap** ✅ — Hotfix C (#21) closed the two standalone security/compliance bugs (no token revoke on delete/deactivate; GDPR left PII on `ExternalIdentityLink`); PR B converged admin-delete onto a recoverable recycle-bin + grace + GDPR-scrub, with the per-realm sweep job. The "recoverable grace/restore" product question was answered **yes** (built). -- **Phase 3 — Variant-C unlink re-link + EXTLOGIN hardening** ❌ NOT STARTED — see the open-items list below. -- **Phase 4 — Close the membership contract (durable federated membership)** ❌ NOT STARTED (largest remaining piece) — see the open-items list below. +- **Phase 3 — Variant-C unlink re-link + EXTLOGIN hardening** ✅ SHIPPED (2026-06-02) — see the closed-items list below. +- **Phase 4 — Close the membership contract** ⚠️ PARTIAL — the contained link/unlink-recompute slice shipped (2026-06-02); the *durable leased external-membership* feature (the largest remaining piece) is deliberately deferred as a separate multi-PR effort — see the still-open list below. -**Still open — Phase 3 (`UNLINK`, `EXTLOGIN`):** +**Phase 3 (`UNLINK`, `EXTLOGIN`) — ✅ shipped 2026-06-02:** -- Re-link after unlink (Variant C = forget + rematch): add `&& !l.IsUnlinked` to the `ExternalLoginProcessor` link lookup and free the `(iss,sub)` slot via hard-delete + `ArchiveStream` on unlink (today an unlinked link still returns `Idp.Unlinked` "disconnected" and blocks re-link). -- Admin force-unlink endpoint + tombstone visibility + `AuthLog` entries for link/unlink. -- Harden the last-auth-method guard to count passkeys (today approximated via `HasPasswordAsync`, so a passkey-only user can be left with zero factors). -- **`StoredPasskeyCredential` is not registered in `MartenStoreOptionsExtensions` and no delete path removes it** → a deleted user's passkey public keys + user handles survive as orphans. Standalone GDPR/ops bug, shippable independently. -- Document multi-IdP precedence and the SAML `SameSite=Lax` link-flow degradation. +- ✅ Re-link after unlink (Variant C = forget + rematch). Implemented NOT via `!IsUnlinked` + partial index, but via the projection convention: `ExternalIdentityUnlinkedEvent` → `ExternalIdentityLinkProjection.ShouldDelete` **deletes the projection doc**. This frees the `(iss,sub)` unique slot, leaves no blocking tombstone, is rebuild-safe (replay `Linked→Unlinked` nets to "no doc" — empirically pinned by `ProjectionRebuildTests`, incl. a re-link of the same `(iss,sub)` rebuilding without a unique-index violation), and — crucially — keeps the link stream **live + maskable** (no `ArchiveStream`). `ExternalLoginProcessor` loads the user first, forgets (emits the terminal event) when `linkedUser is null || link.IsUnlinked` (legacy/orphan), and only runs the cross-user hijack guard on a **live** link — a released `(iss,sub)` re-homes by policy, a live one cannot be stolen. +- ✅ Admin force-unlink endpoint (`DELETE /api/admin/users/{id}/external-links/{linkId}`, gated `user:write`) + `AuthLog` for unlink (link was already logged). Both self-service + admin share an `UnlinkAsync` helper with the last-auth-method guard. +- ✅ Last-auth-method guard already counts passkeys, and `StoredPasskeyCredential` is registered + cleaned up — both landed in #42 (the original list above was stale). +- ⚠️ **`ArchiveStream`-vs-GDPR gotcha (load-bearing):** Marten's `ApplyEventDataMasking` does **not** rewrite already-archived streams (verified empirically). So unlink must NOT `ArchiveStream` the link stream — it relies on `ShouldDelete` instead. `GdprService.PerformPermanentEraseAsync` now discovers link ids two ways and unions them: the projection-doc query (live links) **plus** the user stream's `UserExternalIdentityLinkedEvent` mirrors (each carries `LinkId`) — so a forgotten (doc-less) link's still-live stream is masked + archived during the erase. Regression-guarded by `UserLifecycleRevocationTests.GdprErase_scrubs_external_identity_links_including_forgotten_streams`. +- ✅ Docs written (2026-06-02): multi-IdP precedence + profile-authority (`docs/admin/login-providers.md`) and the SAML `SameSite=Lax` link-flow degradation (`docs/admin/saml-federation.md`). "Tombstone visibility" is obviated — unlink leaves no tombstone. -**Still open — Phase 4 (`MEMBERSHIP`):** +**Phase 4 (`MEMBERSHIP`) — contained slice ✅ shipped 2026-06-02; durable-lease ❌ still open:** -- Replace the flat `Group.MemberIds: List` with a source-attributed external-membership class `(groupId, principalId, source = provider:, grantedAt, leaseExpiresAt, lastReconfirmedAt)`; effective members = union of { manual } + { local-JsEval } + { per-provider external WHERE leaseExpiresAt > now }. Federation v1 shipped only the *session-scoped* derivation ("the session is the lease"); this is the durable, login-independent version. -- Wire `AutoMembershipSyncHandlers` to link/unlink events + a one-time backfill recompute (link/unlink triggers nothing today — confirmed gap). -- Refresh triggers: login-FORCE SET-reconcile (idempotent remove-what's-absent), lease-expiry sweep (Quartz, fail-closed authority of last resort), optional out-of-band inbound SCIM / scheduled LDAP-or-Graph pull (net-new surface — no SCIM server / LDAP client exists today). -- Privilege guardrail: federated/JsEval auto-membership must never confer `realm:admin` / `app:admin`. -- Resolve the doc-vs-code contradiction: `docs/concepts/auto-membership.md` uses `externalClaims` / `OrganizationalUnit` / `Department` which do not exist in `Person.cs` — these are the **spec of this wanted feature**, not drift to delete (per the superseded-root note below). -- Decide the inert `MembershipScriptDependencies` optimization (wire it or delete it + fix docs) and add a test reconciling the two evaluation engines (in-memory delegate vs Postgres-JSONB) on null / case / collation. +- ✅ Wired `AutoMembershipSyncHandlers` to link/unlink: `AutoMembershipOnExternalIdentityLinked/UnlinkedHandler` recompute on `UserExternalIdentityLinked/UnlinkedEvent` (auto-discovered via `ReferenceSyncRegistration` + `UseFastEventForwarding`), passing `changedPaths=[Person.ExternalIdentities]`. Closes the confirmed stale gap for scripts keying on `p.ExternalIdentities`. +- ✅ `MembershipScriptDependencies` optimization decision: **leave as-is** (it is genuinely inert — both `CreateGroupCommand` + `UpdateGroupCommand` leave `membershipDeps` null, so `DepsIntersect` never skips → safe evaluate-all). Not worth wiring/deleting in this slice; link/unlink + profile updates are infrequent. +- ✅ Two-engine reconciliation test added (`ExternalLinkLifecycleTests.ExternalIdentities_Script_Agrees_Across_Both_Engines`). **Finding:** a membership script reads `Person.ExternalIdentities` via `.some(x => x.Prop === ...)` in BOTH engines, but `p.ExternalIdentities.length` does **not** translate (in-memory evaluates it to false; the JSONB batch would fail-closed). ✅ Documented publicly (`docs/concepts/auto-membership.md`): added `p.ExternalIdentities` to the durable-fields table + the "use `.some(x => ...)`, not `.length`" guidance + the link/unlink recompute trigger. *(Backfill recompute for pre-existing users is deferred — the `p.ExternalIdentities` surface is new, so no stale durable membership exists to backfill yet.)* +- ✅ Handler wiring guarded by `ExternalLinkLifecycleTests.Phase4_Recompute_Handlers_Match_The_ReferenceSync_Registration_Pattern` (deterministic — asserts both handlers are concrete `ReferenceSyncHandler` subclasses, the predicate `ReferenceSyncRegistration` discovers on). The async durable-queue *delivery* is not exercised e2e — the reference-sync queue does not drain inside `WebApplicationFactory`; it is the same forwarding path the existing AutoMembership handlers use in production. +- ❌ STILL OPEN (durable-lease, the large piece): replace the flat `Group.MemberIds: List` with a source-attributed external-membership class `(groupId, principalId, source = provider:, grantedAt, leaseExpiresAt, lastReconfirmedAt)`; effective members = union of { manual } + { local-JsEval } + { per-provider external WHERE leaseExpiresAt > now }. Federation v1 shipped only the *session-scoped* derivation ("the session is the lease"); this is the durable, login-independent version. +- ❌ STILL OPEN: refresh triggers — login-FORCE SET-reconcile, lease-expiry sweep (Quartz, fail-closed), optional inbound SCIM / scheduled LDAP-or-Graph pull (net-new surface). +- ✅ Privilege guardrail (federated/JsEval auto-membership must never confer `realm:admin`) is already enforced by Federation v1 decision G (bidirectional config guard + `ExpandBypassTiers` strip). +- ❌ STILL OPEN: resolve the `docs/concepts/auto-membership.md` `externalClaims`/`OrganizationalUnit`/`Department` doc-vs-code contradiction (the spec of the wanted, unbuilt durable-claims surface). + +**Known low-pri follow-up (surfaced by the Phase-3 adversarial review):** + +- `UserViewProjection.Apply(UserExternalIdentityUnlinkedEvent)` removes `ExternalLoginProviderIds` keyed on `LoginProviderId` only, not `LinkId`. A user holding two links from the **same** provider (two subjects from one tenant — supported, uniqueness is `(Issuer,Subject)`) loses the provider's admin-list "IdP-connected" indicator after unlinking one. Pre-existing; cosmetic (UserView is not consulted for authz — `Person.ExternalIdentities`, keyed on `LinkId`, stays correct). **Resolved as a documented assumption (2026-06-02)** — a per-link fix would need a rebuild migration to reconstruct history from the deduped column, not worth it for a non-rendered admin badge; the limitation is now documented in the `UserView.ExternalLoginProviderIds` XML doc + a projection comment. **Still open — strategic (deliberately deferred):** diff --git a/docs/admin/login-providers.md b/docs/admin/login-providers.md index 3abf3da3..2e45abfc 100644 --- a/docs/admin/login-providers.md +++ b/docs/admin/login-providers.md @@ -198,6 +198,20 @@ user id) and survives email changes on either side. To deny self-service linking for a particular provider, untick **User dürfen diesen Provider im Profil verknüpfen** in the linking tab. +### Multiple linked providers & profile precedence + +A user may hold links to several IdPs at once (e.g. EntraID *and* Google). Identity matching is always by the IdP's stable **`(issuer, subject)`** — never by email (email is only a fallback for the opt-in auto-link / JIT paths). So a returning login resolves to the right account regardless of how many providers are linked. + +On every external login the provider's user-update script *can* patch the four profile fields (first name / last name / email / acronym). To stop two providers fighting over them on alternating logins, only **one provider is authoritative for the profile** at a time: + +- The provider whose login **JIT-created** the user is authoritative by default. +- Any provider can be made authoritative explicitly via the **Authoritative for profile** toggle in the **Verknüpfung & Richtlinien** tab. +- A non-authoritative provider still authenticates the user (and may confer session membership), but does **not** overwrite their profile fields. + +The net effect: a user's display name / email stays stable no matter which linked IdP they signed in with, and there is no per-login flapping. + +> **Unlinking forgets the binding.** Disconnecting a provider in **Profile → Linked accounts** (or via admin force-unlink) frees the `(issuer, subject)` slot, so the same external identity can later be re-linked — to the same user, or, once released, to a different one. The last remaining authentication method (the only password / passkey / link) cannot be removed. + ## Disabling without deleting For OIDC providers, toggle the **Aktiviert** flag in the detail dialog. The diff --git a/docs/admin/saml-federation.md b/docs/admin/saml-federation.md index f3e2f83e..5d8e4e47 100644 --- a/docs/admin/saml-federation.md +++ b/docs/admin/saml-federation.md @@ -88,6 +88,12 @@ When a user signs in via the SAML provider, Modgud: Group / role / permission membership stays under Modgud's control — see [Permission-Modell](../concepts/permissions) for the design rationale. Group-claim → Modgud-Group translation is handled by [auto-membership scripts on Groups](./groups#auto-membership-membership-scripts). +## Linking a SAML identity to an existing account + +OIDC providers can be attached to an already-signed-in account via **Profile → Linked accounts**. For **SAML** this self-service link flow has a **known limitation**: the IdP returns its assertion via a *cross-site POST* to Modgud's ACS endpoint, and Modgud's app session cookie is `SameSite=Lax`, so the browser does not send it on that cross-site POST. Modgud therefore cannot tell that you are already logged in, and the SAML sign-in **falls back to the normal login path** — it resolves by the IdP's `(issuer, subject)`, or by email auto-link / JIT if configured, instead of attaching to the account you started the link from. + +In practice: to associate a SAML identity with a user, sign in *with the SAML provider* on an account whose email matches (with email auto-link enabled on the provider), or let JIT create/resolve the account — don't rely on the **Profile → link** button for SAML. Once a `(issuer, subject)` is bound to a Modgud user it always resolves back to that user on subsequent SAML logins. (A server-side-state fix to make the Profile-link flow work for SAML is tracked as a follow-up.) + ## Cert rotation Two halves: diff --git a/docs/concepts/auto-membership.md b/docs/concepts/auto-membership.md index b4336a4a..630f79bc 100644 --- a/docs/concepts/auto-membership.md +++ b/docs/concepts/auto-membership.md @@ -45,9 +45,24 @@ A TypeScript arrow function from a principal record to `boolean`. It is evaluate | `p.Acronym` | `string?` | short initials | | `p.Email` | `string?` | primary email | | `p.NormalizedUserName` / `p.NormalizedEmail` | `string?` | upper-cased, for case-insensitive compares | +| `p.ExternalIdentities` | `{ LinkId, LoginProviderId, Issuer }[]` | the IdP links the user holds — query with `.some(...)`, see below | > These are the **only** durable fields. There is no `OrganizationalUnit`, `Department`, or generic `externalClaims` dictionary on a person — scripts that reference them either fail to transpile or silently never match. To drive membership from an upstream IdP's groups, use an `ExternallyDrivable` group and `p.ExternalGroups` (below). +### Reading `p.ExternalIdentities` (durable IdP links) + +`p.ExternalIdentities` is the durable list of external-identity links on the person — one entry per linked IdP, each `{ LinkId, LoginProviderId, Issuer }`. Unlike `p.ExternalGroups` (the *ephemeral, session-only* group surface for `ExternallyDrivable` groups), `ExternalIdentities` is a **persisted** field, so a normal `Auto` group (not `ExternallyDrivable`) can key on it — e.g. "everyone federated through a given IdP": + +```typescript +// "Member if linked to the Entra tenant IdP." +(p) => Type.Is(p, 'person') + && p.ExternalIdentities.some(x => x.Issuer === 'https://login.microsoftonline.com//v2.0') +``` + +> **Use `.some(x => ...)`, not `.length`.** Membership on a sub-collection must be expressed as `.some(predicate)` — that is what translates to SQL in the durable batch engine. A count form like `p.ExternalIdentities.length > 0` does **not** translate and silently never matches in the batch engine, so avoid it. The membership-script editor's IntelliSense exposes the element fields (`LinkId` / `LoginProviderId` / `Issuer`). + +Linking or unlinking an external identity re-evaluates `p.ExternalIdentities` scripts (see [Recompute triggers](#recompute-triggers-durable-groups)). + The membership-script editor's IntelliSense is generated from the real CLR types, so the available members are always in sync — use it to discover the surface. ### How it runs (the batch engine) @@ -92,6 +107,7 @@ The durable engine listens for person-mutation events and re-evaluates the affec | user created | check auto-groups whose predicate matches → on match: add | | user updated | re-check auto-groups → add or remove based on the new state | | user deleted | remove from all auto-groups | +| external identity linked / unlinked | re-check auto-groups (e.g. `p.ExternalIdentities` scripts) → add or remove | | membership script changed | full recompute pass for that one group | ## Dependency tracking (selective recompute) diff --git a/src/dotnet/Modgud.Api.Tests/ExternalAuth/ExternalLinkLifecycleTests.cs b/src/dotnet/Modgud.Api.Tests/ExternalAuth/ExternalLinkLifecycleTests.cs new file mode 100644 index 00000000..3e7f977a --- /dev/null +++ b/src/dotnet/Modgud.Api.Tests/ExternalAuth/ExternalLinkLifecycleTests.cs @@ -0,0 +1,340 @@ +using System.Net; +using System.Net.Http.Json; +using System.Text.Json; +using BuildingBlocks.Helper; +using Marten; +using Microsoft.AspNetCore.Identity; +using Microsoft.Extensions.DependencyInjection; +using Modgud.Api.Features.Groups; +using Modgud.Api.Features.Shared; +using Modgud.Api.Tests.Infrastructure; +using Modgud.Authentication.Domain; +using Modgud.Authentication.Domain.ExternalAuth; +using Modgud.Authentication.Domain.ExternalAuth.Events; +using Modgud.Authorization.Apps; +using Modgud.Authorization.Events; +using Modgud.Authorization.Membership; +using Modgud.Authorization.Principals; + +namespace Modgud.Api.Tests.ExternalAuth; + +/// +/// Lifecycle Phase 3 (Variant-C unlink mechanics) + Phase 4 (membership recompute +/// on link/unlink). Pins: self-service + admin unlink hard-delete and free the +/// (Issuer, Subject) slot; the last-auth-method guard; and that linking/unlinking +/// re-evaluates an auto-group keyed on p.ExternalIdentities (the confirmed +/// stale-membership gap), with the in-memory and Postgres-JSONB engines agreeing. +/// +[Collection(IntegrationTestCollection.Name)] +public class ExternalLinkLifecycleTests : IntegrationTestBase +{ + public ExternalLinkLifecycleTests(SharedPostgresFixture fixture) : base(fixture) { } + + private const string Issuer = "https://idp.lifecycle.test/v2.0"; + + // ── Phase 3 — unlink hard-delete + slot reuse ──────────────────────── + + [Fact] + public async Task SelfService_Unlink_HardDeletes_And_Frees_Slot() + { + var ct = TestContext.Current.CancellationToken; + var user = await Factory.CreateTestUserWithIdentityAsync( + "Self", "Unlink", "su", "selfunlink@acme.com", password: "TestPass1234"); + var providerId = Guid.NewGuid(); + var linkId = await SeedLinkAsync(user.Id, providerId, "sub-su-1"); + var client = await CreateAuthenticatedClientAsync("su", "TestPass1234"); + + var resp = await client.DeleteAsync($"/api/account/external-links/{new ShortGuid(linkId)}", ct); + Assert.Equal(HttpStatusCode.NoContent, resp.StatusCode); + + // Hard-deleted, not a soft tombstone. + await using (var qs = GetTenantedSession()) + Assert.Null(await qs.LoadAsync(linkId, ct)); + + // The (Issuer, Subject) slot is free — re-inserting the same pair succeeds + // without a unique-index violation, and exactly one live link results. + await SeedLinkAsync(user.Id, providerId, "sub-su-1"); + await using (var qs2 = GetTenantedSession()) + { + var live = await qs2.Query().Where(l => l.Subject == "sub-su-1").CountAsync(ct); + Assert.Equal(1, live); + } + } + + [Fact] + public async Task Admin_ForceUnlink_HardDeletes_And_Removes_Person_Ref() + { + var ct = TestContext.Current.CancellationToken; + var user = await Factory.CreateTestUserWithIdentityAsync( + "Force", "Unlink", "fu", "forceunlink@acme.com", password: "TestPass1234"); + var providerId = Guid.NewGuid(); + var linkId = await SeedLinkAsync(user.Id, providerId, "sub-fu-1"); + + // The default Client is a realm admin (user:write via realm:admin bypass). + var resp = await Client.DeleteAsync( + $"/api/admin/users/{new ShortGuid(user.Id)}/external-links/{new ShortGuid(linkId)}", ct); + Assert.Equal(HttpStatusCode.NoContent, resp.StatusCode); + + await using var qs = GetTenantedSession(); + Assert.Null(await qs.LoadAsync(linkId, ct)); + var person = await qs.LoadAsync(user.Id, ct); + Assert.DoesNotContain(person!.ExternalIdentities, r => r.LinkId == linkId); + } + + [Fact] + public async Task Unlink_LastAuthMethod_IsBlocked() + { + var ct = TestContext.Current.CancellationToken; + var user = await Factory.CreateTestUserWithIdentityAsync( + "Last", "Method", "lm", "lastmethod@acme.com", password: "TestPass1234"); + + // Strip the password so the external link is the ONLY remaining factor. + using (var scope = Factory.Services.CreateScope()) + { + var um = scope.ServiceProvider.GetRequiredService>(); + var au = await um.FindByIdAsync(user.Id.ToString()); + await um.RemovePasswordAsync(au!); + } + var linkId = await SeedLinkAsync(user.Id, Guid.NewGuid(), "sub-lm-1"); + + var resp = await Client.DeleteAsync( + $"/api/admin/users/{new ShortGuid(user.Id)}/external-links/{new ShortGuid(linkId)}", ct); + Assert.Equal(HttpStatusCode.BadRequest, resp.StatusCode); + var body = await resp.Content.ReadFromJsonAsync(ct); + Assert.Equal("Idp.LastAuthMethod", body.GetProperty("Code").GetString()); + + // Guard prevented deletion — the link is still there. + await using var qs = GetTenantedSession(); + Assert.NotNull(await qs.LoadAsync(linkId, ct)); + } + + [Fact] + public async Task SelfService_Unlink_OtherUsersLink_IsForbidden() + { + var ct = TestContext.Current.CancellationToken; + var userA = await Factory.CreateTestUserWithIdentityAsync("Aaa", "User", "au", "au@acme.com", password: "TestPass1234"); + var userB = await Factory.CreateTestUserWithIdentityAsync("Bbb", "User", "bu", "bu@acme.com", password: "TestPass1234"); + var linkId = await SeedLinkAsync(userB.Id, Guid.NewGuid(), "sub-b-1"); + var clientA = await CreateAuthenticatedClientAsync("au", "TestPass1234"); + + var resp = await clientA.DeleteAsync($"/api/account/external-links/{new ShortGuid(linkId)}", ct); + Assert.Equal(HttpStatusCode.Forbidden, resp.StatusCode); + + await using var qs = GetTenantedSession(); + Assert.NotNull(await qs.LoadAsync(linkId, ct)); + } + + [Fact] + public async Task Admin_ForceUnlink_UserLinkMismatch_IsNotFound() + { + var ct = TestContext.Current.CancellationToken; + var user = await Factory.CreateTestUserWithIdentityAsync("Mis", "Match", "mm", "mismatch@acme.com", password: "TestPass1234"); + var linkId = await SeedLinkAsync(user.Id, Guid.NewGuid(), "sub-mm-1"); + + // Pass a different (random) userId that does not own the link. + var resp = await Client.DeleteAsync( + $"/api/admin/users/{new ShortGuid(Guid.NewGuid())}/external-links/{new ShortGuid(linkId)}", ct); + Assert.Equal(HttpStatusCode.NotFound, resp.StatusCode); + + await using var qs = GetTenantedSession(); + Assert.NotNull(await qs.LoadAsync(linkId, ct)); + } + + [Fact] + public async Task Admin_ForceUnlink_WithoutPermission_IsForbidden() + { + var ct = TestContext.Current.CancellationToken; + var target = await Factory.CreateTestUserWithIdentityAsync("Tgt", "User", "tg", "target@acme.com", password: "TestPass1234"); + var linkId = await SeedLinkAsync(target.Id, Guid.NewGuid(), "sub-tg-1"); + // A non-admin user (no user:write). + await Factory.CreateTestUserWithIdentityAsync("Non", "Admin", "na", "nonadmin@acme.com", password: "TestPass1234", isRealmAdmin: false); + var nonAdmin = await CreateAuthenticatedClientAsync("na", "TestPass1234"); + + var resp = await nonAdmin.DeleteAsync( + $"/api/admin/users/{new ShortGuid(target.Id)}/external-links/{new ShortGuid(linkId)}", ct); + Assert.Equal(HttpStatusCode.Forbidden, resp.StatusCode); + + await using var qs = GetTenantedSession(); + Assert.NotNull(await qs.LoadAsync(linkId, ct)); + } + + // ── Phase 4 — membership recompute on link/unlink ──────────────────── + + [Fact] + public async Task Link_Then_Unlink_Recomputes_ExternalIdentities_Auto_Group() + { + var groupId = await CreateAutoGroupAsync( + $"(p) => Type.Is(p, 'person') && p.ExternalIdentities.some(x => x.Issuer === '{Issuer}')"); + var user = await Factory.CreateTestUserWithIdentityAsync( + "Member", "Drift", "md", "memberdrift@acme.com", password: "TestPass1234"); + + // No link yet → not a member. + await RecalcPrincipalAsync(user.Id); + Assert.DoesNotContain(user.Id, await GetMemberIdsAsync(groupId)); + + // Link (mirror the user-stream event the processor emits) → recompute adds. + var linkId = await SeedUserRefAsync(user.Id, Guid.NewGuid()); + await RecalcPrincipalAsync(user.Id); + Assert.Contains(user.Id, await GetMemberIdsAsync(groupId)); + + // Unlink (ref removed) → recompute drops the membership. + await RemoveUserRefAsync(user.Id, linkId); + await RecalcPrincipalAsync(user.Id); + Assert.DoesNotContain(user.Id, await GetMemberIdsAsync(groupId)); + } + + [Fact] + public void Phase4_Recompute_Handlers_Match_The_ReferenceSync_Registration_Pattern() + { + // Deterministic wiring guard for the exact failure mode the review flagged: + // "if the handlers were accidentally not discovered, production membership + // goes stale on link/unlink while every other test still passes." + // ReferenceSyncRegistration.RegisterAll discovers handlers by scanning the + // API assembly for concrete ReferenceSyncHandler subclasses and + // routing each TEvent's forwarded IEvent to the reference-sync + // queue. Assert both Phase-4 handlers match that exact predicate, so they + // are auto-registered. (The async end-to-end *delivery* is not exercised: + // the durable reference-sync queue does not drain inside + // WebApplicationFactory — it is the same forwarding path the existing + // AutoMembership handlers use in production.) + AssertAutoRegisteredFor(); + AssertAutoRegisteredFor(); + } + + private static void AssertAutoRegisteredFor() + { + Assert.False(typeof(THandler).IsAbstract, $"{typeof(THandler).Name} must be concrete to be discovered"); + var baseType = typeof(THandler).BaseType; + Assert.True( + baseType is { IsGenericType: true } + && baseType.GetGenericTypeDefinition() == typeof(ReferenceSyncHandler<>) + && baseType.GetGenericArguments()[0] == typeof(TEvent), + $"{typeof(THandler).Name} must derive from ReferenceSyncHandler<{typeof(TEvent).Name}> " + + "so ReferenceSyncRegistration auto-routes its forwarded event"); + } + + [Fact] + public async Task ExternalIdentities_Script_Agrees_Across_Both_Engines() + { + var groupId = await CreateAutoGroupAsync( + $"(p) => Type.Is(p, 'person') && p.ExternalIdentities.some(x => x.Issuer === '{Issuer}')"); + + var linked1 = await Factory.CreateTestUserWithIdentityAsync("Eng", "One", "e1", "e1@acme.com"); + var linked2 = await Factory.CreateTestUserWithIdentityAsync("Eng", "Two", "e2", "e2@acme.com"); + var unlinked = await Factory.CreateTestUserWithIdentityAsync("Eng", "Zero", "e0", "e0@acme.com"); + await SeedUserRefAsync(linked1.Id, Guid.NewGuid()); + await SeedUserRefAsync(linked2.Id, Guid.NewGuid()); + + // Engine 1 — Postgres-JSONB full-group pass. + await RecalcGroupAsync(groupId); + var jsonb = await GetMemberIdsAsync(groupId); + + // Engine 2 — in-memory per-principal pass over every person, from empty. + await ResetGroupMembersAsync(groupId); + foreach (var personId in await AllPersonIdsAsync()) + await RecalcPrincipalAsync(personId); + var inMemory = await GetMemberIdsAsync(groupId); + + Assert.Equal(jsonb.OrderBy(x => x), inMemory.OrderBy(x => x)); + Assert.Contains(linked1.Id, jsonb); + Assert.Contains(linked2.Id, jsonb); + Assert.DoesNotContain(unlinked.Id, jsonb); + } + + // ── helpers ────────────────────────────────────────────────────────── + + private async Task SeedLinkAsync(Guid userId, Guid providerId, string subject) + { + var linkId = Guid.NewGuid(); + using var scope = Factory.Services.CreateScope(); + var session = scope.ServiceProvider.GetRequiredService(); + session.Events.StartStream(linkId, new ExternalIdentityLinkedEvent( + linkId, userId, providerId, Issuer, subject, null, null, DateTimeOffset.UtcNow)); + session.Events.Append(userId, new UserExternalIdentityLinkedEvent( + userId, linkId, providerId, Issuer, DateTimeOffset.UtcNow)); + await session.SaveChangesAsync(TestContext.Current.CancellationToken); + return linkId; + } + + /// Seeds only the user-stream ref (drives Person.ExternalIdentities) — no link doc. + private async Task SeedUserRefAsync(Guid userId, Guid providerId) + { + var linkId = Guid.NewGuid(); + using var scope = Factory.Services.CreateScope(); + var session = scope.ServiceProvider.GetRequiredService(); + session.Events.Append(userId, new UserExternalIdentityLinkedEvent( + userId, linkId, providerId, Issuer, DateTimeOffset.UtcNow)); + await session.SaveChangesAsync(TestContext.Current.CancellationToken); + return linkId; + } + + private async Task RemoveUserRefAsync(Guid userId, Guid linkId) + { + using var scope = Factory.Services.CreateScope(); + var session = scope.ServiceProvider.GetRequiredService(); + session.Events.Append(userId, new UserExternalIdentityUnlinkedEvent( + userId, linkId, Guid.NewGuid(), DateTimeOffset.UtcNow)); + await session.SaveChangesAsync(TestContext.Current.CancellationToken); + } + + private async Task CreateAutoGroupAsync(string script) + { + using var scope = Factory.Services.CreateScope(); + var session = scope.ServiceProvider.GetRequiredService(); + var evaluator = scope.ServiceProvider.GetRequiredService(); + var compiled = evaluator.TranspileMembershipScript(script); + var id = Guid.CreateVersion7(); + session.Events.StartStream(id, new GroupCreatedEvent( + id, $"Auto_{Guid.NewGuid():N}"[..12], null, [], [], + MembershipMode.Auto, script, compiled, null, null, EmailMode.Shared, + [AppSlugs.Modgud], ExternallyDrivable: false)); + await session.SaveChangesAsync(TestContext.Current.CancellationToken); + return id; + } + + private async Task RecalcPrincipalAsync(Guid principalId) + { + using var scope = Factory.Services.CreateScope(); + var recalc = scope.ServiceProvider.GetRequiredService(); + var session = scope.ServiceProvider.GetRequiredService(); + await recalc.RecalculateForPrincipalAsync( + principalId, session, + new[] { "Person.ExternalIdentities" }, TestContext.Current.CancellationToken); + await session.SaveChangesAsync(TestContext.Current.CancellationToken); + } + + private async Task RecalcGroupAsync(Guid groupId) + { + using var scope = Factory.Services.CreateScope(); + var recalc = scope.ServiceProvider.GetRequiredService(); + var session = scope.ServiceProvider.GetRequiredService(); + var group = await session.LoadAsync(groupId, TestContext.Current.CancellationToken); + await recalc.RecalculateForGroupAsync(group!, session, TestContext.Current.CancellationToken); + await session.SaveChangesAsync(TestContext.Current.CancellationToken); + } + + private async Task ResetGroupMembersAsync(Guid groupId) + { + using var scope = Factory.Services.CreateScope(); + var session = scope.ServiceProvider.GetRequiredService(); + session.Events.Append(groupId, new GroupMembershipRecomputedEvent(groupId, [])); + await session.SaveChangesAsync(TestContext.Current.CancellationToken); + } + + private async Task> GetMemberIdsAsync(Guid groupId) + { + await using var qs = GetTenantedSession(); + var group = await qs.LoadAsync(groupId, TestContext.Current.CancellationToken); + return group!.MemberIds; + } + + private async Task> AllPersonIdsAsync() + { + await using var qs = GetTenantedSession(); + return await qs.Query() + .Where(p => !p.IsDeleted) + .Select(p => p.Id) + .ToListAsync(TestContext.Current.CancellationToken); + } + +} diff --git a/src/dotnet/Modgud.Api.Tests/ExternalAuth/ExternalLoginProcessorTests.cs b/src/dotnet/Modgud.Api.Tests/ExternalAuth/ExternalLoginProcessorTests.cs index 07990db9..6d7bb165 100644 --- a/src/dotnet/Modgud.Api.Tests/ExternalAuth/ExternalLoginProcessorTests.cs +++ b/src/dotnet/Modgud.Api.Tests/ExternalAuth/ExternalLoginProcessorTests.cs @@ -159,13 +159,59 @@ public async Task AllowedDomains_RejectsMismatchedEmail() } [Fact] - public async Task UnlinkedLink_Fails() + public async Task UnlinkedLink_IsForgotten_AndRematchesByEmailPolicy() { - var config = await CreateEnabledEntraConfig(); - var user = await Factory.CreateTestUserWithIdentityAsync("U", "Nlink", "UN", "un@acme.com"); - var linkId = await LinkUserAsync(user.Id, config.Id, subject: "sub-un-1"); + // Variant C — "unlink forgets the binding". A disconnected IdP (here a + // legacy IsUnlinked tombstone) is no longer hard-blocked with Idp.Unlinked; + // the tombstone is forgotten and the login re-matches by policy. With + // TrustForEmailLink on, the same email re-binds to the same account via a + // fresh link. + var config = await CreateEnabledEntraConfig(trustForEmailLink: true); + var user = await Factory.CreateTestUserWithIdentityAsync("Re", "Link", "RL", "relink@acme.com"); + var linkId = await LinkUserAsync(user.Id, config.Id, subject: "sub-relink-1"); + + // Legacy tombstone: the old soft-unlink path set IsUnlinked=true. + using (var scope = Factory.Services.CreateScope()) + { + var session = scope.ServiceProvider.GetRequiredService(); + session.Events.Append(linkId, new ExternalIdentityUnlinkedEvent(linkId, DateTimeOffset.UtcNow, user.Id)); + await session.SaveChangesAsync(TestContext.Current.CancellationToken); + } + + using (var scope = Factory.Services.CreateScope()) + { + var processor = scope.ServiceProvider.GetRequiredService(); + var external = BuildExternalPrincipal("sub-relink-1", "relink@acme.com", "Re Link"); + var result = await processor.ProcessAsync(external, config.Id, default); + Assert.True(result.Succeeded); + Assert.Equal(user.Id, result.UserId); + } + + // The tombstone is gone (hard-deleted) and a fresh live link owns the slot. + using (var scope = Factory.Services.CreateScope()) + { + var session = scope.ServiceProvider.GetRequiredService(); + Assert.Null(await session.LoadAsync(linkId, TestContext.Current.CancellationToken)); + var live = await session.Query() + .Where(l => l.Subject == "sub-relink-1" && !l.IsUnlinked) + .FirstOrDefaultAsync(); + Assert.NotNull(live); + Assert.Equal(user.Id, live!.UserId); + Assert.NotEqual(linkId, live.Id); + } + } + + [Fact] + public async Task UnlinkedLink_NoRematchPolicy_FallsThrough_NotIdpUnlinked() + { + // Without a re-match policy (AutoCreate off, no TrustForEmailLink), a + // forgotten/unlinked identity falls through to the normal no-link gate — + // proving it is genuinely re-evaluated by policy rather than hard-blocked + // with the old Idp.Unlinked "disconnected" error. The slot is freed either way. + var config = await CreateEnabledEntraConfig(autoCreate: false); + var user = await Factory.CreateTestUserWithIdentityAsync("No", "Match", "NM", "nomatch@acme.com"); + var linkId = await LinkUserAsync(user.Id, config.Id, subject: "sub-nomatch-1"); - // Unlink using (var scope = Factory.Services.CreateScope()) { var session = scope.ServiceProvider.GetRequiredService(); @@ -176,10 +222,88 @@ public async Task UnlinkedLink_Fails() using (var scope = Factory.Services.CreateScope()) { var processor = scope.ServiceProvider.GetRequiredService(); - var external = BuildExternalPrincipal("sub-un-1", "un@acme.com", "U Nlink"); + var external = BuildExternalPrincipal("sub-nomatch-1", "nomatch@acme.com", "No Match"); var result = await processor.ProcessAsync(external, config.Id, default); Assert.False(result.Succeeded); - Assert.Equal("Idp.Unlinked", result.ErrorCode); + Assert.NotEqual("Idp.Unlinked", result.ErrorCode); + Assert.Equal("Idp.NoUserAndAutoCreateOff", result.ErrorCode); + } + + using (var scope = Factory.Services.CreateScope()) + { + var session = scope.ServiceProvider.GetRequiredService(); + Assert.Null(await session.LoadAsync(linkId, TestContext.Current.CancellationToken)); + } + } + + [Fact] + public async Task LiveLink_DifferentAuthenticatedUser_IsRejected() + { + // The cross-user hijack guard: a LIVE link can never be stolen by a + // different authenticated user (it is only re-homable once released). + var config = await CreateEnabledEntraConfig(); + var userA = await Factory.CreateTestUserWithIdentityAsync("Owner", "Live", "OL", "ownerlive@acme.com"); + var userB = await Factory.CreateTestUserWithIdentityAsync("Other", "User", "OU", "otheruser@acme.com"); + var linkId = await LinkUserAsync(userA.Id, config.Id, subject: "sub-live-1"); + + using (var scope = Factory.Services.CreateScope()) + { + var processor = scope.ServiceProvider.GetRequiredService(); + var external = BuildExternalPrincipal("sub-live-1", "ownerlive@acme.com", "Owner Live"); + var result = await processor.ProcessAsync(external, config.Id, default, authenticatedUserId: userB.Id); + Assert.False(result.Succeeded); + Assert.Equal("Idp.LinkedToOtherUser", result.ErrorCode); + } + + // The live link is untouched — still owned by A, still live. + using (var scope = Factory.Services.CreateScope()) + { + var session = scope.ServiceProvider.GetRequiredService(); + var link = await session.LoadAsync(linkId, TestContext.Current.CancellationToken); + Assert.NotNull(link); + Assert.Equal(userA.Id, link!.UserId); + Assert.False(link.IsUnlinked); + } + } + + [Fact] + public async Task ForgottenLink_ReHomes_To_Different_Authenticated_User() + { + // Once A releases the identity (unlink → doc deleted), a DIFFERENT + // authenticated user B may claim the same (iss,sub) — "unlink forgets the + // binding", match key is (iss,sub), not the old link id. + var config = await CreateEnabledEntraConfig(); + var userA = await Factory.CreateTestUserWithIdentityAsync("Owner", "Aaa", "OA", "ownera@acme.com"); + var userB = await Factory.CreateTestUserWithIdentityAsync("Claim", "Bbb", "CB", "claimb@acme.com"); + var linkId = await LinkUserAsync(userA.Id, config.Id, subject: "sub-rehome-1"); + + // A unlinks → the terminal event makes the projection drop the doc. + using (var scope = Factory.Services.CreateScope()) + { + var session = scope.ServiceProvider.GetRequiredService(); + session.Events.Append(linkId, new ExternalIdentityUnlinkedEvent(linkId, DateTimeOffset.UtcNow, userA.Id)); + session.Events.Append(userA.Id, new UserExternalIdentityUnlinkedEvent(userA.Id, linkId, config.Id, DateTimeOffset.UtcNow)); + await session.SaveChangesAsync(TestContext.Current.CancellationToken); + } + + using (var scope = Factory.Services.CreateScope()) + { + var processor = scope.ServiceProvider.GetRequiredService(); + var external = BuildExternalPrincipal("sub-rehome-1", "claimb@acme.com", "Claim Bbb"); + var result = await processor.ProcessAsync(external, config.Id, default, authenticatedUserId: userB.Id); + Assert.True(result.Succeeded); + Assert.Equal(userB.Id, result.UserId); + } + + using (var scope = Factory.Services.CreateScope()) + { + var session = scope.ServiceProvider.GetRequiredService(); + var live = await session.Query() + .Where(l => l.Subject == "sub-rehome-1" && !l.IsUnlinked) + .FirstOrDefaultAsync(); + Assert.NotNull(live); + Assert.Equal(userB.Id, live!.UserId); + Assert.NotEqual(linkId, live.Id); } } diff --git a/src/dotnet/Modgud.Api.Tests/ExternalAuth/ProjectionRebuildTests.cs b/src/dotnet/Modgud.Api.Tests/ExternalAuth/ProjectionRebuildTests.cs new file mode 100644 index 00000000..536c9419 --- /dev/null +++ b/src/dotnet/Modgud.Api.Tests/ExternalAuth/ProjectionRebuildTests.cs @@ -0,0 +1,176 @@ +using Marten; +using Microsoft.Extensions.DependencyInjection; +using Modgud.Api.Tests.Infrastructure; +using Modgud.Authentication.Domain.ExternalAuth; +using Modgud.Authentication.Domain.ExternalAuth.Events; +using Modgud.Authentication.Gdpr; +using Modgud.Authentication.Identity.ExternalAuth; +using Modgud.Authentication.Projections; +using Modgud.Authorization.Apps; +using Modgud.Authorization.Events; +using Modgud.Authorization.Membership; +using Modgud.Authorization.Principals; + +namespace Modgud.Api.Tests.ExternalAuth; + +/// +/// Proves the lifecycle changes survive a full projection rebuild ("replay"). +/// Two load-bearing claims from the Phase-3/4 design: +/// +/// Variant-C unlink (terminal event → ShouldDelete) replays to +/// "no doc" — an unlinked link is NOT resurrected, and a re-link of the +/// same (Issuer, Subject) does not trigger a unique-index +/// violation mid-rebuild (Marten applies the stream's +/// Linked → Unlinked → Linked in sequence order). +/// Auto-group membership is reproduced verbatim from the frozen, +/// self-contained GroupMembershipRecomputedEvent — rebuild does +/// NOT re-evaluate scripts, so the MemberIds set is identical before and +/// after. +/// GDPR-archived streams are skipped on rebuild — an erased user's +/// Person + link docs stay gone (erased PII does not come back). +/// +/// +[Collection(IntegrationTestCollection.Name)] +public class ProjectionRebuildTests : IntegrationTestBase +{ + public ProjectionRebuildTests(SharedPostgresFixture fixture) : base(fixture) { } + + [Fact] + public async Task Rebuild_Preserves_Membership_And_Does_Not_Resurrect_Unlinked_Links() + { + var ct = TestContext.Current.CancellationToken; + const string issuer = "https://idp.rebuild-a.test/v2.0"; + var groupId = await CreateAutoGroupAsync( + $"(p) => Type.Is(p, 'person') && p.ExternalIdentities.some(x => x.Issuer === '{issuer}')"); + + var userKeep = await Factory.CreateTestUserWithIdentityAsync("Keep", "Member", "km", "keep@acme.com"); + var userRelink = await Factory.CreateTestUserWithIdentityAsync("Re", "Link", "rl", "relink@acme.com"); + + await SeedLinkAsync(userKeep.Id, Guid.NewGuid(), issuer, "sub-keep"); + + // link → unlink (ShouldDelete drops the doc) → re-link the SAME (iss,sub). + // The old + new streams both carry Subject="sub-relink": the strongest + // rebuild stress for the unique index. + var oldLinkId = await SeedLinkAsync(userRelink.Id, Guid.NewGuid(), issuer, "sub-relink"); + await UnlinkViaEventsAsync(userRelink.Id, oldLinkId, Guid.NewGuid()); + var newLinkId = await SeedLinkAsync(userRelink.Id, Guid.NewGuid(), issuer, "sub-relink"); + + await RecalcGroupAsync(groupId); + + var before = (await MemberIdsAsync(groupId)).OrderBy(x => x).ToArray(); + Assert.Contains(userKeep.Id, before); + Assert.Contains(userRelink.Id, before); + + await RebuildAsync(ct); + + // Membership reproduced verbatim from the frozen recompute event. + var after = (await MemberIdsAsync(groupId)).OrderBy(x => x).ToArray(); + Assert.Equal(before, after); + + // The unlinked link did NOT resurrect; exactly one live link per (iss,sub). + await using var qs = GetTenantedSession(); + Assert.Null(await qs.LoadAsync(oldLinkId, ct)); + Assert.NotNull(await qs.LoadAsync(newLinkId, ct)); + Assert.Equal(1, await qs.Query().Where(l => l.Subject == "sub-relink").CountAsync(ct)); + Assert.Equal(1, await qs.Query().Where(l => l.Subject == "sub-keep").CountAsync(ct)); + } + + [Fact] + public async Task Rebuild_Skips_GdprArchived_Streams_And_Does_Not_Resurrect_Erased_Data() + { + var ct = TestContext.Current.CancellationToken; + const string issuer = "https://idp.rebuild-b.test/v2.0"; + var user = await Factory.CreateTestUserWithIdentityAsync("Erase", "Me", "em", "eraseme@acme.com"); + var linkId = await SeedLinkAsync(user.Id, Guid.NewGuid(), issuer, "sub-erase"); + + await using (var qs = GetTenantedSession()) + { + Assert.NotNull(await qs.LoadAsync(user.Id, ct)); + Assert.NotNull(await qs.LoadAsync(linkId, ct)); + } + + // GDPR permanent erase archives the user stream + the link stream. + using (var scope = Factory.Services.CreateScope()) + { + var gdpr = scope.ServiceProvider.GetRequiredService(); + var r = await gdpr.PermanentlyEraseAsync(user.Id, adminUserId: null, reason: "rebuild-test", ct); + Assert.False(r.IsError, r.IsError ? r.FirstError.Description : null); + } + + await RebuildAsync(ct); + + // Archived streams are skipped → neither doc comes back; erased data stays erased. + await using (var qs = GetTenantedSession()) + { + Assert.Null(await qs.LoadAsync(user.Id, ct)); + Assert.Null(await qs.LoadAsync(linkId, ct)); + } + } + + // ── helpers ────────────────────────────────────────────────────────── + + private async Task RebuildAsync(CancellationToken ct) + { + // MasterTableTenancy disables the default tenant — build the daemon for + // the "system" realm DB explicitly (mirrors RecoveryCli rebuild-projections). + var store = Factory.Services.GetRequiredService(); + using var daemon = await store.BuildProjectionDaemonAsync("system"); + var timeout = TimeSpan.FromMinutes(2); + await daemon.RebuildProjectionAsync(timeout, ct); + await daemon.RebuildProjectionAsync(timeout, ct); + } + + private async Task SeedLinkAsync(Guid userId, Guid providerId, string issuer, string subject) + { + var linkId = Guid.NewGuid(); + using var scope = Factory.Services.CreateScope(); + var session = scope.ServiceProvider.GetRequiredService(); + session.Events.StartStream(linkId, new ExternalIdentityLinkedEvent( + linkId, userId, providerId, issuer, subject, null, null, DateTimeOffset.UtcNow)); + session.Events.Append(userId, new UserExternalIdentityLinkedEvent( + userId, linkId, providerId, issuer, DateTimeOffset.UtcNow)); + await session.SaveChangesAsync(TestContext.Current.CancellationToken); + return linkId; + } + + private async Task UnlinkViaEventsAsync(Guid userId, Guid linkId, Guid providerId) + { + using var scope = Factory.Services.CreateScope(); + var session = scope.ServiceProvider.GetRequiredService(); + session.Events.Append(linkId, new ExternalIdentityUnlinkedEvent(linkId, DateTimeOffset.UtcNow, userId)); + session.Events.Append(userId, new UserExternalIdentityUnlinkedEvent(userId, linkId, providerId, DateTimeOffset.UtcNow)); + await session.SaveChangesAsync(TestContext.Current.CancellationToken); + } + + private async Task CreateAutoGroupAsync(string script) + { + using var scope = Factory.Services.CreateScope(); + var session = scope.ServiceProvider.GetRequiredService(); + var evaluator = scope.ServiceProvider.GetRequiredService(); + var compiled = evaluator.TranspileMembershipScript(script); + var id = Guid.CreateVersion7(); + session.Events.StartStream(id, new GroupCreatedEvent( + id, $"Reb_{Guid.NewGuid():N}"[..12], null, [], [], + MembershipMode.Auto, script, compiled, null, null, EmailMode.Shared, + [AppSlugs.Modgud], ExternallyDrivable: false)); + await session.SaveChangesAsync(TestContext.Current.CancellationToken); + return id; + } + + private async Task RecalcGroupAsync(Guid groupId) + { + using var scope = Factory.Services.CreateScope(); + var recalc = scope.ServiceProvider.GetRequiredService(); + var session = scope.ServiceProvider.GetRequiredService(); + var group = await session.LoadAsync(groupId, TestContext.Current.CancellationToken); + await recalc.RecalculateForGroupAsync(group!, session, TestContext.Current.CancellationToken); + await session.SaveChangesAsync(TestContext.Current.CancellationToken); + } + + private async Task> MemberIdsAsync(Guid groupId) + { + await using var qs = GetTenantedSession(); + var group = await qs.LoadAsync(groupId, TestContext.Current.CancellationToken); + return group!.MemberIds; + } +} diff --git a/src/dotnet/Modgud.Api.Tests/Security/UserLifecycleRevocationTests.cs b/src/dotnet/Modgud.Api.Tests/Security/UserLifecycleRevocationTests.cs index 044c9ad3..4524083b 100644 --- a/src/dotnet/Modgud.Api.Tests/Security/UserLifecycleRevocationTests.cs +++ b/src/dotnet/Modgud.Api.Tests/Security/UserLifecycleRevocationTests.cs @@ -109,47 +109,80 @@ public async Task Deactivate_revokes_tokens_and_sessions_but_keeps_consent() } [Fact] - public async Task GdprErase_scrubs_external_identity_links() + public async Task GdprErase_scrubs_external_identity_links_including_forgotten_streams() { + var ct = TestContext.Current.CancellationToken; var user = await Factory.CreateTestUserWithIdentityAsync( firstname: "Gdpr", lastname: "Erase", acronym: "GE", email: "gdpr-erase@test.com"); var activeLinkId = Guid.NewGuid(); - var unlinkedLinkId = Guid.NewGuid(); + var forgottenLinkId = Guid.NewGuid(); var providerId = Guid.NewGuid(); using (var scope = Factory.Services.CreateScope()) { var session = scope.ServiceProvider.GetRequiredService(); - // An active link… + // An active link (projection doc present) — discovered the normal way. session.Events.StartStream(activeLinkId, new ExternalIdentityLinkedEvent( Id: activeLinkId, UserId: user.Id, LoginProviderId: providerId, Issuer: "https://idp.test", Subject: "sub-active", Email: "gdpr-erase@test.com", DisplayName: "Gdpr Erase", LinkedAt: DateTimeOffset.UtcNow)); - // …and an already-unlinked tombstone, which still carries PII. - session.Events.StartStream(unlinkedLinkId, + session.Events.Append(user.Id, new UserExternalIdentityLinkedEvent( + user.Id, activeLinkId, providerId, "https://idp.test", DateTimeOffset.UtcNow)); + + // A link that carries PII on its stream — including the raw IdP claims. + session.Events.StartStream(forgottenLinkId, new ExternalIdentityLinkedEvent( - Id: unlinkedLinkId, UserId: user.Id, LoginProviderId: providerId, - Issuer: "https://idp.test", Subject: "sub-old", + Id: forgottenLinkId, UserId: user.Id, LoginProviderId: providerId, + Issuer: "https://idp.test", Subject: "sub-forgotten", Email: "gdpr-erase@test.com", DisplayName: "Gdpr Erase", LinkedAt: DateTimeOffset.UtcNow)); - session.Events.Append(unlinkedLinkId, - new ExternalIdentityUnlinkedEvent(unlinkedLinkId, DateTimeOffset.UtcNow, null)); - await session.SaveChangesAsync(TestContext.Current.CancellationToken); + session.Events.Append(user.Id, new UserExternalIdentityLinkedEvent( + user.Id, forgottenLinkId, providerId, "https://idp.test", DateTimeOffset.UtcNow)); + await session.SaveChangesAsync(ct); + } + + // Variant-C unlink representation: the terminal ExternalIdentityUnlinkedEvent + // makes the projection ShouldDelete drop the doc + the user-stream mirror is + // appended. The PII-bearing events now live ONLY on the orphaned (doc-less) + // link stream — the projection-doc query can no longer find them, so GDPR + // must recover the id from the user stream. + using (var scope = Factory.Services.CreateScope()) + { + var session = scope.ServiceProvider.GetRequiredService(); + session.Events.Append(forgottenLinkId, + new ExternalIdentityUnlinkedEvent(forgottenLinkId, DateTimeOffset.UtcNow, user.Id)); + session.Events.Append(user.Id, new UserExternalIdentityUnlinkedEvent( + user.Id, forgottenLinkId, providerId, DateTimeOffset.UtcNow)); + await session.SaveChangesAsync(ct); } using (var scope = Factory.Services.CreateScope()) { var gdpr = scope.ServiceProvider.GetRequiredService(); var result = await gdpr.PermanentlyEraseAsync( - user.Id, adminUserId: null, reason: "test-erase", TestContext.Current.CancellationToken); + user.Id, adminUserId: null, reason: "test-erase", ct); Assert.False(result.IsError, result.IsError ? result.FirstError.Description : null); } await using var read = GetTenantedSession(); - Assert.Null(await read.LoadAsync(activeLinkId, TestContext.Current.CancellationToken)); - Assert.Null(await read.LoadAsync(unlinkedLinkId, TestContext.Current.CancellationToken)); + Assert.Null(await read.LoadAsync(activeLinkId, ct)); + Assert.Null(await read.LoadAsync(forgottenLinkId, ct)); + + // The forgotten link's archived stream must have its PII masked in place — + // not left verbatim (archiving alone only flags rows). This is the + // regression guard for the hard-delete erase gap. + var archived = await read.Events.QueryAllRawEvents() + .Where(e => e.IsArchived) + .ToListAsync(ct); + var forgottenLinked = archived + .Where(e => e.StreamId == forgottenLinkId) + .Select(e => e.Data) + .OfType() + .Single(); + Assert.Equal("[DELETED]", forgottenLinked.Subject); + Assert.Equal("[DELETED]", forgottenLinked.Email); } } diff --git a/src/dotnet/Modgud.Api/Features/Groups/AutoMembershipSyncHandlers.cs b/src/dotnet/Modgud.Api/Features/Groups/AutoMembershipSyncHandlers.cs index 710ed6b6..c51ff1e3 100644 --- a/src/dotnet/Modgud.Api/Features/Groups/AutoMembershipSyncHandlers.cs +++ b/src/dotnet/Modgud.Api/Features/Groups/AutoMembershipSyncHandlers.cs @@ -5,6 +5,7 @@ using Modgud.Api.Features.Shared; using Modgud.Domain.Users.Events; using Modgud.Authentication.Events; +using Modgud.Authentication.Domain.ExternalAuth.Events; namespace Modgud.Api.Features.Groups; @@ -27,6 +28,7 @@ internal static class PrincipalPaths public const string PersonLastname = PersonPrefix + "Lastname"; public const string PersonAcronym = PersonPrefix + "Acronym"; public const string PersonUserName = PersonPrefix + "AccountName"; + public const string PersonExternalIdentities = PersonPrefix + "ExternalIdentities"; // Group paths (for group-as-principal scripts) public const string GroupEmail = GroupPrefix + "Email"; @@ -129,6 +131,49 @@ protected override Task SyncAsync(UserDeletedEvent @event, IDocumentSession sess => recalculator.RecalculateForPrincipalAsync(@event.Id, session, changedPaths: null); } +/// +/// Reacts to UserExternalIdentityLinkedEvent / ...UnlinkedEvent — linking or +/// unlinking an external identity mutates Person.ExternalIdentities, which +/// a membership script may read (e.g. "is a member of any federated IdP", or a +/// specific-provider gate). Without these handlers the durable auto-group +/// membership goes stale until an unrelated profile event happens to fire — the +/// confirmed Phase-4 gap (link/unlink triggered no recompute today). +/// +/// Scoped to the Person.ExternalIdentities changed-path so the dependency +/// filter can skip scripts that don't read it once that optimization is live +/// (it is inert today — deps are never collected — so this currently evaluates +/// every auto-group, which is correct and cheap at link/unlink frequency). +/// +/// +public class AutoMembershipOnExternalIdentityLinkedHandler( + IAutoMembershipRecalculator recalculator, + ILogger logger) + : ReferenceSyncHandler(logger) +{ + protected override bool ShouldSync(UserExternalIdentityLinkedEvent @event) => true; + + protected override Task SyncAsync(UserExternalIdentityLinkedEvent @event, IDocumentSession session) + => recalculator.RecalculateForPrincipalAsync(@event.UserId, session, + new[] { PrincipalPaths.PersonExternalIdentities }); +} + +/// +/// Symmetric to — an +/// unlink (now a hard-delete) removes a ref from Person.ExternalIdentities, +/// so any script keyed on it must re-evaluate. +/// +public class AutoMembershipOnExternalIdentityUnlinkedHandler( + IAutoMembershipRecalculator recalculator, + ILogger logger) + : ReferenceSyncHandler(logger) +{ + protected override bool ShouldSync(UserExternalIdentityUnlinkedEvent @event) => true; + + protected override Task SyncAsync(UserExternalIdentityUnlinkedEvent @event, IDocumentSession session) + => recalculator.RecalculateForPrincipalAsync(@event.UserId, session, + new[] { PrincipalPaths.PersonExternalIdentities }); +} + /// /// Paths that can change on a Group-as-principal when GroupUpdatedEvent /// fires. Sent to the recalculator so auto-groups whose scripts match other groups diff --git a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/ExternalLoginProcessor.cs b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/ExternalLoginProcessor.cs index fd5d646a..199663db 100644 --- a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/ExternalLoginProcessor.cs +++ b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/ExternalLoginProcessor.cs @@ -93,66 +93,65 @@ public async Task ProcessAsync( // session membership derivation in Success() (always an array post-capture). var externalGroups = ClaimValues(rawClaims, "groups"); - // 1. Existing link → happy path + // 1. Existing link → happy path (or "forget + rematch" for a dead link) var link = await session.Query() .Where(l => l.Issuer == issuer && l.Subject == subject) .FirstOrDefaultAsync(ct); if (link is not null) { - // Cross-user hijack guard: if the caller is already authenticated as - // a different user, refuse regardless of link state. - if (authenticatedUserId is { } authId && authId != link.UserId) + var linkedUser = await userManager.FindByIdAsync(link.UserId.ToString()); + + // Variant C — "unlink forgets the binding". A link is dead when its + // user no longer exists (stale/orphaned) OR it carries a legacy + // IsUnlinked tombstone (pre-ShouldDelete data). A dead link must never + // block a fresh login: append the terminal ExternalIdentityUnlinkedEvent + // so the inline projection's ShouldDelete drops the doc — freeing the + // (Issuer, Subject) slot — then fall through to the no-link matching + // chain (authed-link / email-trust / JIT) which re-binds by policy. + // (Driving the delete via the event rather than session.Delete keeps the + // stream live + maskable for a later GDPR erase and is rebuild-safe.) + // The match key is (Issuer, Subject), not the old link id — so the + // identity can re-home to a different user once it has been released. + if (linkedUser is null || link.IsUnlinked) { - logger.LogWarning( - "Auth: Link-attempt rejected — external subject already linked to different user (authUser={AuthId}, linkUser={LinkId})", - authId, link.UserId); - return ExternalLoginResult.Failed("Idp.LinkedToOtherUser", - "This identity is already linked to another Modgud account."); - } - - var user = await userManager.FindByIdAsync(link.UserId.ToString()); - - if (user is null) - { - // Stale link — the referred user no longer exists. Whether it - // was soft-unlinked (legacy code path) or just orphaned, the - // right action is the same: hard-delete + archive the stream - // so the (Issuer, Subject) slot frees up for the fresh - // matched/JIT user below. Soft-unlink is not enough — the - // unique index on (Issuer, Subject) is not partial on - // IsUnlinked, so a tombstone still blocks a new link. - logger.LogWarning( - "Auth: Stale link {LinkId} → missing user {UserId}; hard-deleting and falling through", - link.Id, link.UserId); - session.Delete(link.Id); - session.Events.ArchiveStream(link.Id); + logger.LogInformation( + "Auth: External identity {State} link {LinkId} forgotten (was user {UserId}) — re-matching by policy", + link.IsUnlinked ? "unlinked" : "stale", link.Id, link.UserId); + session.Events.Append(link.Id, + new ExternalIdentityUnlinkedEvent(link.Id, capturedAt, link.UserId)); await session.SaveChangesAsync(ct); link = null; // drop through to the no-link branches below } - else if (link.IsUnlinked) - { - // User still exists but has explicitly unlinked this IdP from - // their profile. Don't silently re-link — require them to go - // through Profile → Security again. - return ExternalLoginResult.Failed("Idp.Unlinked", "This external identity has been disconnected."); - } else { + // Live link. Cross-user hijack guard: a live link cannot be + // stolen by a different authenticated user. (A forgotten / + // unlinked (iss,sub) is fair game for re-homing above — but a + // live one is not.) + if (authenticatedUserId is { } authId && authId != link.UserId) + { + logger.LogWarning( + "Auth: Link-attempt rejected — external subject already linked to different user (authUser={AuthId}, linkUser={LinkId})", + authId, link.UserId); + return ExternalLoginResult.Failed("Idp.LinkedToOtherUser", + "This identity is already linked to another Modgud account."); + } + // Apply user-update-script patches only when this provider is // authoritative for the profile (decision A) — the existing link's // IsCreator covers the JIT-creator default so a JIT-created user's // profile isn't frozen. Email-conflict is a hard reject. if (ShouldPatchProfile(config, link)) { - var applyResult = await ApplyUserUpdatesAsync(user, scriptResult, ct); + var applyResult = await ApplyUserUpdatesAsync(linkedUser, scriptResult, ct); if (applyResult is not null) return ExternalLoginResult.Failed(applyResult.ErrorCode, applyResult.ErrorMessage); } await RecordScriptRunAsync(link, config, scriptResult, rawClaims, capturedAt, ct); - logger.LogInformation("Auth: External login (returning) user {UserId} via IdP {IdpId}", user.Id, loginProviderId); - return await Success(user, link, externalPrincipal, loginProviderId, issuer, config, externalGroups, ct); + logger.LogInformation("Auth: External login (returning) user {UserId} via IdP {IdpId}", linkedUser.Id, loginProviderId); + return await Success(linkedUser, link, externalPrincipal, loginProviderId, issuer, config, externalGroups, ct); } } diff --git a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/ProfileLinkEndpoints.cs b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/ProfileLinkEndpoints.cs index 34ce0ef9..881ae46e 100644 --- a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/ProfileLinkEndpoints.cs +++ b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/ProfileLinkEndpoints.cs @@ -3,6 +3,7 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Identity; using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.Logging; using Modgud.Authorization.AspNetCore; using Modgud.Authentication.ExtensionMethods; using Modgud.Authentication.Domain; @@ -84,14 +85,16 @@ public static void MapProfileLinkEndpoints(this IEndpointRouteBuilder endpoints, .RequireAuthorization() .RequiresPermission("user:read"); - // Disconnect a link. Soft-delete (IsUnlinked=true) so the same external - // identity can be re-added to a different user later if needed. + // Self-service disconnect. Variant C — "unlink forgets the binding": + // hard-deletes + archives the link so the same external identity can be + // re-linked on a later login (to this or, once released, another user). group.MapDelete("{linkId}", async ( ShortGuid linkId, HttpContext http, TimeProvider clock, [FromServices] IDocumentSession writeSession, [FromServices] UserManager userManager, + [FromServices] ILoggerFactory loggerFactory, CancellationToken ct) => { var userId = http.GetUserId(); @@ -101,36 +104,92 @@ public static void MapProfileLinkEndpoints(this IEndpointRouteBuilder endpoints, if (link is null || link.IsUnlinked) return Results.NotFound(); if (link.UserId != userId.Value) return Results.Forbid(); - // Self-lockout guard: if this is the last auth method (no local - // password AND no passkey AND no other external links), refuse. - var user = await userManager.FindByIdAsync(userId.Value.ToString()); - if (user is not null) + return await UnlinkAsync(writeSession, userManager, + loggerFactory.CreateLogger(UnlinkLogCategory), clock, link, isAdmin: false, ct); + }); + + // Admin force-unlink: disconnect any user's external link. Same + // hard-delete + last-auth-method guard as self-service; gated on + // user:write. (An admin who wants the account gone deletes the user — + // force-unlink must not silently strip a user's last remaining factor.) + endpoints.MapDelete($"{path}/admin/users/{{userId}}/external-links/{{linkId}}", async ( + ShortGuid userId, + ShortGuid linkId, + TimeProvider clock, + [FromServices] IDocumentSession writeSession, + [FromServices] UserManager userManager, + [FromServices] ILoggerFactory loggerFactory, + CancellationToken ct) => + { + var link = await writeSession.LoadAsync(linkId.Guid, ct); + if (link is null || link.IsUnlinked || link.UserId != userId.Guid) return Results.NotFound(); + + return await UnlinkAsync(writeSession, userManager, + loggerFactory.CreateLogger(UnlinkLogCategory), clock, link, isAdmin: true, ct); + }) + .RequireAuthorization() + .RequiresPermission("user:write"); + } + + private const string UnlinkLogCategory = "Modgud.Authentication.ExternalAuth.Unlink"; + + /// + /// Shared unlink core for the self-service and admin force-unlink endpoints. + /// Enforces the last-auth-method guard, then "forgets the binding" (Variant C): + /// appends the terminal on the link + /// stream — the inline projection's ShouldDelete drops the projection doc, + /// freeing the (Issuer, Subject) slot for a later re-link without leaving a + /// blocking tombstone and without archiving (so a later GDPR erase can still mask + /// the stream's PII) — plus the user-stream + /// (which keeps + /// Person.ExternalIdentities + UserView in sync and drives the + /// auto-membership recompute). + /// + private static async Task UnlinkAsync( + IDocumentSession writeSession, + UserManager userManager, + ILogger logger, + TimeProvider clock, + ExternalIdentityLink link, + bool isAdmin, + CancellationToken ct) + { + var user = await userManager.FindByIdAsync(link.UserId.ToString()); + + // Self-lockout guard: refuse to remove the last remaining auth method + // (no local password AND no passkey AND no other live external link). + if (user is not null) + { + var otherLinks = await writeSession.Query() + .Where(l => l.UserId == link.UserId && !l.IsUnlinked && l.Id != link.Id) + .AnyAsync(ct); + var hasPassword = await userManager.HasPasswordAsync(user); + var hasPasskey = await writeSession.Query() + .AnyAsync(c => c.UserId == link.UserId, ct); + if (!otherLinks && !hasPassword && !hasPasskey) { - var otherLinks = await writeSession.Query() - .Where(l => l.UserId == userId.Value && !l.IsUnlinked && l.Id != link.Id) - .AnyAsync(ct); - var hasPassword = await userManager.HasPasswordAsync(user); - var hasPasskey = await writeSession.Query() - .AnyAsync(c => c.UserId == userId.Value, ct); - if (!otherLinks && !hasPassword && !hasPasskey) + return Results.BadRequest(new { - return Results.BadRequest(new - { - Code = "Idp.LastAuthMethod", - Message = "Cannot remove the only remaining authentication method. Set a password or add another provider first.", - }); - } + Code = "Idp.LastAuthMethod", + Message = "Cannot remove the only remaining authentication method. Set a password or add another provider first.", + }); } - - var now = clock.GetUtcNow(); - writeSession.Events.Append(link.Id, - new ExternalIdentityUnlinkedEvent(link.Id, now, userId)); - writeSession.Events.Append(link.UserId, - new UserExternalIdentityUnlinkedEvent(link.UserId, link.Id, link.LoginProviderId, now)); - await writeSession.SaveChangesAsync(ct); - - return Results.NoContent(); - }); + } + + var now = clock.GetUtcNow(); + // Terminal event on the link stream → ShouldDelete drops the projection doc + // (frees the unique slot, rebuild-safe, stream stays maskable). + writeSession.Events.Append(link.Id, + new ExternalIdentityUnlinkedEvent(link.Id, now, link.UserId)); + writeSession.Events.Append(link.UserId, + new UserExternalIdentityUnlinkedEvent(link.UserId, link.Id, link.LoginProviderId, now)); + await writeSession.SaveChangesAsync(ct); + + logger.LogInformation( + "Auth: External identity disconnected{AdminTag} — {UserName} unlinked provider {ProviderId} (link {LinkId})", + isAdmin ? " by admin" : "", user?.UserName, link.LoginProviderId, link.Id); + + return Results.NoContent(); } private static LinkDto ToDto(ExternalIdentityLink l, Dictionary providerByName) diff --git a/src/dotnet/Modgud.Authentication/Gdpr/GdprService.cs b/src/dotnet/Modgud.Authentication/Gdpr/GdprService.cs index cbd5599e..def5b137 100644 --- a/src/dotnet/Modgud.Authentication/Gdpr/GdprService.cs +++ b/src/dotnet/Modgud.Authentication/Gdpr/GdprService.cs @@ -1,5 +1,6 @@ using Modgud.Authentication.Domain; using Modgud.Authentication.Domain.ExternalAuth; +using Modgud.Authentication.Domain.ExternalAuth.Events; using Modgud.Authentication.Events; using Modgud.Authentication.Sessions; using Modgud.Authorization.Apps; @@ -270,13 +271,26 @@ private async Task> PerformPermanentEraseAsync(Guid userId, Guid? // projection doc here; the PII-bearing events are masked + archived // below alongside the user stream (archiving alone only flags the // rows — the raw events must be masked to truly erase the PII). - // Include already-unlinked tombstones, which still hold that PII - // (so drop the !IsUnlinked filter). - var linkIds = (await session.Query() + // Discover the link ids two ways and union them: + // • live links via the projection-doc query, and + // • EVERY link the user ever held via the user stream's + // UserExternalIdentityLinkedEvent mirrors (each carries the + // LinkId). When a link is unlinked/re-homed (Variant C) the + // projection doc is dropped via ShouldDelete — but the stream is + // deliberately LEFT LIVE (un-archived), because Marten's masking + // does NOT rewrite already-archived streams. So the doc query + // alone misses the forgotten link, yet its still-live stream holds + // the PII; the user-stream mirror is the durable index back to it, + // and the mask + archive loops below scrub it during the erase. + var liveLinkIds = (await session.Query() .Where(l => l.UserId == userId) .ToListAsync(ct)) - .Select(l => l.Id) - .ToList(); + .Select(l => l.Id); + var historicalLinkIds = (await session.Events.FetchStreamAsync(userId, token: ct)) + .Select(e => e.Data) + .OfType() + .Select(e => e.LinkId); + var linkIds = liveLinkIds.Concat(historicalLinkIds).Distinct().ToList(); foreach (var linkId in linkIds) session.Delete(linkId); diff --git a/src/dotnet/Modgud.Authentication/Identity/ExternalAuth/ExternalIdentityLinkProjection.cs b/src/dotnet/Modgud.Authentication/Identity/ExternalAuth/ExternalIdentityLinkProjection.cs index fa93bb37..c14015b4 100644 --- a/src/dotnet/Modgud.Authentication/Identity/ExternalAuth/ExternalIdentityLinkProjection.cs +++ b/src/dotnet/Modgud.Authentication/Identity/ExternalAuth/ExternalIdentityLinkProjection.cs @@ -41,11 +41,16 @@ public ExternalIdentityLink Apply( return current; } - public ExternalIdentityLink Apply( - ExternalIdentityUnlinkedEvent @event, - ExternalIdentityLink current) - { - current.IsUnlinked = true; - return current; - } + /// + /// Variant C — "unlink forgets the binding". Unlinking DELETES the projection + /// doc rather than leaving a soft IsUnlinked tombstone, which (a) frees + /// the (Issuer, Subject) unique slot so the same identity can be + /// re-linked, and (b) — because the delete is driven by a terminal event, not + /// a session.Delete + ArchiveStream — a full projection rebuild + /// replays Linked → Unlinked and nets to "no doc", so the slot stays + /// free without archiving. Keeping the stream live (un-archived) is what lets a + /// later GDPR erase still reach + mask the PII the link events carry (Marten's + /// data-masking does not rewrite already-archived streams). + /// + public bool ShouldDelete(ExternalIdentityUnlinkedEvent @event) => true; } diff --git a/src/dotnet/Modgud.Authentication/Projections/UserViewProjection.cs b/src/dotnet/Modgud.Authentication/Projections/UserViewProjection.cs index a7c7a0f8..37be67fd 100644 --- a/src/dotnet/Modgud.Authentication/Projections/UserViewProjection.cs +++ b/src/dotnet/Modgud.Authentication/Projections/UserViewProjection.cs @@ -134,6 +134,10 @@ public UserView Apply(UserExternalIdentityLinkedEvent @event, UserView current) public UserView Apply(UserExternalIdentityUnlinkedEvent @event, UserView current) { + // Deduplicated provider-id set (not per-link): unlinking one of two links + // from the SAME provider drops the provider from this indicator. Accepted + // cosmetic limitation — see UserView.ExternalLoginProviderIds docs. Authz + // never reads this; Person.ExternalIdentities is the per-link source. if (!current.ExternalLoginProviderIds.Contains(@event.LoginProviderId)) return current; return current with { diff --git a/src/dotnet/Modgud.Infrastructure/Persistence/Marten/Projections/Users/UserView.cs b/src/dotnet/Modgud.Infrastructure/Persistence/Marten/Projections/Users/UserView.cs index d6c29874..58e99b3f 100644 --- a/src/dotnet/Modgud.Infrastructure/Persistence/Marten/Projections/Users/UserView.cs +++ b/src/dotnet/Modgud.Infrastructure/Persistence/Marten/Projections/Users/UserView.cs @@ -20,6 +20,16 @@ public record UserView /// Empty = local-only user. Multi-entry = linked to several providers. /// Drives the admin user list's "IdP-connected" indicator; frontend /// resolves ids to display names via the LoginProvider store. + /// + /// KNOWN LIMITATION (accepted): this is a deduplicated provider-id set, not + /// per-link, so it cannot distinguish one link from two links to the SAME + /// provider. A user holding two links from one provider who unlinks one will + /// drop that provider from the indicator until their next link event or a + /// projection rebuild. Cosmetic only — this field is never consulted for any + /// authorization decision (durable membership reads `Person.ExternalIdentities`, + /// which IS keyed per-link). A per-link fix would need a rebuild migration to + /// reconstruct the history, which is not worth it for an admin-list badge. + /// /// public List ExternalLoginProviderIds { get; init; } = []; diff --git a/src/dotnet/Modgud.Tests.Unit/Api/Features/Groups/AutoMembershipSyncHandlersTests.cs b/src/dotnet/Modgud.Tests.Unit/Api/Features/Groups/AutoMembershipSyncHandlersTests.cs index a2f77691..786c5902 100644 --- a/src/dotnet/Modgud.Tests.Unit/Api/Features/Groups/AutoMembershipSyncHandlersTests.cs +++ b/src/dotnet/Modgud.Tests.Unit/Api/Features/Groups/AutoMembershipSyncHandlersTests.cs @@ -1,5 +1,6 @@ using Modgud.Api.Features.Groups; using Modgud.Authentication.Events; +using Modgud.Authentication.Domain.ExternalAuth.Events; using Modgud.Authorization.Events; using Modgud.Authorization.Membership; using Modgud.Authorization.Principals; @@ -57,6 +58,7 @@ public class PrincipalPathsConstants [InlineData("Person.Lastname")] [InlineData("Person.Acronym")] [InlineData("Person.AccountName")] + [InlineData("Person.ExternalIdentities")] public void Person_paths_match_dotted_prefix_format(string expected) { var values = new[] @@ -64,7 +66,7 @@ public void Person_paths_match_dotted_prefix_format(string expected) PrincipalPaths.IsActive, PrincipalPaths.IsDeleted, PrincipalPaths.Email, PrincipalPaths.NormalizedEmail, PrincipalPaths.PersonFirstname, PrincipalPaths.PersonLastname, PrincipalPaths.PersonAcronym, - PrincipalPaths.PersonUserName, + PrincipalPaths.PersonUserName, PrincipalPaths.PersonExternalIdentities, }; Assert.Contains(expected, values); } @@ -205,6 +207,18 @@ public TestableGroupDeleted() : base(new ThrowingRecalculator(), NullLogger base.ShouldSync(@event); } + private sealed class TestableExternalLinked : AutoMembershipOnExternalIdentityLinkedHandler + { + public TestableExternalLinked() : base(new ThrowingRecalculator(), NullLogger.Instance) { } + public new bool ShouldSync(UserExternalIdentityLinkedEvent @event) => base.ShouldSync(@event); + } + + private sealed class TestableExternalUnlinked : AutoMembershipOnExternalIdentityUnlinkedHandler + { + public TestableExternalUnlinked() : base(new ThrowingRecalculator(), NullLogger.Instance) { } + public new bool ShouldSync(UserExternalIdentityUnlinkedEvent @event) => base.ShouldSync(@event); + } + [Fact] public void UserCreated_always_syncs() => Assert.True(new TestableCreated().ShouldSync( @@ -246,5 +260,15 @@ public void GroupCreated_always_syncs() [Fact] public void GroupDeleted_always_syncs() => Assert.True(new TestableGroupDeleted().ShouldSync(new GroupDeletedEvent(Guid.NewGuid()))); + + [Fact] + public void ExternalIdentityLinked_always_syncs() => + Assert.True(new TestableExternalLinked().ShouldSync( + new UserExternalIdentityLinkedEvent(Guid.NewGuid(), Guid.NewGuid(), Guid.NewGuid(), "https://idp.test", DateTimeOffset.UtcNow))); + + [Fact] + public void ExternalIdentityUnlinked_always_syncs() => + Assert.True(new TestableExternalUnlinked().ShouldSync( + new UserExternalIdentityUnlinkedEvent(Guid.NewGuid(), Guid.NewGuid(), Guid.NewGuid(), DateTimeOffset.UtcNow))); } }