[PM-44143][BEEEP] Add ability to gate Admin Portal access with OIDC

[trmartin4]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-44143

📔 Objective

Adds the ability to configure an SSO provider to gate access to the Admin Portal, based on environment variables.

Allows an administrator to configure an Admin Portal OIDC IdP, by setting the following environment variables.

This has come up in the following contexts:

1. Internal admins want an easier way to sign in, and
2. We need to secure this behind a FedRAMP IdP in order to be FedRAMP-compliant.

Logout

When a user signs in via SSO and later clicks Logout, the Admin Portal triggers OIDC RP-initiated logout — the browser is redirected to the IdP's end_session_endpoint (with the ID token as id_token_hint), the IdP clears its own session, and the user is returned to the Admin login page. Magic-link users log out locally as before.

For this to work, the value of AdminSettings__Oidc__SignedOutCallbackPath must be registered on the IdP:

• Auth0: Application → Allowed Logout URLs
• Keycloak: Client → Valid Post Logout Redirect URIs
• Okta: Application → Sign-out redirect URIs

Configuration

Set as environment variables (or user-secrets for local dev):

Additional authentication settings exposed

Setting	Required	Default	Notes
AdminSettings__EnablePasswordlessLogin	no	true	Enable login via magic link (the existing auth scheme)
AdminSettings__SessionTimeoutMinutes	no	2880 (2 days)	Idle timeout on the admin session cookie (FedRAMP AC-11). Sliding — extends on activity.
AdminSettings__AbsoluteSessionTimeoutMinutes	no	0 (disabled)	Absolute session cap measured from initial sign-in (FedRAMP AC-12). Enforced against an anchor stored in AuthenticationProperties.Items — sessions terminate at this age regardless of activity. Must be > SessionTimeoutMinutes when both are set.

OIDC-specific configuration

Setting	Required	Default	Notes
AdminSettings__Oidc__Authority	yes	—	IdP issuer URL
AdminSettings__Oidc__ClientId	yes	—
AdminSettings__Oidc__ClientSecret	yes	—
AdminSettings__Oidc__Scopes	no	openid profile email
AdminSettings__Oidc__EmailClaimType	no	email	Claim used to match the user against AdminSettings__Admins
AdminSettings__Oidc__DisplayName	no	SSO	Sign-in button label
AdminSettings__Oidc__CallbackPath	no	/login/sso-callback	Must be registered as an Allowed Callback URL on the IdP
AdminSettings__Oidc__SignedOutCallbackPath	no	/login/sso-signout
AdminSettings__Oidc__RequireEmailVerifiedClaim	no	true	Reject sign-in unless the IdP returns email_verified=true. Set false only for operator-controlled IdPs that don't emit the claim.

OIDC activates only when Authority, ClientId, and ClientSecret are all set. If any is missing, the login page reverts to email-magic-link only.

Additional IdP requirements

• The IdP must include email and email_verified claims in the ID token (not only in the UserInfo response). Auth0, Azure AD/Entra, and Keycloak do this by default when the email scope is requested. Okta requires enabling "Include user info in ID token" (or equivalent) on the app's OpenID Connect settings.
• email_verified must be a JSON boolean true; string values other than "true" are rejected (fail-secure). Set RequireEmailVerifiedClaim=false only for operator-controlled IdPs that don't emit the claim at all.

📸 Screenshots

Full flow, including showing upstream logout (Auth0 requesting IdP login again)

login_logout.mov

UI without passwordless also enabled

Session expiration logout

🔐 OIDC compliance report

Mapping of the configuration to relevant specs and best-current-practice documents. All items ✅ unless noted.

OAuth 2.0 Security BCP (RFC 9700) / OAuth 2.1

Requirement	Status	Where
Authorization Code flow (no implicit, no ROPC)	✅	options.ResponseType = Code
PKCE required	✅	options.UsePkce = true
Exact redirect URI matching (registered at IdP)	✅	CallbackPath + SignedOutCallbackPath documented as must-match
HTTPS on all authorization endpoints	✅	options.RequireHttpsMetadata = true (explicit)
CSRF protection via state	✅	OIDC middleware's correlation cookie
Client authentication on token exchange	✅	client_secret_post via ClientId + ClientSecret
Least privilege on stored tokens	✅	Only id_token stored in the app cookie (access/refresh dropped)

OpenID Connect Core 1.0

Requirement	Status	Where
openid scope requested	✅	Documented as required on the Scopes setting; failure to include causes IdP to reject the request
nonce in authorize + ID-token binding	✅	Handled by the OIDC middleware
ID token signature validation	✅	ValidateIssuerSigningKey = true, RequireSignedTokens = true
ID token iss validation	✅	ValidateIssuer = true (bound to Authority via discovery)
ID token aud validation	✅	ValidateAudience = true (bound to ClientId)
ID token exp validation	✅	ValidateLifetime = true, RequireExpirationTime = true, ClockSkew = 2 min
Claims usage: email matched against allowlist	✅	AdminSettings__Admins
Claims usage: email_verified required	✅	RequireEmailVerifiedClaim = true by default; fail-secure when claim absent or non-boolean
Claims kept in OIDC-native form	✅	options.MapInboundClaims = false

OpenID Connect RP-Initiated Logout 1.0

Requirement	Status	Where
post_logout_redirect_uri registered at IdP	✅	SignedOutCallbackPath documented per-IdP
id_token_hint sent on end-session	✅	OnRedirectToIdentityProviderForSignOut event pulls id_token from the app cookie and attaches it
Local session terminated in addition to IdP session	✅	_signInManager.SignOutAsync() before the RP-initiated logout redirect

Session freshness / re-authentication

Requirement	Status	Where
Request fresh IdP authentication on Admin sign-in	⚠️ IdP-dependent	prompt=login on the authorize request. Per OIDC Core 3.1.2.1 this is a request the IdP SHOULD honor — not something we verify server-side (nothing in the response proves it happened). Auth0, Okta, Entra, and Keycloak all honor it in practice.
Idle timeout on admin session (FedRAMP AC-11)	⚠️ configurable	SessionTimeoutMinutes. Sliding — extends on activity; short values (15-60 min) satisfy AC-11.
Absolute session lifetime cap (FedRAMP AC-12)	⚠️ configurable	AbsoluteSessionTimeoutMinutes. Anchor stored in AuthenticationProperties.Items (survives cookie renewal); enforced via OnValidatePrincipal regardless of activity or IssuedUtc rewrites by SecurityStampValidator.

Cookie / session security

Requirement	Status	Where
HttpOnly on app cookie	✅	options.Cookie.HttpOnly = true
Secure on app cookie (explicit, not proxy-dependent)	✅	options.Cookie.SecurePolicy = Always
SameSite explicit	✅	options.Cookie.SameSite = Lax (required for OIDC redirect-back)
Cookie payload encrypted via Data Protection	✅	ASP.NET Core default; Data Protection service registered elsewhere
Session revocation propagation	✅	SecurityStampValidatorOptions.ValidationInterval = 5 min

Fail-closed / secure-by-default posture

Property	Behavior
Incomplete OIDC config (missing Authority/ClientId/ClientSecret)	Handler is not registered; all SSO endpoints return 404
Missing email claim	Reject (error=5), external cookie cleared
email_verified absent when required	Reject (error=5), external cookie cleared
email_verified present but not JSON-boolean true	Reject (error=5), external cookie cleared
Email not on AdminSettings__Admins allowlist	Reject (error=5, same as other rejections to prevent admin enumeration), external cookie cleared
Passwordless email login disabled via config	POST and Confirm endpoints return 404

