feat(federation): Federation v1 (broker → session-derived authz) + v1.1 token layer#24
Merged
Merged
Conversation
…in guard Additive, fail-closed groundwork for federation v1 (no login-path behavior yet — nothing reads these until phases 1-4). - Group.ExternallyDrivable (default false), threaded through the event, the PrincipalProjectionBase Create+Apply seam (which the integration map omitted), both group commands, and the group endpoints. Full-replace GroupUpdatedEvent producers (UsersEndpoints add/remove member, RealmAdminBootstrapper) now preserve the flag instead of resetting it. - LoginProvider.TrustForAuthorization + AuthoritativeForProfile (default false), full 20-site lockstep incl. the 4th LoginProviderAddedEvent construction site (LoginProviderRealmSeeder). New event params are trailing-defaulted so existing callers + old streams replay fail-closed. - ExternalClaimsStore + ClaimEntry: plain non-event-sourced Marten doc keyed on userId (mirrors UserDeletionState); scrubbed by Delete on delete/GDPR in later phases. No masking rule (not event-sourced). - Bidirectional realm:admin config guard (decision G): a group conferring realm:admin can never be ExternallyDrivable; enforced in both group command handlers via shared GroupMembershipGuards. Tests: 5 new Phase-0 integration tests (config guard reject/allow, default false, projection round-trip, LoginProvider flag round-trip). Full suite 1217 green (1212 baseline + 5). Refs dev-docs/future-features/federation-v1-implementation-plan.md (phase 0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…, scrub Every successful external login now refreshes the per-user ExternalClaimsStore for the current provider only (delete+rewrite), tagged provider:<slug>, staged onto the existing login SaveChanges so it is atomic with the link/script write. Covers all four success branches via RecordScriptRunAsync (returning) and CreateLinkAsync (link-to-authed / email-link / JIT; +providerSlug param). - ExtractRawClaims: known-multi-valued claims (groups/roles/amr) stay arrays even at count 1, so membership scripts can always use array semantics (decision F/I15 — fixes the single-value-collapse hazard). - GDPR erase (GdprService) + admin delete (DeleteUsersCommand) now Delete the store in their existing batches — plain doc, no masking rule needed. No authorization behavior yet (the store is written but nothing reads it until phases 2-4). Tests: 4 new Phase-1 integration tests (capture round-trip incl. groups, delete+rewrite on re-login, GDPR + admin-delete scrub). Full suite 1221 green (+4). Refs dev-docs/future-features/federation-v1-implementation-plan.md (phase 1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e parity
The core engine. Computes a login's ephemeral group membership in-memory from
the current provider's claims, without ever touching durable Group.MemberIds.
Not wired into the login path yet (phase 3 calls it).
- EvalPrincipal: a transient Person subclass with the ephemeral ExternalGroups
surface. De-risked empirically (EvalPrincipalMembershipTests): Type.Is(p,'person')
is a CLR-type check (a non-Principal wrapper returns false — so it MUST derive
from Person), array .includes translates, dictionary-indexer does NOT — so the
v1 federation surface is p.ExternalGroups.includes('...') (the canonical
'user in upstream group X'); generic claim-dict access is a documented v1
boundary. Never persisted (no Marten/STJ registration; deriver is IQuerySession).
- MembershipPredicateEvaluation.EvaluateSafe: extracted shared helper so the
recalculator (durable) and the deriver share ONE swallow-NRE-as-false impl.
- ILoginTimeMembershipDeriver / LoginTimeMembershipDeriver (Authorization,
read-only): evaluates Auto+ExternallyDrivable groups over EvalPrincipal,
defensively drops realm:admin-conferring groups, never appends
GroupMembershipRecomputedEvent. Registered scoped.
- AutoMembershipRecalculator: both engines now skip ExternallyDrivable groups
(&& !g.ExternallyDrivable / early return) so external scripts never hit the
JSONB batch (which can't see ExternalGroups) and durable MemberIds stays
source=local in v1.
Tests: 4 de-risk unit + 4 integration (match/ignore-non-drivable, realm:admin
drop, never-writes-MemberIds, mandatory JSONB-batch-vs-in-memory reconciliation).
Full suite 1229 green (+8).
Refs dev-docs/future-features/federation-v1-implementation-plan.md (phase 2).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ty gate
First behavior activation. On every external login through a TrustForAuthorization
provider, ExternalLoginProcessor.Success() now derives ExternallyDrivable group
membership in-memory (local ∪ this provider's groups) and bakes each matched
group id as the internal no-destination claim modgud:session-group on the sign-in
cookie. Runs in all four success branches (Success is the single convergence
point); a password / untrusted-provider / non-matching login carries none. An
AuthLog line is emitted per privileged external-derived grant. The claim sits
inert on the cookie until phase 4 unions it into resource_access — nothing reads
it yet, so no token behavior changes.
Profile authority gate (decision A): ApplyUserUpdatesAsync is now gated on
ShouldPatchProfile — only an explicitly AuthoritativeForProfile provider, or the
returning-link path of the JIT creator (new ExternalIdentityLink.IsCreator marker,
set true only at JIT), writes the four profile fields. Ends the every-provider-
patches-every-login flapping. New links (link-to-authed / email-match) are not
creators, so they patch only when explicitly authoritative.
- FederationClaimTypes.SessionGroup ('modgud:session-group') in
Modgud.Permissions.Abstractions (shared issuer→carrier→union literal).
- ExternalIdentityLink.IsCreator + event/projection wiring (trailing-defaulted).
- ExtractRawClaims groups surfaced to the deriver via ClaimValues.
Tests: 4 new (trusted bakes claim, untrusted/non-matching bake none, profile gate
creator-vs-non-creator). Full suite 1233 green (+4).
Refs dev-docs/future-features/federation-v1-implementation-plan.md (phase 3).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…resource_access union) Closes the federation v1 end-to-end flow read side. The session-group carrier baked on the login cookie in phase 3 now flows cookie→grant→resource_access, unioned with durable membership at token/UserInfo time, held inside the hub boundary, and frozen across refresh. - IPermissionService: three union OVERLOADS (GetUserGroupsAsync / GetUserPermissionsAsync / GetUserRolesAsync) taking IReadOnlyCollection<Guid> sessionGroupIds (I8 — distinct overloads, not optional params, so the existing call sites stay byte-identical). - PermissionService.ResolveGroupsWithProvenanceAsync: durable BFS from the user (source=local) unioned with a session BFS seeded from the matched group ids PLUS their ancestors (a session child still confers its parents' roles), each group tagged local/session; durable wins on overlap. - Provenance-aware realm:admin strip (I9 / decision G): the synthetic realm:admin marker and the realm-admin role are emitted ONLY for durable groups — a session-sourced group can never confer realm:admin, even via an inherited ancestor role. <app>:admin and below remain externally drivable. Invariant comment pinned at ExpandBypassTiers. - AuthorizationEndpoints: GetDestinations yields nothing for the carrier (hub boundary, mirrors SecurityStamp) + cc-flow SetDestinations belt-and-braces; CreateClaimsPrincipalAsync gains cookiePrincipal and copies the carrier with no destination — authorize passes the live cookie principal, refresh/code/device passes the rehydrated reference-token principal (frozen, no recompute — session = lease, decision E); BuildResourceAccessAsync threads sessionGroupIds; the human UserInfo path reads + parses the carrier, while cc-flow and Service-Account paths pass empty. Reference-access-token clients only (JWT-access is the documented v1 non-goal — degrades to durable-only authz). Tests: +1 unit (carrier suppressed from both tokens, mirroring the SecurityStamp leak guard) and +5 integration (union, ancestor inheritance, provenance-aware realm:admin strip vs durable-kept, BoundTo gating, empty-session/no-arg parity). Full suite 1239 green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Phase 5 of federation v1: the supporting changes around the now-complete token/grant union (phases 0–4). No change to the core authz path. - Client lib (hub boundary): quarantine the dead RS-side groups flattener in Modgud.Client.AspNetCore/ModgudClaimsTransformation.cs. The IdP never emits a `groups` block in resource_access (group membership is expanded into roles/permissions before emission), so FlattenGroupObjectArray was dead — removed it and its call, kept the public `GroupClaimType` const but marked it [Obsolete] (binary-compat, NuGet minor bump per the versioning convention). Tests now pin the hub-boundary (a groups block yields no claims); README + ResourceApi sample updated. - Docs: rewrite docs/concepts/auto-membership.md. The old examples used p.OrganizationalUnit / p.Department / p.externalClaims.department, none of which exist on Person (they transpile-fail / zero-match). Now documents the real Person script surface and the federation ephemeral surface — p.ExternalGroups (string[], use .includes(...)) and p.Source — as session-scoped, live-only, ExternallyDrivable-only, with the realm:admin-local-only and two-layer-source rules. - AMR note (I15): SamlFlavorData.AmrMapping is configured/seeded but parsed-but-not-consumed; the XML doc now says so and points to a new dev-docs follow-up (dev-docs/future-features/saml-amr-wiring.md, registered in the index + sidebar). - Admin UI (verified via devtools — screenshots + network round-trip): - LoginProvider connection editor (linking tab): TrustForAuthorization + AuthoritativeForProfile toggles next to TrustForEmailLink (OIDC + SAML share the modal). Threaded through FormState / emptyForm / load / create / update + the frontend DTO + request types. Save round-trip confirmed (PUT body + response carry both fields). - Group editor (General tab): an ExternallyDrivable toggle that mirrors the backend bidirectional guard — disabled with an explanatory hint when the group is Manual, and disabled when a selected role confers realm:admin (realm:admin is hard local-only); the flag is force-cleared in those states. Threaded through the form / save payload + the frontend GroupDto / GroupPayload. All three states verified live. - i18n: German strings for the new keys in de.json; English inline fallbacks per the i18n convention. - Monaco membership editor: federation script examples (p.ExternalGroups.includes(...) and p.Source) added to the examples popover for discoverability. Verified: dotnet build clean (0 new warnings); unit suite green (1039); frontend `pnpm type-check` (build mode) green; all three VitePress builds green (docs public + in-app + dev-docs, no dead links); admin UI smoke-tested in the browser (both editors, save payloads, disabled-state guards). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…keys RealmTokenValidationHandler skipped installing the per-realm verification keys whenever IsReferenceToken was true. That guard was only correct while access tokens were JWTs: RealmSigningKeyHandler signs access and id tokens — including the stored payload of a reference access token — with the realm key. With UseReferenceAccessTokens (the federation-v1 default, decision I14), /connect/userinfo and /connect/introspect load that realm-signed payload and validate it against the global key pool, yielding 401 invalid_token (OpenIddict ID2090, "signing key not found") and introspect active:false. Key the handler off token TYPE instead of reference-vs-JWT: install realm keys when access_token or id_token is among ValidTokenTypes, mirroring RealmSigningKeyHandler. Validations that accept neither (e.g. a client_assertion, whose JWS is signed by the client's key) keep their stock key set — which the old code got wrong too. Cross-realm isolation is preserved: the global pool never signs access/id tokens, so it cannot admit a foreign realm's token. Adds regression coverage to UserInfoPerAudienceTests (previously JWT-only, so the reference path was untested): reference-token UserInfo, and reference refresh-token redemption -> UserInfo. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ns (v1.1) v1 limited AccessTokenType.Jwt clients to durable-membership authz: the session-group carrier is no-destination and a JWT has no server-side payload for UserInfo to read it back from, so the federated overlay never reached JWT clients. But not every resource server can consume opaque reference tokens, so JWT must support federation too (reference stays the recommended default). For JWT clients, compute the per-audience resource_access (durable ∪ session-derived, via the same BuildResourceAccessAsync as the reference/UserInfo path) at token issuance — while the carrier is still on the principal — and bake the RESULT into the self-contained access token. Only the computed permissions/roles (which the RS is entitled to) are emitted; the carrier itself stays no-destination, so the hub boundary holds. Audiences come from the requested resource= indicators, so the baked blocks match the token's narrowed aud and never over-share. UserInfo echoes the token's own block for JWT clients instead of recomputing a narrower carrier-less set. Freeze-on-refresh is automatic: the refresh token is a reference token carrying the frozen carrier, so each refresh re-bakes the same set. Trade-off (ratified): for JWT clients the federated membership is frozen for the access-token lifetime and the session lease is enforced at the refresh boundary rather than instantly — the same staleness JWT clients already accept for durable permissions. Clients needing instant server-authoritative revocation use reference tokens. Reference clients are unchanged. Tests: JWT client bakes resource_access into the token and UserInfo echoes it; the baked block honours the requested audience only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… bug) The end-to-end federated test added here exposed a pre-existing v1 bug: reference-token federation (the recommended default) silently dropped the session-derived permissions. OpenIddict's PrepareAccessTokenPrincipal clones the sign-in principal and strips every claim WITHOUT the access_token destination before building the access token — and that filtered principal is what gets persisted as the reference token's payload. So the no-destination session-group carrier never survives into the reference token, and UserInfo's ReadSessionGroupIds reconstructs nothing → durable-only authz. Never caught: FederationV1Phase4Tests pins the service-level union, not the UserInfo reconstruction; the manual Keycloak smoke was blocked by the ID2090 hotfix before reaching a working UserInfo. Fix: generalize the bake (BakeFederatedResourceAccessForJwtAsync → BakeFederatedResourceAccessAsync) to run for BOTH reference and JWT clients. The computed resource_access is an access-token-destined claim, so it survives the strip: for JWT it rides the self-contained token; for reference it is persisted in the server-side payload, stays opaque on the wire, and is echoed at UserInfo. The carrier itself never gains a destination — the hub boundary holds. Consistent with decision E: the set is frozen for the token's life and re-baked at refresh; reference tokens additionally keep instant revocation. Tests: end-to-end federated-cookie tests forge a valid ApplicationScheme cookie carrying the session-group carrier (via the app's real SignInManager principal + TicketDataFormat under the system tenant), then prove the session-derived permission reaches a JWT access token and a reference token's UserInfo, with a non-federated control. New CookieContainerHandler.Seed test seam. 205 integration + 1039 unit green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
windischb
enabled auto-merge (squash)
May 29, 2026 14:42
This was referenced Jun 2, 2026
Merged
windischb
added a commit
that referenced
this pull request
Jun 2, 2026
…48) A code-vs-docs audit (15-agent workflow) found the product surface is ahead of what the docs/comments claim: several merged features were still labelled "not started" / "stub" / "deferred". This corrects the framing so future audits don't re-litigate shipped work. Docs + code COMMENTS only — no runtime behavior changes. dev-docs status lines flipped to verified reality (all cited commits are ancestors of develop): - federation-v1-design / -implementation-plan → Shipped (PR #23 4fa3af0, PR #24 0b70b31); Phase 6 per-realm TTL + durable leased-membership v2 kept as the genuine remainder. - saml-federation + index → Shipped (PR #17 8fc3df0); SLO + SAML IdP-mode kept explicitly deferred. - versioning-publishing-conventions → Shipped (GHCR retention, moving Docker tags, NuGet feed-gate are live workflows). - app-resources-as-permissions → ID-anchored model shipped. - white-label-customization (index) → Phase 1 shipped (8c8dea5/2ec0e58/ ae2f9ca); page-builder runtime + custom-CSS kept deferred. - production-readiness-audit → SAML SP DONE (PR #17), rescored 1→3; LDAP/AD kept open. - identity-lifecycle-untangle → auto-membership externalClaims contradiction RESOLVED (PR #24); durable-lease piece kept open. - permission-modell §5 + userinfo-hybrid-flat-emission → corrected to "groups NOT emitted (IdP-internal)" — matches AuthorizationEndpoints.cs + UserInfoPerAudienceTests. (The line a future groups-claim decision would consciously lift; left at today's reality.) False in-code comments removed/corrected (comment-only): - SamlEndpoints.cs: dropped the false "handlers are 501 stubs" note — they delegate to the live SamlLoginFlow. - SamlSetup.cs: dropped the "still to come task #13/#14/#15" block. - Program.cs: SAML hook is wired, not a "placeholder". - AuthorizationEndpoints.cs: claims injection is shipped via IPermissionService, not "deferred / legacy IRoleRepository". - CI workflow comments: :staging → :beta (the tag actually pushed). Deliberately NOT touched: signing-key rotation-overlap docs — those belong to the separate rotation thread (implement-vs-document still open). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
windischb
added a commit
that referenced
this pull request
Jun 19, 2026
…tyStamp sign-in foundation) (#89) * test(auth): RED — SecurityStamp sign-in bug class (AppBase v5.1.1/v5.1.2) Pin the SecurityStamp-sign-in bug before fixing it. The security stamp is authoritative on UserSecurityData; ApplicationUser.SecurityStamp is a transient mirror hydrated only by the UserManager finders. Two defects make non-password sign-in mint a cookie that the SecurityStampValidator rejects on its next pass (silent logout), and a third drops federated authz claims when the validator re-mints the principal: - EventSourcedUserStore.GetSecurityStampAsync returns the transient mirror instead of re-fetching the authoritative value (GetPasswordHashAsync does re-fetch — the asymmetry is the root cause). - EventSourcedUserStore.CreateAsync seeds UserSecurityData with an independent GUID → mirror and authoritative diverge from birth. - Magic-link/passkey-web raw-load the user; OIDC/SAML hand-build the principal with no stamp claim at all. Harness: force SecurityStampValidatorOptions.ValidationInterval = Zero so every authenticated request becomes a stamp-revalidation test (production 5-min cache masked the bug). Password logins re-align via FindByName and stay green. New targeted tests (Security/SecurityStampSignInTests.cs + one in ExternalLoginProcessorTests): store re-fetch, birth alignment, magic-link revalidation survival, OIDC/SAML principal stamp claim. The flip additionally surfaces 11 pre-existing tests as genuine bug-class manifestations — all turn green with the fix: - magic-link e2e + the two UserInfoPerAudience *Federated* tests (federated session-group claims lost on revalidation — the Modgud-specific amplifier), - 6 MfaTests + AuthEnforcement.MfaDisable + EmailOtp both-methods (stamp rotation on 2FA setup/verify/disable self-logs-out the acting session — the v5.1.2 RefreshSignInAsync-after-2FA item). RED state: 15 failed / 312 passed (Modgud.Api.Tests). Fix lands next. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): GREEN — SecurityStamp authoritative across all sign-in paths (AppBase v5.1.1/v5.1.2) Make the SecurityStampValidator see a consistent stamp on every sign-in path so non-password sessions are no longer silently logged out, and federated authz survives revalidation. Turns the RED checkpoint green (326 integration + 1174 unit, 0 failures). Root cause (store): - EventSourcedUserStore.GetSecurityStampAsync now RE-FETCHES the authoritative stamp from UserSecurityData (mirrors GetPasswordHashAsync). This is the single-point fix: every ClaimsPrincipalFactory-based path (magic-link, passkey, password, MFA, OIDC/SAML) mints a cookie/principal with the correct stamp regardless of how the user was loaded. - EventSourcedUserStore.CreateAsync seeds UserSecurityData.SecurityStamp from the ApplicationUser mirror Identity already generated, so the two no longer diverge from birth. Federated sign-in (OIDC + SAML, shared ExternalLoginProcessor): - The hand-built principal now carries the authoritative SecurityStamp claim, so the federated cookie passes revalidation instead of being rejected on the first pass. Federation amplifier (Modgud-specific): - SecurityStampValidatorOptions.OnRefreshingPrincipal re-injects the federation session-group + external.* claims when the validator re-mints the principal via the factory. Without it a federated session silently lost its externally-derived authorization (unioned into resource_access at token time) after one interval. 2FA self-service (the v5.1.2 RefreshSignInAsync item): - MfaEndpoints setup/verify/disable rotate the stamp; each now calls RefreshSignInAsync so the acting session survives while the user's OTHER sessions stay invalidated (the security purpose of the rotation). Mirrors the existing change-password behaviour. Tests: - SecurityStampSignInTests keeps the two deterministic store-level root-cause tests (cover the magic-link + passkey raw-load mechanism). The earlier magic-link end-to-end test was removed: it duplicated MagicLinkTests.MagicLink_FullFlow_CompletesSignIn (now a real stamp-revalidation guard under ValidationInterval=0) and was order-flaky on shared harness state (MagicLinkSelfService cache survives ResetMartenData). - OIDC/SAML + federation + MFA/email-otp e2e guards are the pre-existing tests that went red in the checkpoint and are green again here. Note: magic-link/passkey /login still raw-load the user — correct via the store re-fetch above; switching them to UserManager.FindByIdAsync is optional defense-in-depth (their IsActive/IsDeleted gates already exist), deferred to keep the fix minimal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): load magic-link/passkey login user via UserManager finder, not raw session Follow-up hardening to the SecurityStamp fix. The store re-fetch already made the minted cookie's stamp correct, but the raw session.LoadAsync left the in-memory ApplicationUser carrying ALL its other security fields as potentially-stale mirror values (TwoFactorEnabled, LockoutEnd, AuthenticatorKey, ...). Concretely, magic-link /login reads user.TwoFactorEnabled directly to decide TOTP step-up — off a raw-loaded mirror. Today that field happens to stay in sync, but it rests on the same "the two docs never diverge" assumption that the security stamp violated. Load both /login paths through UserManager.FindByIdAsync, which runs PopulateSecurityDataAsync and hydrates the user from the authoritative UserSecurityData. This closes the whole stale-mirror class (not just the stamp), matches every other sign-in path (password/2FA/email-otp/native already use the finder), and filters IsDeleted at the source. The explicit IsActive/IsDeleted gates stay as defense-in-depth. Full suite green: 326 integration / 0 failures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): audit remediation wave 1 — revoke live access on security-state change (#1-#5) Rotation/revocation/refresh cluster from the similar-bugs audit: a security-state change must fully revoke live access (OAuth tokens + device-session rows + auth cookies), not just rotate the stamp or delete tracking rows. - #1 SessionEndpoints "log out everywhere": RevokeAllSessionsAsync only deleted tracking rows (invisible to the cookie middleware) — other devices stayed signed in for up to 30 days and OAuth tokens survived. Now routes through IUserAccessRevoker.RevokeAllAccessAsync(ForceSignOut) (rotates stamp → kills all cookies + revokes tokens + deletes rows) and RefreshSignInAsync for the acting device. Targeted single-session revoke documented as a known limitation (needs a session-id claim on the cookie — follow-up). - #2 Admin password reset (UsersEndpoints): added RevokeAllAccessAsync — the incident-response lever now actually kills the user's tokens/sessions. - #3 Self-service forgot/reset-password (PasswordResetEndpoints): added RevokeAllAccessAsync on the anonymous ATO-recovery path. - #4 Email change (UpdateUserHandler): email is the recovery anchor and was mutated via raw session.Store (no stamp rotation, no revoke). Now revokes live access when the email actually changes. - #5 External-identity unlink (ProfileLinkEndpoints, self + admin): added RevokeAllAccessAsync so unlinking ends the in-flight federated session + tokens. Tests: new SecurityAuditWave1Tests (RED→GREEN) prove #1 via a second device cookie and #2/#3 via device-session revocation. ProfileSelfServiceTests.Approve updated to re-authenticate after the (now-revoking) email change. Full suite green (328 integration + 1174 unit; one unrelated ColdStart flake passes on retry). Stacked on fix/security-stamp-signin-tests (shared files). Branch: fix/security-audit-remediation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): audit remediation wave 2 — lifecycle gate on 2FA completion (#13, #14) The 2FA-completion endpoints minted a full, kill-switch-surviving session without re-checking IsActive/IsDeleted. The partial 2FA cookie (TwoFactorUserId scheme) is not stamp-validated, so a user deactivated AFTER the password step but before the 2FA step could complete the second factor and obtain a durable session. - #13 Email-OTP completion (EmailOtpEndpoints /login): the OTP service does not gate; added an IsActive/IsDeleted re-check (+ partial-scheme sign-out) before SignInAsync. - #14 TOTP completion (MfaEndpoints /login): TwoFactorAuthenticatorSignInAsync only runs CanSignInAsync (lockout + confirmation), never IsActive/IsDeleted — deactivation sets IsActive=false without a LockoutEnd so it passed. Added the same gate on the already-captured twoFactorUser before sign-in. Both endpoints already resolve the user via GetTwoFactorAuthenticationUserAsync (which filters IsDeleted); the live vector is deactivation (IsActive=false). Tests: SecurityAuditWave2Tests deactivate the user mid-login (after the partial cookie) and assert a valid TOTP/OTP submission is now rejected (401). Full suite green (330 + 2 new; the one ColdStart RealmCreateAtomicity failure is a pre-existing flake, green in isolation). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): audit remediation wave 3 — block cross-account takeover via script-remapped email (#15) TrustForEmailLink auto-linked to an existing account using the user-update SCRIPT's output email, but email_verified attests only the raw 'email' claim. An admin-authored script can remap its output email from any claim (e.g. upn / preferred_username), so an attacker holding a genuinely-verified throwaway 'email' could set the match key to a victim's address and be auto-linked into the victim's account (full takeover; amplified by the shipped Entra default `claims.email ?? claims.preferred_username`). Fix: IsExternalEmailVerified → IsEmailLinkTrustworthy(config, rawClaims, matchEmail). For OIDC it now requires email_verified==true AND the match key to equal the raw 'email' claim the IdP actually verified. SAML stays exempt (signed assertion is the trust anchor). Scoped to the auto-link-to-existing match key only — JIT creation of a new account and the coarse domain allowlist are unchanged, so scripts that derive email from preferred_username for brand-new users still work. Tests: SecurityAuditWave3Tests prove (a) a upn→match-email remap with a verified throwaway email is rejected (Idp.EmailNotVerified, no takeover) and (b) a legit login whose script email IS the verified raw email still auto-links (no over-blocking). Full suite green (333/0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(gdpr): audit remediation wave 4 — scrub PII-bearing projections on permanent erase (#20, #21) GDPR PermanentlyEraseAsync masked + archived the event streams and PII-cleared the ApplicationUser doc, but never touched the inline Principal/Person projection or the async UserView read-model — both carry Email/Firstname/Lastname/NormalizedEmail. Marten event-masking rewrites event JSON only (no projection re-run), and the UserDeletedEvent Apply handlers merely flag IsDeleted while KEEPING the PII. So the "forgotten" user's name + email survived as queryable docs and via /api/principal/lookup (Art. 17), and the Person stayed IsActive=true (#21 — selectable in pickers / auto-group recompute). Fix: hard-delete Principal + UserView in PerformPermanentEraseAsync. The stream is masked + archived in the same operation, so a rebuild (archived events excluded) won't recreate them. Deleting the Principal doc also resolves #21 (no active forgotten Person). Appending a UserDeletedEvent was rejected: its Apply only flags IsDeleted (keeps PII) and would re-run the inline projection, re-creating the doc. Test: SecurityAuditWave4Tests asserts the PII is present pre-erase, then gone (Person + UserView null, no Person matches the email) post-erase. Full suite green (334/0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(oauth): audit remediation wave 5 — guard refresh/OTP/magic-link redeem races (#22, #24, #25, #26) TOCTOU / optimistic-concurrency cluster from the similar-bugs audit. Each finding was a read-modify-write that two concurrent requests both win under default last-writer-wins semantics. #22 Refresh-token redeem replay (High): OpenIddictToken/AuthorizationDocument now UseOptimisticConcurrency. UpdateAsync re-loads the live row in its own session (so same-session version checking engages) and compares the caller's ConcurrencyToken — preserved by OpenIddict across its load-here/update-there boundary — against the live one, rejecting a stale redeem with OpenIddict's ConcurrencyException so the losing racer gets invalid_grant instead of a replayed token pair (RFC 6749 §10.4). Revoke/prune sweeps retry on conflict so they never throw spuriously under concurrent rotation. #23 Revoke-sweep miss (Low): retry handles a raced row modification; a row INSERTED mid-sweep still isn't caught (not a version conflict) — documented as stamp-mitigated (lifecycle revocation rotates the stamp first). #24 Email-OTP MaxAttempts race (Medium): the wrong-code counter is now an atomic jsonb Patch.Increment, so concurrent guesses can't lose each other's increment — the brute-force lockout is reliable rather than racy. #25 Magic-link one-time-use (Medium): Marten does not version-check deletes, so the consume is now a version-checked Store of a ConsumedAt flag (+ IsConsumed gate for non-concurrent replays); the losing concurrent redeemer's ConcurrencyException maps to 401. #26 ConsentTicket (Low): corrected the false "cannot both succeed" atomicity comment; documented the benign residual and the opt-in hardening. Tests: SecurityAuditWave5Tests (deterministic two-session guards, not flaky parallelism). 340 integration + 1174 unit green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(passkey): audit remediation wave 6 — fix enroll uniqueness query + require UV on login (#33, #16) #33 Passkey enroll byte[]== Marten query: the IsCredentialIdUniqueToUserCallback compared CredentialId (byte[]) with `==` inside a Marten LINQ query, which is untranslatable (Postgres 22P02) — so web enroll 500'd and the uniqueness guard was effectively skipped. Materialize then SequenceEqual, mirroring the login path. #16 Passkey login user-verification: login-options requested UserVerification "preferred", so an authenticator could skip UV and MakeAssertionAsync would still accept the assertion — downgrading a passkey login to possession-only. Require UV so the library rejects a non-verified assertion and a passkey stays MFA-grade. Tests: SecurityAuditWave6Tests (in-memory uniqueness + the byte[]== throw; options carry userVerification=required). 342 integration + 1174 unit green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): audit remediation wave 7 — gate principal lookup + revoke SA tokens on lifecycle change (#19, #6, #7, #8) #19 principal/lookup PII leak: the cross-type lookup was authenticated-only, so any zero-role user could enumerate the whole realm directory incl. emails. Gate it with authorization-group:read (its sole consumer, the group-member picker, already requires that) and drop the Email field from the projection (the picker never rendered it). #6/#7/#8 service-account token revocation: a deactivated/deleted SA, or a rotated credential, left its already-issued client_credentials (M2M) tokens valid — deactivation only blocks NEW issuance and a deleted/rotated client doc doesn't invalidate outstanding bearer tokens. SA tokens carry sub = sa.Id, so deactivate (#6) and delete (#7) revoke by subject (kills every credential's tokens); a single credential rotate/delete (#8) revokes by application id (credId == the OAuth app id) so it cuts off exactly that client. Adds IOAuthGrantRevoker. RevokeTokensByApplicationIdAsync for the narrow case. Tests: SecurityAuditWave7Tests (lookup 403 for zero-role + no Email; deactivate/ delete/rotate revoke the seeded token). 347 integration + 1174 unit green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): audit remediation wave 8 — hygiene/Low cluster (#9-#12, #18, #27, #29, #31, #32, #34) The remaining Low/hygiene findings from the similar-bugs audit: #9 UserInfo now re-checks IsActive/IsDeleted before serving identity/authz (mirrors the token-exchange guard) — closes the deactivated-user JWT window. #10 TOTP enable/disable now revoke OAuth reference tokens (stamp rotation alone doesn't, since stock introspection trusts store status). #11 Corrected the false OAUTH-07 comment claiming role-revocation rotates the stamp (RBAC is FK-based; demotions land at the next refresh). #12 Email-OTP disable now reaches stamp-rotation + RefreshSignIn parity with TOTP and revokes tokens; enable revokes tokens too. #18 GetTwoFactorEnabledAsync + GetAuthenticatorKeyAsync re-fetch authoritatively from UserSecurityData (same fix as GetSecurityStampAsync) — closes the latent raw-load 2FA-bypass booby-trap before the native-grant branches merge. #27 CIMD resolver re-checks the realm opt-in on EVERY resolve (was cache-miss only), so disabling CIMD takes effect immediately instead of at TTL. #29 Admin magic-link issuance also rejects IsDeleted (mirrors the finder). #31 Corrected the false "one transaction" invite-consume docs and mark a spent invite used when the target admin already exists (no longer armed forever). #32 resend-bootstrap-invite filters to UsedAt==null — no re-arming a used invite. #34 Email-OTP code generation is now rejection-sampled (modulo-bias-free). Tests: SecurityAuditWave8Tests (2FA-disable token revocation + authoritative re-fetch). 350 integration + 1174 unit green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(oauth): harden consent-ticket consume to a real one-time-use guard (#26 follow-up) Wave 5 only corrected #26's false atomicity comment; the TOCTOU itself stayed (rated cosmetic — no auth code is minted, the duplicate authorization is deduped on the next /authorize and revoked at logout). Now closed properly with the same pattern as #25: ConsentTicket gets UseOptimisticConcurrency, and the consent endpoint CLAIMS the ticket via a version-checked Store BEFORE creating any authorization. Two parallel POSTs both load ConsumedAt==null but only one commits the claim; the loser's stale Store throws and maps to a 409 — and because the claim happens first, the loser never mints the duplicate authorization row either. Test: SecurityAuditWave5Tests.ConsentTicket_ConcurrentConsume_SecondWriterRejected (deterministic two-session guard). The live consent flow stays green (DcrFullFlow / CimdFullFlow exercise the real endpoint). 351 integration + 1174 unit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): re-audit — fix two regressions introduced by the remediation A verification pass (52-agent re-audit) confirmed all 31 findings genuinely fixed and surfaced two functional regressions the fixes themselves introduced: 1. (Medium) "Log out everywhere" left the acting device with NO UserSession row: RevokeAllAccessAsync deletes every session row including the actor's, and RefreshSignInAsync keeps the cookie but doesn't re-track it — so the user's own "active sessions" list read empty while still signed in. Re-record the acting session after refresh. Guarded by an added assertion in SecurityAuditWave1Tests. 2. (Low) Magic-link consumed-row spuriously rate-limited the next request: Wave 5 switched the consume from Delete to a mark-consumed Store, but the per-user request rate-limit predicate excluded IsExpired and not IsConsumed, so a just-used link blocked a new one for up to RateLimitMinutes. Add !IsConsumed. 351 integration + 1174 unit green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(service-accounts): make SA credential token format configurable, default Reference (#6/#7/#8 follow-up) Re-audit found SA credentials were hard-pinned to AccessTokenType.Jwt, so the W7 revoke (flip the store doc to Revoked) was a no-op against an already-issued self-validating M2M JWT until expiry — the unifying residual behind #6/#7/#8. Per owner decision, make it configurable instead of forced: expose AccessTokenType on the SA credential issue/update DTOs (+ the admin credential modal), defaulting to Reference — opaque, stored, INSTANTLY revocable, so deactivate/delete/rotate cuts off live access immediately. JWT stays opt-in for resource servers that must self-validate without introspection; their tokens then survive a revoke until expiry, so the UI hint nudges a short lifetime. Existing credentials keep their stored format. Update path merges format+lifetime into one settings revision. Backend: 352 integration + 1174 unit green. Frontend type-check clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): re-audit follow-up — email-otp verify rate-limit (#24) + close cheap test gaps #24 (per owner decision): the atomic Patch.Increment closed the lost-update half of the Email-OTP lockout race, but the Attempts>=Max gate still reads a stale snapshot, so a concurrent burst at Attempts=0 could evaluate many guesses before lockout trips. Add a per-IP fixed-window rate-limit policy ("email-otp", 30/min) on the anonymous verify endpoint to bound the burst. Sized above any legitimate verify cadence. Test gaps (close where cheap): - #26: add an endpoint-level reuse test (CimdFullFlowTests) proving the claim-first consume makes a second consent POST 409 (the concurrent leg stays store-tested). - #25: cross-referenced — endpoint reuse→401 is already covered by MagicLink_TokenIsOneTimeUse; the Wave5 test covers the concurrency leg. - #33: documented as a known gap (no software WebAuthn authenticator in-checkout, so the test verifies the Marten query-translation contract, not the full endpoint). 352 integration + 1174 unit green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(frontend): bump dompurify + ws via pnpm overrides to clear audit advisories The PR's CI Frontend Build failed at the `pnpm audit --prod --audit-level=moderate` gate on two pre-existing TRANSITIVE deps (untouched by this PR): ws (high, via @microsoft/signalr) and dompurify (5× moderate, via monaco-editor). Raise the existing dompurify override to >=3.4.7 and add a scoped ws override (>=7.5.11 for the vulnerable 7.x range). Resolves to ws 8.21.0 / dompurify 3.4.11. ws is only used by SignalR's Node path, not the browser bundle, so the bump has no runtime impact on the app. type-check + vite build green; pnpm audit clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(service-accounts): gate SA-deactivate revoke on persisted transition (clears CodeQL FP) CodeQL flagged the W7 revoke as a "user-controlled bypass of sensitive method": the revoke was guarded by `dto.IsActive == false` (a request-controlled flag). It's a false positive — the guard scopes an ADDITIVE hardening action (revoke M2M tokens) to the deactivation case, not an access-control check; skipping it when the request isn't a deactivation is correct. De-taint + improve: capture the prior active-state from the loaded record, and gate the revoke on the PERSISTED active→inactive transition (prior from the load, new state read back from the store) instead of the raw request flag. No request-tainted value guards the revoke anymore, and a benign edit / re-save of an already-inactive SA no longer triggers a pointless revoke sweep. SecurityAuditWave7Tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
windischb
added a commit
that referenced
this pull request
Jul 20, 2026
…r-side passkey ceremony, lockout concurrency (#159) * fix(security): apply existing SSRF + one-time-use guards where they were missing First remediation wave from the code-verified July security review. Every fix here reuses a pattern this repo had already solved once and simply hadn't wired up at the second site — no new security machinery is invented. SSRF guard, now shared The CIMD metadata fetcher's transport guard (DNS resolved and validated before connect, closing the rebinding window; redirects off; tight timeouts) moves out of the CIMD namespace to Modgud.Infrastructure/Http as SsrfIpGuard + SsrfSafeHttpHandlerFactory, with a purpose label so a refusal is diagnosable from the log. The IP range table is unchanged; its tests moved with it. Newly protected, both previously a bare HttpClient: - SAML IdP metadata fetch (SamlSetup) - dynamic OIDC discovery + backchannel (DynamicOidcSchemeManager) Both URLs are realm-admin supplied, and a realm admin is a lower-trust tier than the platform operator, so "an admin configured it" is not a reason to skip the guard. One-time use, actually enforced EmailOtpChallenge and PasskeyCeremony consumed via Delete. Marten does NOT version-check deletes, so two concurrent redemptions of the same code or ceremony_id both passed and each completed a login / minted a token — the single-use guarantee existed only in a comment. Both documents now carry a ConsumedAt marker written through a version-checked Store (the pattern MagicLinkChallenge already used), so exactly one racer wins. The native urn:cocoar:magic grant queried by user + token hash and checked only expiry, never IsConsumed. Because the web flow marks links consumed rather than deleting them, a link already used in the browser stayed redeemable through the native channel. Now gated. Follow-on fix found while writing the tests: since a consumed challenge now survives, the issue-path rate limit would have throttled the next request and locked a user out of email OTP right after a successful login. Consumed challenges are exempt, and the re-issue path mutates the loaded row instead of storing a fresh instance (a fresh instance carries no version and would be rejected now that the document is version-checked). SAML ACS body cap The anonymous ACS endpoint does XML parsing + signature validation per request and inherited Kestrel's 30 MB default. Capped at 512 KB, matching the tightening AssetsEndpoints already applies. Tests Four regression tests: concurrent consume of a passkey ceremony and of an email OTP each let exactly one racer win; a consumed OTP is rejected while re-issue still works; and a magic link consumed in the web flow is rejected by the native grant. That last one was negative-controlled — with the gate removed it mints tokens successfully, so it pins the actual vulnerability rather than asserting current behaviour. The passkey ceremony test helper now asserts the security property (still redeemable?) instead of the storage mechanism (row deleted?). Gated locally: build clean, unit 1395/1395, integration 521/521. Deferred deliberately: the SAML ACS rate limit. Its policy enum is per-realm configurable and would pull in DTOs, manifest export, settings patch and frontend — out of scope for a "wire up the existing guard" change, and P3. * fix(security): move the web passkey ceremony server-side (P0-3) The web login ceremony shipped the full AssertionOptions to the browser as plain Base64 in the Modgud.Passkey.Challenge cookie — no signature, no encryption — and the login endpoint parsed it back as trusted input. A client could therefore rewrite the ceremony options or re-present an old challenge together with an old assertion, and the cookie was never actually cleared because Delete was called without the Path the cookie was set with. The native /connect/passkey/begin flow already solved this with a server-side PasskeyCeremony record; the code comment even noted that the two flows differed only in "challenge transport". They no longer differ: - login-options persists a PasskeyCeremony (realm-scoped, so ClientId is null) and the cookie carries nothing but its opaque id. Tampering with the id merely fails to resolve. - login loads the ceremony, rejects it when expired or already consumed, and consumes it BEFORE verifying via a version-checked Store of ConsumedAt — the same single-use mechanism wave 1 gave this document, so a captured ceremony cannot be replayed and two concurrent logins cannot both redeem one challenge. - The RP ID is pinned at begin-time and reused at redeem, so an admin editing PrimaryDomain mid-ceremony cannot cause a begin/redeem drift (matching the native flow's rationale). - Cookie name and path are constants now, shared by Append and Delete, so the delete actually matches. - Expired ceremonies are cleaned opportunistically on the same traffic that creates them, as the native begin endpoint does. Test: the cookie must parse as a bare Guid, the authoritative options must live in the server-side record, and a cookie in the OLD client-held format (Base64 of attacker-chosen options) must not authenticate anything. Gated locally: build clean, unit 1395/1395, integration 522/522. One run showed the known load-dependent UserView catch-up flake in an unrelated class's fixture setup (documented in the Critter-Stack blocker notes); it passed in isolation and the suite was green on a re-run. Not included: the account-lockout concurrency fix (P0-4). An atomic increment alone does nothing there because UpdateAsync writes AccessFailedCount back from the in-memory user after every AccessFailedAsync, and the reset-detection that emits a security event keys off that same write-back. Untangling the counter from that round-trip is a focused change to the Identity store contract and needs its own pass with genuinely concurrent tests. * fix(security): make the account-lockout counter concurrency-safe (P0-4) The five-attempt lockout was bypassable by sending the guesses in parallel instead of sequentially. IncrementAccessFailedCountAsync only did an in-memory `user.AccessFailedCount++`, and EventSourcedUserStore.UpdateAsync -- which ASP.NET Identity calls after EVERY AccessFailedAsync -- wrote the whole UserSecurityData document back from a load-time snapshot. Concurrent failed logins therefore all read N and all wrote N+1, so a burst registered as roughly one failure and the threshold never tripped. Fixing the increment alone would not have helped: the whole-document Store was the second half of the race. So the lockout fields are now owned end to end by the IUserLockoutStore methods: - IncrementAccessFailedCountAsync does a server-side jsonb increment (the existing "Audit #24" pattern from the email-OTP attempt counter) and reads the authoritative value back, because UserManager compares that return value against MaxFailedAccessAttempts. - ResetAccessFailedCountAsync and SetLockoutEndDateAsync write through atomic patches. - GetAccessFailedCountAsync and GetLockoutEndDateAsync re-fetch authoritatively, the same idiom as GetSecurityStampAsync. This also closes a smaller gap: IsLockedOutAsync read a mirror that could not see a lockout set by a concurrent request. - UpdateAsync patches only the fields it actually owns and no longer touches AccessFailedCount or LockoutEnd. That incidentally protects the grace-period fields from the same clobbering. The lock/unlock and resolved-failure-streak audit events were stale-vs-fresh diffs inside UpdateAsync that only worked because of the whole-document Store. They move to the paths that actually change those values, unchanged in shape. Tests: three integration tests driving the real login endpoint, so each attempt gets its own DI scope and Marten session. Verified as real coverage by reverting the store and confirming both concurrency tests fail without the fix. Unit 1395/1395, integration 525/525. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements Federation v1: an app integrates only Modgud, which brokers to the tenant's external IdP (EntraID/Okta/Keycloak/SAML). External-login
groupsclaims drive group membership for the session only — never durable, never leaking raw to the resource server. Spec + decisions live in #23 (dev-docs/federation-v1-design.md); this PR is the implementation (phases 0–5) plus the v1.1 token-layer fixes.What's here
LoginTimeMembershipDeriver), two-engine parity, profile-authority gate, token/grant union (cookie→grant carrier,resource_accessunion), hardening + admin UI for the three flags (TrustForAuthorization,AuthoritativeForProfile,ExternallyDrivable).401 invalid_token(OpenIddict ID2090) —RealmTokenValidationHandlerkeyed offIsReferenceTokeninstead of token type, so realm-signed reference payloads were validated against the global key pool. Now keyed offaccess_token/id_tokeninValidTokenTypes.resource_access(durable ∪ session-derived) is baked into the access token at issuance for both reference and JWT clients. This lets JWT clients federate (reference stays the recommendation) and fixes a discovered bug where OpenIddict'sPrepareAccessTokenPrincipalstrips the no-destination carrier from the reference token too — silently degrading reference federation to durable-only. The carrier never gains a destination; only the rendered result leaves. UserInfo echoes the baked block.Hub boundary
Raw external group IDs / the session-group carrier never reach a token or UserInfo — only the computed
resource_access(permissions/roles the RS is entitled to).realm:adminstays hard local-only (config guard + provenance-aware strip).Verification
/authorize→ reference token →/connect/userinforendersresource_access["https://fed-smoke-api"] = { permissions: ["widget:write"], roles: ["Fed Widget Writer"] }, sourced solely from the Keycloakengineersgroup via the session carrier.Notes
🤖 Generated with Claude Code