Skip to content
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions src/Admin/AdminSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ DEBT: After 9d24324, this comment says Admins "does not gate SSO sign-in". On self-hosted it is now the only SSO gate, and boot fails if it is empty.

Details and fix

LoginController.SsoSignIn checks IsOnAdminAllowlist(email) when _globalSettings.SelfHosted, and AddAdminUpstreamOidc throws on a self-hosted deployment when Admins is empty. A self-hosted operator who reads this comment would conclude that SSO admins are managed through IdP groups and leave them off this list. They would then be locked out of SSO, or the app would fail to boot.

The comments elsewhere in this change describe the earlier design and should change with it:

  • AdminSettings.cs:82-84 (GroupPrefix): "A principal with no mapped groups still signs in". On cloud that principal is now rejected. This leftover was already pointed out on the LoginController thread and is still here.
  • GroupClaimRoleMapper.cs constructor: "when OIDC is enabled, AddAdminUpstreamOidc fail-louds on an empty prefix ... so a null here means OIDC is off". On self-hosted, OIDC can be enabled with no prefix set. The behavior is still correct (it returns an empty set), but the reasoning in the comment is wrong. The MapRoles_ReturnsEmpty_WhenPrefixUnset test comment has the same problem.

Suggested wording for this comment:

// Comma-separated allowlist of email addresses ("email" or "email:stamp").
// Passwordless path: gates sign-in on every deployment.
// SSO path: gates sign-in on self-hosted only (required at boot when SSO is enabled there).
// Cloud SSO uses IdP group claims mapped via AdminSettings:Oidc:GroupPrefix instead.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

92ff0e7 and 7379247 fix most of this. The Admins, GroupPrefix, and GroupClaimRoleMapper constructor comments now describe the deployment-driven split. Two test comments are still stale:

  • test/Admin.Test/Auth/Services/GroupClaimRoleMapperTests.cs:114-116 (MapRoles_ReturnsEmpty_WhenPrefixUnset) says a null prefix only happens "in the OIDC-off topology". On self-hosted SSO, a null prefix is now a supported configuration.
  • test/Admin.Test/Auth/Services/SsoSignInPrincipalBuilderTests.cs:54-56 (Build_StampsNoRoleClaims_WhenNoGroupsMap) says a user with no mapped groups "still gets a valid principal". On cloud, LoginController.SsoSignIn now rejects that principal before sign-in. The builder still returns the principal, but the controller does not sign it in.

These are minor and do not block merge.

public virtual string Admins { get; set; }

// Retention window for soft-deleted items before the background cleanup job hard-deletes them.
Expand Down Expand Up @@ -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; }
}
}
40 changes: 21 additions & 19 deletions src/Admin/Auth/Controllers/LoginController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -16,19 +17,19 @@ namespace Bit.Admin.Auth.Controllers;
public class LoginController : Controller
{
private readonly PasswordlessSignInManager<IdentityUser> _signInManager;
private readonly UserManager<IdentityUser> _userManager;
private readonly AdminSettings _adminSettings;
private readonly ISsoSignInPrincipalBuilder _ssoSignInPrincipalBuilder;
private readonly ILogger<LoginController> _logger;

public LoginController(
PasswordlessSignInManager<IdentityUser> signInManager,
UserManager<IdentityUser> userManager,
IOptions<AdminSettings> adminSettings,
ISsoSignInPrincipalBuilder ssoSignInPrincipalBuilder,
ILogger<LoginController> logger)
{
_signInManager = signInManager;
_userManager = userManager;
_adminSettings = adminSettings.Value;
_ssoSignInPrincipalBuilder = ssoSignInPrincipalBuilder;
_logger = logger;
}

Expand Down Expand Up @@ -143,16 +144,6 @@ public async Task<IActionResult> SsoSignIn(string returnUrl = null, string remot
return RedirectToAction("Index", new { error = 5 });
}

var user = await _userManager.FindByEmailAsync(email);
if (user == null)
{
// Same error code as other reject branches. Distinct codes would let any authenticated
// IdP user enumerate which emails are on the admin allowlist.
_logger.LogWarning("SSO sign-in rejected: principal not on admin allowlist.");
await HttpContext.SignOutAsync(IdentityConstants.ExternalScheme);
return RedirectToAction("Index", new { error = 5 });
}

// Set IsPersistent = false, so SSO sessions die when the browser closes.
// The passwordless flow uses IsPersistent = true and survives a browser restart.
var props = new AuthenticationProperties { IsPersistent = false };
Expand All @@ -171,7 +162,14 @@ public async Task<IActionResult> SsoSignIn(string returnUrl = null, string remot
props.StoreTokens([new AuthenticationToken { Name = "id_token", Value = idToken }]);
}

await _signInManager.SignInWithClaimsAsync(user, props, Array.Empty<Claim>());
// SSO signs in to its own dedicated cookie scheme with a hand-built principal 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.
var principal = _ssoSignInPrincipalBuilder.Build(email, external.Principal);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ IMPORTANT: An SSO user with no mapped groups still gets a cookie, and several [Authorize] actions have no RequirePermission gate, so in cloud that user can read and change customer data.

Details and fix

The allowlist check was removed and nothing replaced it. In cloud, SsoSignIn now signs in any IdP-authenticated user with a verified email, even when MapRoles returns nothing. The PR description says such a user is denied "every gated action". But these actions under [Authorize] are not gated by RequirePermission:

