feat: SAML 2.0 SP federation + login-provider single-modal Add+Edit#17
Merged
Conversation
SAML plan-page: - SLO deferred to v2 (saves 2-3 days in v1, additive later) - IdP-mode on concrete customer demand (lib covers both modes) - Metadata refresh 24h default + per-provider 1h/6h/24h/7d override - Group-claim mapping = JsEval Membership Scripts + Quick-Map UI (Identity-Hub pattern, no pass-through to downstream tokens — consistent with existing OIDC behaviour, see new memory anchor project_identity_hub_vs_federation_proxy_open for the broader product-positioning question that stays open) - Multi-IdP login UX split out into its own plan-page (provider-protocol-agnostic, applies to OIDC + SAML + future protocols equally — not a SAML problem) - Endpoints table updated (no SLO endpoint in v1) - Effort re-estimated: ~11 days (was ~13 with SLO) LoginProviderType.cs: - Saml = 2 doc-comment rephrased: "External SAML 2.0 Identity Provider (Modgud acts as Service Provider)" — was misleadingly worded as if Modgud itself were the IdP - Stale "Phase 2+" markers stripped from Ldap + Kerberos comments multi-idp-login-ux.md (new): - Designspace for Picker vs Email-Routing vs Hybrid - Provider-protocol-agnostic, sits as own future-features wave - Recommendation: Pattern C (Hybrid), can phase as Pattern B → C
Adds ITfoxtec.Identity.Saml2 4.18.0 + .MvcCore 4.18.0 NuGet refs to Directory.Packages.props (BSD-3-Clause, compatible with our Apache-2.0 — see plan-page library audit) and references them from Modgud.Authentication. `SamlSetup.AddModgudSaml()` is a placeholder extension method on IServiceCollection so Program.cs has the wiring slot in place; subsequent commits on this branch fill in the actual registrations (flavor registry, dynamic scheme manager, SP cert services, metadata refresh hosted service) without touching Program.cs again. Build green, 0 errors.
The Provider-Edit modal in the SAML wave needs a Flavor picker in the modal header (next to the title) so the body's connection fields can morph based on which flavor is selected. Slot is per- instance template-scoped, so nested modals each get their own independent slot content — no DOM-id conflicts like a Teleport portal would have. MainLayout has a different pattern (hardcoded AppContextSelector + an unused #header-outlet-right portal). Aligning MainLayout to this slot-based pattern is a separate future-feature item; not in scope here.
Typed view over the JSON blob stored on LoginProvider.FlavorData when Type == Saml. Shape mirrors dev-docs/future-features/ saml-federation.md FlavorData proposal: - IdP federation metadata (URL or pasted XML) - IdP Entity ID - Denormalised IdP signing certs (cache from metadata refresh) - NameID format URI (default: SAML 2.0 emailAddress) - Security toggles (signed/encrypted assertions, signed response, signed AuthnRequest) — secure defaults: all on except encryption - AttributeMap: logical-claim → SAML attribute URIs (multi-value to absorb cross-IdP naming differences) - AmrMapping: AuthnContextClassRef URI → AMR values for federated- MFA detection - MetadataRefreshIntervalSeconds (default 24h) FromJson() is forward-compatible: tolerates unknown fields, falls back to defaults on missing fields, never throws on shape variance. SamlNameIdFormats holds the on-wire URN constants. 18 unit tests cover defaults, parse paths, round-trip, and the URN-vs-constant pinning. All green.
Mirrors the OIDC LoginProviderFlavorRegistry shape — parallel ISamlFlavor interface + SamlFlavorRegistry, three concrete implementations. Two registries (one per protocol) instead of one unified to preserve per-protocol type-shape: OIDC needs OidcEndpoints from DeriveEndpoints, SAML needs SamlFlavorData from ApplyDefaults. Forcing them into a single ILoginProviderFlavor would require object-typed returns at the consumption sites. Flavor identity: - GenericSaml — vendor-neutral, no presets, MetadataUrl/Xml only - EntraIdSaml — Microsoft Entra Enterprise App; pre-seeds the http://schemas.xmlsoap.org/.../emailaddress family + groups + Microsoft AMR mapping (MultipleAuthn → mfa) - AdfsSaml — on-prem AD FS; pre-seeds AD claim URIs + windows- authentication AMR; MetadataUrl not required (XML paste path for firewalled deployments) Domain LoginProviderFlavor catalog extended with the three SAML keys. OIDC EntraId vs SAML EntraIdSaml are intentionally distinct — silent collision would be a runtime-auth incident. DI wired through SamlSetup.AddModgudSaml() (placeholder filled in). 22 new flavor + registry tests; 40 SAML tests total green.
Adds dev/saml-idp/ as a self-contained docker-compose for spinning up a local SAML 2.0 IdP — kristophjunge/test-saml-idp wrapping simpleSAMLphp with two pre-baked test users. Lets the SAML federation slice be iterated offline without ngrok, EntraID config friction, or HTTPS-cert plumbing. EntraID-specific quirks (claim URIs, NameID quirks, MFA AuthnContext) still need a real EntraID hop with the right tunnel, but the SP code-path itself gets shaken out here first. Endpoints (host): IdP UI http://localhost:8080/simplesaml/ IdP metadata http://localhost:8080/simplesaml/saml2/idp/metadata.php Test users: user1 / user1pass user2 / user2pass SP env vars (ENTITY_ID + ACS + SLO) are placeholders; README documents the steps to swap in the real LoginProvider GUID once a SAML provider has been created in Modgud and recreate the container.
Mirrors the OIDC DynamicOidcSchemeManager + OidcSchemeBootstrap +
event-handler chain for the SAML side. Key architectural difference:
ITfoxtec.Identity.Saml2 is NOT an ASP.NET Core AuthenticationHandler
like OpenIdConnectHandler, so there's no AuthenticationScheme to
register dynamically. Instead, DynamicSamlSchemeManager owns a
ConcurrentDictionary<Guid, RegisteredSamlProvider> cache that the
SAML endpoint handlers (login / acs / metadata) consume by provider
GUID lookup.
Components:
- RegisteredSamlProvider — lightweight cache value type carrying
provider id, realm slug, flavor key, resolved SamlFlavorData
- DynamicSamlSchemeManager — Register/Unregister/TryGet/Get*
lifecycle, flavor-aware (calls ISamlFlavor.ApplyDefaults), type-
discriminator gate (Saml only), realm-context required on register
- SamlSchemeBootstrap — IHostedService, walks active realms with
TenantContext.Enter, seeds the cache with enabled SAML providers.
WOLV-02-style fix: re-enter tenant per realm so the per-tenant
Marten session reads the right DB
- SamlLoginProviderEventHandlers — six Wolverine handlers parallel
to the OIDC ones, route through SamlLoginProviderReRegister which
short-circuits non-Saml types
- SamlEndpoints.MapSamlEndpoints — three route handlers (sp-metadata
GET, /{providerId}/login POST, /{providerId}/acs POST) — login +
acs return 404 for unknown providers (defensive — don't disclose
existence), 501 NotImplemented for the placeholder bodies until
task #14 fills in the actual SAML protocol logic
DI: AddModgudSaml() now registers manager + bootstrap; Program.cs
wires MapSamlEndpoints().
Verified locally:
- 53 SAML unit tests green (40 existing + 13 new for the manager)
- 997 / 997 unit tests passing overall — no regressions
- Backend boots cleanly, log shows "SamlSchemeBootstrap registered 0
SAML providers across 1 realm(s)" — bootstrap walks realms
- GET /saml/sp-metadata → 501, POST /saml/{guid}/login → 404,
POST /saml/{guid}/acs → 404 (expected for the stubs / unregistered
provider IDs)
Tenant-scoped Marten singleton document SamlSpCertificateDocument
holds the active + previous SP certificate (PFX bytes encrypted via
ASP.NET DataProtection — separate Purpose string from the OIDC
client-secret store, so a leak in one domain doesn't compromise
the other). Plaintext metadata (thumbprint, NotBefore, NotAfter,
created-at, rotation-retire-at) lives alongside so admins / logs
can see cert state without decrypting.
SamlSpCertificateService (scoped, consumes IDocumentSession) owns
the lifecycle:
- GetActiveAsync — lazy-generates on first call, returns X509 with
private key for signing
- GetMetadataCertsAsync — returns the active + (if still in overlap
window) previous cert; this is what SP metadata XML advertises
- RotateAsync — generates fresh cert, demotes active → previous
with 30-day retire timer, installs new active
- RetireExpiredPreviousAsync — clears the previous slot once the
retire timer has passed; no-op when not due
Self-signed cert defaults: RSA 2048, SHA256, 2-year validity,
subject CN=modgud-sp-{realm-slug}, SAN dnsName matching realm
slug. KU=DigitalSignature+KeyEncipherment, EKU=serverAuth+
clientAuth.
DI: SamlSpCertificateStore singleton, SamlSpCertificateService
scoped — registered in SamlSetup. Marten schema registration for
the doc lands in MartenStoreOptionsExtensions alongside the
RealmSettings singleton, same pattern.
13 new unit tests (Store round-trip, Document shape invariants,
service-constant stability). Full lifecycle (rotate/retire) gets
integration coverage once #14 makes the cert service routable.
Backend boots cleanly with the new schema registration; 66 / 66
SAML unit tests green; SAML endpoints still 501/404 (real protocol
logic lands in #14).
End-to-end SAML 2.0 SP protocol implementation against the ITfoxtec
library. Three protocol operations now functional, replacing the
501-stubs from the per-realm endpoint wiring commit:
- SamlMetadataFetcher — fetches IdP federation metadata (URL or
pasted XML), parses EntityID + signing certs + SSO URLs via
XDocument. Named HttpClient via IHttpClientFactory so the fetcher
stays singleton (matches singleton DynamicSamlSchemeManager). 30-
line LINQ-to-XML reader instead of ITfoxtec's full metadata model
— we use 4 fields, not 40.
- DynamicSamlSchemeManager — RegisterAsync now triggers metadata
fetch on URL or parses pasted XML, populates RegisteredSamlProvider
with the parsed SamlIdpMetadata so endpoint handlers don't re-fetch.
Manager flips to async (was Task.FromResult before).
- SamlContextBuilder (scoped) — per-request Saml2Configuration
factory. Loads SP cert via SamlSpCertificateService, builds the
IdP signing-cert list from cached metadata, derives SP EntityID +
ACS URL from the live HttpContext.
- SamlLoginFlow (scoped) — orchestrates the three operations:
* StartLoginAsync: build AuthnRequest (signed per FlavorData
toggle), redirect via HTTP-Redirect binding; returnUrl rides
RelayState round-trip.
* HandleAcsAsync: parse SAMLResponse via Saml2PostBinding, validate
against the IdP's signing certs, extract NameID + attributes,
build OIDC-shaped ClaimsPrincipal (iss=IdP EntityID, sub=NameID,
plus the attribute-map-translated logical-name claims), hand off
to ExternalLoginProcessor, sign in via SignInManager, redirect
to RelayState returnUrl (relative-paths only, open-redirect-safe).
* BuildSpMetadataAsync: emits SP metadata XML with our active +
overlap-window-previous certs, ACS URL, NameIDFormat preference.
- ExternalLoginProcessor — type-discriminator gate widened from Oidc-
only to Oidc OR Saml. Rest of the processor (rawClaims dict, user-
update script, JIT, link-or-create) is protocol-agnostic and
unchanged.
- CreateLoginProviderCommand — SAML branch added. Validates the
Flavor against SamlFlavorRegistry, seeds FlavorData with the
flavor's defaults (EntraID Microsoft URIs, ADFS AD URIs, etc.),
creates the LoginProvider record with Enabled=false (admin enables
after smoke-test). Replaces the old `Create_SamlType_NotYetSupported`
hard gate.
- SamlEndpoints — wires the three routes to SamlLoginFlow. SP
metadata path becomes per-provider (/saml/{guid}/sp-metadata) since
each provider has its own ACS URL.
Integration test `Create_SamlType_NotYetSupported` rewritten as two
new tests: `Create_SamlType_With_KnownFlavor_Succeeds` (happy path)
and `Create_SamlType_With_UnknownFlavor_Rejected` (validation).
Wolverine source-gen handler signature regenerated for the new
ctor (SamlFlavorRegistry added alongside the OIDC one in
CreateLoginProviderHandler).
Verified locally: builds clean, backend boots, all 3 SAML endpoints
route correctly (404 for unknown provider IDs — defensive, no
silent enumeration).
SamlMetadataRefreshService (BackgroundService) wakes every 15 min and re-fetches metadata for any cached provider whose per-provider cadence (FlavorData.MetadataRefreshIntervalSeconds, default 24h, override range 1h/6h/24h/7d) has elapsed since last fetch. Picks up IdP cert rotations ahead of the activation date — most IdPs advertise the next signing key in metadata 1-2 weeks before flipping. DynamicSamlSchemeManager: - Constructor gains TimeProvider so cache entries can be timestamped - New RefreshMetadataAsync(Guid) refetches IdP metadata for a single cached provider and updates the entry in place; returns false (and keeps stale cache) on fetch failure - Cert-change detection logs a clear info line on rotation transitions so admins / log search can see when an IdP rotated keys RegisteredSamlProvider grew a MetadataFetchedAt timestamp slot, populated on register + refresh, consulted by the background service to decide which entries are due. Failures keep the stale data — the cache never goes empty just because a single refresh failed. Bootstrap remains the only path that can leave a provider without any metadata at all. DI: hosted service registered in SamlSetup.AddModgudSaml alongside the cold-start bootstrap. Verified: backend boots clean with the refresh service ticking; 66 / 66 SAML unit tests green; manager test wiring updated for the new TimeProvider ctor arg.
Phase 1 of the login-providers UI work landed alongside the SAML
wave — SAML providers are creatable, listable, and editable via
the existing two-step add-flow + detail-modal pattern. Single-modal
refactor + Quick-Map UI for groups is deferred as a separate wave,
captured at dev-docs/future-features/login-providers-ui-refactor.md.
Changes here:
- Backend FlavorDto.Type — new field ("Oidc" / "Saml") so the admin
UI knows which protocol family each flavor implements
- /api/admin/login-providers/flavors endpoint concatenates the OIDC
registry's flavors with the SAML registry's flavors, both tagged
with their Type. Unified list keeps the picker simple
- Frontend FlavorDto + store consumes the Type field; LoginProvider-
List.vue infers the LoginProviderType to send on Create from the
picked flavor (OIDC flavors → Type=Oidc, SAML flavors → Type=Saml)
- placeholderFlavorData() — SAML branch returns null since SAML
config is supplied on the detail modal (MetadataUrl / Xml are
filled there), not at create time
- FlavorConnectionPanel.vue — adds MultilineText input variant (a
styled textarea) so SAML's MetadataXml field renders with usable
height instead of a single-line input
Build green; ts type-check green; backend bootable; UI exposes
GenericSaml / EntraIdSaml / AdfsSaml in the Add Provider picker
alongside the existing OIDC flavors.
Wired up the final fixes needed for the SAML flow to complete a real
SP-initiated login against the dev simplesamlphp IdP:
- UpdateLoginProviderCommand — branch on Type (Oidc vs Saml) and
validate the flavor against the right registry. OIDC keeps its
DeriveEndpoints validation; SAML defers metadata validation to the
manager's fetch path
- LoginProviderLifecycleCommands.EnableHandler — type-aware
preflight: Oidc requires ClientId + Secret, Saml requires
MetadataUrl or MetadataXml in FlavorData. Internal stays
unchecked
- SamlFlavorData.FromJson — case-insensitive property lookup that
prefers the variant carrying an actual value, so the round-trip
works when the document carries both lowerCamel (canonical
serialisation) and PascalCase (frontend FlavorConfigField keys)
- SamlLoginProviderReRegister — pull the tenant slug from the
session.TenantId when no ambient TenantContext is set (Wolverine
event-handler context), enter the scope before calling
manager.RegisterAsync. Without this the runtime register-on-event
path threw because Wolverine handlers run outside RealmMiddleware
- dev/saml-idp — env vars updated to reference the real provider
GUID end-to-end (SP EntityID + ACS URL both consistent with what
Modgud's SP metadata + AuthnRequest emit)
Verified end-to-end against simplesamlphp:
1. SP-initiated login at /saml/{guid}/login generates a signed
AuthnRequest, redirects to IdP via HTTP-Redirect binding
2. IdP authenticates user1 / user1pass
3. IdP form-POSTs the SAML Response to /saml/{guid}/acs
4. ACS validates signature, audience, claims; hands off to
ExternalLoginProcessor; SignInManager signs in
5. Backend log: "External identity linked to existing user
... via IdP ..."
6. Redirect to RelayState returnUrl issued (302 OK)
docs/admin/saml-federation.md — admin-facing guide covering the
add-provider flow, IdP wiring, SP metadata URL, cert rotation,
troubleshooting, scope notes (SLO + IdP-mode out of scope for v1).
… providers
The OIDC-shaped Redirect-URI field was meaningless for SAML providers — admins
would have copied /signin-oidc/<guid> into EntraID and broken the setup. SAML
providers now expose the two URLs the IdP actually needs (SP metadata for the
"App Federation Metadata URL" field, ACS for the "Reply URL" field), both
read-only with copy buttons. OIDC providers keep the single Redirect-URI field
unchanged.
Backend: LoginProviderDto gains SamlSpMetadataUrl + SamlAcsUrl, populated only
when Type=Saml using the SamlEndpoints path templates ({providerId:D} canonical
form, matching SamlContextBuilder).
Frontend: Allgemein tab branches on provider.Type === 'Saml' to render the SAML
two-URL block instead of the OIDC redirect URI. Verified against the
SimpleSAML-Test provider; OIDC provider regression-clear.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The test asserted that non-OIDC providers surface LoginProvider.TypeNotSupported, but used Type=Saml — which now flows through ExternalLoginProcessor since SAML is a supported protocol. Ldap (and Kerberos) remain the unsupported enum values the runtime gate at ExternalLoginProcessor.cs:56 still rejects. The event-stream setup (StartStream<LoginProvider>) bypasses the create-command validation that also rejects Ldap/Kerberos, so the test still exercises a real defensive code path: a stale Ldap provider somehow present in the stream gets refused at runtime instead of producing a missing-flavor NPE. 163/163 integration tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…er state Today's two-step Add flow (Create with minimal payload → reopen as Edit modal → Update with the rest) is an outlier vs Auth0 / Keycloak / Okta. The first step toward the single-modal refactor is teaching the Create command to accept everything in one go. CreateLoginProviderCommand gains the same optional fields UpdateLoginProviderCommand already carries (Enabled, ClientId, Scopes, UserUpdateScript, StoreRawClaims, RawClaimsRetentionDays, AutoCreateUsers, AllowLinking, TrustForEmailLink, AllowedEmailDomains, IconName, ButtonColorHex). Each is nullable; null falls back to the flavor's default — preserving the existing two-step caller's behavior bit-for-bit. ScriptInputLimits guard runs on UserUpdateScript at Create too when provided, matching the Update path. Secret rotation stays on its own command for audit clarity. Internal-typed creates ignore the extended fields (the seed is fixed). SAML and OIDC paths both honor them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eader
Eliminates the two-step "open inline picker → submit minimal Create → re-open
as Edit modal → fill in everything else" pattern. The login-provider modal
now hosts both flows.
Header-actions slot carries a flavor dropdown (grouped OIDC · / SAML ·). In
Add mode (id='create') it's active and switching morphs the body — Connection
tab gates ClientId/Scopes/Secret behind isSaml, FlavorData clears between
flavors so each ConfigSchema starts from scratch, and the per-flavor defaults
(Scopes, UserUpdateScript, StoreRawClaims, IconName) re-seed automatically
while admin-typed identity stays intact. In Edit mode the picker locks
(Type/Flavor are immutable post-create) and just labels what the provider
runs on.
Save in Add mode posts the full provider state in one Create call (using the
extended CreateLoginProviderCommand) and — for OIDC — fires RotateClientSecret
right after if the admin entered an initial secret. The modal then re-routes
its fragment to the new id, transitioning to Edit mode in-place so the SP/ACS
URLs become visible without a re-open. Enabled defaults to false at Create —
admin opts in explicitly via the toggle that appears in Edit mode.
LoginProviderList drops the inline overlay (and the placeholderFlavorData
hack that papered over OIDC's Required ConfigSchema at minimal-Create time)
and just calls navigateToModal('create').
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…p deferred Phase 2a — single-modal Add+Edit with header-actions flavor picker and the backend Create-with-full-state extension — landed this wave. Phase 2b (Quick-Map UI for groups) stays deferred and the page now spells out the hidden backend cost so the next agent doesn't re-discover it: today's MembershipScript is a per-Group boolean predicate over a projected Principal and never sees raw IdP claims; the Quick-Map generator's claim-driven shape needs a new claims-aware mapping path on LoginProvider before the frontend generator becomes anything more than UI theater. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…adiness gates, Add-flow required-field validation Three findings from the code-review pass on this branch: 1. SAML signature enforcement (CheckRequiredSignatures in SamlLoginFlow). ITfoxtec's Saml2Configuration carries no WantAssertionsSigned / WantResponseSigned properties — the library validates whatever IS signed but never requires signatures to be present. An IdP could send an unsigned Response / Assertion and ReadSamlResponse would happily return. Post-parse XML check now rejects when admin-configured FlavorData asks for signatures and they aren't there, including per-Assertion signatures (the XML-signature-wrapping defense — a Response-level signature alone doesn't satisfy WantAssertionsSigned). 2. Create readiness gates parity with EnableLoginProviderHandler. An OIDC provider cannot authenticate without a ClientSecret, and Create never carries one (separate RotateSecret command for audit clarity), so Create with Enabled=true is structurally unsafe — refused. SAML providers need IdP metadata to be registered usefully, so Create with Enabled=true requires MetadataUrl or MetadataXml in FlavorData. The single-modal Add UI already hardcodes Enabled=false; these gates catch stale/scripted callers that bypass the UI. 3. Frontend Add-flow regression. The old two-step List dialog auto-seeded placeholder GUIDs into Required ConfigSchema fields so Create couldn't fail validation; the single-modal Add path dropped that and would relay a cryptic backend FlavorDataInvalid back to the admin. createProvider now validates Required fields client-side and auto-switches to the Verbindung tab pointing at the missing fields by Label. 20/20 LoginProvider integration tests + 66/66 SAML unit tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ata, document link-flow gap Two real fixes plus an explicit known-limitation marker for the third item. DecryptionCertificates (plural) — SamlContextBuilder.BuildAsync now uses ITfoxtec's plural decryption-cert surface, populated with [active, previous] from a new SamlSpCertificateService.GetDecryptionCertsAsync method, so an IdP that hasn't refreshed its metadata yet during the 30-day rotation overlap can still encrypt assertions to the OLD SP public key and we can still decrypt them. The singular DecryptionCertificate property was already marked obsolete by ITfoxtec 4.18 — the build-warning goes away as a bonus. SamlMetadataFetcher.Parse — explicitly refuses IdP metadata that has no usable signing certs (KeyDescriptor entries with empty or whitespace-only X509Certificate values, or encryption-only KeyDescriptors). Returns null with a structured log warning so the scheme manager treats the provider as unregistered, instead of caching a structurally-broken entry whose SignatureValidationCertificates list is empty. SameSite=Lax link-flow degrade — KNOWN LIMITATION marker added inline at HandleAcsAsync where the existing-app-cookie lookup happens, plus a dev-docs/future-features/saml-link-flow-samesite.md page that captures the three possible fixes (signed RelayState, SameSite=None+Secure marker cookie, server-side state store) and explains why the fix is blocked on the test server + real-IdP verification that the simplesamlphp dev rig can't reproduce (it's on localhost, the bug is cross-site only). 66/66 SAML unit tests + 4/4 SAML integration tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…, stale-cache eviction, Type-aware Update Five hygiene + correctness fixes from the code-review pass. Saml2RequestContext (cert disposal). SamlContextBuilder.BuildAsync now returns a disposable wrapper that owns every X509Certificate2 it constructs (active + previous SP certs from GetDecryptionCertsAsync, every IdP signing cert parsed from metadata). Saml2Configuration is not IDisposable and ITfoxtec never disposes the certs it borrows, so under sustained SAML traffic the native CNG/CAPI key handles would otherwise leak until GC finalisers caught up. Call-sites in SamlLoginFlow.StartLoginAsync and HandleAcsAsync use `using var ctx = ...` so handles return to the OS immediately on response. SamlLoginFlow.BuildSpMetadataAsync got the same treatment via try/finally — the SP-metadata endpoint is AllowAnonymous and a scraper hammering it was the easiest DoS surface. Switched the IdP-cert constructor from the obsolete X509Certificate2(byte[]) to X509CertificateLoader.LoadCertificate as a free upgrade — the build warning goes with it. Rotation cooldown. SamlSpCertificateService.RotateAsync now refuses a second rotation while the previous-cert slot is still in its 30-day overlap window. The original buggy behaviour silently dropped the cert IdPs were most likely still cached on, breaking AuthnRequest signature validation across the federation for ~24h until each IdP refreshed metadata. Admin can wait for RetireExpiredPreviousAsync to clear the slot, or — if the cert is compromised — needs a future force-rotate escape hatch. Stale-cache eviction on Type change. SamlLoginProviderReRegister was returning early without touching the cache when an Updated event arrived for a non-SAML record. That path is only reachable via direct event-stream surgery or a future force-recreate that recycles the Guid, but if it does happen the manager keeps a stale SAML entry under a now-OIDC id. Now it unregisters on the way out so SAML lookups don't hit the wrong-protocol entry. Type-aware Update. UpdateLoginProviderHandler now forces Scopes=[] + ClientId=string.Empty on SAML providers regardless of what the client submits. Previously a stale UI or scripted caller could store non-empty Scopes/ClientId on a SAML record — a data anomaly that admin grids / exports might mis-classify as OIDC. 168/168 integration tests + 66/66 SAML unit tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The login-providers + saml-federation pages both walked through the old
two-step flow ("click Erstellen → detail modal opens → switch tabs →
fill in everything else"). Phase 2a of the login-provider refactor
collapsed that into one modal where the admin fills every tab before
clicking Erstellen.
login-providers.md — Entra walkthrough rewrites the "In Modgud" section
to describe filling Verbindung + User-Update-Script tabs BEFORE Create,
calls out that the Redirect-URI field only appears post-Create (the
provider id is needed to derive it), and adds the optional "Initiales
Secret" field. SAML 2.0 added to the supported-types list.
saml-federation.md — same shape: pick flavor via header dropdown, fill
the Verbindung MetadataUrl tab BEFORE Create, then the SP-Metadata + ACS
URLs appear post-Create. Plus a "you can enable on Create" tip since the
backend now allows it when readiness conditions are met. Also fixes a
pre-existing dead link to ./membership-scripts (the page is groups.md,
section Auto-membership) that landed during the SAML wave and was
quietly blocking vitepress build.
Both VitePress builds (docs/ public + in-app variant) green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Captures the design discussion from the PR #17 review session: per-PR Docker image building gated on an explicit /build-image comment in the PR, with a belt-and-braces required-checks-green verification inside the workflow. Designed for both human reviewers and AI-agent flows (three-line gh CLI sequence: create PR, gh pr checks --watch, gh pr comment with the slash command). Coupled to the still-open GHCR retention policy item — both decisions land together so per-PR image tags don't accumulate unbounded. Path: dev-docs/future-features/pr-image-build-on-comment.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #17 surfaced four new cs/log-forging alerts via the CodeQL review-comments bot, all on structured-logging calls in the SAML slice (DynamicSamlSchemeManager + SamlSpCertificateService). Pattern is identical to the 10 already-documented FPs — admin-provided values (config.DisplayName, realmSlug, config.Flavor, idpMetadata.EntityId) flow into _logger.LogXxx(...) calls as structured-template properties, not string-concatenated into the message template. Serilog renders them as escaped JSON property values; CRLF in the input cannot forge log entries. Count updated 10 → 14. Dismissals of the new alerts in the GitHub Security UI are pending — once that's done, the open-alert count for this rule on the Code Scanning page should go to zero again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…se-label
Workflow_dispatch on feat/saml-federation hit an `Invalid NuGet version
string: '0.5.0-.27'` build error. Two-layered bug:
GitVersion.yml — the `feature:` regex matched `feature|bugfix|fix` but
not `feat`, the actual Cocoar branch-prefix convention (see conventional-
commit messages on this repo). Branches like `feat/saml-federation` fell
through to the `other:` rule, which in this GitVersion version doesn't
materialise the `label: alpha` into the `pre-release-label` output —
emitted an empty string instead. Added `feat` to the alternation so the
intended `label: '{BranchName}'` template fires.
cd-publish-staging-image.yml + cd-publish-nuget-prerelease.yml — both
unconditionally interpolated `${major-minor-patch}-${pre-release-label}`,
so an empty label produced a trailing `-` that combined with the commit-
count `.N` suffix to yield the malformed `0.5.0-.N`. Replaced with an
if-empty fallback chain: pre-release-label → escaped-branch-name → drop
the segment entirely. Belt-and-braces: even if GitVersion config gains
a new gap in the future, the workflow can't emit an unparseable string.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The PR-image-build plan already covered when to build (slash-command), which workflow listens (issue_comment), and how to clean up (GHCR retention). Surfaced today on PR #17's manual workflow_dispatch: the existing cd-publish-staging-image.yml tags ONLY `:staging`, regardless of trigger source, so a non-develop dispatch overwrites the develop-published image. Added a tag-strategy section that proposes the develop-vs-non-develop tag matrix (`:staging` stays sacred to develop; non-develop builds get `:<branch-slug>` + `:<version>`), spells out the test-server pull contract, and sketches the workflow change. The /build-image comment-trigger workflow will reuse this matrix once it lands — same conditional, single source of truth. Still deferred. Push went into the existing plan-page rather than spawning a new one because the two concerns share the same workflow and the same GHCR-retention coupling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Required-field client validation I added in block 1 of the code-review fix-sweep was too eager for SAML — it iterated flavor.ConfigSchema.filter(f => f.Required) regardless of Type, so SAML's MetadataUrl-required-but-not-at-Create field blocked the Add modal. That contradicts the documented + tested SAML flow: CreateSamlAsync intentionally accepts an empty FlavorData and lands the provider disabled; the admin pastes IdP metadata in the Verbindung tab and hits Speichern afterwards (then Enable). The EnableLoginProviderHandler readiness gate (LoginProviderLifecycleCommands.cs:38-46) catches metadata-missing at Enable time, which is where it belongs. Gate now only applies when flavor.Type === 'Oidc' — OIDC's DeriveEndpoints does throw on empty Required fields at Create, so the client-side guard is still valuable there. Spotted during the EntraID smoke setup on auth.cocoar.dev when the Add modal blocked with "Pflichtfelder fehlen im Verbindung-Tab: Federation Metadata URL" even though the admin's intent was to proceed with empty metadata and fill it after Entra setup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Smoking-gun bug surfaced during the EntraID smoke setup on auth.cocoar.dev: after the admin pasted a new IdP metadata URL into the Verbindung tab and saved, Modgud kept fetching the previous (placeholder) URL. Two coupled bugs in the FlavorData camelCase-vs-PascalCase reconciliation. Backend (SamlFlavorData.TryGetPropertyCaseInsensitive): when BOTH forms were present with real values, the helper picked camelCase. The camelCase value is whatever ToJson wrote previously, so on Update it represents the PRE-edit state that the frontend spread back into form.FlavorData via existing.FlavorData. The PascalCase value is the admin's freshly typed input. Preferring camelCase silently overwrote the edit with the stale value. Flipped the tie-break: PascalCase wins when non-null; falls back to camelCase when PascalCase is null/undefined (forwards-compat for clients that explicitly clear the field). Frontend (LoginProviderDetails.normalizeFlavorData): on modal reload the spread of existing.FlavorData carried camelCase keys, but FlavorConnectionPanel reads modelValue[field.Key] in PascalCase — every SAML connection field rendered empty even with a stored value, hiding the actual state from the admin. New normaliseFlavorData() promotes any schema-known camelCase key to PascalCase (and drops the camel variant) at load time. Schema-unknown keys stay untouched for forwards-compat. Two new SamlFlavorDataTests cover both tie-break directions (PascalCase-wins-on-both-non-null + camelCase-wins-when-PascalCase-null). 20/20 SamlFlavorData unit tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ails The normalizeFlavorData helper added in 57dd275 references FlavorConfigFieldDto but the type was never added to the import line, breaking the frontend type-check in CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vue-tsc --build (the type-check script's strict project config) flags pascal[0] as possibly undefined. charAt(0) returns a string for every index, so the camelCase derivation type-checks without a guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the aggregate Guid in the public provider URLs with an
admin-chosen, per-realm-unique, immutable slug so a delete + recreate
keeps the same URLs (no re-pasting into the upstream IdP):
OIDC callback /signin-oidc/{guid} → /signin-oidc/{slug}
SAML SP surface /saml/{guid}/{action} → /saml/{slug}/{action}
Backend
- LoginProvider.Slug + LoginProviderAddedEvent.Slug (immutable: no Update
event field); projection + Internal seeder ("internal").
- LoginProviderSlugRules (format only; no reserved list — the route shape
can't collide and per-realm uniqueness guards the seeded slug).
- CreateLoginProviderCommand: required Slug, format + per-realm-uniqueness
validation (SlugInvalid / SlugTaken) ahead of the type-specific paths.
- SAML: routes keyed by {slug}; DynamicSamlSchemeManager.TryGetBySlug
(realm, slug) lookup (realm from TenantContext); SamlContextBuilder
SP-EntityID + ACS URL by slug.
- OIDC: callback path by slug. Since slugs are only per-realm-unique and
the framework callback match is host-blind, add HostAwareOpenIdConnect-
Handler that disambiguates by the request's realm (RealmMiddleware) vs
the scheme's realm, tracked in an in-memory OidcSchemeRealmRegistry.
Scheme name stays Oidc_{guid} (global uniqueness); the slug never enters
a scheme name.
- LoginProviderDto.Slug; RedirectUri / SamlSpMetadataUrl / SamlAcsUrl by
slug.
Frontend
- Slug field in the unified Add+Edit modal (above DisplayName), required +
format-validated on create, read-only in edit, auto-suggested from the
Display Name. Model + de.json.
Docs: login-providers.md + saml-federation.md document the slug as a
create-time input and the recreate-stability it buys.
Tests: LoginProviderSlugRules unit tests; SAML TryGetBySlug + cross-realm
same-slug disambiguation; command-level SlugInvalid / SlugTaken. Existing
OIDC manager tests enter TenantContext + assert the host-aware handler
type. Unit 1035/1035, integration 170/170, frontend type-check green.
No migration — pre-release, no existing providers to backfill.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The create-mode slug hint rendered ".../signin-oidc/)" because the
vue-localization interpolator consumed the literal {slug} token. Use
<slug> as a non-interpolated placeholder. Surfaced during the local
DevTools + simplesamlphp smoke.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rivation - Lay Display Name (left) and Slug (right) out in one row (.lp-name-row); when the Slug field is hidden (Internal provider) Display Name fills it. - Derive the slug from the Display Name LIVE as the admin types (was: only on blur), normalised to the slug grammar (lowercase, non-alphanumerics → hyphens, collapse/trim). Spaces become hyphens, not underscores — the backend LoginProviderSlugRules forbids underscores. - Stop auto-deriving once the admin hand-edits the slug; clearing it re-arms the sync. Tracked via slugManuallyEdited + onSlugInput. Verified in-browser (DevTools): "Acme SSO (Prod)!" → "acme-sso-prod"; manual slug edit survives a subsequent Display Name change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…reaker
Direct tests for ShouldHandleRequestAsync — the per-tenant callback
disambiguation that lets two realms share a slug-based /signin-oidc/{slug}
path. Covers: realm match → handle; same path, different realm → decline
(the core multi-realm guarantee); path mismatch → decline; untracked
scheme → fall back to base. No host needed — handler built with a stub
options monitor + hand-built HttpContext.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ation) PR #17 review surfaced alert 37 on DynamicOidcSchemeManager.cs:234 — the OIDC mirror of the already-dismissed SAML registration log line. Audited on its own merits (structured props: identifier scheme name, admin-only DisplayName, validated flavor key, RealmSlugRules-validated realmSlug), dismissed as false positive in the UI. Count 14 → 15. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The /api/account/external-logins discovery endpoint returned OIDC providers
only, so enabled SAML providers never rendered a login-page button (and the
SP-initiated flow was unreachable from the UI). Include SAML providers and
add Kind + Slug to the DTO; the login + profile pages branch the entry-point
URL by Kind: OIDC → /api/account/external-login/{id}/start, SAML →
/saml/{slug}/login. Backend builds, frontend type-checks.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The SignalR live-update layer fanned every CRUD DataEvent through one process-global dispatcher to all connections, with the per-connection Subscribe() filtered by subject only — no tenant scoping. A user created in realm A appeared live in realm B's admin grid (verified bidirectionally). Masked in prod only because tenant realms couldn't connect to /signalr at all; the RealmMiddleware connection fix here unmasks it, so both ship together. Fix (single-node-correct, multi-node deliberately deferred): - DataEvent carries an originating Tenant tag; DataEventDispatcher gains tenant-stamping overloads. - Producers stamp session.TenantId (SignalRProjectionDispatchHandler for User+Inbox, ServiceAccountsEndpoints for ServiceAccount). - Each Subscribe() filters by the connection's authoritative realm (HttpContext.Items["TenantId"], resolved at connect by RealmMiddleware). Untagged events match no realm -> dropped (fail-closed, never leaked). - RealmMiddleware: /signalr no longer skips realm resolution, so per-tenant cookies decrypt and tenant realms can connect. Verified in the 2-realm container: cross-realm create does NOT appear in the other realm's grid; same-realm live update still works; both directions. Unit 1035/1035, integration 174/174. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… admin UI EntraID (and AD FS, and most IdPs) sign only the SAML Assertion, not the Response wrapper, by default. Modgud's SAML providers defaulted to also requiring the Response wrapper signed (WantResponseSigned=true) with no way to change it in the admin UI, so an Entra SP-initiated login failed with "saml-response-unsigned" right after authentication. - WantResponseSigned now defaults to false (record + FromJson). Assertion signing (WantAssertionsSigned=true) stays on — that's the real XML-wrapping defense, so this is not a security regression. - Expose the three SAML signing toggles (WantAssertionsSigned, WantResponseSigned, SignAuthnRequest) as Boolean fields in every SAML flavor's Connection tab via a shared SamlSigningConfigFields, so admins can match the toggles to what their IdP actually signs. - FlavorConfigField / FlavorConfigFieldDto gain a Default; the admin form seeds field defaults on create so checkboxes reflect the real default instead of rendering unchecked while the backend applies a different one. Verified in the 2-realm container: existing provider shows its stored value (Response checked); a fresh Entra SAML provider defaults Response unchecked, Assertions + AuthnRequest checked. Unit 1035/1035 (SAML 20), frontend type-check green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ve SP URLs SAML is one protocol, so the set of advanced knobs is identical across all flavors — flavors should differ only in default values, not in which settings exist. Make that structural so a flavor can't silently omit a knob (the class of bug that hit Entra with WantResponseSigned). - FlavorConfigField gains Section + a Select field type with Options (+ the Default added earlier); FlavorConfigFieldDto + frontend model mirror it. - SamlSigningConfigFields → SamlAdvancedConfigFields: one shared advanced set (signing toggles + WantAssertionsEncrypted + NameIdFormat select + EntityId + metadata refresh-interval select), Section="advanced", spread into all three SAML flavors. FromJson tolerates a string-encoded refresh interval (Select submits strings). - Frontend: section-aware FlavorConnectionPanel (+ CoarSelect rendering), new "Advanced" tab (SAML) and "Claim-Mapping" tab. Defaults seeded into the form on create so checkboxes/selects show the real default. - ClaimMapEditor: structured key→string[] editor for AttributeMap (claim → IdP attribute URIs) and AmrMapping (AuthnContextClassRef → AMR), so admins can fine-tune when an IdP changes a claim URI instead of being stuck with the seeded defaults. - SAML SP-Metadata + ACS URLs are now derived live from window.location.origin + the slug (create + edit), instead of the backend DTO's configured PublicUrl which could surface an internal/unreachable host (showed 0.0.0.0:8081 in the rig). Matches the runtime SamlContextBuilder (req.Host). Verified in the 2-realm rig: OIDC shows no Advanced/Claim tabs; SAML Entra create shows both tabs, correct advanced defaults (response unsigned, NameID email, refresh 24h), and the live SP URL with the right host before save; AttributeMap row round-trips through save + reopen. Unit 1035/1035, frontend type-check green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
UI polish on the SAML admin surface: - The provider enable/disable control is now a CoarSwitch (was a button), bound to provider.Enabled as the source of truth so a failed enable (e.g. the SAML readiness gate) snaps it back and surfaces the error. - ClaimMapEditor rebuilt on CoarGridBuilder/CoarDataGrid — same inline-edit pattern as EditableStringList (two editable columns + delete icon + add toolbar) instead of a hand-rolled layout, for consistency with the other admin grids. Verified in the 2-realm rig: switch toggles enabled/disabled (grid reflects Nein/Ja) and reverts on failure; claim-mapping tab renders both maps as grids with headers + add toolbar. Frontend type-check green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…le/disable
Replace the full-replace PUT + dedicated enable/disable endpoints with one
PATCH-style update where every field is Optional<T> (the same pattern the
User/Profile updates already use). Absent fields keep their current value, so:
- the edit modal submits the full form (Enabled now a staged switch that
commits on Save, not an immediate side-effect), and
- the grid's inline toggle submits just { Enabled } — no full provider needed.
Enabled folds the former Enable/Disable commands into the update handler: a
false→true transition runs the readiness gate (extracted to
LoginProviderReadiness, checked against the POST-merge values so "set metadata
+ enable" works in one save) and emits the distinct LoginProviderEnabledEvent;
true→false emits Disabled. A bare Enabled PATCH yields only that audit event,
no spurious "updated". The /enable + /disable endpoints and their commands are
removed.
Verified in the 2-realm rig: partial PATCH changes only Enabled (DisplayName +
FlavorData preserved); enable without metadata is rejected (gate); metadata +
enable in one PATCH succeeds; the modal switch staged-flip does NOT persist
without Save; grid toggle PATCHes immediately. Unit 1035/1035, integration
174/174, frontend type-check green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bring the SAML "Advanced tab" treatment to OIDC. OIDC has no rich FlavorData
model like SAML, so rather than an empty tab, expose the genuinely useful,
handler-consumable OpenIdConnectOptions knobs as real new settings.
- OidcAdvancedConfigFields (shared across the OIDC flavors, Section=advanced):
UsePkce, GetClaimsFromUserInfoEndpoint, SaveTokens (toggles) + Prompt (select:
login / select_account / consent / none). Defaults match what
DynamicOidcSchemeManager previously hard-coded, so existing providers are
unchanged.
- DynamicOidcSchemeManager reads these from config.FlavorData and applies them
to OpenIdConnectOptions (Prompt only when set).
- Live OIDC redirect URI ({origin}/signin-oidc/{slug}) shown from the slug at
create + edit, same slug-derived fix as the SAML SP URLs (replaces the DTO's
configured-PublicUrl value that could be an unreachable internal host).
The Advanced tab + Boolean/Select rendering already exist (built for SAML), so
the OIDC tab appears automatically — no new frontend code; OIDC correctly shows
no Claim-Mapping tab (claims map via the user-update script, not a structured
map).
Verified in the rig: OIDC create shows the Advanced tab with correct defaults
(PKCE on, UserInfo on, SaveTokens off, Prompt none); live redirect URI from
slug; advanced values round-trip through create + partial FlavorData PATCH.
Unit 1035/1035, integration 174/174, frontend type-check green. (A live OIDC
auth applying the options wasn't exercised — no OIDC test IdP in the rig; the
wiring is a direct options assignment.)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The OIDC event handlers called DynamicOidcSchemeManager.RegisterAsync without an ambient TenantContext, so runtime add/enable/update threw and the scheme was never registered — only the boot-time bootstrap worked. Mirror the SAML handler: pull the tenant from the message-scoped session and enter it before registering. Verified live: add/enable/disable/update now register and unregister the scheme without a restart. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jun 2, 2026
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>
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
2 participants
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.
Summary
Four waves on one branch (42 commits, 90 files, +7.8K/-1.7K vs
develop).Wave 1 — SAML 2.0 SP federation (ITfoxtec 4.18)
End-to-end SP-initiated SAML against a customer IdP. Per-realm SP signing certs with 30-day rotation overlap, dynamic per-provider scheme manager, periodic IdP-metadata refresh, AttributeMap + AMR mapping. Three flavors out of the box: Generic SAML 2.0, EntraID SAML, ADFS SAML. Plan-page in
dev-docs/future-features/saml-federation.md, admin walkthrough atdocs/admin/saml-federation.md. Local dev IdP via simplesamlphp underdev/saml-idp/.Wave 2 — Login-provider single-modal Add+Edit (Phase 2a of ui-refactor)
Eliminates the old two-step Add flow (minimal Create → reopen as Edit modal).
CreateLoginProviderCommandaccepts the full provider state; the Vue modal hosts a flavor picker in the header-actions slot, morphs the body on flavor switch (OIDC ↔ SAML), and transitions Add → Edit in-place after Save. Quick-Map UI (Phase 2b) intentionally deferred — seedev-docs/future-features/login-providers-ui-refactor.mdfor the documented backend cost.Wave 3 — Code-review fix-sweep
Multi-angle code-review pass found 10 candidate findings; 9 fixed in code, 1 documented:
WantAssertionsSigned/WantResponseSignedpost-parse XML check)Saml2Configuration.DecryptionCertificatesplural for rotation-overlap encrypted assertionsSaml2RequestContextdisposable wrapper for X509Certificate2 leaks (signing/decryption/validation)EnableLoginProviderHandlerSamlLoginProviderReRegisteron Type changeWave 4 — Signing controls, shared advanced settings, OIDC runtime fix
Follow-up wave after the first test-server deploy surfaced the EntraID SAML signing gotcha; it snowballed into a full signing + advanced-settings + OIDC pass.
WantResponseSigneddefault flipped to false (Entra/ADFS sign only the assertion, not the response envelope) + the three signing toggles exposed in the Connection tab. (32ca276)SamlAdvancedConfigFields/OidcAdvancedConfigFields) surfaced in an "Advanced" tab; SAML "Claim-Mapping" tab with a grid editor; enabled-switch staged in the modal. (e7a4378,7008c88)Optional<T>per field; the separate/enable+/disableendpoints removed and folded intoPATCH {Enabled}(distinct enable/disable audit events kept). (f546053)UsePkce/Prompt/SaveTokens/GetClaimsFromUserInfoEndpoint) wired into the dynamic scheme fromFlavorData+ live/signin-oidc/{slug}redirect URI in the admin UI. (144fff3)DynamicOidcSchemeManager.RegisterAsyncwithout an ambientTenantContext, so runtime add/enable/update threw and the scheme was never registered — only the boot-time bootstrap worked, producing a 500 on the login-button click. Now mirrors the SAML handler (tenant pulled from the message-scoped session). (038f3fd)Also riding along on this branch:
149ce90fix(signalr): scope realtime push to the originating realm— a multi-tenancy leak fix in the SignalR push path (realm partition fromsession.TenantId+ connection realm-filter). Unrelated to federation but merges with this PR.Known limitations
SAML link-flow under SameSite=Lax —
HandleAcsAsyncreads the Modgud.Auth cookie which is suppressed on cross-site POST from the IdP. Link-flow silently degrades to JIT / email-auto-link. MarkedKNOWN LIMITATIONinline + plan-page atdev-docs/future-features/saml-link-flow-samesite.md. Fix is blocked on test-server + real-IdP verification (the simplesamlphp dev rig runs on localhost, so it's not cross-site and can't reproduce). Mitigated byTrustForEmailLink=falsebeing the security default and labelled DANGEROUS in the admin UI.Soft-delete retains PII — deleting a user currently only sets
IsDeleted=true; the email + password-hash stay in the doc and noUserDeletionStateis written. Surfaced while testing OIDC JIT. Out of scope for this PR — tracked for a separate PR to clarify the intended soft-delete/masking semantics.Test plan
dotnet test Modgud.Tests.Unit— 1035/1035 green (incl. SAML + flavor suites)dotnet test Modgud.Api.Tests— 174/174 greenpnpm type-check, build mode)pnpm exec vitepress build(docs/ + in-app variant) both greenWantResponseSigned=true, so untick "Require signed response" once for Entralogin.microsoftonline.com(PKCE/S256) → callback → JIT user created/signin-oidc/{slug}→ 200 SPA fallback when disabled vs 302 OIDC-handler when enabled)038f3fd— pending: SAML smoke + OIDC smoke under HTTPS/Production (form_post + SameSite=None cookie path is not exercised by the Development rig)🤖 Generated with Claude Code