Repository navigation
Conversation
SSO sign-in now maps the IdP `groups` claim onto Admin Portal role claims via GroupClaimRoleMapper and signs the resulting principal into the application cookie through SsoSignInPrincipalBuilder, bypassing SignInManager/UserManager. AccessControlService.UserHasPermission takes the union across every role the principal holds. The AdminSettings:Admins CSV allowlist is no longer the gate for the SSO path - IdP group membership is. Passwordless sign-in still uses the allowlist. AddAdminUpstreamOidc fail-louds on an empty AdminSettings:Oidc:GroupPrefix.
trmartin4
changed the title
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Re-reviewed through 9d24324. That commit replaces the dual-gate SSO tenancy check with one that depends on the deployment type: on cloud the principal must hold a mapped role claim, and on self-hosted the email must be on Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## admin/upstream-oidc-minimal #8587 +/- ##
===============================================================
- Coverage 66.90% 65.56% -1.35%
===============================================================
Files 2591 2548 -43
Lines 111391 109729 -1662
Branches 10154 9982 -172
===============================================================
- Hits 74523 71939 -2584
- Misses 34382 35421 +1039
+ Partials 2486 2369 -117 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| AdminAuthenticationSchemes.SsoCookie) | ||
| .RequireAuthenticatedUser() | ||
| .Build(); | ||
| options.FallbackPolicy = options.DefaultPolicy; |
There was a problem hiding this comment.
❌ CRITICAL: The new FallbackPolicy requires sign-in on every endpoint that has no [Authorize]/[AllowAnonymous], which includes the /login page itself, so no one can sign in.
Details and fix
Before this PR FallbackPolicy was null. ASP.NET Core only applies it to endpoints that carry no IAuthorizeData, and several Admin endpoints carry none:
LoginController.Index(GET and POST) andLoginController.Confirm: onlySsoandSsoSignInare[AllowAnonymous]. An anonymousGET /loginis challenged by the default scheme (Identity's app cookie,LoginPath = "/login"). That redirects to/login?ReturnUrl=%2Flogin, which is challenged again, so the browser loops until it gives up. The passwordless magic link (/login/confirm) is challenged the same way, so neither sign-in path can be reached.InfoController/aliveand/now: health probes now get a 302 instead of a 200.ErrorController/error:UseExceptionHandler("/error")re-executes into an endpoint that challenges.MapVersionEndpoint()/version.
The tests don't catch this because they build controllers directly and never run the authorization middleware.
Making bare [Authorize] accept both cookies only needs DefaultPolicy. The fallback isn't part of that goal:
| options.FallbackPolicy = options.DefaultPolicy; | |
| // FallbackPolicy deliberately left unset: LoginController.Index/Confirm, /alive, | |
| // /error and /version have no [Authorize]/[AllowAnonymous] and must stay reachable. |
If you do want deny-by-default, add [AllowAnonymous] to LoginController (class level), InfoController, and ErrorController, and call .AllowAnonymous() on the version endpoint. Then add a WebApplicationFactory test that checks GET /login returns 200.
There was a problem hiding this comment.
Resolved in f4955f9. FallbackPolicy is no longer set, and only DefaultPolicy lists both cookie schemes. LoginController.Index/Confirm, /alive, /error, and /version are anonymous again.
The previous commit set FallbackPolicy = DefaultPolicy, which required authentication on every endpoint without an explicit [Authorize] or [AllowAnonymous]. That locked out the login pages (LoginController.Index and Confirm have no attributes), the /alive health probe, /error, and the version endpoint. The login page would 302 to itself; health probes would 302 to the login page. Keep DefaultPolicy so [Authorize] still accepts either cookie scheme, but let endpoints without [Authorize] stay anonymous.
| // is bounded by the cookie expiry. A separate scheme (not the Identity application | ||
| // cookie) is required because Identity's SecurityStampValidator does a 30s re-fetch | ||
| // against the user store, which would reject the SSO principal within 30 seconds. | ||
| var principal = _ssoSignInPrincipalBuilder.Build(email, external.Principal); |
There was a problem hiding this comment.
[Authorize] actions have no RequirePermission gate, so in cloud that user can read and change customer data.
Details and fix
The allowlist check was removed and nothing replaced it. In cloud, SsoSignIn now signs in any IdP-authenticated user with a verified email, even when MapRoles returns nothing. The PR description says such a user is denied "every gated action". But these actions under [Authorize] are not gated by RequirePermission:
UsersController.View/Edit(GET): user email, ciphers metadata, billing and billing historyOrganizationsController.View/Edit(GET), plusTriggerBillingSyncandResendOwnerInvite(POST)
Before this change, only operators on the AdminSettings:Admins allowlist could reach those actions. Now anyone the IdP authenticates for the app can reach them.
The comment added in AddAdminUpstreamOidc already assumes this case is refused ("every SSO sign-in would be rejected as 'no mapped group'"), but the code never refuses it. Suggested fix: when not self-hosted and the mapper returns no roles, reject the sign-in the same way the other branches do:
var principal = _ssoSignInPrincipalBuilder.Build(email, external.Principal);
if (!_globalSettings.SelfHosted && !principal.HasClaim(c => c.Type == ClaimTypes.Role))
{
_logger.LogWarning("SSO sign-in rejected: principal holds no mapped Admin Portal group.");
await HttpContext.SignOutAsync(IdentityConstants.ExternalScheme);
return RedirectToAction("Index", new { error = 5 });
}Add a test for the rejection, and update the PR description and the AdminSettings.GroupPrefix comment, which say no-group principals "still sign in".
There was a problem hiding this comment.
Resolved in 1dfda58. SsoSignIn now rejects any principal with no ClaimTypes.Role claim before SignInAsync: it signs out the external scheme and redirects with the generic error 5. SsoSignIn_RedirectsError5_WhenPrincipalHasNoRoleClaims covers this. The gate runs on both self-hosted and cloud, which is stricter than what I suggested and fine.
One leftover: the GroupPrefix comment at src/Admin/AdminSettings.cs:83 still says a principal with no mapped groups "still signs in", and the Build_StampsNoRoleClaims_WhenNoGroupsMap test comment says the same. Both now contradict the gate and should be updated.
…cheme The ClaimsIdentity was declaring IdentityConstants.ApplicationScheme as its AuthenticationType, carried over from when the principal was signed into the Identity cookie. Now that it signs into AdminAuthenticationSchemes. SsoCookie, Identity.AuthenticationType was lying about the actual scheme - not a functional bug (IsAuthenticated just requires non-null), but anything keying off AuthenticationType for middleware traces or debug tooling would have reported the wrong scheme.
Several controller actions in UsersController and OrganizationsController carry only bare [Authorize] and no [RequirePermission] gate (View, Edit, TriggerBillingSync, ResendOwnerInvite). On cloud, with the AdminSettings: Admins allowlist removed from the SSO path, an IdP-authenticated user whose groups do not map to any admin role would receive an application cookie and be able to reach those actions. Add a tenancy gate at sign-in: after SsoSignInPrincipalBuilder.Build returns, verify the principal carries at least one ClaimTypes.Role claim. If none, log a warning, sign out the external scheme, and redirect with the generic SSO-failed error. The gate is independent of per-action [RequirePermission] checks, which remain a desirable defense-in-depth follow-up. Design rationale and the considered alternatives are captured in src/Admin/Auth/SSO.md, following the ADR format from the engineering documentation standard (ADR-0034).
Drop the ADR-style frontmatter and the Considered Options / Decision Outcome / Positive-Negative-Plan structure. Document the SSO sign-in path as it is: the two sign-in paths, the SSO flow, the tenancy gate, the authn/tenancy/authz separation, the cookie schemes, logout, and operator configuration. Audience is engineers modifying the SSO code and operators provisioning Admin Portal access, not reviewers evaluating a design decision.
| // AuthenticationType is required for ClaimsIdentity.IsAuthenticated to return true. | ||
| // Match the scheme the controller signs this principal into so middleware traces and | ||
| // any code keying off Identity.AuthenticationType report the actual scheme. | ||
| var identity = new ClaimsIdentity(AdminAuthenticationSchemes.SsoCookie); |
There was a problem hiding this comment.
SignInManager.IsSignedIn returns false for SSO sessions. The layout hides the whole nav and shows "Log In" instead of "Log Out", so SSO users have no way to start RP-initiated logout from the UI.
Details and fix
_Layout.cshtml gates both the nav menu (line 51) and the Log Out form (line 150) on SignInManager.IsSignedIn(User). The stock implementation (not overridden by PasswordlessSignInManager) is:
return principal.Identities.Any(i => i.AuthenticationType == AuthenticationScheme); // IdentityConstants.ApplicationSchemeThe cookie ticket serializer keeps ClaimsIdentity.AuthenticationType across the round trip. The SSO identity now comes back as "sso-cookie", so the check fails even on [Authorize] pages where PolicyEvaluator has already authenticated the user. Before e7d6e7d the identity carried IdentityConstants.ApplicationScheme, which is why this worked. So the commit message's "not a functional bug" doesn't hold for this caller.
Either fix works:
- Revert to
new ClaimsIdentity(IdentityConstants.ApplicationScheme)and restore the matching test assertion. - Keep
SsoCookieand change both layout checks toUser.Identity?.IsAuthenticated == true, which covers both cookie schemes.
Whichever you choose, add a test that pins the contract, so the layout's sign-in signal can't silently drift from the principal again.
Rewrite SSO.md to describe what the system does today without rehashing alternatives considered or temporal references to in-flight work. Shorten the tenancy-gate comment in LoginController.cs for the same reason - the "this PR removed" phrasing is meaningless once the PR is merged and the list of currently-ungated [Authorize] actions is accurate only until the first [RequirePermission] follow-up lands.
Operators can now configure the SSO sign-in path with either the AdminSettings:Admins allowlist, the AdminSettings:Oidc:GroupPrefix group mapping, or both. Each configured signal is a tenancy gate; every configured gate must admit the user. Signals that are not configured are skipped. - AddAdminUpstreamOidc fails loud at boot when SSO is enabled with neither Admins nor GroupPrefix set. - LoginController.SsoSignIn runs each configured gate and rejects with the generic SSO-failed error when any gate denies. - SSO.md documents the three configurations (CSV only, groups only, both) and what each means for sign-in, role claims, and authorization on cloud vs self-hosted. - LoginControllerTests cover every pass/deny combination across the three configurations. ServiceCollectionExtensionsTests covers the loosened boot-time fail-loud and the CSV-only configuration.
| The `Admins`-only configuration suits operators who already use an IdP for authentication but | ||
| prefer to manage the admin list in config rather than in IdP groups. On cloud, those operators | ||
| also need to populate `adminSettings:role:{roleName}` entries if they want anyone to have | ||
| permissions. |
There was a problem hiding this comment.
adminSettings:role:{roleName} gives SSO users no permissions. The SSO path never reads that config, so this guidance is wrong.
Details and fix
adminSettings:role:{roleName} is read only by AccessControlService.GetUserRole. Its only caller is CustomClaimsPrincipalFactory, which runs on the passwordless SignInManager path. SsoSignInPrincipalBuilder.Build gets role claims only from GroupClaimRoleMapper.MapRoles. UserHasPermission checks only the role claims on the principal. So on cloud, an Admins-only SSO user has zero permissions no matter what the operator puts in adminSettings:role:*. The matrix row at line 37 says the same thing.
An operator who follows this paragraph will end up with admitted SSO users who can't use any gated feature, and nothing will tell them why. The PR description makes the same claim.
Fix one of these two ways:
- Have the SSO principal builder also add
GetUserRole(email)as a role claim whenGroupPrefixis unset, and add a test for it. Or: - Rewrite this paragraph to say that, on cloud,
Admins-only SSO grants no gated permissions and operators needGroupPrefixto get any. Update the PR description to match.
There was a problem hiding this comment.
Resolved in 9d24324. SSO.md no longer mentions adminSettings:role:{roleName}. The case it described, an operator on cloud configured with Admins only, can no longer happen: AddAdminUpstreamOidc now throws at boot on cloud when GroupPrefix is unset, so SSO role claims always come from GroupClaimRoleMapper. On self-hosted, AccessControlService.UserHasPermission short-circuits on SelfHosted, so role claims are never consulted there.
Cloud and self-hosted have different tenancy and authorization models; expose that in the sign-in path rather than making operators configure their way to it. - Cloud: tenancy = principal has at least one role claim. Authorization = union of permissions from role claims via RolePermissionMapping. Role claims come from AdminSettings:Oidc:GroupPrefix mapping. - Self-hosted: tenancy = email on AdminSettings:Admins. Authorization = SelfHosted short-circuit grants everything. Role claims are irrelevant; GroupPrefix is not required. LoginController.SsoSignIn branches on IGlobalSettings.SelfHosted and applies the appropriate gate. AddAdminUpstreamOidc fail-louds at boot when the deployment-required config is missing (Admins on self-hosted, GroupPrefix on cloud). SSO.md replaces the dual-gate configuration matrix with a cloud-vs-self-hosted table. Dual-gate tests replaced with deployment-type-aware tests covering both the pass and deny paths of each deployment.
| // Comma-separated allowlist of email addresses permitted to sign in to the Admin Portal | ||
| // via the passwordless email flow. Authentication proves identity, this list decides who | ||
| // is an admin on the passwordless path. The SSO path uses IdP group claims mapped via | ||
| // AdminSettings:Oidc:GroupPrefix instead, so this list does not gate SSO sign-in. |
There was a problem hiding this comment.
♻️ DEBT: After 9d24324, this comment says Admins "does not gate SSO sign-in". On self-hosted it is now the only SSO gate, and boot fails if it is empty.
Details and fix
LoginController.SsoSignIn checks IsOnAdminAllowlist(email) when _globalSettings.SelfHosted, and AddAdminUpstreamOidc throws on a self-hosted deployment when Admins is empty. A self-hosted operator who reads this comment would conclude that SSO admins are managed through IdP groups and leave them off this list. They would then be locked out of SSO, or the app would fail to boot.
The comments elsewhere in this change describe the earlier design and should change with it:
AdminSettings.cs:82-84(GroupPrefix): "A principal with no mapped groups still signs in". On cloud that principal is now rejected. This leftover was already pointed out on theLoginControllerthread and is still here.GroupClaimRoleMapper.csconstructor: "when OIDC is enabled, AddAdminUpstreamOidc fail-louds on an empty prefix ... so a null here means OIDC is off". On self-hosted, OIDC can be enabled with no prefix set. The behavior is still correct (it returns an empty set), but the reasoning in the comment is wrong. TheMapRoles_ReturnsEmpty_WhenPrefixUnsettest comment has the same problem.
Suggested wording for this comment:
// Comma-separated allowlist of email addresses ("email" or "email:stamp").
// Passwordless path: gates sign-in on every deployment.
// SSO path: gates sign-in on self-hosted only (required at boot when SSO is enabled there).
// Cloud SSO uses IdP group claims mapped via AdminSettings:Oidc:GroupPrefix instead.This branch has not been deployed
1 participant
🎟️ Tracking
PM-44143
📔 Objective
Builds on top of the minimal upstream-OIDC work (#8508) to derive Admin Portal role claims from the IdP's
groupsclaim at SSO sign-in time, route SSO sessions through a dedicated cookie scheme so the Identity cookie'sSecurityStampValidatordoes not reject them within 30 seconds, and enforce a deployment-driven tenancy gate at sign-in so principals who have no admin membership on their deployment never receive a session.What's new
GroupClaimRoleMapperstrips the configuredAdminSettings:Oidc:GroupPrefixoff eachgroupsclaim and keeps the suffix only if it matches a key inRolePermissionMapping(owner,admin,billing,cs,sales). Prefix match is case-insensitive. Multiple groups union into a role set.SsoSignInPrincipalBuilderassembles theClaimsPrincipalwritten to the SSO cookie on sign-in. BypassesSignInManager/UserManager- no local user record, noSecurityStamp, no periodic re-fetch. The IdP owns identity, session lifetime is bounded by cookie expiry and RP-initiated logout.AccessControlService.UserHasPermissionnow unions across every role claim the principal holds. If any role grants the permission, the user has it.LoginController.SsoSignInenforces a deployment-driven tenancy gate: on cloud, the principal must carry at least one role claim; on self-hosted, the email must be onAdminSettings:Admins.AddAdminUpstreamOidcfail-louds at boot when the deployment-required config is missing.src/Admin/Auth/SSO.mddocuments the deployment-driven model.🧩 Two cookie schemes, resolved by the authorization policy
The Admin app now has two auth cookies:
IdentityConstants.ApplicationSchemeSignInManager)AdminSettings:AdminsCSVAdminAuthenticationSchemes.SsoCookie(new)SsoSignInPrincipalBuilder→HttpContext.SignInAsync)Why two schemes: the Identity cookie's
SecurityStampValidatorre-fetches the user from the user store every 30 seconds and signs them out on mismatch. The SSO principal has no backing user record, so Identity's cookie would eject it. A dedicated cookie scheme with no validator wired up is the honest fit for IdP-owned sessions.How bare
[Authorize]resolves across both:The authorization
DefaultPolicylists both schemes. ASP.NET Core'sPolicyEvaluatorcallsAuthenticateAsyncon each scheme in order and setsHttpContext.Userto the first successful result, so both passwordless and SSO sessions satisfy bare[Authorize]without each controller having to list schemes. Each scheme is actually decrypted and validated - no cookie-name heuristic that would short-circuit a valid session when a stale cookie from the other scheme is on the request.FallbackPolicyis deliberately not set, so endpoints without an explicit[Authorize]stay anonymous (the login pages,/alive,/error, and the version endpoint).DefaultAuthenticateSchemeandDefaultChallengeSchemestay at Identity's application cookie (theAddIdentitydefault), so bare[Authorize]still redirects to/loginvia Identity andSignInManagerstill writes to the Identity cookie.The old SSO path required the authenticated email to also appear in the
AdminSettings:AdminsCSV allowlist (viaUserManager.FindByEmailAsync). This PR replaces that uniform check with a deployment-driven split.ClaimTypes.RoleclaimAdminSettings:AdminsRolePermissionMappingSelfHostedshort-circuit grants allAdminSettings:Oidc:GroupPrefix→GroupClaimRoleMapperAdminSettings:Oidc:GroupPrefixAdminSettings:AdminsThe passwordless email flow is unchanged and continues to use the
AdminSettings:Adminsallowlist with the 30s re-check.🤖 AI-assisted review
bitwarden-code-review:code-review-local) against baseadmin/upstream-oidc-minimalSecurityStampValidatorrejection on the original one-cookie design; addressed by the two-scheme split aboveFallbackPolicy = DefaultPolicythat would have locked out the login pages and health probes; dropped so only[Authorize]-decorated endpoints require authentication[Authorize]actions onUsersController/OrganizationsControllerwere reachable by role-less SSO principals; addressed by the deployment-driven tenancy gate above. Adding[RequirePermission]to those ungated actions remains a desirable defense-in-depth follow-up and is tracked separately