  • UsersController.View / Edit (GET): user email, ciphers metadata, billing and billing history
  • OrganizationsController.View / Edit (GET), plus TriggerBillingSync and ResendOwnerInvite (POST)

Before this change, only operators on the AdminSettings:Admins allowlist could reach those actions. Now anyone the IdP authenticates for the app can reach them.

The comment added in AddAdminUpstreamOidc already assumes this case is refused ("every SSO sign-in would be rejected as 'no mapped group'"), but the code never refuses it. Suggested fix: when not self-hosted and the mapper returns no roles, reject the sign-in the same way the other branches do:

var principal = _ssoSignInPrincipalBuilder.Build(email, external.Principal);
if (!_globalSettings.SelfHosted && !principal.HasClaim(c => c.Type == ClaimTypes.Role))
{
    _logger.LogWarning("SSO sign-in rejected: principal holds no mapped Admin Portal group.");
    await HttpContext.SignOutAsync(IdentityConstants.ExternalScheme);
    return RedirectToAction("Index", new { error = 5 });
}

Add a test for the rejection, and update the PR description and the AdminSettings.GroupPrefix comment, which say no-group principals "still sign in".

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in 1dfda58. SsoSignIn now rejects any principal with no ClaimTypes.Role claim before SignInAsync: it signs out the external scheme and redirects with the generic error 5. SsoSignIn_RedirectsError5_WhenPrincipalHasNoRoleClaims covers this. The gate runs on both self-hosted and cloud, which is stricter than what I suggested and fine.

One leftover: the GroupPrefix comment at src/Admin/AdminSettings.cs:83 still says a principal with no mapped groups "still signs in", and the Build_StampsNoRoleClaims_WhenNoGroupsMap test comment says the same. Both now contradict the gate and should be updated.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AdminSettings.cs GroupPrefix leftover was fixed in 92ff0e7. The Build_StampsNoRoleClaims_WhenNoGroupsMap test comment still says the user "still gets a valid principal". It is tracked in the stale-comments thread on AdminSettings.cs so it is not raised twice.

await HttpContext.SignInAsync(AdminAuthenticationSchemes.SsoCookie, principal, props);
await HttpContext.SignOutAsync(IdentityConstants.ExternalScheme);
_logger.LogInformation("SSO sign-in succeeded.");

Expand All @@ -187,12 +185,16 @@ public async Task<IActionResult> SsoSignIn(string returnUrl = null, string remot
[ValidateAntiForgeryToken]
public async Task<IActionResult> Logout()
{
// Presence of `id_token` on the app cookie means this is an SSO session (the
// passwordless flow never stores tokens), so it's both the "was SSO" signal and the
// value we need for id_token_hint. Read before SignOutAsync clears the cookie.
var idToken = await HttpContext.GetTokenAsync("id_token");

// Presence of `id_token` on the SSO cookie is both (a) the marker that this is an SSO
// session (the passwordless flow writes to a different cookie and stores no tokens)
// and (b) the value we need as id_token_hint on RP-initiated logout. Read before the
// sign-outs below clear the cookie.
var idToken = await HttpContext.GetTokenAsync(AdminAuthenticationSchemes.SsoCookie, "id_token");

// Sign out of both cookie schemes. SignOutAsync on a scheme the user isn't currently
// signed in to is a no-op, so this covers both passwordless and SSO sessions cleanly.
await _signInManager.SignOutAsync();
await HttpContext.SignOutAsync(AdminAuthenticationSchemes.SsoCookie);

var loggedOutRedirect = Url.Action(nameof(Index), "Login", new { success = 1 });

Expand Down
10 changes: 9 additions & 1 deletion src/Admin/Auth/IdentityServer/AdminAuthenticationSchemes.cs
Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
ο»Ώnamespace Bit.Admin.Auth.IdentityServer;
namespace Bit.Admin.Auth.IdentityServer;

Check failure on line 1 in src/Admin/Auth/IdentityServer/AdminAuthenticationSchemes.cs

View workflow job for this annotation

GitHub Actions / Lint

Fix file encoding.

public static class AdminAuthenticationSchemes
{
// Registered scheme name for the upstream federated OIDC handler. Used by controller
// actions to Challenge/SignOut the correct handler when the app has multiple auth
// schemes registered (this one plus ASP.NET Identity's application/external cookies).
public const string UpstreamOidc = "sso";

// Application cookie issued at the end of a successful SSO sign-in. Separate from the
// Identity application cookie by design: the SSO path has no local user store and no
// SecurityStamp to track - the IdP owns identity, so routing it through the Identity
// cookie (whose SecurityStampValidator does a 30s re-fetch against the user store) would
// sign the session out within 30 seconds. Startup's authorization DefaultPolicy lists
// both this scheme and IdentityConstants.ApplicationScheme so [Authorize] accepts either.
public const string SsoCookie = "sso-cookie";
}
65 changes: 65 additions & 0 deletions src/Admin/Auth/Services/GroupClaimRoleMapper.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
ο»Ώusing System.Security.Claims;
using Bit.Admin.Utilities;

namespace Bit.Admin.Auth.Services;

// Translates IdP group claims into Admin Portal role names.
//
// An IdP ships group memberships in the `groups` claim of the ID token. By
// convention each Admin Portal group is named `<GroupPrefix><role>` (e.g.
// `us-prd-cloud-adminportal-admin`); the mapper strips the prefix and keeps the suffix only if
// it matches one of the role names in RolePermissionMapping. The suffix match is
// case-insensitive. A principal in multiple matched groups gets the union of their roles.
//
// Decoupled from the ClaimsPrincipal source so the SSO sign-in controller and the AccessControl
// pipeline can share the same mapper and unit tests do not need to spin up ASP.NET.
public interface IGroupClaimRoleMapper
{
IReadOnlyCollection<string> MapRoles(ClaimsPrincipal principal);
}

public class GroupClaimRoleMapper : IGroupClaimRoleMapper
{
// The "groups" claim is an OIDC convention currently supported by all major IdPs.
public const string GroupsClaimType = "groups";

private readonly string? _groupPrefix;

public GroupClaimRoleMapper(AdminSettings adminSettings)
{
// Tolerant of an unset prefix so the type can be registered unconditionally. The SSO path
// is the only caller; when OIDC is enabled, AddAdminUpstreamOidc fail-louds on an empty
// prefix at startup, so a null here means OIDC is off and the mapper will never be hit.
_groupPrefix = adminSettings?.Oidc?.GroupPrefix;
}

public IReadOnlyCollection<string> MapRoles(ClaimsPrincipal principal)
{
if (principal is null || string.IsNullOrWhiteSpace(_groupPrefix))
{
return Array.Empty<string>();
}

var roles = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
foreach (var claim in principal.FindAll(GroupsClaimType))
{
if (string.IsNullOrWhiteSpace(claim.Value))
{
continue;
}

if (!claim.Value.StartsWith(_groupPrefix, StringComparison.OrdinalIgnoreCase))
{
continue;
}

var suffix = claim.Value.Substring(_groupPrefix.Length).Trim().ToLowerInvariant();
if (RolePermissionMapping.RolePermissions.ContainsKey(suffix))
{
roles.Add(suffix);
}
}

return roles;
}
}
51 changes: 51 additions & 0 deletions src/Admin/Auth/Services/SsoSignInPrincipalBuilder.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
ο»Ώ// 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 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. 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);
}

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));
Comment thread
trmartin4 marked this conversation as resolved.

foreach (var role in _groupClaimRoleMapper.MapRoles(externalPrincipal))
{
identity.AddClaim(new Claim(ClaimTypes.Role, role));
}

return new ClaimsPrincipal(identity);
}
}
9 changes: 9 additions & 0 deletions src/Admin/IdentityServer/ServiceCollectionExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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 =>
{
Expand Down
27 changes: 20 additions & 7 deletions src/Admin/Services/AccessControlService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -61,9 +67,16 @@ public string GetUserRole(string userEmail)
return userRole.Key.ToLowerInvariant();
}

private string GetUserRoleFromClaim()
private IEnumerable<string> 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());
}
}
Loading
Loading