From 353193315b3da7092d94ad787fa94ab73d089d68 Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Tue, 13 Oct 2026 15:44:19 -0400 Subject: [PATCH 01/12] Add RBAC from IdP group claims to Admin Portal 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. --- src/Admin/AdminSettings.cs | 15 +- src/Admin/Auth/Controllers/LoginController.cs | 24 ++- .../Auth/Services/GroupClaimRoleMapper.cs | 65 ++++++++ .../Services/SsoSignInPrincipalBuilder.cs | 49 ++++++ .../ServiceCollectionExtensions.cs | 9 ++ src/Admin/Services/AccessControlService.cs | 27 +++- src/Admin/Startup.cs | 3 + .../Auth/Controllers/LoginControllerTests.cs | 99 +++++++----- .../Services/GroupClaimRoleMapperTests.cs | 152 ++++++++++++++++++ .../SsoSignInPrincipalBuilderTests.cs | 79 +++++++++ .../ServiceCollectionExtensionsTests.cs | 26 +++ .../Services/AccessControlServiceTests.cs | 90 +++++++++++ .../RequirePermissionAttributeTests.cs | 85 ++++++++++ 13 files changed, 657 insertions(+), 66 deletions(-) create mode 100644 src/Admin/Auth/Services/GroupClaimRoleMapper.cs create mode 100644 src/Admin/Auth/Services/SsoSignInPrincipalBuilder.cs create mode 100644 test/Admin.Test/Auth/Services/GroupClaimRoleMapperTests.cs create mode 100644 test/Admin.Test/Auth/Services/SsoSignInPrincipalBuilderTests.cs create mode 100644 test/Admin.Test/Services/AccessControlServiceTests.cs create mode 100644 test/Admin.Test/Utilities/RequirePermissionAttributeTests.cs diff --git a/src/Admin/AdminSettings.cs b/src/Admin/AdminSettings.cs index c055dcb1dc74..733172471790 100644 --- a/src/Admin/AdminSettings.cs +++ b/src/Admin/AdminSettings.cs @@ -5,9 +5,10 @@ namespace Bit.Admin; public class AdminSettings { - // Comma-separated allowlist of email addresses permitted to sign in to the Admin Portal. - // Authorization gate for both passwordless and SSO paths - authentication proves identity, - // this list decides who's an admin. + // 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. public virtual string Admins { get; set; } // Retention window for soft-deleted items before the background cleanup job hard-deletes them. @@ -74,5 +75,13 @@ public class OidcSettings // against IdPs that permit self-registration with unverified email addresses. Set to // false only for IdPs that don't emit the claim and are operator-controlled. public bool RequireEmailVerifiedClaim { get; set; } = true; + + // Prefix the IdP uses on Admin Portal group names (e.g. `us-prd-cloud-adminportal-`). + // The suffix after the prefix is matched case-insensitively against the known role names + // in RolePermissionMapping (owner, admin, billing, cs, sales). A group that does not + // start with this prefix, or whose suffix is not a known role, is ignored. A principal + // with no mapped groups still signs in, but holds no role claims and therefore no + // permissions. + public string GroupPrefix { get; set; } } } diff --git a/src/Admin/Auth/Controllers/LoginController.cs b/src/Admin/Auth/Controllers/LoginController.cs index 8876a2bb0547..b85a91da881e 100644 --- a/src/Admin/Auth/Controllers/LoginController.cs +++ b/src/Admin/Auth/Controllers/LoginController.cs @@ -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 _signInManager; - private readonly UserManager _userManager; private readonly AdminSettings _adminSettings; + private readonly ISsoSignInPrincipalBuilder _ssoSignInPrincipalBuilder; private readonly ILogger _logger; public LoginController( PasswordlessSignInManager signInManager, - UserManager userManager, IOptions adminSettings, + ISsoSignInPrincipalBuilder ssoSignInPrincipalBuilder, ILogger logger) { _signInManager = signInManager; - _userManager = userManager; _adminSettings = adminSettings.Value; + _ssoSignInPrincipalBuilder = ssoSignInPrincipalBuilder; _logger = logger; } @@ -143,16 +144,6 @@ public async Task 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,12 @@ public async Task SsoSignIn(string returnUrl = null, string remot props.StoreTokens([new AuthenticationToken { Name = "id_token", Value = idToken }]); } - await _signInManager.SignInWithClaimsAsync(user, props, Array.Empty()); + // SSO signs in directly to the application cookie with a hand-built principal rather + // than routing through SignInManager / UserManager. 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. + var principal = _ssoSignInPrincipalBuilder.Build(email, external.Principal); + await HttpContext.SignInAsync(IdentityConstants.ApplicationScheme, principal, props); await HttpContext.SignOutAsync(IdentityConstants.ExternalScheme); _logger.LogInformation("SSO sign-in succeeded."); diff --git a/src/Admin/Auth/Services/GroupClaimRoleMapper.cs b/src/Admin/Auth/Services/GroupClaimRoleMapper.cs new file mode 100644 index 000000000000..d8d09b055205 --- /dev/null +++ b/src/Admin/Auth/Services/GroupClaimRoleMapper.cs @@ -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 `` (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 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 MapRoles(ClaimsPrincipal principal) + { + if (principal is null || string.IsNullOrWhiteSpace(_groupPrefix)) + { + return Array.Empty(); + } + + var roles = new HashSet(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; + } +} diff --git a/src/Admin/Auth/Services/SsoSignInPrincipalBuilder.cs b/src/Admin/Auth/Services/SsoSignInPrincipalBuilder.cs new file mode 100644 index 000000000000..26c824ddb84f --- /dev/null +++ b/src/Admin/Auth/Services/SsoSignInPrincipalBuilder.cs @@ -0,0 +1,49 @@ +// FIXME: Update this file to be null safe and then delete the line below +#nullable disable + +using System.Security.Claims; +using Microsoft.AspNetCore.Identity; + +namespace Bit.Admin.Auth.Services; + +// Assembles the ClaimsPrincipal that gets written to the Admin Portal application cookie on +// SSO sign-in. The caller hands over the verified external principal and the email, and gets back a +// principal ready for HttpContext.SignInAsync. +// +// Deliberately bypasses SignInManager / UserManager so the SSO path does not need a backing +// user store entry and does not stamp a SecurityStamp claim. Without the stamp claim, the +// SecurityStampValidator skips its periodic re-fetch, which is what we want for a session +// whose source of truth is the IdP, not local config. +public interface ISsoSignInPrincipalBuilder +{ + ClaimsPrincipal Build(string email, ClaimsPrincipal externalPrincipal); +} + +public class SsoSignInPrincipalBuilder : ISsoSignInPrincipalBuilder +{ + private readonly IGroupClaimRoleMapper _groupClaimRoleMapper; + + public SsoSignInPrincipalBuilder(IGroupClaimRoleMapper groupClaimRoleMapper) + { + _groupClaimRoleMapper = groupClaimRoleMapper; + } + + public ClaimsPrincipal Build(string email, ClaimsPrincipal externalPrincipal) + { + // AuthenticationType is required for ClaimsIdentity.IsAuthenticated to return true. + // Using the application scheme name is conventional and makes the identity recognizable + // in middleware traces. + var identity = new ClaimsIdentity(IdentityConstants.ApplicationScheme); + + identity.AddClaim(new Claim(ClaimTypes.Name, email)); + identity.AddClaim(new Claim(ClaimTypes.Email, email)); + identity.AddClaim(new Claim(ClaimTypes.NameIdentifier, email)); + + foreach (var role in _groupClaimRoleMapper.MapRoles(externalPrincipal)) + { + identity.AddClaim(new Claim(ClaimTypes.Role, role)); + } + + return new ClaimsPrincipal(identity); + } +} diff --git a/src/Admin/IdentityServer/ServiceCollectionExtensions.cs b/src/Admin/IdentityServer/ServiceCollectionExtensions.cs index 3d2526d4829d..d056d65fd6f0 100644 --- a/src/Admin/IdentityServer/ServiceCollectionExtensions.cs +++ b/src/Admin/IdentityServer/ServiceCollectionExtensions.cs @@ -85,6 +85,15 @@ public static IServiceCollection AddAdminUpstreamOidc( "AdminSettings:Oidc:EmailClaimType must not be null or empty when SSO is enabled."); } + // Fail-loud when the operator enables SSO but never set the group prefix. Without it + // the groups-to-roles mapper would match nothing and every SSO sign-in would be rejected + // as "no mapped group" - better to refuse to boot than to silently lock everyone out. + if (string.IsNullOrWhiteSpace(oidc.GroupPrefix)) + { + throw new InvalidOperationException( + "AdminSettings:Oidc:GroupPrefix must not be null or empty when SSO is enabled."); + } + services.AddAuthentication() .AddOpenIdConnect(AdminAuthenticationSchemes.UpstreamOidc, oidc.DisplayName, options => { diff --git a/src/Admin/Services/AccessControlService.cs b/src/Admin/Services/AccessControlService.cs index f512ec7494ca..b53de3a62c4a 100644 --- a/src/Admin/Services/AccessControlService.cs +++ b/src/Admin/Services/AccessControlService.cs @@ -31,13 +31,19 @@ public bool UserHasPermission(Permission permission) return true; } - var userRole = GetUserRoleFromClaim(); - if (string.IsNullOrEmpty(userRole) || !RolePermissionMapping.RolePermissions.TryGetValue(userRole, out var rolePermissions)) + // A principal may hold multiple role claims when the IdP groups-to-roles mapper stamps + // more than one role on an SSO sign-in. Treat permissions as the union across every + // role the user holds: if any role grants the permission, the user has it. + foreach (var role in GetUserRolesFromClaims()) { - return false; + if (RolePermissionMapping.RolePermissions.TryGetValue(role, out var rolePermissions) && + rolePermissions.Contains(permission)) + { + return true; + } } - return rolePermissions.Contains(permission); + return false; } public string GetUserRole(string userEmail) @@ -61,9 +67,16 @@ public string GetUserRole(string userEmail) return userRole.Key.ToLowerInvariant(); } - private string GetUserRoleFromClaim() + private IEnumerable GetUserRolesFromClaims() { - return _httpContextAccessor.HttpContext?.User?.Claims? - .FirstOrDefault(c => c.Type == ClaimTypes.Role)?.Value; + var claims = _httpContextAccessor.HttpContext?.User?.Claims; + if (claims == null) + { + return []; + } + + return claims + .Where(c => c.Type == ClaimTypes.Role && !string.IsNullOrEmpty(c.Value)) + .Select(c => c.Value.ToLowerInvariant()); } } diff --git a/src/Admin/Startup.cs b/src/Admin/Startup.cs index 1eb602b96911..0a4637a02421 100644 --- a/src/Admin/Startup.cs +++ b/src/Admin/Startup.cs @@ -8,6 +8,7 @@ using Stripe; using Microsoft.AspNetCore.Mvc.Razor; using Microsoft.Extensions.DependencyInjection.Extensions; +using Bit.Admin.Auth.Services; using Bit.Admin.Controllers; using Bit.Admin.Services; using Bit.Core.Billing.Extensions; @@ -95,6 +96,8 @@ public void ConfigureServices(IServiceCollection services) services.AddBaseServices(globalSettings); services.AddDefaultServices(globalSettings); services.AddScoped(); + services.AddSingleton(_ => new GroupClaimRoleMapper(adminSettings)); + services.AddSingleton(); services.AddDistributedCache(globalSettings); services.AddBillingOperations(); services.AddHttpClient(); diff --git a/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs b/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs index e6f48625fc80..dfb3debf05a1 100644 --- a/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs +++ b/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs @@ -3,6 +3,7 @@ using Bit.Admin.Auth.Controllers; using Bit.Admin.Auth.IdentityServer; using Bit.Admin.Auth.Models; +using Bit.Admin.Auth.Services; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; @@ -150,7 +151,7 @@ private static void StubExternalPrincipalWithTokens( [Fact] public async Task SsoSignIn_RedirectsError5_WhenExternalAuthFails() { - var controller = BuildController(oidcEnabled: true, out _, out _); + var controller = BuildController(oidcEnabled: true, out _); var authService = AttachAuthenticationServices(controller); authService.AuthenticateAsync(Arg.Any(), IdentityConstants.ExternalScheme) .Returns(AuthenticateResult.NoResult()); @@ -164,7 +165,7 @@ public async Task SsoSignIn_RedirectsError5_WhenExternalAuthFails() [Fact] public async Task SsoSignIn_RedirectsError5_WhenEmailClaimMissing() { - var controller = BuildController(oidcEnabled: true, out _, out _); + var controller = BuildController(oidcEnabled: true, out _); var authService = AttachAuthenticationServices(controller); StubExternalPrincipal(authService, new Claim("email_verified", "true")); @@ -179,7 +180,7 @@ await authService.Received(1).SignOutAsync( [Fact] public async Task SsoSignIn_RedirectsError5_WhenEmailVerifiedIsFalse() { - var controller = BuildController(oidcEnabled: true, out _, out _); + var controller = BuildController(oidcEnabled: true, out _); var authService = AttachAuthenticationServices(controller); StubExternalPrincipal(authService, new Claim("email", "you@example.com"), @@ -194,7 +195,7 @@ public async Task SsoSignIn_RedirectsError5_WhenEmailVerifiedIsFalse() [Fact] public async Task SsoSignIn_RedirectsError5_WhenEmailVerifiedMissingAndRequired() { - var controller = BuildController(oidcEnabled: true, out _, out _); + var controller = BuildController(oidcEnabled: true, out _); var authService = AttachAuthenticationServices(controller); StubExternalPrincipal(authService, new Claim("email", "you@example.com")); @@ -210,18 +211,21 @@ public async Task SsoSignIn_AcceptsSignIn_WhenEmailVerifiedMissingAndNotRequired // Verifies the security-weakening branch of IsEmailVerified: with // RequireEmailVerifiedClaim=false, an absent email_verified claim is allowed. // This is the only path where an operator flag reduces posture; pin the contract. - var controller = BuildController(oidcEnabled: true, out var signInManager, out var userManager, + var controller = BuildController(oidcEnabled: true, out var builder, requireEmailVerified: false); var authService = AttachAuthenticationServices(controller); - var user = new IdentityUser { Email = "you@example.com" }; StubExternalPrincipal(authService, new Claim("email", "you@example.com")); - userManager.FindByEmailAsync("you@example.com").Returns(user); + var builtPrincipal = new ClaimsPrincipal(new ClaimsIdentity("test")); + builder.Build("you@example.com", Arg.Any()).Returns(builtPrincipal); var result = await controller.SsoSignIn(); Assert.IsType(result); - await signInManager.Received(1).SignInWithClaimsAsync( - user, Arg.Any(), Arg.Any>()); + await authService.Received(1).SignInAsync( + Arg.Any(), + IdentityConstants.ApplicationScheme, + builtPrincipal, + Arg.Any()); } [Fact] @@ -229,7 +233,7 @@ public async Task SsoSignIn_RejectsSignIn_WhenEmailVerifiedFalse_EvenWhenNotRequ { // Fail-secure: RequireEmailVerifiedClaim=false relaxes only the *absent* case. // A claim present with value "false" (or any non-true) still rejects. - var controller = BuildController(oidcEnabled: true, out var signInManager, out _, + var controller = BuildController(oidcEnabled: true, out _, requireEmailVerified: false); var authService = AttachAuthenticationServices(controller); StubExternalPrincipal(authService, @@ -240,26 +244,34 @@ public async Task SsoSignIn_RejectsSignIn_WhenEmailVerifiedFalse_EvenWhenNotRequ var redirect = Assert.IsType(result); Assert.Equal(5, redirect.RouteValues!["error"]); - await signInManager.DidNotReceive().SignInWithClaimsAsync( - Arg.Any(), Arg.Any(), Arg.Any>()); + await authService.DidNotReceive().SignInAsync( + Arg.Any(), + IdentityConstants.ApplicationScheme, + Arg.Any(), + Arg.Any()); } [Fact] - public async Task SsoSignIn_RedirectsError5_WhenEmailNotOnAllowlist() + public async Task SsoSignIn_SignsInToApplicationCookie_WithBuiltPrincipal() { - var controller = BuildController(oidcEnabled: true, out var signInManager, out var userManager); + // Controller hands the external principal + email to the builder and signs in the + // returned principal directly to the application cookie. No SignInManager, no + // IdentityUser. + var controller = BuildController(oidcEnabled: true, out var builder); var authService = AttachAuthenticationServices(controller); StubExternalPrincipal(authService, - new Claim("email", "stranger@example.com"), + new Claim("email", "you@example.com"), new Claim("email_verified", "true")); - userManager.FindByEmailAsync("stranger@example.com").Returns((IdentityUser)null); + var builtPrincipal = new ClaimsPrincipal(new ClaimsIdentity("test")); + builder.Build("you@example.com", Arg.Any()).Returns(builtPrincipal); - var result = await controller.SsoSignIn(); + await controller.SsoSignIn(); - var redirect = Assert.IsType(result); - Assert.Equal(5, redirect.RouteValues!["error"]); - await signInManager.DidNotReceive().SignInWithClaimsAsync( - Arg.Any(), Arg.Any(), Arg.Any>()); + await authService.Received(1).SignInAsync( + Arg.Any(), + IdentityConstants.ApplicationScheme, + builtPrincipal, + Arg.Any()); } [Theory] @@ -269,7 +281,7 @@ await signInManager.DidNotReceive().SignInWithClaimsAsync( [InlineData("")] public async Task SsoSignIn_RedirectsError5_WhenEmailVerifiedIsNonBoolean(string value) { - var controller = BuildController(oidcEnabled: true, out _, out _); + var controller = BuildController(oidcEnabled: true, out _); var authService = AttachAuthenticationServices(controller); StubExternalPrincipal(authService, new Claim("email", "you@example.com"), @@ -287,37 +299,38 @@ public async Task SsoSignIn_StoresIdTokenOnAppCookie_OnSuccess() // Presence of id_token on the app cookie is both (a) the marker that this is an SSO // session, so Logout will trigger RP-initiated logout, and (b) the value used as // id_token_hint on the end-session request. Assert it lands on the sign-in props. - var controller = BuildController(oidcEnabled: true, out var signInManager, out var userManager); + var controller = BuildController(oidcEnabled: true, out var builder); var authService = AttachAuthenticationServices(controller); - var user = new IdentityUser { Email = "you@example.com" }; StubExternalPrincipalWithTokens(authService, tokens: [new AuthenticationToken { Name = "id_token", Value = "the-id-token" }], claims: [ new Claim("email", "you@example.com"), new Claim("email_verified", "true"), ]); - userManager.FindByEmailAsync("you@example.com").Returns(user); + builder.Build(Arg.Any(), Arg.Any()) + .Returns(new ClaimsPrincipal(new ClaimsIdentity("test"))); var result = await controller.SsoSignIn(); Assert.IsType(result); - await signInManager.Received(1).SignInWithClaimsAsync( - user, - Arg.Is(p => p.GetTokenValue("id_token") == "the-id-token"), - Arg.Any>()); + await authService.Received(1).SignInAsync( + Arg.Any(), + IdentityConstants.ApplicationScheme, + Arg.Any(), + Arg.Is(p => p.GetTokenValue("id_token") == "the-id-token")); } [Fact] public async Task SsoSignIn_RedirectsToLocalReturnUrl_WhenLocal() { - var controller = BuildController(oidcEnabled: true, out _, out var userManager); + var controller = BuildController(oidcEnabled: true, out var builder); var authService = AttachAuthenticationServices(controller); controller.Url.IsLocalUrl("/dashboard").Returns(true); - var user = new IdentityUser { Email = "you@example.com" }; StubExternalPrincipal(authService, new Claim("email", "you@example.com"), new Claim("email_verified", "true")); - userManager.FindByEmailAsync("you@example.com").Returns(user); + builder.Build(Arg.Any(), Arg.Any()) + .Returns(new ClaimsPrincipal(new ClaimsIdentity("test"))); var result = await controller.SsoSignIn(returnUrl: "/dashboard"); @@ -341,14 +354,14 @@ public void Sso_ReturnsChallengeResultForUpstreamOidc_WhenEnabled() [Fact] public async Task SsoSignIn_RedirectsToHome_WhenReturnUrlNotLocal() { - var controller = BuildController(oidcEnabled: true, out _, out var userManager); + var controller = BuildController(oidcEnabled: true, out var builder); var authService = AttachAuthenticationServices(controller); controller.Url.IsLocalUrl("https://evil.example.com").Returns(false); - var user = new IdentityUser { Email = "you@example.com" }; StubExternalPrincipal(authService, new Claim("email", "you@example.com"), new Claim("email_verified", "true")); - userManager.FindByEmailAsync("you@example.com").Returns(user); + builder.Build(Arg.Any(), Arg.Any()) + .Returns(new ClaimsPrincipal(new ClaimsIdentity("test"))); var result = await controller.SsoSignIn(returnUrl: "https://evil.example.com"); @@ -358,12 +371,11 @@ public async Task SsoSignIn_RedirectsToHome_WhenReturnUrlNotLocal() } private static LoginController BuildController(bool oidcEnabled) => - BuildController(oidcEnabled, out _, out _); + BuildController(oidcEnabled, out _); private static LoginController BuildController( bool oidcEnabled, - out PasswordlessSignInManager signInManager, - out UserManager userManager, + out ISsoSignInPrincipalBuilder ssoSignInPrincipalBuilder, bool requireEmailVerified = true, bool enablePasswordlessLogin = true) { @@ -376,13 +388,14 @@ private static LoginController BuildController( Authority = "https://idp.example.com", ClientId = "admin-portal", ClientSecret = "supersecret", + GroupPrefix = "us-prd-cloud-adminportal-", RequireEmailVerifiedClaim = requireEmailVerified, } : null, }; var userStore = Substitute.For>(); - userManager = Substitute.For>( + var userManager = Substitute.For>( userStore, Substitute.For>(), Substitute.For>(), @@ -393,7 +406,7 @@ private static LoginController BuildController( Substitute.For(), Substitute.For>>()); - signInManager = Substitute.For>( + var signInManager = Substitute.For>( userManager, Substitute.For(), Substitute.For>(), @@ -403,7 +416,9 @@ private static LoginController BuildController( Substitute.For>(), Substitute.For()); - return new LoginController(signInManager, userManager, Options.Create(settings), - Substitute.For>()); + ssoSignInPrincipalBuilder = Substitute.For(); + + return new LoginController(signInManager, Options.Create(settings), + ssoSignInPrincipalBuilder, Substitute.For>()); } } diff --git a/test/Admin.Test/Auth/Services/GroupClaimRoleMapperTests.cs b/test/Admin.Test/Auth/Services/GroupClaimRoleMapperTests.cs new file mode 100644 index 000000000000..784edad04a99 --- /dev/null +++ b/test/Admin.Test/Auth/Services/GroupClaimRoleMapperTests.cs @@ -0,0 +1,152 @@ +using System.Security.Claims; +using Bit.Admin; +using Bit.Admin.Auth.Services; + +namespace Admin.Test.Auth.Services; + +public class GroupClaimRoleMapperTests +{ + private const string Prefix = "us-prd-cloud-adminportal-"; + + [Fact] + public void MapRoles_StripsPrefix_AndReturnsKnownRole() + { + var mapper = Build(Prefix); + var principal = PrincipalWithGroups($"{Prefix}admin"); + + var roles = mapper.MapRoles(principal); + + Assert.Equal(new[] { "admin" }, roles); + } + + [Fact] + public void MapRoles_IsCaseInsensitiveOnPrefix() + { + var mapper = Build(Prefix); + var principal = PrincipalWithGroups("US-PRD-CLOUD-ADMINPORTAL-admin"); + + var roles = mapper.MapRoles(principal); + + Assert.Equal(new[] { "admin" }, roles); + } + + [Fact] + public void MapRoles_IsCaseInsensitiveOnRoleName() + { + var mapper = Build(Prefix); + var principal = PrincipalWithGroups($"{Prefix}Admin"); + + var roles = mapper.MapRoles(principal); + + Assert.Equal(new[] { "admin" }, roles); + } + + [Fact] + public void MapRoles_SkipsGroupsThatDoNotStartWithPrefix() + { + var mapper = Build(Prefix); + var principal = PrincipalWithGroups( + "unrelated-group", + "eu-prd-cloud-adminportal-admin", + $"{Prefix}cs"); + + var roles = mapper.MapRoles(principal); + + Assert.Equal(new[] { "cs" }, roles); + } + + [Fact] + public void MapRoles_SkipsSuffixesThatAreNotKnownRoles() + { + // A group that happens to match the prefix but has a role name we do not recognize + // (because someone added a group in Okta that the app does not grant any permissions + // for) is silently dropped. No permissions is the safe default. + var mapper = Build(Prefix); + var principal = PrincipalWithGroups($"{Prefix}superuser"); + + var roles = mapper.MapRoles(principal); + + Assert.Empty(roles); + } + + [Fact] + public void MapRoles_DeduplicatesRepeatedRoleGroups() + { + var mapper = Build(Prefix); + var principal = PrincipalWithGroups($"{Prefix}admin", $"{Prefix}admin"); + + var roles = mapper.MapRoles(principal); + + Assert.Equal(new[] { "admin" }, roles); + } + + [Fact] + public void MapRoles_ReturnsUnionAcrossMultipleGroups() + { + var mapper = Build(Prefix); + var principal = PrincipalWithGroups( + $"{Prefix}admin", + $"{Prefix}billing", + $"{Prefix}cs"); + + var roles = mapper.MapRoles(principal); + + Assert.Equal(new HashSet { "admin", "billing", "cs" }, new HashSet(roles)); + } + + [Fact] + public void MapRoles_ReturnsEmpty_WhenPrincipalHasNoGroupsClaim() + { + var mapper = Build(Prefix); + var principal = new ClaimsPrincipal(new ClaimsIdentity(new[] + { + new Claim("email", "you@example.com"), + })); + + var roles = mapper.MapRoles(principal); + + Assert.Empty(roles); + } + + [Fact] + public void MapRoles_ReturnsEmpty_WhenPrefixUnset() + { + // Defensive: if the type is somehow resolved while OIDC is disabled, it should return + // empty rather than throw. AddAdminUpstreamOidc fail-louds on an empty prefix when + // OIDC is enabled, so this path is only hit in the OIDC-off topology. + var mapper = Build(groupPrefix: null); + var principal = PrincipalWithGroups("us-prd-cloud-adminportal-admin"); + + var roles = mapper.MapRoles(principal); + + Assert.Empty(roles); + } + + [Fact] + public void MapRoles_ReturnsEmpty_WhenPrincipalIsNull() + { + var mapper = Build(Prefix); + + var roles = mapper.MapRoles(null); + + Assert.Empty(roles); + } + + private static GroupClaimRoleMapper Build(string groupPrefix) + { + var settings = new AdminSettings + { + Oidc = new AdminSettings.OidcSettings + { + GroupPrefix = groupPrefix, + }, + }; + return new GroupClaimRoleMapper(settings); + } + + private static ClaimsPrincipal PrincipalWithGroups(params string[] groups) + { + var claims = groups.Select(g => new Claim(GroupClaimRoleMapper.GroupsClaimType, g)); + return new ClaimsPrincipal(new ClaimsIdentity(claims)); + } +} diff --git a/test/Admin.Test/Auth/Services/SsoSignInPrincipalBuilderTests.cs b/test/Admin.Test/Auth/Services/SsoSignInPrincipalBuilderTests.cs new file mode 100644 index 000000000000..c5c972f07ece --- /dev/null +++ b/test/Admin.Test/Auth/Services/SsoSignInPrincipalBuilderTests.cs @@ -0,0 +1,79 @@ +using System.Security.Claims; +using Bit.Admin; +using Bit.Admin.Auth.Services; +using Microsoft.AspNetCore.Identity; + +namespace Admin.Test.Auth.Services; + +public class SsoSignInPrincipalBuilderTests +{ + [Fact] + public void Build_PrincipalIsAuthenticated_WithApplicationSchemeAuthenticationType() + { + // IsAuthenticated depends on AuthenticationType being non-null. The application cookie + // middleware expects any principal written via HttpContext.SignInAsync to resolve as + // authenticated. + var builder = BuildBuilder(); + + var principal = builder.Build("you@example.com", PrincipalWithGroups()); + + Assert.True(principal.Identity!.IsAuthenticated); + Assert.Equal(IdentityConstants.ApplicationScheme, principal.Identity.AuthenticationType); + } + + [Fact] + public void Build_StampsEmailAndNameClaims() + { + var builder = BuildBuilder(); + + var principal = builder.Build("you@example.com", PrincipalWithGroups()); + + Assert.Equal("you@example.com", principal.FindFirst(ClaimTypes.Name)?.Value); + Assert.Equal("you@example.com", principal.FindFirst(ClaimTypes.Email)?.Value); + Assert.Equal("you@example.com", principal.FindFirst(ClaimTypes.NameIdentifier)?.Value); + } + + [Fact] + public void Build_StampsRoleClaims_FromMappedGroups() + { + var builder = BuildBuilder(); + var external = PrincipalWithGroups( + "us-prd-cloud-adminportal-admin", + "us-prd-cloud-adminportal-billing"); + + var principal = builder.Build("you@example.com", external); + + var roles = principal.FindAll(ClaimTypes.Role).Select(c => c.Value).ToArray(); + Assert.Contains("admin", roles); + Assert.Contains("billing", roles); + } + + [Fact] + public void Build_StampsNoRoleClaims_WhenNoGroupsMap() + { + // Mirror of the controller contract: a signed-in SSO user with no mapped groups still + // gets a valid principal, but holds no role claims. AccessControlService.UserHasPermission + // denies every gated action from there. + var builder = BuildBuilder(); + var external = PrincipalWithGroups("unrelated-group"); + + var principal = builder.Build("you@example.com", external); + + Assert.Empty(principal.FindAll(ClaimTypes.Role)); + } + + private static SsoSignInPrincipalBuilder BuildBuilder() + { + var mapper = new GroupClaimRoleMapper(new AdminSettings + { + Oidc = new AdminSettings.OidcSettings { GroupPrefix = "us-prd-cloud-adminportal-" }, + }); + return new SsoSignInPrincipalBuilder(mapper); + } + + private static ClaimsPrincipal PrincipalWithGroups(params string[] groups) + { + var claims = groups.Select(g => new Claim(GroupClaimRoleMapper.GroupsClaimType, g)); + return new ClaimsPrincipal(new ClaimsIdentity(claims)); + } +} diff --git a/test/Admin.Test/IdentityServer/ServiceCollectionExtensionsTests.cs b/test/Admin.Test/IdentityServer/ServiceCollectionExtensionsTests.cs index 6898c6899f7c..5bad1b2291fa 100644 --- a/test/Admin.Test/IdentityServer/ServiceCollectionExtensionsTests.cs +++ b/test/Admin.Test/IdentityServer/ServiceCollectionExtensionsTests.cs @@ -24,6 +24,7 @@ public void AddAdminUpstreamOidc_ThrowsWhenEmailClaimTypeIsNull() Authority = "https://idp.example.com", ClientId = "id", ClientSecret = "secret", + GroupPrefix = "us-prd-cloud-adminportal-", EmailClaimType = null, } }; @@ -34,6 +35,30 @@ public void AddAdminUpstreamOidc_ThrowsWhenEmailClaimTypeIsNull() Assert.Contains("EmailClaimType", ex.Message); } + [Fact] + public void AddAdminUpstreamOidc_ThrowsWhenGroupPrefixIsNull() + { + // GroupPrefix is the key the groups-to-roles mapper strips off every IdP group name. + // Without it, every SSO sign-in would be rejected as "no mapped group" - safer to + // fail boot loudly than let the operator discover it the first time anyone tries SSO. + var services = new ServiceCollection(); + var adminSettings = new AdminSettings + { + Oidc = new AdminSettings.OidcSettings + { + Authority = "https://idp.example.com", + ClientId = "id", + ClientSecret = "secret", + GroupPrefix = null, + } + }; + + var ex = Assert.Throws(() => + services.AddAdminUpstreamOidc(adminSettings)); + + Assert.Contains("GroupPrefix", ex.Message); + } + [Fact] public async Task AddAdminUpstreamOidc_OnTokenValidated_FailsWhenAuthTimeMissingAsync() { @@ -91,6 +116,7 @@ private static OpenIdConnectOptions BuildOidcOptions() Authority = "https://idp.example.com", ClientId = "id", ClientSecret = "secret", + GroupPrefix = "us-prd-cloud-adminportal-", } }); diff --git a/test/Admin.Test/Services/AccessControlServiceTests.cs b/test/Admin.Test/Services/AccessControlServiceTests.cs new file mode 100644 index 000000000000..29f041fb7b36 --- /dev/null +++ b/test/Admin.Test/Services/AccessControlServiceTests.cs @@ -0,0 +1,90 @@ +using System.Security.Claims; +using Bit.Admin.Enums; +using Bit.Admin.Services; +using Bit.Core.Settings; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Configuration; +using NSubstitute; + +namespace Admin.Test.Services; + +public class AccessControlServiceTests +{ + [Fact] + public void UserHasPermission_ReturnsTrue_ForSelfHostedRegardlessOfRole() + { + // Self-hosted admin runs single-tenant with no SSO or allowlist infrastructure; + // whoever reached it is authorized for everything. + var service = BuildService(selfHosted: true); + + Assert.True(service.UserHasPermission(Permission.Org_Delete)); + } + + [Fact] + public void UserHasPermission_ReturnsFalse_WhenNoRoleClaims() + { + var service = BuildService(selfHosted: false, roleClaims: Array.Empty()); + + Assert.False(service.UserHasPermission(Permission.Org_Delete)); + } + + [Fact] + public void UserHasPermission_ReturnsTrue_WhenAnyRoleGrantsPermission() + { + // sales does not have Org_Delete; admin does. The union logic should flip the gate to + // true when any held role grants the requested permission. + var service = BuildService(selfHosted: false, roleClaims: new[] { "sales", "admin" }); + + Assert.True(service.UserHasPermission(Permission.Org_Delete)); + } + + [Fact] + public void UserHasPermission_ReturnsFalse_WhenNoRoleGrantsPermission() + { + // cs is read-mostly and specifically does not have Tools_PromoteAdmin. + var service = BuildService(selfHosted: false, roleClaims: new[] { "cs" }); + + Assert.False(service.UserHasPermission(Permission.Tools_PromoteAdmin)); + } + + [Fact] + public void UserHasPermission_IgnoresUnknownRoleClaim() + { + // A role claim whose value is not a key in RolePermissionMapping must not grant + // anything. Fail-closed on unexpected values that might slip in from a misconfigured + // IdP group. + var service = BuildService(selfHosted: false, roleClaims: new[] { "root" }); + + Assert.False(service.UserHasPermission(Permission.Org_Delete)); + } + + [Fact] + public void UserHasPermission_IsCaseInsensitiveOnRoleName() + { + var service = BuildService(selfHosted: false, roleClaims: new[] { "ADMIN" }); + + Assert.True(service.UserHasPermission(Permission.Org_Delete)); + } + + private static AccessControlService BuildService(bool selfHosted, string[] roleClaims = null) + { + var httpContextAccessor = Substitute.For(); + if (roleClaims is not null) + { + var identity = new ClaimsIdentity(roleClaims.Select(r => new Claim(ClaimTypes.Role, r))); + httpContextAccessor.HttpContext.Returns(new DefaultHttpContext + { + User = new ClaimsPrincipal(identity), + }); + } + + var globalSettings = Substitute.For(); + globalSettings.SelfHosted.Returns(selfHosted); + + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary()) + .Build(); + + return new AccessControlService(httpContextAccessor, configuration, globalSettings); + } +} diff --git a/test/Admin.Test/Utilities/RequirePermissionAttributeTests.cs b/test/Admin.Test/Utilities/RequirePermissionAttributeTests.cs new file mode 100644 index 000000000000..d33c851713a1 --- /dev/null +++ b/test/Admin.Test/Utilities/RequirePermissionAttributeTests.cs @@ -0,0 +1,85 @@ +using System.Security.Claims; +using Bit.Admin.Enums; +using Bit.Admin.Services; +using Bit.Admin.Utilities; +using Bit.Core.Settings; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.AspNetCore.Mvc.Filters; +using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using NSubstitute; + +namespace Admin.Test.Utilities; + +public class RequirePermissionAttributeTests +{ + [Fact] + public void OnActionExecuting_Throws_WhenUserHasNoRoleClaims() + { + // Pins the no-permission UX: a signed-in principal without any role claims (the SSO + // "authenticated but no mapped groups" case) hits the attribute on any gated action + // and triggers UnauthorizedAccessException. UseExceptionHandler("/error") turns that + // into the generic error page downstream. + var attribute = new RequirePermissionAttribute(Permission.Org_Delete); + var context = BuildActionContext(roleClaims: Array.Empty()); + + Assert.Throws(() => attribute.OnActionExecuting(context)); + } + + [Fact] + public void OnActionExecuting_Allows_WhenAnyRoleGrantsPermission() + { + var attribute = new RequirePermissionAttribute(Permission.Org_Delete); + var context = BuildActionContext(roleClaims: new[] { "sales", "admin" }); + + attribute.OnActionExecuting(context); + + Assert.Null(context.Result); + } + + [Fact] + public void OnActionExecuting_Throws_WhenNoHeldRoleGrantsPermission() + { + var attribute = new RequirePermissionAttribute(Permission.Tools_PromoteAdmin); + var context = BuildActionContext(roleClaims: new[] { "cs" }); + + Assert.Throws(() => attribute.OnActionExecuting(context)); + } + + private static ActionExecutingContext BuildActionContext(string[] roleClaims) + { + var identity = new ClaimsIdentity(roleClaims.Select(r => new Claim(ClaimTypes.Role, r))); + var principal = new ClaimsPrincipal(identity); + + var httpContextAccessor = Substitute.For(); + httpContextAccessor.HttpContext.Returns(new DefaultHttpContext { User = principal }); + + var globalSettings = Substitute.For(); + globalSettings.SelfHosted.Returns(false); + + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary()) + .Build(); + + var accessControlService = new AccessControlService(httpContextAccessor, configuration, globalSettings); + + var services = new ServiceCollection(); + services.AddSingleton(accessControlService); + + var httpContext = new DefaultHttpContext + { + User = principal, + RequestServices = services.BuildServiceProvider(), + }; + + var actionContext = new ActionContext(httpContext, new RouteData(), new ActionDescriptor()); + return new ActionExecutingContext( + actionContext, + new List(), + new Dictionary(), + controller: new object()); + } +} From 65cb42250003dccdcc8ad52d637dd8d0171c125a Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Sat, 17 Oct 2026 14:54:31 -0400 Subject: [PATCH 02/12] Add dedicated scheme for SSO so that security stamp can still exist on paswordless. --- src/Admin/Auth/Controllers/LoginController.cs | 22 ++++--- .../AdminAuthenticationSchemes.cs | 10 +++- .../Services/SsoSignInPrincipalBuilder.cs | 12 ++-- src/Admin/Startup.cs | 58 ++++++++++++++++++- .../Auth/Controllers/LoginControllerTests.cs | 14 +++-- 5 files changed, 95 insertions(+), 21 deletions(-) diff --git a/src/Admin/Auth/Controllers/LoginController.cs b/src/Admin/Auth/Controllers/LoginController.cs index b85a91da881e..1d1d2a690ca1 100644 --- a/src/Admin/Auth/Controllers/LoginController.cs +++ b/src/Admin/Auth/Controllers/LoginController.cs @@ -162,12 +162,14 @@ public async Task SsoSignIn(string returnUrl = null, string remot props.StoreTokens([new AuthenticationToken { Name = "id_token", Value = idToken }]); } - // SSO signs in directly to the application cookie with a hand-built principal rather + // SSO signs in to its own dedicated cookie scheme with a hand-built principal rather // than routing through SignInManager / UserManager. 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. + // 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); - await HttpContext.SignInAsync(IdentityConstants.ApplicationScheme, principal, props); + await HttpContext.SignInAsync(AdminAuthenticationSchemes.SsoCookie, principal, props); await HttpContext.SignOutAsync(IdentityConstants.ExternalScheme); _logger.LogInformation("SSO sign-in succeeded."); @@ -183,12 +185,16 @@ public async Task SsoSignIn(string returnUrl = null, string remot [ValidateAntiForgeryToken] public async Task 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 }); diff --git a/src/Admin/Auth/IdentityServer/AdminAuthenticationSchemes.cs b/src/Admin/Auth/IdentityServer/AdminAuthenticationSchemes.cs index b4ec5909f0e0..875d7536ae48 100644 --- a/src/Admin/Auth/IdentityServer/AdminAuthenticationSchemes.cs +++ b/src/Admin/Auth/IdentityServer/AdminAuthenticationSchemes.cs @@ -1,4 +1,4 @@ -namespace Bit.Admin.Auth.IdentityServer; +namespace Bit.Admin.Auth.IdentityServer; public static class AdminAuthenticationSchemes { @@ -6,4 +6,12 @@ public static class AdminAuthenticationSchemes // 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"; } diff --git a/src/Admin/Auth/Services/SsoSignInPrincipalBuilder.cs b/src/Admin/Auth/Services/SsoSignInPrincipalBuilder.cs index 26c824ddb84f..00640192e50a 100644 --- a/src/Admin/Auth/Services/SsoSignInPrincipalBuilder.cs +++ b/src/Admin/Auth/Services/SsoSignInPrincipalBuilder.cs @@ -6,14 +6,16 @@ namespace Bit.Admin.Auth.Services; -// Assembles the ClaimsPrincipal that gets written to the Admin Portal application cookie on -// SSO sign-in. The caller hands over the verified external principal and the email, and gets back a +// Assembles the ClaimsPrincipal that gets written to the Admin Portal SSO cookie on sign-in. +// The caller hands over the verified external principal and the email, and gets back a // principal ready for HttpContext.SignInAsync. // // Deliberately bypasses SignInManager / UserManager so the SSO path does not need a backing -// user store entry and does not stamp a SecurityStamp claim. Without the stamp claim, the -// SecurityStampValidator skips its periodic re-fetch, which is what we want for a session -// whose source of truth is the IdP, not local config. +// user store entry and does not stamp a SecurityStamp claim. The companion to that choice is +// that the controller signs the result into the dedicated AdminAuthenticationSchemes.SsoCookie +// scheme (not Identity's application cookie); the Identity cookie's SecurityStampValidator +// would otherwise reject this principal within 30 seconds because no matching user exists in +// the store. public interface ISsoSignInPrincipalBuilder { ClaimsPrincipal Build(string email, ClaimsPrincipal externalPrincipal); diff --git a/src/Admin/Startup.cs b/src/Admin/Startup.cs index 0a4637a02421..dc701b6c9abd 100644 --- a/src/Admin/Startup.cs +++ b/src/Admin/Startup.cs @@ -1,9 +1,12 @@ using System.Globalization; +using Bit.Admin.Auth.IdentityServer; using Bit.Admin.IdentityServer; using Bit.Core.Context; using Bit.Core.Settings; using Bit.Core.Utilities; using Bit.SharedWeb.Utilities; +using Microsoft.AspNetCore.Authentication.Cookies; +using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Identity; using Stripe; using Microsoft.AspNetCore.Mvc.Razor; @@ -80,16 +83,67 @@ public void ConfigureServices(IServiceCollection services) services.AddAdminUpstreamOidc(adminSettings); services.Configure(options => { - // This interval defines the maximum delay in access changes - // being reflected in the application. + // This interval defines the maximum delay in access changes + // being reflected in the application. Applies to the Identity application cookie + // (used by the passwordless-email path), which has a backing user store with a + // SecurityStamp to validate. The SSO cookie added below has no backing store, + // so this validator is not wired to it. options.ValidationInterval = TimeSpan.FromSeconds(30); }); + services.AddAuthentication() + .AddCookie(AdminAuthenticationSchemes.SsoCookie, options => + { + // Shared with the Identity cookie so a session timeout on either lands on the + // same login page. + options.LoginPath = "/login"; + options.AccessDeniedPath = "/login?accessDenied=true"; + options.Cookie.Name = $"Bitwarden_{globalSettings.ProjectName}.SSO"; + options.Cookie.HttpOnly = true; + // Secure=Always is safe here because this cookie is only ever issued at the end + // of an SSO sign-in, and SSO already requires HTTPS end-to-end (the OIDC + // handler sets RequireHttpsMetadata=true and its correlation/nonce cookies are + // SameSite=None, which browsers only honor over HTTPS). + options.Cookie.SecurePolicy = CookieSecurePolicy.Always; + // Lax matches the Identity cookie: required so the cookie is sent on the + // top-level GET redirect back from the OIDC callback. + options.Cookie.SameSite = SameSiteMode.Lax; + options.ExpireTimeSpan = TimeSpan.FromDays(2); + options.SlidingExpiration = true; + options.ReturnUrlParameter = "returnUrl"; + // Deliberately NO SecurityStampValidator wired to this scheme. The IdP owns + // identity, there is no local user record, and no stamp to compare against. + // Session lifetime is bounded by the cookie expiry above and by RP-initiated + // logout on sign-out. + }); + services.AddAuthorization(options => + { + // Try the Identity cookie first, then the SSO cookie. The PolicyEvaluator calls + // AuthenticateAsync on each listed scheme and sets HttpContext.User to the first + // successful result, so both passwordless and SSO sessions satisfy bare [Authorize] + // and are visible to AccessControlService. Each scheme is actually decrypted and + // validated - no cookie-name presence heuristic that would short-circuit a valid + // session when a stale cookie from the other scheme happens to be on the request. + // + // DefaultAuthenticateScheme / DefaultChallengeScheme stay at Identity's application + // cookie (set by AddIdentity), so bare [Authorize] redirects to /login via Identity + // and SignInManager still writes to the Identity cookie. + options.DefaultPolicy = new AuthorizationPolicyBuilder( + IdentityConstants.ApplicationScheme, + AdminAuthenticationSchemes.SsoCookie) + .RequireAuthenticatedUser() + .Build(); + options.FallbackPolicy = options.DefaultPolicy; + }); if (globalSettings.SelfHosted) { services.ConfigureApplicationCookie(options => { options.Cookie.Path = "/admin"; }); + services.Configure(AdminAuthenticationSchemes.SsoCookie, options => + { + options.Cookie.Path = "/admin"; + }); } // Services diff --git a/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs b/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs index dfb3debf05a1..ddbbdd9841c8 100644 --- a/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs +++ b/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs @@ -123,9 +123,13 @@ private static IAuthenticationService AttachAuthenticationServices(LoginControll private static void StubCurrentAuth(IAuthenticationService authService, AuthenticationProperties properties) { + // HttpContext.GetTokenAsync(scheme, name) under the hood calls + // IAuthenticationService.AuthenticateAsync(ctx, scheme). The Logout action reads the + // id_token from the SSO cookie scheme, so stub that scheme to return the ticket whose + // properties carry the token. var identity = new ClaimsIdentity(authenticationType: "TestAuth"); var ticket = new AuthenticationTicket(new ClaimsPrincipal(identity), properties, "TestAuth"); - authService.AuthenticateAsync(Arg.Any(), null) + authService.AuthenticateAsync(Arg.Any(), AdminAuthenticationSchemes.SsoCookie) .Returns(AuthenticateResult.Success(ticket)); } @@ -223,7 +227,7 @@ public async Task SsoSignIn_AcceptsSignIn_WhenEmailVerifiedMissingAndNotRequired Assert.IsType(result); await authService.Received(1).SignInAsync( Arg.Any(), - IdentityConstants.ApplicationScheme, + AdminAuthenticationSchemes.SsoCookie, builtPrincipal, Arg.Any()); } @@ -246,7 +250,7 @@ public async Task SsoSignIn_RejectsSignIn_WhenEmailVerifiedFalse_EvenWhenNotRequ Assert.Equal(5, redirect.RouteValues!["error"]); await authService.DidNotReceive().SignInAsync( Arg.Any(), - IdentityConstants.ApplicationScheme, + AdminAuthenticationSchemes.SsoCookie, Arg.Any(), Arg.Any()); } @@ -269,7 +273,7 @@ public async Task SsoSignIn_SignsInToApplicationCookie_WithBuiltPrincipal() await authService.Received(1).SignInAsync( Arg.Any(), - IdentityConstants.ApplicationScheme, + AdminAuthenticationSchemes.SsoCookie, builtPrincipal, Arg.Any()); } @@ -315,7 +319,7 @@ public async Task SsoSignIn_StoresIdTokenOnAppCookie_OnSuccess() Assert.IsType(result); await authService.Received(1).SignInAsync( Arg.Any(), - IdentityConstants.ApplicationScheme, + AdminAuthenticationSchemes.SsoCookie, Arg.Any(), Arg.Is(p => p.GetTokenValue("id_token") == "the-id-token")); } From f4955f950d538a3929482794b43e796da7056d53 Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Sat, 17 Oct 2026 16:26:09 -0400 Subject: [PATCH 03/12] Drop FallbackPolicy from Admin authorization 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. --- src/Admin/Startup.cs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/Admin/Startup.cs b/src/Admin/Startup.cs index dc701b6c9abd..1173382159a9 100644 --- a/src/Admin/Startup.cs +++ b/src/Admin/Startup.cs @@ -117,12 +117,19 @@ public void ConfigureServices(IServiceCollection services) }); services.AddAuthorization(options => { - // Try the Identity cookie first, then the SSO cookie. The PolicyEvaluator calls - // AuthenticateAsync on each listed scheme and sets HttpContext.User to the first - // successful result, so both passwordless and SSO sessions satisfy bare [Authorize] - // and are visible to AccessControlService. Each scheme is actually decrypted and - // validated - no cookie-name presence heuristic that would short-circuit a valid - // session when a stale cookie from the other scheme happens to be on the request. + // DefaultPolicy applies when [Authorize] is present without its own policy. Listing + // both schemes makes PolicyEvaluator call AuthenticateAsync on each one in order + // and set HttpContext.User to the first successful result, so both passwordless + // and SSO sessions satisfy bare [Authorize] and are visible to AccessControlService. + // Each scheme is actually decrypted and validated - no cookie-name presence + // heuristic that would short-circuit a valid session when a stale cookie from the + // other scheme is on the request. + // + // Deliberately NOT set as FallbackPolicy: a fallback would require auth on every + // endpoint without an explicit [Authorize]/[AllowAnonymous], locking out the login + // pages (LoginController.Index/Confirm), health probes (/alive), /error, and the + // version endpoint. Anonymous-by-default + opt-in [Authorize] on controllers is + // the Admin app's established posture. // // DefaultAuthenticateScheme / DefaultChallengeScheme stay at Identity's application // cookie (set by AddIdentity), so bare [Authorize] redirects to /login via Identity @@ -132,7 +139,6 @@ public void ConfigureServices(IServiceCollection services) AdminAuthenticationSchemes.SsoCookie) .RequireAuthenticatedUser() .Build(); - options.FallbackPolicy = options.DefaultPolicy; }); if (globalSettings.SelfHosted) { From e7d6e7db225686ba85e72fc4b9185addbe75bdbf Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Sat, 17 Oct 2026 19:10:11 -0400 Subject: [PATCH 04/12] Fix SsoSignInPrincipalBuilder AuthenticationType to match SsoCookie scheme 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. --- src/Admin/Auth/Services/SsoSignInPrincipalBuilder.cs | 8 ++++---- .../Auth/Services/SsoSignInPrincipalBuilderTests.cs | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/Admin/Auth/Services/SsoSignInPrincipalBuilder.cs b/src/Admin/Auth/Services/SsoSignInPrincipalBuilder.cs index 00640192e50a..ae03e53566ef 100644 --- a/src/Admin/Auth/Services/SsoSignInPrincipalBuilder.cs +++ b/src/Admin/Auth/Services/SsoSignInPrincipalBuilder.cs @@ -2,7 +2,7 @@ #nullable disable using System.Security.Claims; -using Microsoft.AspNetCore.Identity; +using Bit.Admin.Auth.IdentityServer; namespace Bit.Admin.Auth.Services; @@ -33,9 +33,9 @@ public SsoSignInPrincipalBuilder(IGroupClaimRoleMapper groupClaimRoleMapper) public ClaimsPrincipal Build(string email, ClaimsPrincipal externalPrincipal) { // AuthenticationType is required for ClaimsIdentity.IsAuthenticated to return true. - // Using the application scheme name is conventional and makes the identity recognizable - // in middleware traces. - var identity = new ClaimsIdentity(IdentityConstants.ApplicationScheme); + // 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); identity.AddClaim(new Claim(ClaimTypes.Name, email)); identity.AddClaim(new Claim(ClaimTypes.Email, email)); diff --git a/test/Admin.Test/Auth/Services/SsoSignInPrincipalBuilderTests.cs b/test/Admin.Test/Auth/Services/SsoSignInPrincipalBuilderTests.cs index c5c972f07ece..a2fa35190b7b 100644 --- a/test/Admin.Test/Auth/Services/SsoSignInPrincipalBuilderTests.cs +++ b/test/Admin.Test/Auth/Services/SsoSignInPrincipalBuilderTests.cs @@ -1,24 +1,24 @@ using System.Security.Claims; using Bit.Admin; +using Bit.Admin.Auth.IdentityServer; using Bit.Admin.Auth.Services; -using Microsoft.AspNetCore.Identity; namespace Admin.Test.Auth.Services; public class SsoSignInPrincipalBuilderTests { [Fact] - public void Build_PrincipalIsAuthenticated_WithApplicationSchemeAuthenticationType() + public void Build_PrincipalIsAuthenticated_WithSsoCookieAuthenticationType() { - // IsAuthenticated depends on AuthenticationType being non-null. The application cookie - // middleware expects any principal written via HttpContext.SignInAsync to resolve as - // authenticated. + // IsAuthenticated depends on AuthenticationType being non-null. The AuthenticationType + // must match the scheme the controller signs this principal into so middleware traces + // and anything keying off Identity.AuthenticationType report the actual scheme. var builder = BuildBuilder(); var principal = builder.Build("you@example.com", PrincipalWithGroups()); Assert.True(principal.Identity!.IsAuthenticated); - Assert.Equal(IdentityConstants.ApplicationScheme, principal.Identity.AuthenticationType); + Assert.Equal(AdminAuthenticationSchemes.SsoCookie, principal.Identity.AuthenticationType); } [Fact] From 1dfda58af00e76b246826b4a74ece3340714712b Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Sat, 17 Oct 2026 19:10:12 -0400 Subject: [PATCH 05/12] Reject Admin SSO sign-in when principal has no mapped admin role 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). --- src/Admin/Auth/Controllers/LoginController.cs | 15 ++ src/Admin/Auth/SSO.md | 154 ++++++++++++++++++ .../Auth/Controllers/LoginControllerTests.cs | 44 ++++- 3 files changed, 208 insertions(+), 5 deletions(-) create mode 100644 src/Admin/Auth/SSO.md diff --git a/src/Admin/Auth/Controllers/LoginController.cs b/src/Admin/Auth/Controllers/LoginController.cs index 1d1d2a690ca1..7d9ca3314478 100644 --- a/src/Admin/Auth/Controllers/LoginController.cs +++ b/src/Admin/Auth/Controllers/LoginController.cs @@ -169,6 +169,21 @@ public async Task SsoSignIn(string returnUrl = null, string remot // 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); + + // Tenancy gate. This PR removed the AdminSettings:Admins allowlist from the SSO path, + // so the only membership signal is "does this user map to at least one admin role". + // Without this gate, an IdP-authenticated user with no mapped group would receive a + // cookie and reach any [Authorize] action that is not additionally gated by + // [RequirePermission] - e.g. UsersController.View/Edit, OrganizationsController.View/ + // Edit/TriggerBillingSync/ResendOwnerInvite. See src/Admin/Auth/SSO.md. + if (!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."); diff --git a/src/Admin/Auth/SSO.md b/src/Admin/Auth/SSO.md new file mode 100644 index 000000000000..8e451ec730b4 --- /dev/null +++ b/src/Admin/Auth/SSO.md @@ -0,0 +1,154 @@ +--- +status: Accepted +date: 2026-10-17 +tags: [server, admin, sso] +--- + +# Admin Portal SSO sign-in design + +Design and rationale for how the Admin Portal admits users signing in via upstream OIDC +(`AdminAuthenticationSchemes.UpstreamOidc`). Audience: engineers modifying `LoginController`, +`SsoSignInPrincipalBuilder`, `GroupClaimRoleMapper`, or the `AdminSettings:Oidc:*` configuration +surface; operators evaluating how to provision Admin Portal access through an IdP. + +## Context and problem statement + +The Admin Portal has two sign-in paths: + +- **Passwordless email** (self-hosted default). `AdminSettings:Admins` is a CSV allowlist that + gates sign-in by email. Permissions on cloud come from `adminSettings:role:{roleName}` CSV + sub-config; on self-hosted, `AccessControlService.UserHasPermission` short-circuits to `true` on + `_globalSettings.SelfHosted`. +- **Upstream OIDC SSO** (added in PR #8508 and extended here). The IdP authenticates the user, + and `GroupClaimRoleMapper` maps the IdP's `groups` claim to Admin Portal role names using the + configured `AdminSettings:Oidc:GroupPrefix`. + +The SSO path removed the `AdminSettings:Admins` allowlist check that the passwordless path still +uses, replacing membership with "has at least one mapped admin role." This introduces a tenancy +question: what signal admits an IdP-authenticated user as a member of the Admin Portal? + +Several controller actions carry only bare `[Authorize]` and are not additionally gated by +`[RequirePermission]`: + +- `UsersController.View`, `UsersController.Edit` (GET + POST) +- `OrganizationsController.View`, `OrganizationsController.Edit` (GET + POST), + `TriggerBillingSync`, `ResendOwnerInvite` + +These actions are reachable by any authenticated principal. Without a tenancy signal applied at +sign-in, an IdP-authenticated email whose groups do not map to any admin role would receive an +application cookie and could invoke these actions on cloud. On self-hosted the exposure is bounded +by the operator-controlled IdP app assignment, which is the first tenancy filter. + +### Terminology + +- **Authentication** — the IdP proving the user owns the claimed email. +- **Tenancy / membership** — whether an authenticated user is a user of *this* app at all. +- **Authorization** — what capabilities a member has within the app. + +The three are distinct. Historically the passwordless path collapsed authentication and tenancy +into one step: `UserManager.FindByEmailAsync` against `AdminSettings:Admins`. The SSO path keeps +them separate: the IdP handles authentication, and this document is about how tenancy is enforced. + +## Considered options + +### Option 1 — No tenancy check; rely on per-action `[RequirePermission]` + +Admit every IdP-authenticated principal and depend on `AccessControlService.UserHasPermission` +returning `false` to block the ungated actions. + +#### Advantages + +- Clean separation of concerns: authn at the door, authz at each action. +- No per-request tenancy work. + +#### Disadvantages + +- Does not address the actual exposure: bare `[Authorize]` actions have no permission check and + remain reachable by any authenticated principal. +- Fragile — every new ungated action adds a new exposure. Review discipline alone is not a + structural guard. +- Issues cookies to principals who have no business holding them, enlarging the attack surface of + the cookie itself. + +### Option 2 — Add `[RequirePermission]` to every bare `[Authorize]` action + +Decorate every consequential controller action with a permission requirement, then rely on +`AccessControlService` to reject role-less principals at each action. + +#### Advantages + +- Pins per-action authorization explicitly, independent of upstream tenancy state. +- Catches the same class of regression (new ungated action) locally where the action is defined. + +#### Disadvantages + +- Cross-cutting work across six-plus actions in two controllers; each action requires a decision + about which permission to require. +- Still issues cookies to role-less principals; defense in depth at the sign-in boundary is + cheaper and more complete. +- Does not change the fact that any new ungated action reintroduces exposure. + +### Option 3 — Tenancy gate at sign-in: reject when the principal has no mapped role + +After `SsoSignInPrincipalBuilder.Build(...)` returns, verify the principal carries at least one +`ClaimTypes.Role` claim. If none, sign the external principal out and redirect to the generic SSO +failure message. + +#### Advantages + +- Addresses the exposure at the correct layer: a non-member never gets a cookie. +- Defense in depth remains possible — adding `[RequirePermission]` to ungated actions is a + compatible follow-up, not a prerequisite. +- Honest about the Admin Portal model: there is no legitimate "authenticated but no capability" + user. Every page is admin-only. +- Mirrors the role of `AdminSettings:Admins` on the passwordless path: a membership gate at the + door, decoupled from per-action authz. + +#### Disadvantages + +- Admits an argument that authn and authz are being conflated. The rebuttal: the check is a + *tenancy* gate, not an authz check, and groups happen to be the only tenancy signal the SSO + path consumes today. +- Operators who want SSO without configuring IdP groups cannot use the SSO path yet. The + `AdminSettings:Oidc:GroupPrefix` setting is already required at boot by `AddAdminUpstreamOidc`, + so this is not a regression from the broader SSO work. + +## Decision outcome + +Chosen option: **option 3, tenancy gate at sign-in**. + +`LoginController.SsoSignIn` evaluates `principal.FindAll(ClaimTypes.Role).Any()` after building +the principal. If false, the controller logs a warning, signs out +`IdentityConstants.ExternalScheme`, and redirects to the generic SSO-failed error. The gate is +test-pinned by `LoginControllerTests.SsoSignIn_RedirectsError5_WhenPrincipalHasNoRoleClaims`. + +### Positive consequences + +- Cloud SSO users whose IdP groups do not map to an admin role never receive a session, closing + the exposure flagged by code review on the bare `[Authorize]` actions. +- The authn/authz/tenancy separation is honest at every boundary: the IdP handles authn, this + gate handles tenancy, `AccessControlService` handles authz. +- Future SSO sign-in paths (e.g., adding CSV-based tenancy for operators who prefer "IdP as + authn only") can be layered as additional tenancy signals under an all-gates-must-pass rule + without changing the authz model. + +### Negative consequences + +- Operators enabling Admin Portal SSO must provision at least one IdP group matching + `AdminSettings:Oidc:GroupPrefix` for intended admins. There is no "SSO without groups" path in + the current PR. +- The bare `[Authorize]` actions on `UsersController` and `OrganizationsController` remain latent + risks if a future change weakens the tenancy gate or introduces a new sign-in path. The + follow-up to add `[RequirePermission]` to those actions is tracked separately. + +### Plan + +- Ship the tenancy gate in the SSO sign-in path and the matching unit test as part of + PR #8587. +- Track as follow-up work: audit every `[Authorize]`-only action in `src/Admin` and either add + `[RequirePermission(...)]` or document why no permission applies. This is defense in depth + against future tenancy-gate regressions. +- Track as follow-up work: evaluate operator demand for CSV-based tenancy on the SSO path (where + `AdminSettings:Admins` acts as the tenancy signal and `AdminSettings:Oidc:GroupPrefix` becomes + optional). The all-gates-must-pass rule discussed in design review generalizes cleanly if + demand materializes. diff --git a/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs b/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs index ddbbdd9841c8..58a023106456 100644 --- a/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs +++ b/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs @@ -219,7 +219,7 @@ public async Task SsoSignIn_AcceptsSignIn_WhenEmailVerifiedMissingAndNotRequired requireEmailVerified: false); var authService = AttachAuthenticationServices(controller); StubExternalPrincipal(authService, new Claim("email", "you@example.com")); - var builtPrincipal = new ClaimsPrincipal(new ClaimsIdentity("test")); + var builtPrincipal = PrincipalWithRole("owner"); builder.Build("you@example.com", Arg.Any()).Returns(builtPrincipal); var result = await controller.SsoSignIn(); @@ -266,7 +266,7 @@ public async Task SsoSignIn_SignsInToApplicationCookie_WithBuiltPrincipal() StubExternalPrincipal(authService, new Claim("email", "you@example.com"), new Claim("email_verified", "true")); - var builtPrincipal = new ClaimsPrincipal(new ClaimsIdentity("test")); + var builtPrincipal = PrincipalWithRole("owner"); builder.Build("you@example.com", Arg.Any()).Returns(builtPrincipal); await controller.SsoSignIn(); @@ -278,6 +278,35 @@ await authService.Received(1).SignInAsync( Arg.Any()); } + [Fact] + public async Task SsoSignIn_RedirectsError5_WhenPrincipalHasNoRoleClaims() + { + // Tenancy gate. An IdP-authenticated principal with no mapped admin role must be + // rejected at sign-in; without this gate the principal could reach any [Authorize] + // action that isn't additionally gated by [RequirePermission]. See src/Admin/Auth/SSO.md. + var controller = BuildController(oidcEnabled: true, out var builder); + var authService = AttachAuthenticationServices(controller); + StubExternalPrincipal(authService, + new Claim("email", "you@example.com"), + new Claim("email_verified", "true")); + builder.Build(Arg.Any(), Arg.Any()) + .Returns(new ClaimsPrincipal(new ClaimsIdentity(authenticationType: "test"))); + + var result = await controller.SsoSignIn(); + + var redirect = Assert.IsType(result); + Assert.Equal(5, redirect.RouteValues!["error"]); + await authService.DidNotReceive().SignInAsync( + Arg.Any(), + AdminAuthenticationSchemes.SsoCookie, + Arg.Any(), + Arg.Any()); + await authService.Received(1).SignOutAsync( + Arg.Any(), + IdentityConstants.ExternalScheme, + Arg.Any()); + } + [Theory] [InlineData("1")] [InlineData("0")] @@ -312,7 +341,7 @@ public async Task SsoSignIn_StoresIdTokenOnAppCookie_OnSuccess() new Claim("email_verified", "true"), ]); builder.Build(Arg.Any(), Arg.Any()) - .Returns(new ClaimsPrincipal(new ClaimsIdentity("test"))); + .Returns(PrincipalWithRole("owner")); var result = await controller.SsoSignIn(); @@ -334,7 +363,7 @@ public async Task SsoSignIn_RedirectsToLocalReturnUrl_WhenLocal() new Claim("email", "you@example.com"), new Claim("email_verified", "true")); builder.Build(Arg.Any(), Arg.Any()) - .Returns(new ClaimsPrincipal(new ClaimsIdentity("test"))); + .Returns(PrincipalWithRole("owner")); var result = await controller.SsoSignIn(returnUrl: "/dashboard"); @@ -365,7 +394,7 @@ public async Task SsoSignIn_RedirectsToHome_WhenReturnUrlNotLocal() new Claim("email", "you@example.com"), new Claim("email_verified", "true")); builder.Build(Arg.Any(), Arg.Any()) - .Returns(new ClaimsPrincipal(new ClaimsIdentity("test"))); + .Returns(PrincipalWithRole("owner")); var result = await controller.SsoSignIn(returnUrl: "https://evil.example.com"); @@ -374,6 +403,11 @@ public async Task SsoSignIn_RedirectsToHome_WhenReturnUrlNotLocal() Assert.Equal("Home", redirect.ControllerName); } + private static ClaimsPrincipal PrincipalWithRole(string role) => + new(new ClaimsIdentity( + new[] { new Claim(ClaimTypes.Role, role) }, + authenticationType: "test")); + private static LoginController BuildController(bool oidcEnabled) => BuildController(oidcEnabled, out _); From 4293d4336c52f6b60ac222dae05fcba830c47e5c Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Sat, 17 Oct 2026 20:40:10 -0400 Subject: [PATCH 06/12] Reshape SSO.md as plain documentation 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. --- src/Admin/Auth/SSO.md | 287 ++++++++++++++++++++---------------------- 1 file changed, 133 insertions(+), 154 deletions(-) diff --git a/src/Admin/Auth/SSO.md b/src/Admin/Auth/SSO.md index 8e451ec730b4..9cff10834a27 100644 --- a/src/Admin/Auth/SSO.md +++ b/src/Admin/Auth/SSO.md @@ -1,154 +1,133 @@ ---- -status: Accepted -date: 2026-10-17 -tags: [server, admin, sso] ---- - -# Admin Portal SSO sign-in design - -Design and rationale for how the Admin Portal admits users signing in via upstream OIDC -(`AdminAuthenticationSchemes.UpstreamOidc`). Audience: engineers modifying `LoginController`, -`SsoSignInPrincipalBuilder`, `GroupClaimRoleMapper`, or the `AdminSettings:Oidc:*` configuration -surface; operators evaluating how to provision Admin Portal access through an IdP. - -## Context and problem statement - -The Admin Portal has two sign-in paths: - -- **Passwordless email** (self-hosted default). `AdminSettings:Admins` is a CSV allowlist that - gates sign-in by email. Permissions on cloud come from `adminSettings:role:{roleName}` CSV - sub-config; on self-hosted, `AccessControlService.UserHasPermission` short-circuits to `true` on - `_globalSettings.SelfHosted`. -- **Upstream OIDC SSO** (added in PR #8508 and extended here). The IdP authenticates the user, - and `GroupClaimRoleMapper` maps the IdP's `groups` claim to Admin Portal role names using the - configured `AdminSettings:Oidc:GroupPrefix`. - -The SSO path removed the `AdminSettings:Admins` allowlist check that the passwordless path still -uses, replacing membership with "has at least one mapped admin role." This introduces a tenancy -question: what signal admits an IdP-authenticated user as a member of the Admin Portal? - -Several controller actions carry only bare `[Authorize]` and are not additionally gated by -`[RequirePermission]`: - -- `UsersController.View`, `UsersController.Edit` (GET + POST) -- `OrganizationsController.View`, `OrganizationsController.Edit` (GET + POST), - `TriggerBillingSync`, `ResendOwnerInvite` - -These actions are reachable by any authenticated principal. Without a tenancy signal applied at -sign-in, an IdP-authenticated email whose groups do not map to any admin role would receive an -application cookie and could invoke these actions on cloud. On self-hosted the exposure is bounded -by the operator-controlled IdP app assignment, which is the first tenancy filter. - -### Terminology - -- **Authentication** — the IdP proving the user owns the claimed email. -- **Tenancy / membership** — whether an authenticated user is a user of *this* app at all. -- **Authorization** — what capabilities a member has within the app. - -The three are distinct. Historically the passwordless path collapsed authentication and tenancy -into one step: `UserManager.FindByEmailAsync` against `AdminSettings:Admins`. The SSO path keeps -them separate: the IdP handles authentication, and this document is about how tenancy is enforced. - -## Considered options - -### Option 1 — No tenancy check; rely on per-action `[RequirePermission]` - -Admit every IdP-authenticated principal and depend on `AccessControlService.UserHasPermission` -returning `false` to block the ungated actions. - -#### Advantages - -- Clean separation of concerns: authn at the door, authz at each action. -- No per-request tenancy work. - -#### Disadvantages - -- Does not address the actual exposure: bare `[Authorize]` actions have no permission check and - remain reachable by any authenticated principal. -- Fragile — every new ungated action adds a new exposure. Review discipline alone is not a - structural guard. -- Issues cookies to principals who have no business holding them, enlarging the attack surface of - the cookie itself. - -### Option 2 — Add `[RequirePermission]` to every bare `[Authorize]` action - -Decorate every consequential controller action with a permission requirement, then rely on -`AccessControlService` to reject role-less principals at each action. - -#### Advantages - -- Pins per-action authorization explicitly, independent of upstream tenancy state. -- Catches the same class of regression (new ungated action) locally where the action is defined. - -#### Disadvantages - -- Cross-cutting work across six-plus actions in two controllers; each action requires a decision - about which permission to require. -- Still issues cookies to role-less principals; defense in depth at the sign-in boundary is - cheaper and more complete. -- Does not change the fact that any new ungated action reintroduces exposure. - -### Option 3 — Tenancy gate at sign-in: reject when the principal has no mapped role - -After `SsoSignInPrincipalBuilder.Build(...)` returns, verify the principal carries at least one -`ClaimTypes.Role` claim. If none, sign the external principal out and redirect to the generic SSO -failure message. - -#### Advantages - -- Addresses the exposure at the correct layer: a non-member never gets a cookie. -- Defense in depth remains possible — adding `[RequirePermission]` to ungated actions is a - compatible follow-up, not a prerequisite. -- Honest about the Admin Portal model: there is no legitimate "authenticated but no capability" - user. Every page is admin-only. -- Mirrors the role of `AdminSettings:Admins` on the passwordless path: a membership gate at the - door, decoupled from per-action authz. - -#### Disadvantages - -- Admits an argument that authn and authz are being conflated. The rebuttal: the check is a - *tenancy* gate, not an authz check, and groups happen to be the only tenancy signal the SSO - path consumes today. -- Operators who want SSO without configuring IdP groups cannot use the SSO path yet. The - `AdminSettings:Oidc:GroupPrefix` setting is already required at boot by `AddAdminUpstreamOidc`, - so this is not a regression from the broader SSO work. - -## Decision outcome - -Chosen option: **option 3, tenancy gate at sign-in**. - -`LoginController.SsoSignIn` evaluates `principal.FindAll(ClaimTypes.Role).Any()` after building -the principal. If false, the controller logs a warning, signs out -`IdentityConstants.ExternalScheme`, and redirects to the generic SSO-failed error. The gate is -test-pinned by `LoginControllerTests.SsoSignIn_RedirectsError5_WhenPrincipalHasNoRoleClaims`. - -### Positive consequences - -- Cloud SSO users whose IdP groups do not map to an admin role never receive a session, closing - the exposure flagged by code review on the bare `[Authorize]` actions. -- The authn/authz/tenancy separation is honest at every boundary: the IdP handles authn, this - gate handles tenancy, `AccessControlService` handles authz. -- Future SSO sign-in paths (e.g., adding CSV-based tenancy for operators who prefer "IdP as - authn only") can be layered as additional tenancy signals under an all-gates-must-pass rule - without changing the authz model. - -### Negative consequences - -- Operators enabling Admin Portal SSO must provision at least one IdP group matching - `AdminSettings:Oidc:GroupPrefix` for intended admins. There is no "SSO without groups" path in - the current PR. -- The bare `[Authorize]` actions on `UsersController` and `OrganizationsController` remain latent - risks if a future change weakens the tenancy gate or introduces a new sign-in path. The - follow-up to add `[RequirePermission]` to those actions is tracked separately. - -### Plan - -- Ship the tenancy gate in the SSO sign-in path and the matching unit test as part of - PR #8587. -- Track as follow-up work: audit every `[Authorize]`-only action in `src/Admin` and either add - `[RequirePermission(...)]` or document why no permission applies. This is defense in depth - against future tenancy-gate regressions. -- Track as follow-up work: evaluate operator demand for CSV-based tenancy on the SSO path (where - `AdminSettings:Admins` acts as the tenancy signal and `AdminSettings:Oidc:GroupPrefix` becomes - optional). The all-gates-must-pass rule discussed in design review generalizes cleanly if - demand materializes. +# Admin Portal SSO sign-in + +How the Admin Portal admits users signing in through upstream OIDC, and how it keeps authentication, +tenancy, and authorization distinct. 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 | Who signs in | Who gets admin capability | +|---|---|---|---| +| Passwordless email | `AdminSettings:EnablePasswordlessLogin = true` (default) | Emails on the `AdminSettings:Admins` CSV allowlist | On self-hosted: everyone signed in (`SelfHosted` short-circuit). On cloud: users whose email appears in `adminSettings:role:{roleName}` CSV sub-config. | +| Upstream OIDC SSO | `AdminSettings:Oidc:*` configured | IdP-authenticated users whose `groups` claim maps to at least one Admin Portal role via `AdminSettings:Oidc:GroupPrefix` | On self-hosted: everyone signed in (`SelfHosted` short-circuit). On cloud: the union of permissions across every mapped role. | + +The passwordless path is unchanged by this work. Everything below describes the SSO path. + +## 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 the **tenancy gate** below. + - 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 +deliberately generic so unsuccessful attempts cannot distinguish between "no such user", "wrong +group", and "IdP returned an error." + +## Tenancy gate + +After `SsoSignInPrincipalBuilder.Build(...)` returns, `LoginController.SsoSignIn` checks whether the +built principal carries at least one `ClaimTypes.Role` claim. If it does not, the sign-in is +rejected. + +This gate exists because the SSO path does not consult `AdminSettings:Admins`. The passwordless +path uses that CSV as its membership check. The SSO path uses "has at least one mapped admin role" +as its membership check instead. Without this gate, an IdP-authenticated user whose groups do not +map to any admin role would receive an application cookie and could reach controller actions +decorated with bare `[Authorize]` but no `[RequirePermission]` - specifically `UsersController. +View/Edit` and `OrganizationsController.View/Edit/TriggerBillingSync/ResendOwnerInvite`. + +Pinned by `LoginControllerTests.SsoSignIn_RedirectsError5_WhenPrincipalHasNoRoleClaims`. + +Adding `[RequirePermission(...)]` to the bare `[Authorize]` actions is a desirable defense-in-depth +follow-up but is not a prerequisite - the tenancy gate makes them unreachable to role-less +principals regardless. + +## Authentication, tenancy, and authorization + +These three concerns are kept distinct on the SSO path: + +- **Authentication** is the IdP proving the user owns the claimed email. Enforced by the OIDC + handler plus the email-verified check in `LoginController.SsoSignIn`. +- **Tenancy** is whether the authenticated user is a user of this app at all. Enforced by the + role-presence check in `LoginController.SsoSignIn`. +- **Authorization** is what capabilities the authenticated member has. Enforced by + `AccessControlService.UserHasPermission` against the role claims on the principal, using + `RolePermissionMapping`. + +The role mapping configuration serves both tenancy and authorization: "has any mapped role" is the +tenancy signal, "which permissions those roles grant" is the authorization decision. The two use +the same data source but answer different questions. + +## Cookie schemes + +The Admin app registers two cookie schemes. Both coexist so the two sign-in paths can carry +independent session rules. + +| 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 - the SSO principal has no backing user record to re-fetch | + +The two schemes exist because the Identity cookie's `SecurityStampValidator` re-fetches the user +from `IUserStore` every 30 seconds. The SSO principal has no corresponding row in that store, so +routing it through the Identity cookie would eject the session on the first re-check. + +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` stay at Identity's defaults, so bare `[Authorize]` still redirects to +`/login` and `SignInManager` still writes to the Identity cookie. + +`FallbackPolicy` is deliberately not set, 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 (sign-out against a scheme the user is not currently +signed into is a no-op). 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: + +```jsonc +"AdminSettings": { + "Oidc": { + "Authority": "https://idp.example.com", + "ClientId": "admin-portal", + "ClientSecret": "...", + "GroupPrefix": "us-prd-cloud-adminportal-" + } +} +``` + +`AddAdminUpstreamOidc` fails loud at boot if `GroupPrefix` is empty while OIDC is enabled, because +without it `GroupClaimRoleMapper` would match no groups and every SSO sign-in would fail the tenancy +gate. + +IdP groups are expected to be named `` where `` 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. From 3ebcc4bec003d2f86be3e717f87bba628523bbbb Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Sat, 17 Oct 2026 21:05:42 -0400 Subject: [PATCH 07/12] Describe only current state in SSO.md and the tenancy-gate comment 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. --- src/Admin/Auth/Controllers/LoginController.cs | 20 ++--- src/Admin/Auth/SSO.md | 79 +++++++------------ 2 files changed, 38 insertions(+), 61 deletions(-) diff --git a/src/Admin/Auth/Controllers/LoginController.cs b/src/Admin/Auth/Controllers/LoginController.cs index 7d9ca3314478..3bb228841945 100644 --- a/src/Admin/Auth/Controllers/LoginController.cs +++ b/src/Admin/Auth/Controllers/LoginController.cs @@ -162,20 +162,16 @@ public async Task SsoSignIn(string returnUrl = null, string remot props.StoreTokens([new AuthenticationToken { Name = "id_token", Value = idToken }]); } - // SSO signs in to its own dedicated cookie scheme with a hand-built principal rather - // than routing through SignInManager / UserManager. 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 does a 30s re-fetch - // against the user store, which would reject the SSO principal within 30 seconds. + // 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); - // Tenancy gate. This PR removed the AdminSettings:Admins allowlist from the SSO path, - // so the only membership signal is "does this user map to at least one admin role". - // Without this gate, an IdP-authenticated user with no mapped group would receive a - // cookie and reach any [Authorize] action that is not additionally gated by - // [RequirePermission] - e.g. UsersController.View/Edit, OrganizationsController.View/ - // Edit/TriggerBillingSync/ResendOwnerInvite. See src/Admin/Auth/SSO.md. + // Tenancy gate. SSO membership requires at least one mapped admin role. + // See src/Admin/Auth/SSO.md. if (!principal.FindAll(ClaimTypes.Role).Any()) { _logger.LogWarning( diff --git a/src/Admin/Auth/SSO.md b/src/Admin/Auth/SSO.md index 9cff10834a27..0f9463abce21 100644 --- a/src/Admin/Auth/SSO.md +++ b/src/Admin/Auth/SSO.md @@ -1,9 +1,8 @@ # Admin Portal SSO sign-in -How the Admin Portal admits users signing in through upstream OIDC, and how it keeps authentication, -tenancy, and authorization distinct. Audience: engineers modifying `LoginController`, -`SsoSignInPrincipalBuilder`, `GroupClaimRoleMapper`, or `AdminSettings:Oidc:*`; operators -provisioning Admin Portal access through an IdP. +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 @@ -11,10 +10,10 @@ The Admin Portal supports two sign-in paths. An operator can enable either or bo | Path | Enabled by | Who signs in | Who gets admin capability | |---|---|---|---| -| Passwordless email | `AdminSettings:EnablePasswordlessLogin = true` (default) | Emails on the `AdminSettings:Admins` CSV allowlist | On self-hosted: everyone signed in (`SelfHosted` short-circuit). On cloud: users whose email appears in `adminSettings:role:{roleName}` CSV sub-config. | -| Upstream OIDC SSO | `AdminSettings:Oidc:*` configured | IdP-authenticated users whose `groups` claim maps to at least one Admin Portal role via `AdminSettings:Oidc:GroupPrefix` | On self-hosted: everyone signed in (`SelfHosted` short-circuit). On cloud: the union of permissions across every mapped role. | +| Passwordless email | `AdminSettings:EnablePasswordlessLogin = true` (default) | Emails on the `AdminSettings:Admins` CSV allowlist | On self-hosted: everyone signed in. On cloud: users whose email appears in `adminSettings:role:{roleName}` CSV sub-config. | +| Upstream OIDC SSO | `AdminSettings:Oidc:*` configured | IdP-authenticated users whose `groups` claim maps to at least one Admin Portal role via `AdminSettings:Oidc:GroupPrefix` | On self-hosted: everyone signed in. On cloud: the union of permissions across every mapped role. | -The passwordless path is unchanged by this work. Everything below describes the SSO path. +Everything below describes the SSO path. ## SSO sign-in flow @@ -32,7 +31,7 @@ The passwordless path is unchanged by this work. Everything below describes the - 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 the **tenancy gate** below. + - Applies the tenancy gate described below. - 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). @@ -40,74 +39,58 @@ The passwordless path is unchanged by this work. Everything below describes the 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 -deliberately generic so unsuccessful attempts cannot distinguish between "no such user", "wrong +intentionally generic: unsuccessful attempts cannot distinguish between "no such user", "wrong group", and "IdP returned an error." ## Tenancy gate -After `SsoSignInPrincipalBuilder.Build(...)` returns, `LoginController.SsoSignIn` checks whether the -built principal carries at least one `ClaimTypes.Role` claim. If it does not, the sign-in is -rejected. - -This gate exists because the SSO path does not consult `AdminSettings:Admins`. The passwordless -path uses that CSV as its membership check. The SSO path uses "has at least one mapped admin role" -as its membership check instead. Without this gate, an IdP-authenticated user whose groups do not -map to any admin role would receive an application cookie and could reach controller actions -decorated with bare `[Authorize]` but no `[RequirePermission]` - specifically `UsersController. -View/Edit` and `OrganizationsController.View/Edit/TriggerBillingSync/ResendOwnerInvite`. +After `SsoSignInPrincipalBuilder.Build(...)` returns, `LoginController.SsoSignIn` requires the built +principal to carry at least one `ClaimTypes.Role` claim. A principal with zero role claims is +rejected: the controller logs a warning, signs out of `IdentityConstants.ExternalScheme`, and +redirects to `GET /login?error=5`. Pinned by `LoginControllerTests.SsoSignIn_RedirectsError5_WhenPrincipalHasNoRoleClaims`. -Adding `[RequirePermission(...)]` to the bare `[Authorize]` actions is a desirable defense-in-depth -follow-up but is not a prerequisite - the tenancy gate makes them unreachable to role-less -principals regardless. - ## Authentication, tenancy, and authorization -These three concerns are kept distinct on the SSO path: +The SSO path keeps three concerns distinct: -- **Authentication** is the IdP proving the user owns the claimed email. Enforced by the OIDC - handler plus the email-verified check in `LoginController.SsoSignIn`. -- **Tenancy** is whether the authenticated user is a user of this app at all. Enforced by the +- **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 role-presence check in `LoginController.SsoSignIn`. -- **Authorization** is what capabilities the authenticated member has. Enforced by +- **Authorization** — what capabilities the authenticated member has. Enforced by `AccessControlService.UserHasPermission` against the role claims on the principal, using `RolePermissionMapping`. -The role mapping configuration serves both tenancy and authorization: "has any mapped role" is the -tenancy signal, "which permissions those roles grant" is the authorization decision. The two use -the same data source but answer different questions. +The role mapping serves both tenancy and authorization: "has any mapped role" is the tenancy +signal; "which permissions those roles grant" is the authorization decision. Same data source, two +questions. ## Cookie schemes -The Admin app registers two cookie schemes. Both coexist so the two sign-in paths can carry -independent session rules. +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 - the SSO principal has no backing user record to re-fetch | - -The two schemes exist because the Identity cookie's `SecurityStampValidator` re-fetches the user -from `IUserStore` every 30 seconds. The SSO principal has no corresponding row in that store, so -routing it through the Identity cookie would eject the session on the first re-check. +| `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` stay at Identity's defaults, so bare `[Authorize]` still redirects to -`/login` and `SignInManager` still writes to the Identity cookie. +`DefaultChallengeScheme` are Identity's defaults, so bare `[Authorize]` redirects to `/login` and +`SignInManager` writes to the Identity cookie. -`FallbackPolicy` is deliberately not set, so endpoints without `[Authorize]` stay anonymous. This -keeps the login pages, `/alive`, `/error`, and the version endpoint reachable without a 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 (sign-out against a scheme the user is not currently -signed into is a no-op). 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. +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 @@ -124,9 +107,7 @@ Minimum settings for SSO: } ``` -`AddAdminUpstreamOidc` fails loud at boot if `GroupPrefix` is empty while OIDC is enabled, because -without it `GroupClaimRoleMapper` would match no groups and every SSO sign-in would fail the tenancy -gate. +`AddAdminUpstreamOidc` fails loud at boot if `GroupPrefix` is empty while OIDC is enabled. IdP groups are expected to be named `` where `` is one of `owner`, `admin`, `billing`, `cs`, or `sales` (the keys of `RolePermissionMapping.RolePermissions`). Case is From 5e6af3b4d4f3c91896339ddc931da665653a5fbc Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Sat, 17 Oct 2026 21:11:42 -0400 Subject: [PATCH 08/12] Support dual-gate SSO tenancy: Admins CSV, GroupPrefix, or both 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. --- src/Admin/Auth/Controllers/LoginController.cs | 33 +++- src/Admin/Auth/SSO.md | 78 ++++++--- .../ServiceCollectionExtensions.cs | 13 +- .../Auth/Controllers/LoginControllerTests.cs | 149 +++++++++++++++++- .../ServiceCollectionExtensionsTests.cs | 32 +++- 5 files changed, 261 insertions(+), 44 deletions(-) diff --git a/src/Admin/Auth/Controllers/LoginController.cs b/src/Admin/Auth/Controllers/LoginController.cs index 3bb228841945..19c88db8c882 100644 --- a/src/Admin/Auth/Controllers/LoginController.cs +++ b/src/Admin/Auth/Controllers/LoginController.cs @@ -170,9 +170,19 @@ public async Task SsoSignIn(string returnUrl = null, string remot // local user store. var principal = _ssoSignInPrincipalBuilder.Build(email, external.Principal); - // Tenancy gate. SSO membership requires at least one mapped admin role. - // See src/Admin/Auth/SSO.md. - if (!principal.FindAll(ClaimTypes.Role).Any()) + // 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."); @@ -222,6 +232,23 @@ public async Task 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; diff --git a/src/Admin/Auth/SSO.md b/src/Admin/Auth/SSO.md index 0f9463abce21..a91a27f1d7a5 100644 --- a/src/Admin/Auth/SSO.md +++ b/src/Admin/Auth/SSO.md @@ -8,13 +8,48 @@ operators provisioning Admin Portal access through an IdP. The Admin Portal supports two sign-in paths. An operator can enable either or both. -| Path | Enabled by | Who signs in | Who gets admin capability | +| Path | Enabled by | Membership gate | Role claims come from | |---|---|---|---| -| Passwordless email | `AdminSettings:EnablePasswordlessLogin = true` (default) | Emails on the `AdminSettings:Admins` CSV allowlist | On self-hosted: everyone signed in. On cloud: users whose email appears in `adminSettings:role:{roleName}` CSV sub-config. | -| Upstream OIDC SSO | `AdminSettings:Oidc:*` configured | IdP-authenticated users whose `groups` claim maps to at least one Admin Portal role via `AdminSettings:Oidc:GroupPrefix` | On self-hosted: everyone signed in. On cloud: the union of permissions across every mapped role. | +| 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. + +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 @@ -31,7 +66,7 @@ Everything below describes the SSO path. - 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 the tenancy gate described below. + - 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). @@ -40,16 +75,7 @@ Everything below describes the SSO path. 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", and "IdP returned an error." - -## Tenancy gate - -After `SsoSignInPrincipalBuilder.Build(...)` returns, `LoginController.SsoSignIn` requires the built -principal to carry at least one `ClaimTypes.Role` claim. A principal with zero role claims is -rejected: the controller logs a warning, signs out of `IdentityConstants.ExternalScheme`, and -redirects to `GET /login?error=5`. - -Pinned by `LoginControllerTests.SsoSignIn_RedirectsError5_WhenPrincipalHasNoRoleClaims`. +group", "email not on allowlist", and "IdP returned an error." ## Authentication, tenancy, and authorization @@ -58,15 +84,11 @@ 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 - role-presence check in `LoginController.SsoSignIn`. + 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`. -The role mapping serves both tenancy and authorization: "has any mapped role" is the tenancy -signal; "which permissions those roles grant" is the authorization decision. Same data source, two -questions. - ## Cookie schemes The Admin app registers two cookie schemes. Each sign-in path writes its own. @@ -94,21 +116,25 @@ as `id_token_hint`, so the upstream IdP can honor the app-level Allowed Logout U ## Operator configuration -Minimum settings for SSO: +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 + // where 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-" } } ``` - -`AddAdminUpstreamOidc` fails loud at boot if `GroupPrefix` is empty while OIDC is enabled. - -IdP groups are expected to be named `` where `` 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. diff --git a/src/Admin/IdentityServer/ServiceCollectionExtensions.cs b/src/Admin/IdentityServer/ServiceCollectionExtensions.cs index d056d65fd6f0..ffb075551282 100644 --- a/src/Admin/IdentityServer/ServiceCollectionExtensions.cs +++ b/src/Admin/IdentityServer/ServiceCollectionExtensions.cs @@ -85,13 +85,16 @@ public static IServiceCollection AddAdminUpstreamOidc( "AdminSettings:Oidc:EmailClaimType must not be null or empty when SSO is enabled."); } - // Fail-loud when the operator enables SSO but never set the group prefix. Without it - // the groups-to-roles mapper would match nothing and every SSO sign-in would be rejected - // as "no mapped group" - better to refuse to boot than to silently lock everyone out. - if (string.IsNullOrWhiteSpace(oidc.GroupPrefix)) + // Fail-loud when SSO is enabled but no tenancy signal is configured. The sign-in + // tenancy gate requires at least one of: AdminSettings:Admins (CSV allowlist) or + // AdminSettings:Oidc:GroupPrefix (IdP groups → roles). Without either, every SSO + // sign-in would be rejected at the gate. + if (string.IsNullOrWhiteSpace(adminSettings.Admins) && + string.IsNullOrWhiteSpace(oidc.GroupPrefix)) { throw new InvalidOperationException( - "AdminSettings:Oidc:GroupPrefix must not be null or empty when SSO is enabled."); + "At least one of AdminSettings:Admins or AdminSettings:Oidc:GroupPrefix " + + "must be set when SSO is enabled."); } services.AddAuthentication() diff --git a/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs b/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs index 58a023106456..4f7be7ac29ee 100644 --- a/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs +++ b/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs @@ -279,11 +279,10 @@ await authService.Received(1).SignInAsync( } [Fact] - public async Task SsoSignIn_RedirectsError5_WhenPrincipalHasNoRoleClaims() + public async Task SsoSignIn_RedirectsError5_WhenGroupPrefixConfiguredAndPrincipalHasNoRoleClaims() { - // Tenancy gate. An IdP-authenticated principal with no mapped admin role must be - // rejected at sign-in; without this gate the principal could reach any [Authorize] - // action that isn't additionally gated by [RequirePermission]. See src/Admin/Auth/SSO.md. + // Tenancy gate (groups-only configuration): principal must have at least one mapped + // admin role. See src/Admin/Auth/SSO.md. var controller = BuildController(oidcEnabled: true, out var builder); var authService = AttachAuthenticationServices(controller); StubExternalPrincipal(authService, @@ -307,6 +306,141 @@ await authService.Received(1).SignOutAsync( Arg.Any()); } + [Fact] + public async Task SsoSignIn_RedirectsError5_WhenAdminsConfiguredAndEmailNotOnList() + { + // Tenancy gate (CSV-only configuration): email must be on AdminSettings:Admins. + var controller = BuildController(oidcEnabled: true, out var builder, + adminsAllowlist: "admin@example.com", groupPrefix: null); + var authService = AttachAuthenticationServices(controller); + StubExternalPrincipal(authService, + new Claim("email", "stranger@example.com"), + new Claim("email_verified", "true")); + builder.Build(Arg.Any(), Arg.Any()) + .Returns(PrincipalWithRole("owner")); + + var result = await controller.SsoSignIn(); + + var redirect = Assert.IsType(result); + Assert.Equal(5, redirect.RouteValues!["error"]); + await authService.DidNotReceive().SignInAsync( + Arg.Any(), + AdminAuthenticationSchemes.SsoCookie, + Arg.Any(), + Arg.Any()); + } + + [Fact] + public async Task SsoSignIn_SignsIn_WhenOnlyAdminsConfiguredAndEmailOnList_EvenWithoutRoleClaims() + { + // CSV-only mode: with GroupPrefix unset, groups gate is skipped. A principal with no + // role claims is admitted as long as the email is on the Admins allowlist. + var controller = BuildController(oidcEnabled: true, out var builder, + adminsAllowlist: "you@example.com", groupPrefix: null); + var authService = AttachAuthenticationServices(controller); + StubExternalPrincipal(authService, + new Claim("email", "you@example.com"), + new Claim("email_verified", "true")); + var builtPrincipal = new ClaimsPrincipal(new ClaimsIdentity(authenticationType: "test")); + builder.Build(Arg.Any(), Arg.Any()).Returns(builtPrincipal); + + await controller.SsoSignIn(); + + await authService.Received(1).SignInAsync( + Arg.Any(), + AdminAuthenticationSchemes.SsoCookie, + builtPrincipal, + Arg.Any()); + } + + [Fact] + public async Task SsoSignIn_RedirectsError5_WhenBothConfigured_AndEmailOnListButNoRole() + { + // Both gates configured: both must admit. CSV passes but groups does not. + var controller = BuildController(oidcEnabled: true, out var builder, + adminsAllowlist: "you@example.com"); + var authService = AttachAuthenticationServices(controller); + StubExternalPrincipal(authService, + new Claim("email", "you@example.com"), + new Claim("email_verified", "true")); + builder.Build(Arg.Any(), Arg.Any()) + .Returns(new ClaimsPrincipal(new ClaimsIdentity(authenticationType: "test"))); + + var result = await controller.SsoSignIn(); + + var redirect = Assert.IsType(result); + Assert.Equal(5, redirect.RouteValues!["error"]); + await authService.DidNotReceive().SignInAsync( + Arg.Any(), + AdminAuthenticationSchemes.SsoCookie, + Arg.Any(), + Arg.Any()); + } + + [Fact] + public async Task SsoSignIn_RedirectsError5_WhenBothConfigured_AndRolePresentButEmailNotOnList() + { + // Both gates configured: both must admit. Groups passes but CSV does not. + var controller = BuildController(oidcEnabled: true, out var builder, + adminsAllowlist: "admin@example.com"); + var authService = AttachAuthenticationServices(controller); + StubExternalPrincipal(authService, + new Claim("email", "stranger@example.com"), + new Claim("email_verified", "true")); + builder.Build(Arg.Any(), Arg.Any()) + .Returns(PrincipalWithRole("owner")); + + var result = await controller.SsoSignIn(); + + var redirect = Assert.IsType(result); + Assert.Equal(5, redirect.RouteValues!["error"]); + } + + [Fact] + public async Task SsoSignIn_SignsIn_WhenBothConfigured_AndBothGatesPass() + { + // Both gates configured: both admit. + var controller = BuildController(oidcEnabled: true, out var builder, + adminsAllowlist: "you@example.com"); + var authService = AttachAuthenticationServices(controller); + StubExternalPrincipal(authService, + new Claim("email", "you@example.com"), + new Claim("email_verified", "true")); + var builtPrincipal = PrincipalWithRole("owner"); + builder.Build(Arg.Any(), Arg.Any()).Returns(builtPrincipal); + + await controller.SsoSignIn(); + + await authService.Received(1).SignInAsync( + Arg.Any(), + AdminAuthenticationSchemes.SsoCookie, + builtPrincipal, + Arg.Any()); + } + + [Fact] + public async Task SsoSignIn_SignsIn_WhenAdminsCsvEntryIncludesStampSuffix() + { + // AdminSettings:Admins supports "email:stamp" entries (used by the passwordless + // user store). The CSV gate matches on the email prefix only. + var controller = BuildController(oidcEnabled: true, out var builder, + adminsAllowlist: "you@example.com:abc123", groupPrefix: null); + var authService = AttachAuthenticationServices(controller); + StubExternalPrincipal(authService, + new Claim("email", "you@example.com"), + new Claim("email_verified", "true")); + var builtPrincipal = new ClaimsPrincipal(new ClaimsIdentity(authenticationType: "test")); + builder.Build(Arg.Any(), Arg.Any()).Returns(builtPrincipal); + + await controller.SsoSignIn(); + + await authService.Received(1).SignInAsync( + Arg.Any(), + AdminAuthenticationSchemes.SsoCookie, + builtPrincipal, + Arg.Any()); + } + [Theory] [InlineData("1")] [InlineData("0")] @@ -415,18 +549,21 @@ private static LoginController BuildController( bool oidcEnabled, out ISsoSignInPrincipalBuilder ssoSignInPrincipalBuilder, bool requireEmailVerified = true, - bool enablePasswordlessLogin = true) + bool enablePasswordlessLogin = true, + string adminsAllowlist = null, + string groupPrefix = "us-prd-cloud-adminportal-") { var settings = new AdminSettings { EnablePasswordlessLogin = enablePasswordlessLogin, + Admins = adminsAllowlist, Oidc = oidcEnabled ? new AdminSettings.OidcSettings { Authority = "https://idp.example.com", ClientId = "admin-portal", ClientSecret = "supersecret", - GroupPrefix = "us-prd-cloud-adminportal-", + GroupPrefix = groupPrefix, RequireEmailVerifiedClaim = requireEmailVerified, } : null, diff --git a/test/Admin.Test/IdentityServer/ServiceCollectionExtensionsTests.cs b/test/Admin.Test/IdentityServer/ServiceCollectionExtensionsTests.cs index 5bad1b2291fa..71f94095e4f3 100644 --- a/test/Admin.Test/IdentityServer/ServiceCollectionExtensionsTests.cs +++ b/test/Admin.Test/IdentityServer/ServiceCollectionExtensionsTests.cs @@ -36,14 +36,16 @@ public void AddAdminUpstreamOidc_ThrowsWhenEmailClaimTypeIsNull() } [Fact] - public void AddAdminUpstreamOidc_ThrowsWhenGroupPrefixIsNull() + public void AddAdminUpstreamOidc_ThrowsWhenNoTenancySignalConfigured() { - // GroupPrefix is the key the groups-to-roles mapper strips off every IdP group name. - // Without it, every SSO sign-in would be rejected as "no mapped group" - safer to - // fail boot loudly than let the operator discover it the first time anyone tries SSO. + // The sign-in tenancy gate requires at least one of AdminSettings:Admins or + // AdminSettings:Oidc:GroupPrefix. With neither set, every SSO sign-in would be rejected + // at the gate - fail boot loudly rather than let the operator discover it the first + // time anyone tries SSO. var services = new ServiceCollection(); var adminSettings = new AdminSettings { + Admins = null, Oidc = new AdminSettings.OidcSettings { Authority = "https://idp.example.com", @@ -56,9 +58,31 @@ public void AddAdminUpstreamOidc_ThrowsWhenGroupPrefixIsNull() var ex = Assert.Throws(() => services.AddAdminUpstreamOidc(adminSettings)); + Assert.Contains("Admins", ex.Message); Assert.Contains("GroupPrefix", ex.Message); } + [Fact] + public void AddAdminUpstreamOidc_Succeeds_WhenOnlyAdminsConfigured() + { + // CSV-only mode: AdminSettings:Admins is the tenancy signal, no group mapping needed. + var services = new ServiceCollection(); + services.AddLogging(); + var adminSettings = new AdminSettings + { + Admins = "admin@example.com", + Oidc = new AdminSettings.OidcSettings + { + Authority = "https://idp.example.com", + ClientId = "id", + ClientSecret = "secret", + GroupPrefix = null, + } + }; + + services.AddAdminUpstreamOidc(adminSettings); + } + [Fact] public async Task AddAdminUpstreamOidc_OnTokenValidated_FailsWhenAuthTimeMissingAsync() { From 9d24324eb7cfe21ff25250b7fde0f5e78ccb6620 Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Sat, 17 Oct 2026 21:39:39 -0400 Subject: [PATCH 09/12] Replace dual-gate SSO tenancy with deployment-driven split 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. --- src/Admin/Auth/Controllers/LoginController.cs | 42 ++++--- src/Admin/Auth/SSO.md | 94 ++++++++-------- .../ServiceCollectionExtensions.cs | 23 ++-- src/Admin/Startup.cs | 2 +- .../Auth/Controllers/LoginControllerTests.cs | 94 +++++----------- .../ServiceCollectionExtensionsTests.cs | 105 +++++++++--------- 6 files changed, 167 insertions(+), 193 deletions(-) diff --git a/src/Admin/Auth/Controllers/LoginController.cs b/src/Admin/Auth/Controllers/LoginController.cs index 19c88db8c882..ccd3624feaa7 100644 --- a/src/Admin/Auth/Controllers/LoginController.cs +++ b/src/Admin/Auth/Controllers/LoginController.cs @@ -6,6 +6,7 @@ using Bit.Admin.Auth.IdentityServer; using Bit.Admin.Auth.Models; using Bit.Admin.Auth.Services; +using Bit.Core.Settings; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Identity; @@ -18,17 +19,20 @@ public class LoginController : Controller { private readonly PasswordlessSignInManager _signInManager; private readonly AdminSettings _adminSettings; + private readonly IGlobalSettings _globalSettings; private readonly ISsoSignInPrincipalBuilder _ssoSignInPrincipalBuilder; private readonly ILogger _logger; public LoginController( PasswordlessSignInManager signInManager, IOptions adminSettings, + IGlobalSettings globalSettings, ISsoSignInPrincipalBuilder ssoSignInPrincipalBuilder, ILogger logger) { _signInManager = signInManager; _adminSettings = adminSettings.Value; + _globalSettings = globalSettings; _ssoSignInPrincipalBuilder = ssoSignInPrincipalBuilder; _logger = logger; } @@ -170,24 +174,32 @@ public async Task SsoSignIn(string returnUrl = null, string remot // local user store. var principal = _ssoSignInPrincipalBuilder.Build(email, external.Principal); - // 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)) + // Tenancy gate. The signal depends on deployment: + // - Self-hosted: email must be on AdminSettings:Admins. No RBAC runs on self-hosted + // (AccessControlService.UserHasPermission short-circuits on SelfHosted), so role + // claims are irrelevant. + // - Cloud: principal must carry at least one role claim. RBAC drives authorization + // downstream via RolePermissionMapping. + // See src/Admin/Auth/SSO.md. + if (_globalSettings.SelfHosted) { - _logger.LogWarning( - "SSO sign-in rejected: email not on AdminSettings:Admins allowlist."); - await HttpContext.SignOutAsync(IdentityConstants.ExternalScheme); - return RedirectToAction("Index", new { error = 5 }); + if (!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()) + else { - _logger.LogWarning( - "SSO sign-in rejected: authenticated principal has no mapped admin role."); - await HttpContext.SignOutAsync(IdentityConstants.ExternalScheme); - return RedirectToAction("Index", new { error = 5 }); + if (!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); diff --git a/src/Admin/Auth/SSO.md b/src/Admin/Auth/SSO.md index a91a27f1d7a5..8ae88efbdfdd 100644 --- a/src/Admin/Auth/SSO.md +++ b/src/Admin/Auth/SSO.md @@ -8,47 +8,27 @@ operators provisioning Admin Portal access through an IdP. 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` | +| Path | Enabled by | +|---|---| +| Passwordless email | `AdminSettings:EnablePasswordlessLogin = true` (default) | +| Upstream OIDC SSO | `AdminSettings:Oidc:*` configured | Everything below describes the SSO path. -## SSO tenancy gates +## Deployment-driven tenancy and authorization -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. +The SSO sign-in gate and the authorization model are determined by deployment type. Operators do +not choose between them. -| Signal | Configured when | Admits a user when | +| | Cloud | Self-hosted | |---|---|---| -| `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) | +| **Tenancy gate** (admits sign-in) | Principal has at least one `ClaimTypes.Role` claim | Authenticated email is on `AdminSettings:Admins` | +| **Authorization** (what the admitted user can do) | Union of permissions across role claims, via `RolePermissionMapping` | Full access (`AccessControlService.UserHasPermission` short-circuits on `_globalSettings.SelfHosted`) | +| **Role claims come from** | `AdminSettings:Oidc:GroupPrefix` → `GroupClaimRoleMapper` maps IdP `groups` claim to roles | n/a — role claims are irrelevant on self-hosted | +| **Boot-time required config** | `AdminSettings:Oidc:GroupPrefix` must be set | `AdminSettings:Admins` must be set | -`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. - -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. +`AddAdminUpstreamOidc` fails loud at boot when the required config for the deployment type is +missing. ## SSO sign-in flow @@ -66,7 +46,7 @@ both approvals are required. - 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. + - Applies the deployment-specific tenancy gate (see table above). - 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). @@ -84,10 +64,11 @@ 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`. + deployment-driven gate in `LoginController.SsoSignIn`. +- **Authorization** — what capabilities the authenticated member has. On cloud, enforced by + `AccessControlService.UserHasPermission` against the role claims on the principal using + `RolePermissionMapping`. On self-hosted, the same method short-circuits to `true` on + `_globalSettings.SelfHosted`. ## Cookie schemes @@ -116,25 +97,38 @@ as `id_token_hint`, so the upstream IdP can honor the app-level Allowed Logout U ## Operator configuration -Minimum settings for SSO. Supply at least one of `AdminSettings:Admins` or -`AdminSettings:Oidc:GroupPrefix`. +Required settings depend on deployment. + +### Cloud ```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 - // where 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-" } } ``` + +IdP groups must be named `` where `` 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. + +### Self-hosted + +```jsonc +"AdminSettings": { + "Admins": "admin@example.com,another@example.com", + "Oidc": { + "Authority": "https://idp.example.com", + "ClientId": "admin-portal", + "ClientSecret": "..." + } +} +``` + +`AdminSettings:Admins` is a comma-separated list where each entry is either `email` or +`email:stamp`. The SSO sign-in gate matches on the email prefix of each entry, case-insensitive. +`AdminSettings:Oidc:GroupPrefix` is not required on self-hosted; role claims are not consulted. diff --git a/src/Admin/IdentityServer/ServiceCollectionExtensions.cs b/src/Admin/IdentityServer/ServiceCollectionExtensions.cs index ffb075551282..e9e76a2c2c5a 100644 --- a/src/Admin/IdentityServer/ServiceCollectionExtensions.cs +++ b/src/Admin/IdentityServer/ServiceCollectionExtensions.cs @@ -64,7 +64,9 @@ public static Tuple AddPasswordlessIdentitySer } public static IServiceCollection AddAdminUpstreamOidc( - this IServiceCollection services, AdminSettings adminSettings) + this IServiceCollection services, + AdminSettings adminSettings, + GlobalSettings globalSettings) { // Fail-closed: if the operator hasn't supplied Authority/ClientId/ClientSecret, we don't // register the handler at all. The SSO controller actions then return NotFound on any hit. @@ -85,16 +87,19 @@ public static IServiceCollection AddAdminUpstreamOidc( "AdminSettings:Oidc:EmailClaimType must not be null or empty when SSO is enabled."); } - // Fail-loud when SSO is enabled but no tenancy signal is configured. The sign-in - // tenancy gate requires at least one of: AdminSettings:Admins (CSV allowlist) or - // AdminSettings:Oidc:GroupPrefix (IdP groups → roles). Without either, every SSO - // sign-in would be rejected at the gate. - if (string.IsNullOrWhiteSpace(adminSettings.Admins) && - string.IsNullOrWhiteSpace(oidc.GroupPrefix)) + // Fail-loud when the tenancy signal required by the deployment type is missing. + // Self-hosted uses AdminSettings:Admins as the sign-in gate; cloud uses the + // "has at least one mapped role" gate, which requires AdminSettings:Oidc:GroupPrefix. + // See src/Admin/Auth/SSO.md. + if (globalSettings.SelfHosted && string.IsNullOrWhiteSpace(adminSettings.Admins)) { throw new InvalidOperationException( - "At least one of AdminSettings:Admins or AdminSettings:Oidc:GroupPrefix " + - "must be set when SSO is enabled."); + "AdminSettings:Admins must be set when SSO is enabled on a self-hosted deployment."); + } + if (!globalSettings.SelfHosted && string.IsNullOrWhiteSpace(oidc.GroupPrefix)) + { + throw new InvalidOperationException( + "AdminSettings:Oidc:GroupPrefix must be set when SSO is enabled on a cloud deployment."); } services.AddAuthentication() diff --git a/src/Admin/Startup.cs b/src/Admin/Startup.cs index 1173382159a9..d1e9a5accccf 100644 --- a/src/Admin/Startup.cs +++ b/src/Admin/Startup.cs @@ -80,7 +80,7 @@ public void ConfigureServices(IServiceCollection services) // Identity services.AddPasswordlessIdentityServices(globalSettings, adminSettings); - services.AddAdminUpstreamOidc(adminSettings); + services.AddAdminUpstreamOidc(adminSettings, globalSettings); services.Configure(options => { // This interval defines the maximum delay in access changes diff --git a/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs b/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs index 4f7be7ac29ee..84d1f1f5a28f 100644 --- a/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs +++ b/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs @@ -4,6 +4,7 @@ using Bit.Admin.Auth.IdentityServer; using Bit.Admin.Auth.Models; using Bit.Admin.Auth.Services; +using Bit.Core.Settings; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; @@ -279,11 +280,11 @@ await authService.Received(1).SignInAsync( } [Fact] - public async Task SsoSignIn_RedirectsError5_WhenGroupPrefixConfiguredAndPrincipalHasNoRoleClaims() + public async Task SsoSignIn_Cloud_RedirectsError5_WhenPrincipalHasNoRoleClaims() { - // Tenancy gate (groups-only configuration): principal must have at least one mapped - // admin role. See src/Admin/Auth/SSO.md. - var controller = BuildController(oidcEnabled: true, out var builder); + // Cloud tenancy gate: principal must have at least one mapped admin role. + // See src/Admin/Auth/SSO.md. + var controller = BuildController(oidcEnabled: true, out var builder, selfHosted: false); var authService = AttachAuthenticationServices(controller); StubExternalPrincipal(authService, new Claim("email", "you@example.com"), @@ -307,41 +308,16 @@ await authService.Received(1).SignOutAsync( } [Fact] - public async Task SsoSignIn_RedirectsError5_WhenAdminsConfiguredAndEmailNotOnList() + public async Task SsoSignIn_Cloud_SignsIn_WhenPrincipalHasRoleClaim_RegardlessOfAdmins() { - // Tenancy gate (CSV-only configuration): email must be on AdminSettings:Admins. + // Cloud ignores AdminSettings:Admins: tenancy is role-presence. var controller = BuildController(oidcEnabled: true, out var builder, - adminsAllowlist: "admin@example.com", groupPrefix: null); + adminsAllowlist: null, selfHosted: false); var authService = AttachAuthenticationServices(controller); StubExternalPrincipal(authService, new Claim("email", "stranger@example.com"), new Claim("email_verified", "true")); - builder.Build(Arg.Any(), Arg.Any()) - .Returns(PrincipalWithRole("owner")); - - var result = await controller.SsoSignIn(); - - var redirect = Assert.IsType(result); - Assert.Equal(5, redirect.RouteValues!["error"]); - await authService.DidNotReceive().SignInAsync( - Arg.Any(), - AdminAuthenticationSchemes.SsoCookie, - Arg.Any(), - Arg.Any()); - } - - [Fact] - public async Task SsoSignIn_SignsIn_WhenOnlyAdminsConfiguredAndEmailOnList_EvenWithoutRoleClaims() - { - // CSV-only mode: with GroupPrefix unset, groups gate is skipped. A principal with no - // role claims is admitted as long as the email is on the Admins allowlist. - var controller = BuildController(oidcEnabled: true, out var builder, - adminsAllowlist: "you@example.com", groupPrefix: null); - var authService = AttachAuthenticationServices(controller); - StubExternalPrincipal(authService, - new Claim("email", "you@example.com"), - new Claim("email_verified", "true")); - var builtPrincipal = new ClaimsPrincipal(new ClaimsIdentity(authenticationType: "test")); + var builtPrincipal = PrincipalWithRole("owner"); builder.Build(Arg.Any(), Arg.Any()).Returns(builtPrincipal); await controller.SsoSignIn(); @@ -354,17 +330,17 @@ await authService.Received(1).SignInAsync( } [Fact] - public async Task SsoSignIn_RedirectsError5_WhenBothConfigured_AndEmailOnListButNoRole() + public async Task SsoSignIn_SelfHosted_RedirectsError5_WhenEmailNotOnAdmins() { - // Both gates configured: both must admit. CSV passes but groups does not. + // Self-hosted tenancy gate: email must be on AdminSettings:Admins. var controller = BuildController(oidcEnabled: true, out var builder, - adminsAllowlist: "you@example.com"); + adminsAllowlist: "admin@example.com", selfHosted: true); var authService = AttachAuthenticationServices(controller); StubExternalPrincipal(authService, - new Claim("email", "you@example.com"), + new Claim("email", "stranger@example.com"), new Claim("email_verified", "true")); builder.Build(Arg.Any(), Arg.Any()) - .Returns(new ClaimsPrincipal(new ClaimsIdentity(authenticationType: "test"))); + .Returns(PrincipalWithRole("owner")); var result = await controller.SsoSignIn(); @@ -378,35 +354,17 @@ await authService.DidNotReceive().SignInAsync( } [Fact] - public async Task SsoSignIn_RedirectsError5_WhenBothConfigured_AndRolePresentButEmailNotOnList() + public async Task SsoSignIn_SelfHosted_SignsIn_WhenEmailOnAdmins_RegardlessOfRoleClaims() { - // Both gates configured: both must admit. Groups passes but CSV does not. + // Self-hosted ignores role claims: tenancy is Admins CSV, authz is the SelfHosted + // short-circuit in AccessControlService. var controller = BuildController(oidcEnabled: true, out var builder, - adminsAllowlist: "admin@example.com"); - var authService = AttachAuthenticationServices(controller); - StubExternalPrincipal(authService, - new Claim("email", "stranger@example.com"), - new Claim("email_verified", "true")); - builder.Build(Arg.Any(), Arg.Any()) - .Returns(PrincipalWithRole("owner")); - - var result = await controller.SsoSignIn(); - - var redirect = Assert.IsType(result); - Assert.Equal(5, redirect.RouteValues!["error"]); - } - - [Fact] - public async Task SsoSignIn_SignsIn_WhenBothConfigured_AndBothGatesPass() - { - // Both gates configured: both admit. - var controller = BuildController(oidcEnabled: true, out var builder, - adminsAllowlist: "you@example.com"); + adminsAllowlist: "you@example.com", selfHosted: true); var authService = AttachAuthenticationServices(controller); StubExternalPrincipal(authService, new Claim("email", "you@example.com"), new Claim("email_verified", "true")); - var builtPrincipal = PrincipalWithRole("owner"); + var builtPrincipal = new ClaimsPrincipal(new ClaimsIdentity(authenticationType: "test")); builder.Build(Arg.Any(), Arg.Any()).Returns(builtPrincipal); await controller.SsoSignIn(); @@ -419,12 +377,12 @@ await authService.Received(1).SignInAsync( } [Fact] - public async Task SsoSignIn_SignsIn_WhenAdminsCsvEntryIncludesStampSuffix() + public async Task SsoSignIn_SelfHosted_SignsIn_WhenAdminsEntryIncludesStampSuffix() { - // AdminSettings:Admins supports "email:stamp" entries (used by the passwordless - // user store). The CSV gate matches on the email prefix only. + // AdminSettings:Admins supports "email:stamp" entries (used by the passwordless user + // store). The CSV gate matches on the email prefix only. var controller = BuildController(oidcEnabled: true, out var builder, - adminsAllowlist: "you@example.com:abc123", groupPrefix: null); + adminsAllowlist: "you@example.com:abc123", selfHosted: true); var authService = AttachAuthenticationServices(controller); StubExternalPrincipal(authService, new Claim("email", "you@example.com"), @@ -551,7 +509,8 @@ private static LoginController BuildController( bool requireEmailVerified = true, bool enablePasswordlessLogin = true, string adminsAllowlist = null, - string groupPrefix = "us-prd-cloud-adminportal-") + string groupPrefix = "us-prd-cloud-adminportal-", + bool selfHosted = false) { var settings = new AdminSettings { @@ -568,6 +527,7 @@ private static LoginController BuildController( } : null, }; + var globalSettings = new GlobalSettings { SelfHosted = selfHosted }; var userStore = Substitute.For>(); var userManager = Substitute.For>( @@ -593,7 +553,7 @@ private static LoginController BuildController( ssoSignInPrincipalBuilder = Substitute.For(); - return new LoginController(signInManager, Options.Create(settings), + return new LoginController(signInManager, Options.Create(settings), globalSettings, ssoSignInPrincipalBuilder, Substitute.For>()); } } diff --git a/test/Admin.Test/IdentityServer/ServiceCollectionExtensionsTests.cs b/test/Admin.Test/IdentityServer/ServiceCollectionExtensionsTests.cs index 71f94095e4f3..d4283c180915 100644 --- a/test/Admin.Test/IdentityServer/ServiceCollectionExtensionsTests.cs +++ b/test/Admin.Test/IdentityServer/ServiceCollectionExtensionsTests.cs @@ -2,6 +2,7 @@ using Bit.Admin; using Bit.Admin.Auth.IdentityServer; using Bit.Admin.IdentityServer; +using Bit.Core.Settings; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication.OpenIdConnect; using Microsoft.AspNetCore.Http; @@ -16,72 +17,83 @@ public class ServiceCollectionExtensionsTests [Fact] public void AddAdminUpstreamOidc_ThrowsWhenEmailClaimTypeIsNull() { - var services = new ServiceCollection(); - var adminSettings = new AdminSettings - { - Oidc = new AdminSettings.OidcSettings - { - Authority = "https://idp.example.com", - ClientId = "id", - ClientSecret = "secret", - GroupPrefix = "us-prd-cloud-adminportal-", - EmailClaimType = null, - } - }; - var ex = Assert.Throws(() => - services.AddAdminUpstreamOidc(adminSettings)); + new ServiceCollection().AddAdminUpstreamOidc( + BuildAdminSettings(emailClaimType: null), + BuildGlobalSettings(selfHosted: false))); Assert.Contains("EmailClaimType", ex.Message); } [Fact] - public void AddAdminUpstreamOidc_ThrowsWhenNoTenancySignalConfigured() + public void AddAdminUpstreamOidc_SelfHosted_ThrowsWhenAdminsEmpty() { - // The sign-in tenancy gate requires at least one of AdminSettings:Admins or - // AdminSettings:Oidc:GroupPrefix. With neither set, every SSO sign-in would be rejected - // at the gate - fail boot loudly rather than let the operator discover it the first - // time anyone tries SSO. - var services = new ServiceCollection(); - var adminSettings = new AdminSettings - { - Admins = null, - Oidc = new AdminSettings.OidcSettings - { - Authority = "https://idp.example.com", - ClientId = "id", - ClientSecret = "secret", - GroupPrefix = null, - } - }; - + // Self-hosted uses AdminSettings:Admins as the sign-in tenancy gate. Without it every + // SSO sign-in would be rejected at the gate. var ex = Assert.Throws(() => - services.AddAdminUpstreamOidc(adminSettings)); + new ServiceCollection().AddAdminUpstreamOidc( + BuildAdminSettings(admins: null, groupPrefix: "us-prd-adminportal-"), + BuildGlobalSettings(selfHosted: true))); Assert.Contains("Admins", ex.Message); + Assert.Contains("self-hosted", ex.Message); + } + + [Fact] + public void AddAdminUpstreamOidc_Cloud_ThrowsWhenGroupPrefixEmpty() + { + // Cloud uses "has at least one mapped role" as the sign-in tenancy gate, which requires + // AdminSettings:Oidc:GroupPrefix. Without it every SSO sign-in would be rejected. + var ex = Assert.Throws(() => + new ServiceCollection().AddAdminUpstreamOidc( + BuildAdminSettings(admins: "admin@example.com", groupPrefix: null), + BuildGlobalSettings(selfHosted: false))); + Assert.Contains("GroupPrefix", ex.Message); + Assert.Contains("cloud", ex.Message); + } + + [Fact] + public void AddAdminUpstreamOidc_SelfHosted_Succeeds_WhenAdminsConfigured() + { + var services = new ServiceCollection(); + services.AddLogging(); + + services.AddAdminUpstreamOidc( + BuildAdminSettings(admins: "admin@example.com", groupPrefix: null), + BuildGlobalSettings(selfHosted: true)); } [Fact] - public void AddAdminUpstreamOidc_Succeeds_WhenOnlyAdminsConfigured() + public void AddAdminUpstreamOidc_Cloud_Succeeds_WhenGroupPrefixConfigured() { - // CSV-only mode: AdminSettings:Admins is the tenancy signal, no group mapping needed. var services = new ServiceCollection(); services.AddLogging(); - var adminSettings = new AdminSettings + + services.AddAdminUpstreamOidc( + BuildAdminSettings(admins: null, groupPrefix: "us-prd-adminportal-"), + BuildGlobalSettings(selfHosted: false)); + } + + private static AdminSettings BuildAdminSettings( + string admins = "admin@example.com", + string groupPrefix = "us-prd-adminportal-", + string emailClaimType = "email") => + new() { - Admins = "admin@example.com", + Admins = admins, Oidc = new AdminSettings.OidcSettings { Authority = "https://idp.example.com", ClientId = "id", ClientSecret = "secret", - GroupPrefix = null, - } + GroupPrefix = groupPrefix, + EmailClaimType = emailClaimType, + }, }; - services.AddAdminUpstreamOidc(adminSettings); - } + private static GlobalSettings BuildGlobalSettings(bool selfHosted) => + new() { SelfHosted = selfHosted }; [Fact] public async Task AddAdminUpstreamOidc_OnTokenValidated_FailsWhenAuthTimeMissingAsync() @@ -133,16 +145,7 @@ private static OpenIdConnectOptions BuildOidcOptions() { var services = new ServiceCollection(); services.AddLogging(); - services.AddAdminUpstreamOidc(new AdminSettings - { - Oidc = new AdminSettings.OidcSettings - { - Authority = "https://idp.example.com", - ClientId = "id", - ClientSecret = "secret", - GroupPrefix = "us-prd-cloud-adminportal-", - } - }); + services.AddAdminUpstreamOidc(BuildAdminSettings(), BuildGlobalSettings(selfHosted: false)); var sp = services.BuildServiceProvider(); return sp.GetRequiredService>() From d54125fb339fb9282665eb563a036d4d0e1e1afc Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Sat, 17 Oct 2026 21:48:42 -0400 Subject: [PATCH 10/12] Rename Auth docs to README.md and cover both sign-in paths The doc no longer covers only SSO: both the passwordless email path and the SSO path are described, with their respective tenancy gates, authorization models, and operator configurations laid out side by side. Dropped the SSO-specific "sign-in flow" numbered-steps section - the narrative-paragraph introductions to each path section cover what a reader needs to understand. Code references to src/Admin/Auth/SSO.md are updated to the new path. --- src/Admin/Auth/Controllers/LoginController.cs | 2 +- src/Admin/Auth/README.md | 133 +++++++++++++++++ src/Admin/Auth/SSO.md | 134 ------------------ .../ServiceCollectionExtensions.cs | 2 +- .../Auth/Controllers/LoginControllerTests.cs | 2 +- 5 files changed, 136 insertions(+), 137 deletions(-) create mode 100644 src/Admin/Auth/README.md delete mode 100644 src/Admin/Auth/SSO.md diff --git a/src/Admin/Auth/Controllers/LoginController.cs b/src/Admin/Auth/Controllers/LoginController.cs index ccd3624feaa7..d7213b730188 100644 --- a/src/Admin/Auth/Controllers/LoginController.cs +++ b/src/Admin/Auth/Controllers/LoginController.cs @@ -180,7 +180,7 @@ public async Task SsoSignIn(string returnUrl = null, string remot // claims are irrelevant. // - Cloud: principal must carry at least one role claim. RBAC drives authorization // downstream via RolePermissionMapping. - // See src/Admin/Auth/SSO.md. + // See src/Admin/Auth/README.md. if (_globalSettings.SelfHosted) { if (!IsOnAdminAllowlist(email)) diff --git a/src/Admin/Auth/README.md b/src/Admin/Auth/README.md new file mode 100644 index 000000000000..b7cc597a33d8 --- /dev/null +++ b/src/Admin/Auth/README.md @@ -0,0 +1,133 @@ +# Admin Portal sign-in + +How the Admin Portal admits users. Audience: engineers modifying `LoginController`, +`PasswordlessSignInManager`, `SsoSignInPrincipalBuilder`, `GroupClaimRoleMapper`, +`ReadOnlyEnvIdentityUserStore`, or `AdminSettings`; operators provisioning Admin Portal access. + +## Sign-in paths + +The Admin Portal supports two sign-in paths. An operator can enable either or both. + +| Path | Enabled by | Writes cookie | +|---|---|---| +| Passwordless email | `AdminSettings:EnablePasswordlessLogin = true` (default) | `IdentityConstants.ApplicationScheme` | +| Upstream OIDC SSO | `AdminSettings:Oidc:*` configured | `AdminAuthenticationSchemes.SsoCookie` | + +## Passwordless email path + +The user requests a magic link from the login page. `PasswordlessSignInManager` emails a +one-time URL that confirms the login via `GET /login/confirm?...`. Identity's user lookup goes +through `ReadOnlyEnvIdentityUserStore`, which reads from `AdminSettings:Admins` — any email not on +that CSV is unknown to the user store and cannot complete the magic-link exchange. + +| | Cloud | Self-hosted | +|---|---|---| +| Tenancy gate | Email is on `AdminSettings:Admins` | Email is on `AdminSettings:Admins` | +| Authorization | Role claim comes from `adminSettings:role:{roleName}` CSV sub-config, matched against the authenticated email. Permissions come from `RolePermissionMapping` | Full access (`AccessControlService.UserHasPermission` short-circuits on `_globalSettings.SelfHosted`) | + +### Operator configuration + +```jsonc +"AdminSettings": { + "EnablePasswordlessLogin": true, + "Admins": "admin@example.com,another@example.com", + // On cloud only — map each admin email to a role. + "role": { + "owner": "admin@example.com", + "billing": "another@example.com" + } +} +``` + +`AdminSettings:Admins` is a comma-separated list where each entry is either `email` or +`email:stamp`. The optional stamp populates `IdentityUser.SecurityStamp`; rotating a stamp +invalidates the Identity cookie for that admin on the next 30s re-check. + +## Upstream OIDC SSO path + +The user is redirected to the configured IdP, authenticates there, and comes back with a verified +email claim. `SsoSignInPrincipalBuilder` hand-builds the principal (bypassing `SignInManager` / +`UserManager`, so there is no local user record and no `SecurityStamp` to track), stamping +`ClaimTypes.Role` claims from the IdP's `groups` claim via `GroupClaimRoleMapper`. The sign-in +cookie goes to a dedicated scheme (not the Identity cookie), and session lifetime is bounded by +cookie expiry and RP-initiated logout. + +| | Cloud | Self-hosted | +|---|---|---| +| Tenancy gate | Principal has at least one `ClaimTypes.Role` claim | Authenticated email is on `AdminSettings:Admins` | +| Authorization | Union of permissions across role claims via `RolePermissionMapping` | Full access (`AccessControlService.UserHasPermission` short-circuits on `_globalSettings.SelfHosted`) | +| Role claims come from | `AdminSettings:Oidc:GroupPrefix` → `GroupClaimRoleMapper` maps IdP `groups` to roles | n/a — role claims are irrelevant on self-hosted | +| Boot-time required config | `AdminSettings:Oidc:GroupPrefix` | `AdminSettings:Admins` | + +`AddAdminUpstreamOidc` fails loud at boot when the deployment-required config is missing. + +### Operator configuration + +Cloud: + +```jsonc +"AdminSettings": { + "Oidc": { + "Authority": "https://idp.example.com", + "ClientId": "admin-portal", + "ClientSecret": "...", + "GroupPrefix": "us-prd-cloud-adminportal-" + } +} +``` + +IdP groups must be named `` where `` 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. + +Self-hosted: + +```jsonc +"AdminSettings": { + "Admins": "admin@example.com,another@example.com", + "Oidc": { + "Authority": "https://idp.example.com", + "ClientId": "admin-portal", + "ClientSecret": "..." + } +} +``` + +`GroupPrefix` is not required on self-hosted; role claims are not consulted. + +## Authentication, tenancy, and authorization + +Both paths keep three concerns distinct: + +- **Authentication** — the sign-in path proves the user owns the claimed email. Passwordless does + this via the one-time magic link; SSO does it via the IdP plus the `email_verified` check. +- **Tenancy** — whether the authenticated user is a user of the Admin Portal. Enforced by the + per-path, per-deployment gates in the tables above. +- **Authorization** — what capabilities the authenticated member has. On cloud, enforced by + `AccessControlService.UserHasPermission` against role claims via `RolePermissionMapping`. On + self-hosted, the same method short-circuits to `true` on `_globalSettings.SelfHosted`. + +## 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. diff --git a/src/Admin/Auth/SSO.md b/src/Admin/Auth/SSO.md deleted file mode 100644 index 8ae88efbdfdd..000000000000 --- a/src/Admin/Auth/SSO.md +++ /dev/null @@ -1,134 +0,0 @@ -# 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 | -|---|---| -| Passwordless email | `AdminSettings:EnablePasswordlessLogin = true` (default) | -| Upstream OIDC SSO | `AdminSettings:Oidc:*` configured | - -Everything below describes the SSO path. - -## Deployment-driven tenancy and authorization - -The SSO sign-in gate and the authorization model are determined by deployment type. Operators do -not choose between them. - -| | Cloud | Self-hosted | -|---|---|---| -| **Tenancy gate** (admits sign-in) | Principal has at least one `ClaimTypes.Role` claim | Authenticated email is on `AdminSettings:Admins` | -| **Authorization** (what the admitted user can do) | Union of permissions across role claims, via `RolePermissionMapping` | Full access (`AccessControlService.UserHasPermission` short-circuits on `_globalSettings.SelfHosted`) | -| **Role claims come from** | `AdminSettings:Oidc:GroupPrefix` → `GroupClaimRoleMapper` maps IdP `groups` claim to roles | n/a — role claims are irrelevant on self-hosted | -| **Boot-time required config** | `AdminSettings:Oidc:GroupPrefix` must be set | `AdminSettings:Admins` must be set | - -`AddAdminUpstreamOidc` fails loud at boot when the required config for the deployment type is -missing. - -## 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 the deployment-specific tenancy gate (see table above). - - 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 - deployment-driven gate in `LoginController.SsoSignIn`. -- **Authorization** — what capabilities the authenticated member has. On cloud, enforced by - `AccessControlService.UserHasPermission` against the role claims on the principal using - `RolePermissionMapping`. On self-hosted, the same method short-circuits to `true` on - `_globalSettings.SelfHosted`. - -## 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 - -Required settings depend on deployment. - -### Cloud - -```jsonc -"AdminSettings": { - "Oidc": { - "Authority": "https://idp.example.com", - "ClientId": "admin-portal", - "ClientSecret": "...", - "GroupPrefix": "us-prd-cloud-adminportal-" - } -} -``` - -IdP groups must be named `` where `` 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. - -### Self-hosted - -```jsonc -"AdminSettings": { - "Admins": "admin@example.com,another@example.com", - "Oidc": { - "Authority": "https://idp.example.com", - "ClientId": "admin-portal", - "ClientSecret": "..." - } -} -``` - -`AdminSettings:Admins` is a comma-separated list where each entry is either `email` or -`email:stamp`. The SSO sign-in gate matches on the email prefix of each entry, case-insensitive. -`AdminSettings:Oidc:GroupPrefix` is not required on self-hosted; role claims are not consulted. diff --git a/src/Admin/IdentityServer/ServiceCollectionExtensions.cs b/src/Admin/IdentityServer/ServiceCollectionExtensions.cs index e9e76a2c2c5a..ffade3b6702e 100644 --- a/src/Admin/IdentityServer/ServiceCollectionExtensions.cs +++ b/src/Admin/IdentityServer/ServiceCollectionExtensions.cs @@ -90,7 +90,7 @@ public static IServiceCollection AddAdminUpstreamOidc( // Fail-loud when the tenancy signal required by the deployment type is missing. // Self-hosted uses AdminSettings:Admins as the sign-in gate; cloud uses the // "has at least one mapped role" gate, which requires AdminSettings:Oidc:GroupPrefix. - // See src/Admin/Auth/SSO.md. + // See src/Admin/Auth/README.md. if (globalSettings.SelfHosted && string.IsNullOrWhiteSpace(adminSettings.Admins)) { throw new InvalidOperationException( diff --git a/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs b/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs index 84d1f1f5a28f..8484123c29ec 100644 --- a/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs +++ b/test/Admin.Test/Auth/Controllers/LoginControllerTests.cs @@ -283,7 +283,7 @@ await authService.Received(1).SignInAsync( public async Task SsoSignIn_Cloud_RedirectsError5_WhenPrincipalHasNoRoleClaims() { // Cloud tenancy gate: principal must have at least one mapped admin role. - // See src/Admin/Auth/SSO.md. + // See src/Admin/Auth/README.md. var controller = BuildController(oidcEnabled: true, out var builder, selfHosted: false); var authService = AttachAuthenticationServices(controller); StubExternalPrincipal(authService, From 92ff0e72ac4e13c9cb024cf6bd240380681897dc Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Sun, 18 Oct 2026 21:14:21 -0400 Subject: [PATCH 11/12] Fix SSO sessions hidden by _Layout.cshtml and refresh stale comments The layout used SignInManager.IsSignedIn(User), which checks the principal's AuthenticationType against IdentityConstants.ApplicationScheme. SSO principals carry AdminAuthenticationSchemes.SsoCookie as their AuthenticationType to honestly reflect the scheme they're signed into, so the IsSignedIn check returned false for them and the layout hid the nav and Log Out button. The root cause is the layout coupling to a scheme-specific signal. Switch it to User.Identity.IsAuthenticated, which is scheme-agnostic and satisfied by either sign-in path once the authorization DefaultPolicy's PolicyEvaluator has populated HttpContext.User. Also refresh the stale AdminSettings.Admins, AdminSettings.Oidc. GroupPrefix, and GroupClaimRoleMapper constructor comments. Both the Admins field and the GroupPrefix field have deployment-dependent roles that the old comments did not capture. --- src/Admin/AdminSettings.cs | 21 ++++++++++++------- .../Auth/Services/GroupClaimRoleMapper.cs | 7 ++++--- src/Admin/Views/Shared/_Layout.cshtml | 9 +++++--- 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/src/Admin/AdminSettings.cs b/src/Admin/AdminSettings.cs index 733172471790..bd76213a4c77 100644 --- a/src/Admin/AdminSettings.cs +++ b/src/Admin/AdminSettings.cs @@ -5,10 +5,16 @@ namespace Bit.Admin; public class AdminSettings { - // 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. + // Comma-separated allowlist of email addresses permitted to sign in to the Admin Portal. + // Entries are "email" or "email:stamp" (the optional stamp populates IdentityUser + // .SecurityStamp for the passwordless user store). + // + // Used by: + // - Passwordless sign-in (both deployments) - tenancy gate. + // - SSO sign-in on self-hosted - tenancy gate. + // + // Not consulted on cloud SSO, where tenancy comes from AdminSettings:Oidc:GroupPrefix + // mapped role claims. See src/Admin/Auth/README.md. public virtual string Admins { get; set; } // Retention window for soft-deleted items before the background cleanup job hard-deletes them. @@ -79,9 +85,10 @@ public class OidcSettings // Prefix the IdP uses on Admin Portal group names (e.g. `us-prd-cloud-adminportal-`). // The suffix after the prefix is matched case-insensitively against the known role names // in RolePermissionMapping (owner, admin, billing, cs, sales). A group that does not - // start with this prefix, or whose suffix is not a known role, is ignored. A principal - // with no mapped groups still signs in, but holds no role claims and therefore no - // permissions. + // start with this prefix, or whose suffix is not a known role, is ignored. + // + // Required at boot when SSO is enabled on cloud, where "has at least one mapped role" + // is the tenancy gate. Not required on self-hosted. See src/Admin/Auth/README.md. public string GroupPrefix { get; set; } } } diff --git a/src/Admin/Auth/Services/GroupClaimRoleMapper.cs b/src/Admin/Auth/Services/GroupClaimRoleMapper.cs index d8d09b055205..8581f151bc9b 100644 --- a/src/Admin/Auth/Services/GroupClaimRoleMapper.cs +++ b/src/Admin/Auth/Services/GroupClaimRoleMapper.cs @@ -27,9 +27,10 @@ public class GroupClaimRoleMapper : IGroupClaimRoleMapper 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. + // Tolerant of an unset prefix so the type can be registered unconditionally. On cloud + // SSO, AddAdminUpstreamOidc fail-louds at boot when GroupPrefix is unset. On self-hosted + // SSO, GroupPrefix is optional - role claims are not consulted for authorization and + // MapRoles returns an empty set when the prefix is null. _groupPrefix = adminSettings?.Oidc?.GroupPrefix; } diff --git a/src/Admin/Views/Shared/_Layout.cshtml b/src/Admin/Views/Shared/_Layout.cshtml index fa5100bf5ca6..3b065c8457c1 100644 --- a/src/Admin/Views/Shared/_Layout.cshtml +++ b/src/Admin/Views/Shared/_Layout.cshtml @@ -1,12 +1,15 @@ @using Bit.Admin.Enums; @using Bit.Core; -@inject SignInManager SignInManager @inject Bit.Core.Settings.GlobalSettings GlobalSettings @inject Bit.Admin.Services.IAccessControlService AccessControlService @inject Bit.Core.Services.IFeatureService FeatureService @{ + // Scheme-agnostic: a passwordless session on Identity's application cookie and an SSO + // session on AdminAuthenticationSchemes.SsoCookie both satisfy IsAuthenticated once the + // authorization DefaultPolicy's PolicyEvaluator has populated HttpContext.User. + var isSignedIn = User.Identity?.IsAuthenticated == true; var canViewUsers = AccessControlService.UserHasPermission(Permission.User_List_View); var canViewOrgs = AccessControlService.UserHasPermission(Permission.Org_List_View); var canViewProviders = AccessControlService.UserHasPermission(Permission.Provider_List_View); @@ -48,7 +51,7 @@