Threat model coverage

Threat	Mitigation
Open redirect on post-login returnUrl	Url.IsLocalUrl check before redirect
CSRF on Logout	[HttpPost] + [ValidateAntiForgeryToken]
Authorization-code interception	PKCE
ID token replay	nonce (per-request); ClockSkew = 2 min
On-path tampering with IdP discovery / JWKS	RequireHttpsMetadata = true
Cookie downgrade behind mis-configured TLS-terminating proxy	SecurePolicy = Always (explicit, not SameAsRequest)
Stolen IdP session → free admin access	prompt=login requests credential + MFA re-entry on every re-sign-in (IdP-honored; not server-side verified)
Cookie exfiltration → API access via access_token	Only id_token stored
Admin allowlist enumeration by any authenticated IdP user	Reject codes collapsed to a single value
Federation with an IdP that permits unverified email	email_verified enforced, fail-secure on absence
Sign-in bypass when SSO is misconfigured	Fail-closed: SSO endpoints 404 when config is incomplete

[trmartin4]

@claude please review this.

[claude]

Claude finished @trmartin4's task in 5m 26s —— View job

---

Code review

• Gather context (read PR diff + surrounding code)
• Review auth/security correctness
• Review code quality, conventions, tests
• Post review

Nice, self-contained feature — the design decision to keep AdminSettings__Admins as the authorization gate and use OIDC purely for authentication is the right call, and the fail-closed OidcEnabled guard (plus NotFound() on both SSO endpoints when disabled) is solid. ResponseType=Code + UsePkce + Url.IsLocalUrl on the return URL are all correct. Findings below, most impactful first.

---

🔴 Logout will not send id_token_hint

src/Admin/Auth/Controllers/LoginController.cs:154-167

The PR description says the browser is redirected to the IdP's end_session_endpoint "with the ID token as id_token_hint". As written, the ID token isn't available at that point:

