Repository navigation
[PM-44143][BEEEP] feat: add RBAC from IdP group claims #8587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: admin/upstream-oidc-minimal
Are you sure you want to change the base?
Changes from 8 commits
3531933
65cb422
f4955f9
e7d6e7d
1dfda58
4293d43
3ebcc4b
5e6af3b
9d24324
d54125f
92ff0e7
7379247
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| using Bit.Admin.Auth.Filters; | ||
| using Bit.Admin.Auth.IdentityServer; | ||
| using Bit.Admin.Auth.Models; | ||
| using Bit.Admin.Auth.Services; | ||
| using Microsoft.AspNetCore.Authentication; | ||
| using Microsoft.AspNetCore.Authorization; | ||
| using Microsoft.AspNetCore.Identity; | ||
|
|
@@ -16,19 +17,19 @@ namespace Bit.Admin.Auth.Controllers; | |
| public class LoginController : Controller | ||
| { | ||
| private readonly PasswordlessSignInManager<IdentityUser> _signInManager; | ||
| private readonly UserManager<IdentityUser> _userManager; | ||
| private readonly AdminSettings _adminSettings; | ||
| private readonly ISsoSignInPrincipalBuilder _ssoSignInPrincipalBuilder; | ||
| private readonly ILogger<LoginController> _logger; | ||
|
|
||
| public LoginController( | ||
| PasswordlessSignInManager<IdentityUser> signInManager, | ||
| UserManager<IdentityUser> userManager, | ||
| IOptions<AdminSettings> adminSettings, | ||
| ISsoSignInPrincipalBuilder ssoSignInPrincipalBuilder, | ||
| ILogger<LoginController> logger) | ||
| { | ||
| _signInManager = signInManager; | ||
| _userManager = userManager; | ||
| _adminSettings = adminSettings.Value; | ||
| _ssoSignInPrincipalBuilder = ssoSignInPrincipalBuilder; | ||
| _logger = logger; | ||
| } | ||
|
|
||
|
|
@@ -143,16 +144,6 @@ public async Task<IActionResult> SsoSignIn(string returnUrl = null, string remot | |
| return RedirectToAction("Index", new { error = 5 }); | ||
| } | ||
|
|
||
| var user = await _userManager.FindByEmailAsync(email); | ||
| if (user == null) | ||
| { | ||
| // Same error code as other reject branches. Distinct codes would let any authenticated | ||
| // IdP user enumerate which emails are on the admin allowlist. | ||
| _logger.LogWarning("SSO sign-in rejected: principal not on admin allowlist."); | ||
| await HttpContext.SignOutAsync(IdentityConstants.ExternalScheme); | ||
| return RedirectToAction("Index", new { error = 5 }); | ||
| } | ||
|
|
||
| // Set IsPersistent = false, so SSO sessions die when the browser closes. | ||
| // The passwordless flow uses IsPersistent = true and survives a browser restart. | ||
| var props = new AuthenticationProperties { IsPersistent = false }; | ||
|
|
@@ -171,7 +162,35 @@ public async Task<IActionResult> SsoSignIn(string returnUrl = null, string remot | |
| props.StoreTokens([new AuthenticationToken { Name = "id_token", Value = idToken }]); | ||
| } | ||
|
|
||
| await _signInManager.SignInWithClaimsAsync(user, props, Array.Empty<Claim>()); | ||
| // SSO signs in to its own dedicated cookie scheme with a hand-built principal. | ||
| // There's no local user record to look up and no SecurityStamp to track - the IdP owns | ||
| // identity, and session lifetime is bounded by the cookie expiry. | ||
| // A separate scheme (not the Identity application cookie) is required because | ||
| // Identity's SecurityStampValidator enforces security stamp consistency for the | ||
| // local user store. | ||
| var principal = _ssoSignInPrincipalBuilder.Build(email, external.Principal); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Details and fixThe allowlist check was removed and nothing replaced it. In cloud,
Before this change, only operators on the The comment added in 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved in 1dfda58. One leftover: the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
|
||
| // Tenancy gates. Each configured signal must admit the user. See src/Admin/Auth/SSO.md. | ||
| var allowlistConfigured = !string.IsNullOrWhiteSpace(_adminSettings.Admins); | ||
| var groupMappingConfigured = !string.IsNullOrWhiteSpace(_adminSettings.Oidc.GroupPrefix); | ||
|
|
||
| if (allowlistConfigured && !IsOnAdminAllowlist(email)) | ||
| { | ||
| _logger.LogWarning( | ||
| "SSO sign-in rejected: email not on AdminSettings:Admins allowlist."); | ||
| await HttpContext.SignOutAsync(IdentityConstants.ExternalScheme); | ||
| return RedirectToAction("Index", new { error = 5 }); | ||
| } | ||
|
|
||
| if (groupMappingConfigured && !principal.FindAll(ClaimTypes.Role).Any()) | ||
| { | ||
| _logger.LogWarning( | ||
| "SSO sign-in rejected: authenticated principal has no mapped admin role."); | ||
| await HttpContext.SignOutAsync(IdentityConstants.ExternalScheme); | ||
| return RedirectToAction("Index", new { error = 5 }); | ||
| } | ||
|
|
||
| await HttpContext.SignInAsync(AdminAuthenticationSchemes.SsoCookie, principal, props); | ||
| await HttpContext.SignOutAsync(IdentityConstants.ExternalScheme); | ||
| _logger.LogInformation("SSO sign-in succeeded."); | ||
|
|
||
|
|
@@ -187,12 +206,16 @@ public async Task<IActionResult> SsoSignIn(string returnUrl = null, string remot | |
| [ValidateAntiForgeryToken] | ||
| public async Task<IActionResult> Logout() | ||
| { | ||
| // Presence of `id_token` on the app cookie means this is an SSO session (the | ||
| // passwordless flow never stores tokens), so it's both the "was SSO" signal and the | ||
| // value we need for id_token_hint. Read before SignOutAsync clears the cookie. | ||
| var idToken = await HttpContext.GetTokenAsync("id_token"); | ||
| // Presence of `id_token` on the SSO cookie is both (a) the marker that this is an SSO | ||
| // session (the passwordless flow writes to a different cookie and stores no tokens) | ||
| // and (b) the value we need as id_token_hint on RP-initiated logout. Read before the | ||
| // sign-outs below clear the cookie. | ||
| var idToken = await HttpContext.GetTokenAsync(AdminAuthenticationSchemes.SsoCookie, "id_token"); | ||
|
|
||
| // Sign out of both cookie schemes. SignOutAsync on a scheme the user isn't currently | ||
| // signed in to is a no-op, so this covers both passwordless and SSO sessions cleanly. | ||
| await _signInManager.SignOutAsync(); | ||
| await HttpContext.SignOutAsync(AdminAuthenticationSchemes.SsoCookie); | ||
|
|
||
| var loggedOutRedirect = Url.Action(nameof(Index), "Login", new { success = 1 }); | ||
|
|
||
|
|
@@ -209,6 +232,23 @@ public async Task<IActionResult> Logout() | |
| return Redirect(loggedOutRedirect); | ||
| } | ||
|
|
||
| // AdminSettings:Admins is a comma-separated list where each entry is either "email" or | ||
| // "email:stamp" (the optional stamp is used by the passwordless path's user store). | ||
| // Match the email prefix of each entry, case-insensitive. | ||
| private bool IsOnAdminAllowlist(string email) | ||
| { | ||
| var csv = _adminSettings.Admins; | ||
| if (string.IsNullOrWhiteSpace(csv)) | ||
| { | ||
| return false; | ||
| } | ||
| var normalized = email.Trim().ToLowerInvariant(); | ||
| return csv.ToLowerInvariant() | ||
| .Split(',', StringSplitOptions.RemoveEmptyEntries) | ||
| .Select(entry => entry.Split(':', 2)[0].Trim()) | ||
| .Any(e => e == normalized); | ||
| } | ||
|
|
||
| private bool IsEmailVerified(ClaimsPrincipal principal) | ||
| { | ||
| var claim = principal.FindFirst("email_verified")?.Value; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,17 @@ | ||
| namespace Bit.Admin.Auth.IdentityServer; | ||
| namespace Bit.Admin.Auth.IdentityServer; | ||
|
|
||
| public static class AdminAuthenticationSchemes | ||
| { | ||
| // Registered scheme name for the upstream federated OIDC handler. Used by controller | ||
| // actions to Challenge/SignOut the correct handler when the app has multiple auth | ||
| // schemes registered (this one plus ASP.NET Identity's application/external cookies). | ||
| public const string UpstreamOidc = "sso"; | ||
|
|
||
| // Application cookie issued at the end of a successful SSO sign-in. Separate from the | ||
| // Identity application cookie by design: the SSO path has no local user store and no | ||
| // SecurityStamp to track - the IdP owns identity, so routing it through the Identity | ||
| // cookie (whose SecurityStampValidator does a 30s re-fetch against the user store) would | ||
| // sign the session out within 30 seconds. Startup's authorization DefaultPolicy lists | ||
| // both this scheme and IdentityConstants.ApplicationScheme so [Authorize] accepts either. | ||
| public const string SsoCookie = "sso-cookie"; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,140 @@ | ||
| # Admin Portal SSO sign-in | ||
|
|
||
| How the Admin Portal admits users signing in through upstream OIDC. Audience: engineers modifying | ||
| `LoginController`, `SsoSignInPrincipalBuilder`, `GroupClaimRoleMapper`, or `AdminSettings:Oidc:*`; | ||
| operators provisioning Admin Portal access through an IdP. | ||
|
|
||
| ## Sign-in paths | ||
|
|
||
| The Admin Portal supports two sign-in paths. An operator can enable either or both. | ||
|
|
||
| | Path | Enabled by | Membership gate | Role claims come from | | ||
| |---|---|---|---| | ||
| | Passwordless email | `AdminSettings:EnablePasswordlessLogin = true` (default) | Email must be on `AdminSettings:Admins` | `adminSettings:role:{roleName}` CSV sub-config, matched against the authenticated email | | ||
| | Upstream OIDC SSO | `AdminSettings:Oidc:*` configured | Every configured SSO tenancy gate must admit the user (see below) | `GroupClaimRoleMapper` maps the IdP `groups` claim via `AdminSettings:Oidc:GroupPrefix` | | ||
|
|
||
| Everything below describes the SSO path. | ||
|
|
||
| ## SSO tenancy gates | ||
|
|
||
| The SSO path has two independent tenancy signals. Operators configure one or both. Every signal | ||
| that is configured must admit the user; signals that are not configured are skipped. | ||
|
|
||
| | Signal | Configured when | Admits a user when | | ||
| |---|---|---| | ||
| | `AdminSettings:Admins` (CSV allowlist) | `AdminSettings:Admins` is non-empty | The authenticated email appears in the CSV (case-insensitive, matches the email prefix of each `email` or `email:stamp` entry) | | ||
| | `AdminSettings:Oidc:GroupPrefix` (IdP group mapping) | `AdminSettings:Oidc:GroupPrefix` is non-empty | `SsoSignInPrincipalBuilder` produces a principal with at least one `ClaimTypes.Role` claim (i.e. at least one of the user's IdP groups has the configured prefix and a suffix matching a known role) | | ||
|
|
||
| `AddAdminUpstreamOidc` fails loud at boot if SSO is enabled with neither `Admins` nor `GroupPrefix` | ||
| configured, because every SSO sign-in would otherwise fail the gates. | ||
|
|
||
| ### Configuration matrix | ||
|
|
||
| The three valid SSO configurations and what they mean for sign-in and authorization: | ||
|
|
||
| | `Admins` | `GroupPrefix` | SSO sign-in admits | Role claims on the principal | Cloud authorization | Self-hosted authorization | | ||
| |---|---|---|---|---|---| | ||
| | set | unset | Users whose email is on `Admins` | None | No permissions (`AccessControlService.UserHasPermission` returns `false`) | Full access (`SelfHosted` short-circuit) | | ||
| | unset | set | Users whose IdP groups map to at least one role | The mapped roles | Union of permissions across mapped roles | Full access | | ||
| | set | set | Users whose email is on `Admins` AND whose IdP groups map to at least one role | The mapped roles | Union of permissions across mapped roles | Full access | | ||
|
|
||
| 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Details and fix
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:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved in 9d24324. SSO.md no longer mentions |
||
|
|
||
| The `GroupPrefix`-only configuration suits operators who manage admin membership through IdP | ||
| groups and want a single source of truth. | ||
|
|
||
| The combined configuration is the tightest posture — a user must satisfy both the local allowlist | ||
| and the IdP group mapping. Appropriate when the two mechanisms are managed by different teams and | ||
| both approvals are required. | ||
|
|
||
| ## SSO sign-in flow | ||
|
|
||
| 1. The user hits `GET /login/sso` (`LoginController.Sso`). The controller issues a challenge against | ||
| `AdminAuthenticationSchemes.UpstreamOidc` (the OpenID Connect handler registered by | ||
| `AddAdminUpstreamOidc`). The browser is redirected to the IdP's authorize endpoint with | ||
| `response_type=code`, `prompt=login`, `max_age=0`, and PKCE. | ||
| 2. The IdP authenticates the user and redirects back to the configured callback. The OIDC handler | ||
| validates the ID token (issuer, audience, signature, lifetime, and `auth_time` via | ||
| `OnTokenValidated`) and stages the resulting principal into `IdentityConstants.ExternalScheme`. | ||
| 3. The browser follows to `GET /login/sso-signin` (`LoginController.SsoSignIn`). The controller: | ||
| - Reads the external principal from `IdentityConstants.ExternalScheme`. | ||
| - Verifies the email claim is present and (unless `RequireEmailVerifiedClaim = false`) the | ||
| `email_verified` claim is `true`. | ||
| - Calls `SsoSignInPrincipalBuilder.Build(email, external.Principal)`, which stamps | ||
| `ClaimTypes.Name` / `ClaimTypes.Email` / `ClaimTypes.NameIdentifier` claims and asks | ||
| `GroupClaimRoleMapper` to translate the IdP's `groups` claim into `ClaimTypes.Role` claims. | ||
| - Applies each configured tenancy gate (see above). Admits only if every configured gate passes. | ||
| - Signs the principal into `AdminAuthenticationSchemes.SsoCookie` with | ||
| `IsPersistent = false` and the `id_token` stored on the auth properties (used later as | ||
| `id_token_hint` on RP-initiated logout). | ||
| - Signs out of `IdentityConstants.ExternalScheme`. | ||
|
|
||
| Any failure in steps 1-3 logs a warning and redirects to `GET /login?error=5`, which renders the | ||
| generic "SSO sign-in failed. Try again or use the email link." message. The error message is | ||
| intentionally generic: unsuccessful attempts cannot distinguish between "no such user", "wrong | ||
| group", "email not on allowlist", and "IdP returned an error." | ||
|
|
||
| ## Authentication, tenancy, and authorization | ||
|
|
||
| The SSO path keeps three concerns distinct: | ||
|
|
||
| - **Authentication** — the IdP proves the user owns the claimed email. Enforced by the OIDC handler | ||
| plus the email-verified check in `LoginController.SsoSignIn`. | ||
| - **Tenancy** — whether the authenticated user is a user of the Admin Portal. Enforced by the | ||
| configured tenancy gates in `LoginController.SsoSignIn`. | ||
| - **Authorization** — what capabilities the authenticated member has. Enforced by | ||
| `AccessControlService.UserHasPermission` against the role claims on the principal, using | ||
| `RolePermissionMapping`. | ||
|
|
||
| ## Cookie schemes | ||
|
|
||
| The Admin app registers two cookie schemes. Each sign-in path writes its own. | ||
|
|
||
| | Scheme | Set by | SecurityStampValidator | | ||
| |---|---|---| | ||
| | `IdentityConstants.ApplicationScheme` | Passwordless path (`SignInManager`) | Yes, 30s re-check against the backing `ReadOnlyEnvIdentityUserStore` | | ||
| | `AdminAuthenticationSchemes.SsoCookie` | SSO path (`HttpContext.SignInAsync` in `LoginController.SsoSignIn`) | No | | ||
|
|
||
| Bare `[Authorize]` works for either path because the authorization `DefaultPolicy` lists both | ||
| schemes. ASP.NET Core's `PolicyEvaluator` calls `AuthenticateAsync` on each scheme and sets | ||
| `HttpContext.User` to the first successful result. `DefaultAuthenticateScheme` and | ||
| `DefaultChallengeScheme` are Identity's defaults, so bare `[Authorize]` redirects to `/login` and | ||
| `SignInManager` writes to the Identity cookie. | ||
|
|
||
| `FallbackPolicy` is unset, so endpoints without `[Authorize]` stay anonymous. This keeps the login | ||
| pages, `/alive`, `/error`, and the version endpoint reachable without a cookie. | ||
|
|
||
| ## Logout | ||
|
|
||
| `LoginController.Logout` reads `id_token` from `AdminAuthenticationSchemes.SsoCookie` before signing | ||
| out, then signs out of both cookie schemes. If `id_token` was present and `OidcEnabled` is true, it | ||
| triggers RP-initiated logout against `AdminAuthenticationSchemes.UpstreamOidc` with the saved token | ||
| as `id_token_hint`, so the upstream IdP can honor the app-level Allowed Logout URLs list. | ||
|
|
||
| ## Operator configuration | ||
|
|
||
| Minimum settings for SSO. Supply at least one of `AdminSettings:Admins` or | ||
| `AdminSettings:Oidc:GroupPrefix`. | ||
|
|
||
| ```jsonc | ||
| "AdminSettings": { | ||
| // Required when choosing the CSV allowlist as a tenancy signal. Comma-separated email | ||
| // addresses. Entries may be "email" or "email:stamp". | ||
| "Admins": "admin@example.com,another@example.com", | ||
|
|
||
| "Oidc": { | ||
| "Authority": "https://idp.example.com", | ||
| "ClientId": "admin-portal", | ||
| "ClientSecret": "...", | ||
|
|
||
| // Required when choosing IdP group mapping as a tenancy signal. IdP groups must be named | ||
| // <GroupPrefix><role> where <role> is one of owner, admin, billing, cs, or sales (the keys | ||
| // of RolePermissionMapping.RolePermissions). Case is insensitive. A principal in multiple | ||
| // matching groups receives the union of their permissions. | ||
| "GroupPrefix": "us-prd-cloud-adminportal-" | ||
| } | ||
| } | ||
| ``` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| using System.Security.Claims; | ||
| using Bit.Admin.Utilities; | ||
|
|
||
| namespace Bit.Admin.Auth.Services; | ||
|
|
||
| // Translates IdP group claims into Admin Portal role names. | ||
| // | ||
| // An IdP ships group memberships in the `groups` claim of the ID token. By | ||
| // convention each Admin Portal group is named `<GroupPrefix><role>` (e.g. | ||
| // `us-prd-cloud-adminportal-admin`); the mapper strips the prefix and keeps the suffix only if | ||
| // it matches one of the role names in RolePermissionMapping. The suffix match is | ||
| // case-insensitive. A principal in multiple matched groups gets the union of their roles. | ||
| // | ||
| // Decoupled from the ClaimsPrincipal source so the SSO sign-in controller and the AccessControl | ||
| // pipeline can share the same mapper and unit tests do not need to spin up ASP.NET. | ||
| public interface IGroupClaimRoleMapper | ||
| { | ||
| IReadOnlyCollection<string> MapRoles(ClaimsPrincipal principal); | ||
| } | ||
|
|
||
| public class GroupClaimRoleMapper : IGroupClaimRoleMapper | ||
| { | ||
| // The "groups" claim is an OIDC convention currently supported by all major IdPs. | ||
| public const string GroupsClaimType = "groups"; | ||
|
|
||
| private readonly string? _groupPrefix; | ||
|
|
||
| public GroupClaimRoleMapper(AdminSettings adminSettings) | ||
| { | ||
| // Tolerant of an unset prefix so the type can be registered unconditionally. The SSO path | ||
| // is the only caller; when OIDC is enabled, AddAdminUpstreamOidc fail-louds on an empty | ||
| // prefix at startup, so a null here means OIDC is off and the mapper will never be hit. | ||
| _groupPrefix = adminSettings?.Oidc?.GroupPrefix; | ||
| } | ||
|
|
||
| public IReadOnlyCollection<string> MapRoles(ClaimsPrincipal principal) | ||
| { | ||
| if (principal is null || string.IsNullOrWhiteSpace(_groupPrefix)) | ||
| { | ||
| return Array.Empty<string>(); | ||
| } | ||
|
|
||
| var roles = new HashSet<string>(StringComparer.OrdinalIgnoreCase); | ||
| foreach (var claim in principal.FindAll(GroupsClaimType)) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(claim.Value)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| if (!claim.Value.StartsWith(_groupPrefix, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| var suffix = claim.Value.Substring(_groupPrefix.Length).Trim().ToLowerInvariant(); | ||
| if (RolePermissionMapping.RolePermissions.ContainsKey(suffix)) | ||
| { | ||
| roles.Add(suffix); | ||
| } | ||
| } | ||
|
|
||
| return roles; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
♻️ 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.SsoSignInchecksIsOnAdminAllowlist(email)when_globalSettings.SelfHosted, andAddAdminUpstreamOidcthrows on a self-hosted deployment whenAdminsis 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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
92ff0e7 and 7379247 fix most of this. The
Admins,GroupPrefix, andGroupClaimRoleMapperconstructor comments now describe the deployment-driven split. Two test comments are still stale:test/Admin.Test/Auth/Services/GroupClaimRoleMapperTests.cs:114-116(MapRoles_ReturnsEmpty_WhenPrefixUnset) says a null prefix only happens "in the OIDC-off topology". On self-hosted SSO, a null prefix is now a supported configuration.test/Admin.Test/Auth/Services/SsoSignInPrincipalBuilderTests.cs:54-56(Build_StampsNoRoleClaims_WhenNoGroupsMap) says a user with no mapped groups "still gets a valid principal". On cloud,LoginController.SsoSignInnow rejects that principal before sign-in. The builder still returns the principal, but the controller does not sign it in.These are minor and do not block merge.