• SaveTokens = true stores the tokens on the ExternalScheme ticket (that's the SignInScheme).
• SsoSignIn signs the user in with SignInWithClaimsAsync (LoginController.cs:141) — a fresh application-cookie ticket with default AuthenticationProperties, carrying no tokens — then deletes the external cookie at LoginController.cs:142.
• By the time Logout() runs, _signInManager.SignOutAsync() (:159) has also dropped the application cookie, and the AuthenticationProperties handed to SignOut(...) (:165) contain only RedirectUri.

So the end-session request goes out with post_logout_redirect_uri only. Auth0 and Keycloak generally tolerate that; Okta rejects /v1/logout without id_token_hint, and Okta is explicitly listed as a supported IdP in the description. To make this work, capture the ID token during SsoSignIn (info.AuthenticationTokens) and persist it onto the application-cookie properties via the SignInWithClaimsAsync overload that accepts AuthenticationProperties, then read it back (before SignOutAsync()) and attach it to the sign-out properties. If you'd rather not carry the ID token in the cookie, drop SaveTokens and update the PR description — but expect Okta to fail. Fix this →

🔴 CI will fail on encoding — .cs files must have a UTF-8 BOM

.editorconfig sets charset = utf-8-bom for [*.{cs,csx,vb,vbx}], and .github/workflows/build.yml:37 runs dotnet format --verify-no-changes.

This PR strips the BOM from two existing files and adds three new ones without it:

File	State
src/Admin/AdminSettings.cs:1	BOM removed
src/Admin/Auth/Controllers/LoginController.cs:1	BOM removed
src/Admin/IdentityServer/AdminAuthenticationSchemes.cs	no BOM
test/Admin.Test/AdminSettingsTests.cs	no BOM
test/Admin.Test/Auth/Controllers/LoginControllerTests.cs	no BOM

(src/Admin/IdentityServer/ServiceCollectionExtensions.cs still has its BOM, so this looks like an editor artifact rather than intent.) Running dotnet format locally should fix all five. Fix this →

🟠 The email claim is trusted without email_verified

src/Admin/Auth/Controllers/LoginController.cs:122-135

Matching on the email claim alone means anyone who can get an account at the configured IdP with an email string matching an entry in AdminSettings__Admins gets full Admin Portal access. With a single-tenant, operator-controlled IdP that's fine; with any IdP that permits self-registration or unverified email addresses (or a multi-tenant Auth0/Okta org), it's an account-takeover path into the highest-privilege surface Bitwarden operates.

Given this is being positioned as the FedRAMP access gate, I'd require email_verified == true when the claim is present (and make rejecting-on-absent configurable). Worth stating the trust assumption explicitly in the config table either way. Fix this →

🟠 No logging on the SSO auth path

src/Admin/Auth/Controllers/LoginController.cs:104-150

Every failure mode — remote error, null ExternalLoginInfo, missing email claim, email not on the allowlist — collapses into the same error=5/error=4 redirect with nothing recorded. For the Admin Portal, failed and successful privileged sign-ins are exactly the events that should be auditable (and FedRAMP AU-2/AU-3 will want them). Inject ILogger<LoginController> and log each branch distinctly. Per CLAUDE.md, keep the email out of the message — logging the reason ("SSO sign-in rejected: email claim not present in admin allowlist") is enough to diagnose without PII. This also directly affects the next item: today an operator hitting it sees only "SSO sign-in failed" with no way to tell why. Fix this →

🟠 Verify GetExternalLoginInfoAsync() works with MapInboundClaims = false

src/Admin/IdentityServer/ServiceCollectionExtensions.cs (options.MapInboundClaims = false) + LoginController.cs:116

SignInManager.GetExternalLoginInfoAsync() derives ProviderKey from ClaimTypes.NameIdentifier and returns null if it can't find one. With MapInboundClaims = false the ID token's sub stays typed as "sub" and is never mapped to the WS-Fed nameidentifier URI, and the OIDC handler's default claim actions map sub → sub as well. If that's the case here, info is always null and every SSO sign-in fails with error=5.

The demo video suggests it works against your IdP, so this may already be settled — but please confirm which IdP/claim shape was tested, since it'd be IdP-dependent. A robust alternative is to skip GetExternalLoginInfoAsync entirely and read the principal directly:

var result = await HttpContext.AuthenticateAsync(IdentityConstants.ExternalScheme);
if (!result.Succeeded) { /* error */ }
var email = result.Principal.FindFirst(_adminSettings.Oidc.EmailClaimType)?.Value;

Either way, adding options.ClaimActions.MapUniqueJsonKey(ClaimTypes.NameIdentifier, "sub") makes the intent explicit.

🟡 Scope parsing can silently produce an invalid request

src/Admin/IdentityServer/ServiceCollectionExtensions.cs

options.Scope.Clear() followed by splitting on ' ' means an operator who omits openid (or who sets AdminSettings__Oidc__Scopes=openid,profile,email, which is a very natural thing to type into an env var) gets a broken flow with no diagnostic. Suggest splitting on ' ' and ',', and unconditionally ensuring openid is in the set.

🟡 Tests don't cover the security-relevant branches

test/Admin.Test/Auth/Controllers/LoginControllerTests.cs

The current tests cover the disabled-feature guards, the remote-error redirect, and view-model wiring. The branches that actually decide who gets admin access are untested:

• email claim present but not in AdminSettings__Admins → error=4, no sign-in
• email claim missing entirely → error=5, external cookie cleared
• successful sign-in → SignInWithClaimsAsync called with the admin_auth_method=sso marker
• returnUrl is non-local → redirects to Home rather than the external URL

Also worth aligning with the convention already used in this project: test/Admin.Test/Controllers/UsersControllerTests.cs uses [ControllerCustomize]/[SutProviderCustomize]/[BitAutoData] with SutProvider<T>, which would remove the ~30 lines of hand-rolled UserManager/SignInManager construction in BuildController. Fix this →

🟡 Smaller items

• launchSettings.json:19 — https://localhost:5001 is the generic ASP.NET Core default and is the one port in this repo likely to already be in use by something else on a dev's machine; every other project here uses a unique port. A project-specific HTTPS port (e.g. 62913) would be less collision-prone.
• Startup.cs:41-43 — AdminSettings is bound twice. var adminSettings = Configuration.GetSection("AdminSettings").Get<AdminSettings>() ?? new AdminSettings(); collapses it to one line.
• src/Admin/IdentityServer/AdminAuthenticationSchemes.cs — auth-owned code in this app lives under src/Admin/Auth/ (Auth/IdentityServer/PasswordlessSignInManager.cs). This file is auth-domain and used only from Auth/Controllers/LoginController.cs; src/Admin/Auth/IdentityServer/ seems like the better home for CODEOWNERS purposes.
• LoginController.cs:129 — email.ToLowerInvariant() is redundant: UserManager.FindByEmailAsync applies LowerInvariantLookupNormalizer (registered in ServiceCollectionExtensions.cs:16) before it reaches ReadOnlyEnvIdentityUserStore.
• LoginController.cs:141 — the SSO path calls SignInWithClaimsAsync directly, bypassing PreSignInCheck/CanSignInAsync that the passwordless path goes through. Impact is nil against ReadOnlyEnvIdentityUserStore (no lockout, always confirmed), but it's a divergence worth a comment if deliberate.
• AdminSettings.cs:10 — public virtual OidcSettings Oidc { get; set; } = new OidcSettings() is virtual for no apparent reason, and the non-null default sits slightly oddly next to the Oidc != null / Oidc?. null checks everywhere else. Not wrong, just mixed signals about whether it can be null.
• Docs — for self-hosted, app.UsePathBase("/admin") (Startup.cs:148) means the callback and post-logout URLs registered at the IdP need the /admin prefix. Worth adding to the config table so self-hosters don't lose an hour to it.
• The github-code-quality ternary suggestion on BuildController is cosmetic; I wouldn't act on it (and it goes away if you move to SutProvider).

---

Not changed, by design, and I agree with it: AllowAnonymous on both SSO actions (the controller has no [Authorize], so it's redundant but explicitly self-documenting), no ValidateAntiForgeryToken on the GET challenge (the handler's state/nonce/correlation cookie covers it), and keeping the magic-link flow intact as a fallback.
• branch admin/upstream-oidc

[codecov]

Codecov Report

❌ Patch coverage is 51.91083% with 151 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.83%. Comparing base (aa786fc) to head (07b8370).

Files with missing lines	Patch %	Lines
...dmin/IdentityServer/ServiceCollectionExtensions.cs	32.78%	117 Missing and 6 partials ⚠️
src/Admin/Auth/Views/Login/Index.cshtml	0.00%	14 Missing ⚠️
src/Admin/Auth/Controllers/LoginController.cs	90.21%	5 Missing and 4 partials ⚠️
src/Admin/Startup.cs	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8458      +/-   ##
==========================================
+ Coverage   64.81%   64.83%   +0.01%     
==========================================
  Files        2490     2490              
  Lines      107143   107448     +305     
  Branches     9699     9738      +39     
==========================================
+ Hits        69442    69660     +218     
- Misses      35361    35436      +75     
- Partials     2340     2352      +12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[trmartin4]

@claude re-review please.

[claude]

Claude finished @trmartin4's task in 5m 33s —— View job

---

Re-review

• Read current diff
• Verify prior findings addressed
• Look for new issues
• Post re-review

Big improvement — the rewrite of SsoSignIn to read the external principal directly is the right call and resolves the MapInboundClaims/GetExternalLoginInfoAsync uncertainty outright. email_verified enforcement, distinct-branch logging without PII, collapsed reject codes, BOMs restored, the Auth/IdentityServer/ move, the Startup single-bind, and the test suite are all in. The server-side auth_time/max_age enforcement is a nice addition I hadn't asked for.

Three things I'd want resolved before merge, then some smaller ones.

---

🔴 The admin_auth_method=sso marker is dropped ~5 minutes after sign-in, disabling RP-initiated logout

src/Admin/Auth/Controllers/LoginController.cs:172-185 and :201-202, with src/Admin/Startup.cs:80-83

Logout() decides whether to do RP-initiated logout by checking for the admin_auth_method claim on the application cookie. That claim doesn't survive:

1. SecurityStampValidatorOptions.ValidationInterval is 5 minutes (Startup.cs:82).
2. On the first request after that window, SecurityStampValidator.ValidateAsync calls VerifySecurityStamp. ReadOnlyEnvIdentityUserStore implements IUserSecurityStampStore and returns a stable stamp (ReadOnlyEnvIdentityUserStore.cs:59 — the email, or the configured email:stamp value), so verification succeeds.
3. SecurityStampVerified then rebuilds the principal from scratch via CustomClaimsPrincipalFactory and calls context.ReplacePrincipal(newPrincipal). The factory (src/Admin/IdentityServer/CustomClaimsPrincipalFactory.cs:23-41) only adds the role claim — nothing carries admin_auth_method across. The framework preserves only amr / ClaimTypes.AuthenticationMethod, and only in SignInManager.RefreshSignInAsync, which isn't on this path.

So: sign in via SSO, work for 5+ minutes (the realistic case — the cookie lives 2 days with sliding expiration), click Logout → signedInViaSso is false, you get a plain local redirect, and the IdP session is never terminated. The demo video doesn't catch this because the logout happens inside the 5-minute window.

Note the asymmetry that makes this easy to miss: ReplacePrincipal leaves context.Properties alone, so the id_token you stashed at :183 does survive — only the claim is lost.

Cleanest fix is to stop using a claim for this and put the marker where the token already lives:

// SsoSignIn
props.Items["admin_auth_method"] = AdminAuthenticationSchemes.AuthMethodSso;

// Logout
var auth = await HttpContext.AuthenticateAsync();
var signedInViaSso = auth.Properties?.Items.TryGetValue("admin_auth_method", out var m) == true
    && m == AdminAuthenticationSchemes.AuthMethodSso;

Alternatively set SecurityStampValidatorOptions.OnRefreshingPrincipal to copy the claim from CurrentPrincipal to NewPrincipal. Either way this needs a regression test — the existing Logout_TriggersRpInitiatedLogout_WhenSsoMarkerClaimPresent passes today and would keep passing through this bug. Fix this →

🟠 auth_time check has no clock-skew tolerance, so a fast IdP clock locks out all SSO

src/Admin/Auth/Controllers/LoginController.cs:240-241

var age = DateTimeOffset.UtcNow.ToUnixTimeSeconds() - authTime;
return age >= 0 && age <= AdminAuthenticationSchemes.MaxIdpAuthAgeSeconds;

With prompt=login forcing a fresh authentication, auth_time is effectively "now at the IdP". If the IdP's clock leads the Admin Portal's by even a second or two — ordinary NTP drift, and exactly what the 2-minute ClockSkew at ServiceCollectionExtensions.cs:139 exists to absorb — age goes negative and every SSO sign-in is rejected with the generic error=5. Fail-closed is right for staleness; it's the wrong answer for skew, and the failure is both total and hard to attribute from the operator's side.

Suggest allowing the same tolerance you already grant token validation:

var skew = TimeSpan.FromMinutes(2).TotalSeconds;
return age >= -skew && age <= AdminAuthenticationSchemes.MaxIdpAuthAgeSeconds + skew;

Related, and worth calling out in the config table rather than changing: because the claim is required, any IdP that ignores max_age and omits auth_time will fail 100% of sign-ins. Spec-wise you're right (OIDC Core 3.1.3.7 makes auth_time mandatory when max_age is sent), and the new log line at :157 makes it diagnosable — just make sure a self-hoster reading the table knows this is a hard requirement on their IdP. Fix this →

🟠 Logout() can 500 when OIDC config is removed

src/Admin/Auth/Controllers/LoginController.cs:199-216

Every other SSO entry point is guarded by OidcEnabled, but Logout() isn't. If an operator unsets AdminSettings__Oidc__* and restarts while an SSO-signed-in admin still holds a valid cookie, SignOut(..., AdminAuthenticationSchemes.UpstreamOidc) targets a scheme that was never registered (AddAdminUpstreamOidc returns early at ServiceCollectionExtensions.cs:62) and the sign-out result throws InvalidOperationException → 500. The local session is already gone by then, so it's cosmetic in effect, but it's an unhandled 500 on the highest-privilege surface. signedInViaSso && _adminSettings.OidcEnabled closes it. Fix this →

---

🟡 id_token_hint retrieval depends on handler-level caching

src/Admin/IdentityServer/ServiceCollectionExtensions.cs:159-166 + LoginController.cs:204

OnRedirectToIdentityProviderForSignOut calls HttpContext.GetTokenAsync("id_token"), which authenticates the default (application) scheme — but _signInManager.SignOutAsync() has already run by then. This works only because CookieAuthenticationHandler caches its read ticket (_readCookieTask) and AuthenticationHandler caches _authenticateTask, so the post-sign-out AuthenticateAsync returns the pre-sign-out properties. That's true today, but it's an implementation detail rather than contract, and a silent null here means no id_token_hint — the exact Okta failure the previous round was about.

Reading the token in Logout() before SignOutAsync() and passing it on the sign-out AuthenticationProperties (then ctx.Properties.GetTokenValue("id_token") in the event) removes the dependency. This falls out naturally if you take the properties-based approach to the 🔴 above, since you'd already be calling AuthenticateAsync up front.

🟡 Scope parsing (carried over, unchanged)

src/Admin/IdentityServer/ServiceCollectionExtensions.cs:118-122

Still splits on ' ' only, and openid isn't force-added. AdminSettings__Oidc__Scopes=openid,profile,email is a very natural thing to type into an env var and produces a single bogus scope with no diagnostic. Splitting on ' ' and ',' and unconditionally options.Scope.Add("openid") is a two-line hardening of a config surface operators will touch. Fine to decline — but if so, the comment at :57 in AdminSettings.cs should say "space-delimited, commas not supported".

🟡 Magic-link submit button is disabled in markup and only enabled by JS

src/Admin/Auth/Views/Login/Index.cshtml:32-34 + the new @section Scripts

<button ... disabled> plus an inline script that enables it on input means passwordless login is completely unusable if that script doesn't run. The required attribute on the input already blocks empty submission via native validation, so the script is buying nothing that HTML doesn't already do. If you want to keep the visual affordance, set disabled from the script instead of in the markup so the no-JS path degrades to a working form. (Admin has no CSP today, so the inline script itself is fine — worth knowing it's a blocker if one is ever added.)

🟡 Smaller items

• launchSettings.json:19 — https://localhost:5001 is still there. It's the generic ASP.NET Core default, and the only port in this repo likely to collide with something else already running on a dev machine; every other project (46273, 62911, 50024, 33656, 44518, 61840, 54103) uses a unique one. https://localhost:62913 would fit the convention.
• Test gaps — the new suite covers the reject branches well. Three things it doesn't assert, all cheap: the id_token actually landing in the sign-in AuthenticationProperties (the whole point of the logout change), a local returnUrl being honored, and Sso() returning a ChallengeResult for the UpstreamOidc scheme when enabled.
• LoginController.cs:185 — the SSO path still bypasses PreSignInCheck/CanSignInAsync that the passwordless path goes through. Against ReadOnlyEnvIdentityUserStore (no lockout, always confirmed) the impact is nil, and I read this as deliberate — a one-line comment saying so would stop the next reader from re-deriving it.
• Docs — Startup.cs:148 applies UsePathBase("/admin") for self-hosted, so the registered callback and post-logout URLs need the /admin prefix there. The code comments at ServiceCollectionExtensions.cs:92-93 and AdminSettings.cs:47-54 call it out, but the PR-description config table (which is what a self-hoster will actually read) doesn't.

---

Previously raised, now resolved

id_token_hint plumbing • UTF-8 BOMs on all five files • email_verified enforcement with a configurable absent-claim policy • per-branch logging without PII • GetExternalLoginInfoAsync/MapInboundClaims replaced with a direct AuthenticateAsync read • security-branch test coverage • double binding in Startup • AdminAuthenticationSchemes moved under Auth/ • redundant ToLowerInvariant • stray virtual on AdminSettings.Oidc • reject codes collapsed to prevent allowlist enumeration.

I couldn't run dotnet build/dotnet test locally in this environment (command not permitted), so the above is static analysis only. The Build and Testing workflows were still in progress when I finished — worth confirming they're green. The Build (Fork PR) startup_failure is a workflow-trigger artifact, not a code failure.
• branch admin/upstream-oidc

[trmartin4]

@claude please review again.

[claude]

Claude finished @trmartin4's task in 5m 25s —— View job

---

Re-review

• Read current diff (through bd9f9be)
• Verify prior findings addressed
• Look for new issues
• Post re-review

All three blockers from the last round are properly fixed, and the fixes are the right ones. Replacing the admin_auth_method claim with the presence of id_token in AuthenticationProperties is the correct move — I traced CookieAuthenticationHandler.FinishResponseAsync, which re-protects the ticket using the original ticket.Properties after ReplacePrincipal, so the token genuinely survives the SecurityStampValidator refresh where the claim did not. Reading it before SignOutAsync() and passing it through the sign-out properties also removes the handler-caching dependency. ClockSkewSeconds shared between TokenValidationParameters.ClockSkew and the auth_time window is a nice touch, and the OidcEnabled guard on Logout() has a regression test.

Two things I'd want resolved before merge, then some smaller ones. Both are new — neither was visible in the prior diff.

---

🟠 email from the UserInfo endpoint lands under ClaimTypes.Email, not email — which breaks exactly the IdPs UserInfo was enabled for

src/Admin/IdentityServer/ServiceCollectionExtensions.cs:108-116 + src/Admin/Auth/Controllers/LoginController.cs:139

The comment at :108-111 states the rationale:

Some IdPs (Okta) leave email and email_verified out of the ID token; UserInfo is the authoritative source.

That's true, but MapInboundClaims = false doesn't reach the UserInfo path. It controls the JWT handler's inbound short-name → WS-Fed mapping for the ID token only. UserInfo claims are produced by ClaimActions, which run separately in OpenIdConnectHandler.GetUserInformationAsync over the raw JSON, and OpenIdConnectOptions ships this default:

ClaimActions.MapUniqueJsonKey(ClaimTypes.Email, "email");            // → http://schemas.xmlsoap.org/.../emailaddress
ClaimActions.MapUniqueJsonKey("email_verified", "email_verified");   // → email_verified  ✅

So the two claims diverge:

Source of email	Resulting claim type	FindFirst("email")
ID token	email (MapInboundClaims=false)	✅
UserInfo only	ClaimTypes.Email (xmlsoap URI)	❌ null

For an IdP that omits email from the ID token — the Okta case the comment cites — LoginController.cs:139 returns null, the missing-email branch fires, and every SSO sign-in fails with error=5. email_verified is unaffected, so this would present as "the email claim is missing" even though UserInfo returned it.

Since EmailClaimType is configurable, the general fix is one line inside the options callback:

// ClaimActions apply to the UserInfo response and are unaffected by MapInboundClaims;
// without this the default MapUniqueJsonKey(ClaimTypes.Email, "email") would file a
// UserInfo-sourced email under the xmlsoap URI instead of EmailClaimType.
options.ClaimActions.MapUniqueJsonKey(oidc.EmailClaimType, oidc.EmailClaimType);

MapUniqueJsonKey dedupes on type+value, so this is a no-op when the ID token already carried the claim. Worth confirming which IdP the demo video ran against — if the ID token carried email there, this path was never exercised. Fix this →

🟠 CookieSecurePolicy.Always is an unconditional breaking change for HTTP-only self-hosts, and it hits the magic-link flow too

src/Admin/IdentityServer/ServiceCollectionExtensions.cs:42-44

This line is in AddPasswordlessIdentityServices, not in AddAdminUpstreamOidc — so it applies to every Admin deployment regardless of whether OIDC is configured, including ones that never opt into this feature. Self-hosters who declined SSL at install time reach nginx over plain HTTP; the browser will silently drop the application cookie, and the admin bounces back to /login with no error message and nothing in the logs. Complete lockout from an opt-out-impossible change, in a PR whose stated scope is an opt-in feature.

The stated justification doesn't hold for the standard topology either:

Default (SameAsRequest) can downgrade behind a TLS-terminating proxy that doesn't forward X-Forwarded-Proto

Startup.cs:146-150 already calls UseForwardedHeaders(globalSettings) for self-hosted, and SharedWeb/Utilities/ServiceCollectionExtensions.cs:541 includes ForwardedHeaders.XForwardedProto. So SameAsRequest resolves correctly behind the Bitwarden proxy today. (That the Admin launchSettings.json had to grow an HTTPS binding in this same PR is the same symptom showing up in local dev.)

The security posture is right where SSO is concerned — the OIDC correlation cookie defaults to SameSite=None, which browsers reject without Secure, so SSO over HTTP can't work regardless. Scoping it accordingly keeps the hardening without the regression:

options.Cookie.SecurePolicy = adminSettings.OidcEnabled
    ? CookieSecurePolicy.Always
    : CookieSecurePolicy.SameAsRequest;

If you'd rather keep it unconditional — defensible, and I wouldn't argue hard against it — it needs to be called out as a breaking change in the PR description and release notes, since nothing else in the description signals that non-SSO deployments are affected at all. Fix this →

---

🟡 The id_token round-trips through the IdP inside the state parameter

src/Admin/Auth/Controllers/LoginController.cs:243-245

OpenIdConnectHandler.HandleSignOutAsync ends with message.State = Options.StateDataFormat.Protect(properties) — that's how RedirectUri survives the trip to SignedOutCallbackPath. Because the id_token is stored on those same properties, it gets Data-Protection-encrypted into state in addition to being sent as id_token_hint. Net effect is the ID token traverses the end-session URL twice: roughly 1–2 KB as the hint plus another ~2 KB of ciphertext in state.

It's encrypted with the app's own keys so the IdP learns nothing, but a 4–6 KB end-session GET is close to common request-line limits (nginx large_client_header_buffers defaults to 8k; several IdPs cap lower), and tokens with large group claims — Entra ID in particular — push it further. A logout that 414s is a poor failure mode for the feature this plumbing exists to enable.

Passing it out-of-band avoids the duplication entirely, since the event runs in the same request:

// Logout
HttpContext.Items["admin_id_token"] = idToken;
return SignOut(new AuthenticationProperties { RedirectUri = loggedOutRedirect },
    AdminAuthenticationSchemes.UpstreamOidc);

// OnRedirectToIdentityProviderForSignOut
if (ctx.HttpContext.Items.TryGetValue("admin_id_token", out var t) && t is string s && s.Length > 0)
{
    ctx.ProtocolMessage.IdTokenHint = s;
}

Worth measuring against your IdP before changing anything — if the end-session URL comes in comfortably under 4 KB, leaving it as-is is reasonable.

🟡 Correlation and nonce cookie policies left implicit

src/Admin/IdentityServer/ServiceCollectionExtensions.cs:69-170

The app cookie gets SecurePolicy/SameSite pinned explicitly, and the token-validation block at :128-136 is pinned with the rationale "implicit defaults can shift across major .NET versions." CorrelationCookie and NonceCookie get neither — and src/Identity/Startup.cs:114-115 pins exactly those two for the existing SSO handler, so there's in-repo precedent. Same one-line treatment for consistency.

🟡 max_age=3600 bounds sign-in freshness but not the resulting admin session

src/Admin/IdentityServer/ServiceCollectionExtensions.cs:49-51

ExpireTimeSpan = 2 days with SlidingExpiration = true (both pre-existing, unchanged here). So the prompt=login + max_age=3600 + server-side auth_time enforcement guarantee only that the operator re-authenticated at the IdP at the moment of sign-in; the admin session it mints then renews indefinitely on activity. Not introduced by this PR, and not something I'd change here — flagging because the description positions session freshness as a security property and cites FedRAMP, where AC-11/AC-12 will ask about the session side too. Probably a follow-up ticket.

🟡 Smaller items

• Scopes (AdminSettings.cs:68-70, ServiceCollectionExtensions.cs:118-122) — the doc comment now says space-delimited and required-openid, which addresses the discoverability half. Still worth two lines: openid isn't force-added, so Scopes=profile email yields a request with no openid, and if the IdP tolerates it and returns no ID token you land on !external.Succeeded → generic error=5 with no hint that the config is the cause. Fine to decline now that it's documented.
• launchSettings.json:19 — https://localhost:5001 is still the generic ASP.NET Core default; every other project here uses a unique port (46273, 62911, 50024, 33656, 44518, 61840, 54103). 62913 would fit. Third mention, lowest stakes on the list.
• Test gaps — the suite is in good shape now. The one branch I'd still pin: RequireEmailVerifiedClaim = false with the claim absent → accepted (LoginController.cs:254-257). It's the single config flag that weakens the security posture, and nothing currently asserts it behaves as documented. Index POST / Confirm returning 404 when EnablePasswordlessLogin is false are also uncovered and cheap.
• LoginController.cs:211 — the SSO path still bypasses PreSignInCheck/CanSignInAsync. Impact is nil against ReadOnlyEnvIdentityUserStore, and the comment block above it is excellent on everything else; one line noting this is deliberate would save the next reader the derivation.

---

Previously raised, now resolved

admin_auth_method claim dropped by SecurityStampValidator → replaced with id_token on AuthenticationProperties, with Logout_LocalOnly_WhenNoIdTokenStored / Logout_TriggersRpInitiatedLogout_WhenIdTokenPresent covering it • auth_time clock-skew tolerance via the shared ClockSkewSeconds constant, with SsoSignIn_AcceptsAuthTimeSlightlyInFuture • OidcEnabled guard on Logout(), with Logout_LocalOnly_WhenIdTokenPresentButOidcDisabled • id_token read before SignOutAsync() so id_token_hint no longer depends on handler-level ticket caching • magic-link submit button no longer disabled-in-markup, inline script removed • SsoSignIn_StoresIdTokenOnAppCookie_OnSuccess, SsoSignIn_RedirectsToLocalReturnUrl_WhenLocal, Sso_ReturnsChallengeResultForUpstreamOidc_WhenEnabled all added.

dotnet build / dotnet test weren't permitted in this environment, so the above is static analysis only. Build, Testing, and Code Quality were still in progress when I finished — worth confirming they're green. Build (Fork PR) startup_failure is a workflow-trigger artifact, not a code failure.
• branch admin/upstream-oidc

[trmartin4]

@claude please review again.

[claude]

Claude finished @trmartin4's task in 3m 24s —— View job

---

Re-review

• Read current diff (through 02f0a74)
• Verify prior findings addressed
• Look for new issues
• Post re-review

Both blockers from the last round are resolved, and I like how. Dropping GetClaimsFromUserInfoEndpoint entirely is a better answer than the ClaimActions mapping I suggested — one fewer claim pipeline to reason about, and the requirement it creates (email/email_verified in the ID token) is now stated in-code and in the description. Scoping CookieSecurePolicy.Always to OidcEnabled is exactly right, and the comment captures the reasoning for the next reader.

The new session-timeout work (2b0e300) is where my findings are this round. The intent is right and the FedRAMP mapping is the right one to be making — but the enforcement mechanism doesn't hold up, in two independent ways, and both fail silently.

---

🔴 Enabling the absolute timeout silently disables session revocation

src/Admin/IdentityServer/ServiceCollectionExtensions.cs:71

options.Events.OnValidatePrincipal = async ctx => { ... };

This is an assignment, not a chain. AddIdentity<IdentityUser, Role>() (:25) registers its own configure action for the ApplicationScheme cookie that does:

o.Events = new CookieAuthenticationEvents
{
    OnValidatePrincipal = SecurityStampValidator.ValidatePrincipalAsync
};

Named IConfigureOptions<CookieAuthenticationOptions> actions run in registration order, and ConfigureApplicationCookie (:36) is registered after AddIdentity (:25) — so this callback runs last and replaces the security-stamp delegate outright. When AbsoluteSessionTimeoutMinutes > 0, SecurityStampValidator never runs on the application cookie and SecurityStampValidatorOptions.ValidationInterval (Startup.cs:80-83) becomes dead config.

That's not cosmetic here, because of how ReadOnlyEnvIdentityUserStore works. SecurityStampValidator.ValidateAsync → VerifySecurityStamp → UserManager.FindByIdAsync(email) → ReadOnlyEnvIdentityUserStore.FindByIdAsync (:63) → FindByEmailAsync → null if the email is no longer in adminSettings:admins → RejectPrincipal(). That lookup is the revocation path for the Admin Portal. Remove someone from AdminSettings__Admins, restart, and today their live session dies within 5 minutes. With the absolute cap enabled it survives for the full SessionTimeoutMinutes — 2 days on the default. The principal also stops being rebuilt through CustomClaimsPrincipalFactory, so config-side role changes stop propagating too.

So the net effect of turning on an AC-12 control is losing a working AC-2(3)-style revocation control, with nothing in logs or config to indicate it. Chain rather than replace:

var inner = options.Events.OnValidatePrincipal;
options.Events.OnValidatePrincipal = async ctx =>
{
    if (/* absolute cap exceeded */)
    {
        ctx.RejectPrincipal();
        await ctx.HttpContext.SignOutAsync(IdentityConstants.ApplicationScheme);
        return;
    }
    await inner(ctx);
};

Fix this →

🔴 IssuedUtc is reset by sliding renewal, so the absolute cap isn't absolute

src/Admin/IdentityServer/ServiceCollectionExtensions.cs:64-74

The comment states the premise:

Enforced against the ticket's IssuedUtc, which the framework sets once at sign-in and sliding never touches (unlike ExpiresUtc).

CookieAuthenticationHandler does touch it. RequestRefresh sets _refreshIssuedUtc = currentUtc alongside _refreshExpiresUtc, and FinishResponseAsync writes both back onto the ticket before re-protecting it:

ticket.Properties.IssuedUtc = _refreshIssuedUtc;
ticket.Properties.ExpiresUtc = _refreshExpiresUtc;

IssuedUtc is therefore "when the cookie was last written," not "when the admin signed in." Two consequences, and they get worse in sequence:

As shipped today — RequestRefresh fires from CheckForRefresh once a ticket is past its half-life, so IssuedUtc resets roughly every SessionTimeoutMinutes / 2 of activity. The cap only ever fires if AbsoluteSessionTimeoutMinutes < SessionTimeoutMinutes / 2. It holds on the defaults (720 < 2880/2), which is presumably why it looked correct. It does not hold for the combination the PR description recommends to strict deployments — "tighten SessionTimeoutMinutes to 15–60" and set AbsoluteSessionTimeoutMinutes=720. There, IssuedUtc resets every 7–30 minutes and the 12-hour cap never fires at all. An operator who follows the docs gets an unenforced control that reports as configured.

After fixing 🔴 #1 — it breaks completely. SecurityStampValidator.SecurityStampVerified ends with context.ShouldRenew = true, and HandleAuthenticateAsync turns that into a RequestRefresh. So restoring the stamp validator resets IssuedUtc every ValidationInterval (5 min), and the cap can never fire under any activity pattern. The two fixes have to land together.

Anchor on something refresh doesn't rewrite. Properties.Items is the natural choice — it survives for the same reason the id_token does (FinishResponseAsync re-protects the original ticket.Properties), which is already load-bearing in this PR:

const string SessionStartItem = "admin_session_start";

if (!ctx.Properties.Items.TryGetValue(SessionStartItem, out var raw) ||
    !DateTimeOffset.TryParse(raw, CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind, out var start))
{
    // Backfill: first request on a ticket issued before the cap was configured.
    start = ctx.Properties.IssuedUtc ?? DateTimeOffset.UtcNow;
    ctx.Properties.Items[SessionStartItem] = start.ToString("O", CultureInfo.InvariantCulture);
    ctx.ShouldRenew = true;
}

if (DateTimeOffset.UtcNow - start > absoluteMax) { /* reject */ }

The backfill keeps it working for cookies minted before the setting was turned on, without having to touch either sign-in path. Fix this →

🟠 SessionTimeoutMinutes = 0 bricks the portal, and 0 means "disabled" on the setting directly above it

src/Admin/AdminSettings.cs:25-31 + ServiceCollectionExtensions.cs:60

AbsoluteSessionTimeoutMinutes = 0 means "no cap." SessionTimeoutMinutes = 0 means ExpireTimeSpan = TimeSpan.Zero — every cookie is expired the instant it's written, so every admin bounces back to /login immediately with no error and nothing in the logs. Two adjacent int settings with the same name shape where 0 means "off" on one and "total lockout" on the other is a trap an operator will eventually walk into, and negative values behave the same way.

A floor at startup (Math.Max(1, ...), or a validation throw so it's loud) closes it. Given the recovery path for a locked-out Admin Portal is "SSH in and fix the env var," failing loudly at startup is probably the friendlier of the two. Fix this →

---

🟡 Neither new setting has a test, and the code shape prevents one

test/Admin.Test/AdminSettingsTests.cs covers only OidcEnabled. The absolute-timeout logic lives as a closure inside a DI registration callback, so there's nowhere for a unit test to reach it — which is part of why both 🔴s above are invisible to the suite. Lifting the decision to a testable seam (static bool IsSessionExpired(AuthenticationProperties props, TimeSpan max), or a small CookieAuthenticationEvents subclass) would let you pin "renewal doesn't extend the cap" directly, which is the property that's currently wrong.

🟡 Carried over, unchanged

• id_token round-trips in state (LoginController.cs:243-245) — HandleSignOutAsync protects the sign-out properties into state, and the token lives on those properties, so it goes out both as id_token_hint and as ~2 KB of ciphertext in state. Passing it via HttpContext.Items avoids the duplication. Still worth a measurement against your IdP before acting.
• Correlation/nonce cookie policies implicit (ServiceCollectionExtensions.cs:98-202) — src/Identity/Startup.cs:114-115 pins both for the existing SSO handler; same one-line treatment here would match the explicit posture taken on everything else in this block.
• Scopes (:150-154) — still splits on ' ' only and doesn't force-add openid. Documented now, so fine to decline.
• launchSettings.json:19 — https://localhost:5001 is still the generic ASP.NET Core default; every other project here uses a unique port. Lowest stakes on the list, fourth mention — happy to stop raising it.
• Test gaps — RequireEmailVerifiedClaim = false with the claim absent → accepted (LoginController.cs:254-257) is the one config flag that weakens the posture and still has nothing asserting it. Index POST / Confirm returning 404 when EnablePasswordlessLogin is false are also uncovered.

🟡 Description accuracy

The config table says AbsoluteSessionTimeoutMinutes gives "an absolute session cap measured from initial sign-in. Sliding renewals cannot extend past this even under continuous activity (FedRAMP AC-12)," and the compliance table repeats it. Per 🔴 #2 that isn't what the code does today. Worth correcting alongside the fix so the description doesn't outrun the implementation — this is the kind of table that ends up quoted in a compliance questionnaire.

---

Previously raised, now resolved

UserInfo-sourced email landing under ClaimTypes.Email → resolved by removing GetClaimsFromUserInfoEndpoint and relying on the ID token, with the IdP requirement documented in-code • CookieSecurePolicy.Always scoped to OidcEnabled so HTTP-only self-hosts keep working, with the UseForwardedHeaders reasoning captured in the comment.

dotnet build / dotnet test weren't permitted in this environment, so the above is static analysis only. Build, Testing, and Code Quality were in progress when I finished — everything else on the run is green. Build (Fork PR) startup_failure is a workflow-trigger artifact, not a code failure.
• branch admin/upstream-oidc

[trmartin4]

@claude please review.

[claude]

Claude finished @trmartin4's task in 4m 37s —— View job

---

Re-review

• Read current diff (through 72dbbe5)
• Verify prior findings addressed
• Look for new issues
• Post re-review

The session refactor (02c0402) is a real improvement — deleting AbsoluteSessionTimeoutMinutes and its hand-rolled OnValidatePrincipal removed both of last round's 🔴s at the root rather than patching them, and the PostConfigure that adds error=6 is chained correctly (captures the prior OnRedirectToLogin and calls it) rather than overwriting, which is exactly the mistake that made the old code break SecurityStampValidator. Startup validation on SessionTimeoutMinutes with a test, the explicit CorrelationCookie/NonceCookie pinning, and dropping GetClaimsFromUserInfoEndpoint in favor of a documented ID-token requirement are all in.

One CI failure and one substantive finding about what SessionSliding actually does, then smaller items.

---

🔴 Lint is failing — ServiceCollectionExtensionsTests.cs has no UTF-8 BOM

test/Admin.Test/IdentityServer/ServiceCollectionExtensionsTests.cs:1

The Build run is red, and it's just encoding:

##[error] test/Admin.Test/IdentityServer/ServiceCollectionExtensionsTests.cs(1,1):
         error CHARSET: Fix file encoding. [test/Admin.Test/Admin.Test.csproj]

.editorconfig requires charset = utf-8-bom for [*.{cs,csx,vb,vbx}] and build.yml runs dotnet format --verify-no-changes. This is the only file dotnet format flagged — the other four .cs files in the diff kept their BOMs, so it's the same editor artifact as the first round rather than anything systemic. dotnet format locally fixes it. Fix this →

🔴 SessionSliding = false does not produce a fixed lifetime — SecurityStampValidator renews the ticket every 5 minutes

src/Admin/IdentityServer/ServiceCollectionExtensions.cs:67-73 with src/Admin/Startup.cs:80-83

The comment states the contract:

With SessionSliding = true (default), this is an idle timeout. With false, it's a fixed lifetime from sign-in - the framework's built-in absolute cap.

The second half doesn't hold once SecurityStampValidator is in the pipeline, which it is here (AddIdentity wires OnValidatePrincipal = SecurityStampValidator.ValidatePrincipalAsync, and the new PostConfigure correctly leaves it intact). The path:

1. SecurityStampValidator.ValidateAsync validates whenever now - IssuedUtc > Options.ValidationInterval — 5 minutes per Startup.cs:82.
2. ReadOnlyEnvIdentityUserStore returns a stable stamp (:59, the email or the configured email:stamp), so verification succeeds and SecurityStampVerified runs.
3. SecurityStampVerified ends with context.ShouldRenew = true.
4. CookieAuthenticationHandler.HandleAuthenticateAsync does if (context.ShouldRenew) RequestRefresh(context.Properties, context.Principal); — unconditionally, not gated on Options.SlidingExpiration. Only the other caller, CheckForRefresh, is gated on it.
5. RequestRefresh sets _refreshIssuedUtc = currentUtc and _refreshExpiresUtc = currentUtc.Add(expiresUtc - issuedUtc), and FinishResponseAsync writes both back onto the ticket before re-protecting it.

So ExpiresUtc is pushed forward by the full ExpireTimeSpan roughly every 5 minutes of activity, regardless of SlidingExpiration. Net effect: SessionSliding = false behaves almost identically to true, just with 5-minute granularity instead of half-life granularity. A session renews indefinitely under continuous use.

This is the same mechanism that killed the previous IssuedUtc-based absolute cap, now reaching the replacement through a different door — the refactor moved the enforcement to the framework, but the framework's own SlidingExpiration=false is what ShouldRenew overrides. It matters because the comment at :67-71 and the PR description both sell this as the FedRAMP AC-12 control, and the compliance table is the kind of thing that gets quoted into a questionnaire.

Cleanest fix is to fold it into the PostConfigure you already added, chaining after the stamp validator so revocation keeps working:

var existingValidate = options.Events.OnValidatePrincipal;
options.Events.OnValidatePrincipal = async ctx =>
{
    await existingValidate(ctx);
    if (!adminSettings.SessionSliding)
    {
        // SecurityStampVerified sets ShouldRenew, which pushes IssuedUtc/ExpiresUtc
        // forward via RequestRefresh even when SlidingExpiration is false. Suppress it
        // so ExpiresUtc stays pinned to sign-in time.
        ctx.ShouldRenew = false;
    }
};

The cost is that with the cookie no longer rewritten, IssuedUtc stays at sign-in and the stamp check runs on every request — against ReadOnlyEnvIdentityUserStore that's a config read, so it's cheap, and it makes revocation more responsive. Alternatively keep a admin_session_start value in Properties.Items (which survives refresh, the same property the id_token relies on) and reject past the cap. Either way this wants a test that asserts renewal doesn't move ExpiresUtc — nothing currently pins it. Fix this →

---

🟠 The FedRAMP guidance comment contradicts prompt=login

src/Admin/IdentityServer/ServiceCollectionExtensions.cs:69-71 vs :189-194

FedRAMP-strict deployments should set SessionSliding=false + short SessionTimeoutMinutes + rely on SSO (which lets the IdP silently re-sign-in when the IdP session is still fresh).

options.AdditionalAuthorizationParameters.Add("prompt", "login") is precisely what prevents silent re-sign-in — the comment at :189-193 says so explicitly ("Force credential entry at the IdP on every Admin Portal sign-in… re-proving control (password + MFA) on each cookie renewal"). The two comments recommend opposite behaviors from the same config.

The practical consequence is a UX cliff an operator won't expect: following this guidance with SessionTimeoutMinutes=15 means full password + MFA entry at the IdP every 15 minutes, not a silent redirect. That's a defensible posture for an admin portal, but it should be stated as the cost rather than described as silent. Worth reconciling :69-71 with reality (and the PR description's "tighten to 15–60" recommendation alongside it).

🟠 Dropping max_age leaves prompt=login unverifiable

src/Admin/IdentityServer/ServiceCollectionExtensions.cs:189-194

Guarantees auth_time on the returned ID token is effectively "now", so we don't also need max_age.

prompt=login is a request, not an enforcement. Per OIDC Core 3.1.2.1 the IdP should honor it, but nothing in the response proves it did — and unlike max_age, sending prompt alone doesn't make auth_time mandatory in the ID token (3.1.3.7 ties that to max_age). So there's now no server-side check that the freshness property the threat model claims ("stolen IdP session → free admin access: prompt=login forces credential + MFA re-entry") actually held.

The previous revision did enforce this; it got dropped along with the clock-skew problem I raised, which is understandable — but the skew issue was fixable (and you'd already added ClockSkewSeconds for it) without giving up the check. I'd either re-add max_age plus a skew-tolerant auth_time comparison, or soften the description's claim to "requested" rather than "forces". Reasonable to decline if you've confirmed your target IdPs honor prompt; it's the unverified-assertion-in-a-compliance-table shape I'd want resolved. Fix this →

---

🟡 SSO sessions are browser-session-scoped; magic-link sessions are not

src/Admin/Auth/Controllers/LoginController.cs:171 vs :84

SSO signs in with new AuthenticationProperties { IsPersistent = false } — no Expires attribute on the cookie, so it dies when the browser closes. The passwordless path calls PasswordlessSignInAsync(email, token, true), i.e. isPersistent: true, so it survives. Same portal, same SessionTimeoutMinutes, materially different observed lifetime depending on how you signed in.

Non-persistent is the stricter and probably intended choice for the SSO path, but the asymmetry isn't mentioned anywhere and it interacts with the prompt=login cost above (every browser restart = full IdP re-auth). A one-line comment on :171 saying it's deliberate, plus a row in the description's table, would cover it.

🟡 Null EmailClaimType throws on the callback

src/Admin/Auth/Controllers/LoginController.cs:139 and ServiceCollectionExtensions.cs:173

ClaimsPrincipal.FindFirst(string) throws ArgumentNullException on null. Config binding turns an empty env var into "" (which just fails the claim lookup and lands on the error=5 branch — fine), but an explicit "EmailClaimType": null in appsettings/user-secrets nulls the property and gives a 500 on the callback instead. A string.IsNullOrWhiteSpace fallback to "email" where it's read, or validation alongside the SessionTimeoutMinutes throw at :22-27, closes it. Low likelihood, one line.

🟡 Carried over, unchanged

• id_token round-trips in state (LoginController.cs:223-225) — OpenIdConnectHandler.HandleSignOutAsync protects the sign-out properties into state, and the token is stored on those properties, so it goes out both as id_token_hint and as ~2 KB of ciphertext in state. HttpContext.Items avoids the duplication since the event runs in the same request. Still worth a measurement against your IdP before acting.
• Scopes (AdminSettings.cs:71-73, ServiceCollectionExtensions.cs:165-169) — splits on ' ' only, openid not force-added. Documented now; fine to decline.
• Test gaps — RequireEmailVerifiedClaim = false with the claim absent → accepted (LoginController.cs:231-239) is the one flag that weakens the posture and still has nothing asserting it. Index POST / Confirm returning 404 when EnablePasswordlessLogin is false are also uncovered and cheap. Plus the ExpiresUtc test from 🔴 Password Requirements Validation #2.

🟡 Question

src/Admin/Views/Shared/_Layout.cshtml:153-156 drops the else branch that rendered a "Log In" link for anonymous users. On the login page that link was redundant, so I assume that's the motivation — but it also removes it from every other anonymous-reachable page (/error, for instance), leaving no navigation back to sign-in. Intentional?

---

Previously raised, now resolved

Absolute-timeout OnValidatePrincipal overwriting SecurityStampValidator.ValidatePrincipalAsync → resolved by removing the hand-rolled cap entirely; the new error=6 hook uses the correct chain-don't-replace pattern (ServiceCollectionExtensions.cs:82-96) with tests both ways • IssuedUtc-based cap → removed • SessionTimeoutMinutes = 0 locking everyone out → startup throw at :22-27 with [InlineData(0)]/[InlineData(-1)] coverage • CorrelationCookie/NonceCookie SameSite/SecurePolicy pinned explicitly at :200-203, matching src/Identity/Startup.cs • UserInfo email → ClaimTypes.Email → resolved by not calling GetClaimsFromUserInfoEndpoint and documenting the ID-token requirement • CookieSecurePolicy.Always scoped to OidcEnabled • PreSignInCheck bypass now carries the explanatory comment at :186-190.

dotnet build / dotnet test weren't permitted in this environment, so everything above except the lint failure is static analysis. Testing and Code Quality were still in progress when I finished; Build is red for the BOM issue above. Build (Fork PR) startup_failure is a workflow-trigger artifact, not a code failure.
• branch admin/upstream-oidc