From 8b92fafffe415402d67ac84e0a60557683b81454 Mon Sep 17 00:00:00 2001 From: Ike Kottlowski Date: Fri, 3 Jul 2026 13:51:38 -0400 Subject: [PATCH 1/5] feat: change to denylist for claim removal when refreshing token --- src/Core/Auth/Identity/Claims.cs | 37 +++++++++++++++++++ src/Identity/IdentityServer/ProfileService.cs | 17 +++++---- 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/src/Core/Auth/Identity/Claims.cs b/src/Core/Auth/Identity/Claims.cs index ac78e987ae3e..f92dbec35404 100644 --- a/src/Core/Auth/Identity/Claims.cs +++ b/src/Core/Auth/Identity/Claims.cs @@ -39,6 +39,43 @@ public static class CustomPermissions public const string ManageResetPassword = "manageresetpassword"; public const string ManageScim = "managescim"; } + + /// + /// The membership and authorization claim types that + /// is authoritative for. These are rebuilt + /// from the database on every token issuance, so the Identity Profile Service must not carry them over from the + /// existing subject when refreshing a token — a refreshed token should always reflect the member's current + /// organization and provider membership. + /// + /// Keep this in sync with BuildIdentityClaims. It intentionally excludes user-identity claims + /// (, email, email_verified, name, ) and non-membership claims + /// (, , , , send_*), + /// which are overwritten or preserved separately. + /// + /// + public static readonly IReadOnlySet MembershipClaimTypes = new HashSet + { + OrganizationOwner, + OrganizationAdmin, + OrganizationUser, + OrganizationCustom, + ProviderAdmin, + ProviderServiceUser, + SecretsManagerAccess, + CustomPermissions.AccessEventLogs, + CustomPermissions.AccessImportExport, + CustomPermissions.AccessReports, + CustomPermissions.CreateNewCollections, + CustomPermissions.EditAnyCollection, + CustomPermissions.DeleteAnyCollection, + CustomPermissions.ManageGroups, + CustomPermissions.ManagePolicies, + CustomPermissions.ManageSso, + CustomPermissions.ManageUsers, + CustomPermissions.ManageResetPassword, + CustomPermissions.ManageScim, + }; + public static class SendAccessClaims { public const string SendId = "send_id"; diff --git a/src/Identity/IdentityServer/ProfileService.cs b/src/Identity/IdentityServer/ProfileService.cs index 9ea8fcf471a9..c153dacb783d 100644 --- a/src/Identity/IdentityServer/ProfileService.cs +++ b/src/Identity/IdentityServer/ProfileService.cs @@ -53,7 +53,7 @@ public async Task GetProfileDataAsync(ProfileDataRequestContext context) // Whenever IdentityServer issues a new access token or services a UserInfo request, it calls // GetProfileDataAsync to determine which claims to include in the token or response. // In normal user identity scenarios, we have to look up the user to get their claims and update - // the issued claims collection as claim info can have changed since the last time the user logged in or the + // the issued claims collection as claim info may have changed since the last time the user logged in or the // last time the token was issued. var newClaims = new List(); var user = await _userService.GetUserByPrincipalAsync(context.Subject); @@ -73,13 +73,15 @@ public async Task GetProfileDataAsync(ProfileDataRequestContext context) } } - // filter out any of the new claims + // Determine which of the existingClaims to carry over onto the refreshed token. + // We do not carry over membership and authorization claims so each refresh token + // reflects the user's current membership and authorization state. var existingClaimsToKeep = existingClaims .Where(c => - // Drop any org claims - !c.Type.StartsWith("org") && - // If we have no new claims, then keep the existing claims - // If we have new claims, then keep the existing claim if it does not match a new claim type + // remove all membership claims so that the refreshed token reflects the user's current membership state + !Claims.MembershipClaimTypes.Contains(c.Type) && + // For all other (user-identity) claims: if we have no new claims, keep the existing claim; + // otherwise keep it only when it is not being overwritten by a freshly built claim of the same type. (newClaims.Count == 0 || !newClaims.Any(nc => nc.Type == c.Type)) ).ToList(); @@ -92,7 +94,8 @@ public async Task GetProfileDataAsync(ProfileDataRequestContext context) public async Task IsActiveAsync(IsActiveContext context) { - // Send Tokens are not refreshed so when the token has expired the user must request a new one via the authentication method assigned to the send. + // Send Tokens are not refreshed so when the token has expired the user must request a new one via + // the authentication method assigned to the send. if (context.Client.ClientId == BitwardenClient.Send) { context.IsActive = true; From 87b93328a158fb5195c701d8245b1f7858876ea6 Mon Sep 17 00:00:00 2001 From: Ike Kottlowski Date: Fri, 3 Jul 2026 13:52:31 -0400 Subject: [PATCH 2/5] test: move tests from generic IdentityServerTests to Grant specific tests for better readability and conceptualization. --- .../Endpoints/IdentityServerTests.cs | 23 -- .../Grants/RefreshTokenGrantTests.cs | 238 ++++++++++++++++++ 2 files changed, 238 insertions(+), 23 deletions(-) create mode 100644 test/Identity.IntegrationTest/Grants/RefreshTokenGrantTests.cs diff --git a/test/Identity.IntegrationTest/Endpoints/IdentityServerTests.cs b/test/Identity.IntegrationTest/Endpoints/IdentityServerTests.cs index 281f2eb2328c..68c3a0f197f5 100644 --- a/test/Identity.IntegrationTest/Endpoints/IdentityServerTests.cs +++ b/test/Identity.IntegrationTest/Endpoints/IdentityServerTests.cs @@ -216,29 +216,6 @@ public async Task TokenEndpoint_GrantTypePassword_WithNonOwnerOrAdmin_WithSsoPol await AssertRequiredSsoAuthenticationResponseAsync(context); } - [Theory, BitAutoData, RegisterFinishRequestModelCustomize] - public async Task TokenEndpoint_GrantTypeRefreshToken_Success(RegisterFinishRequestModel requestModel) - { - requestModel.UserAsymmetricKeys = TEST_ACCOUNT_KEYS; - var localFactory = new IdentityApplicationFactory(); - - var user = await localFactory.RegisterNewIdentityFactoryUserAsync(requestModel); - - var (_, refreshToken) = await localFactory.TokenFromPasswordAsync( - requestModel.Email, requestModel.MasterPasswordHash); - - var context = await localFactory.Server.PostAsync("/connect/token", - new FormUrlEncodedContent(new Dictionary - { - { "grant_type", "refresh_token" }, - { "client_id", "web" }, - { "refresh_token", refreshToken }, - })); - - using var body = await AssertDefaultTokenBodyAsync(context); - AssertRefreshTokenExists(body.RootElement); - } - [Theory, BitAutoData, RegisterFinishRequestModelCustomize] public async Task TokenEndpoint_GrantTypeClientCredentials_Success(RegisterFinishRequestModel model) { diff --git a/test/Identity.IntegrationTest/Grants/RefreshTokenGrantTests.cs b/test/Identity.IntegrationTest/Grants/RefreshTokenGrantTests.cs new file mode 100644 index 000000000000..8c0a3bc806ee --- /dev/null +++ b/test/Identity.IntegrationTest/Grants/RefreshTokenGrantTests.cs @@ -0,0 +1,238 @@ +using System.Text; +using System.Text.Json; +using Bit.Core.AdminConsole.Entities; +using Bit.Core.Auth.Identity; +using Bit.Core.Auth.Models.Api.Request.Accounts; +using Bit.Core.Entities; +using Bit.Core.Enums; +using Bit.Core.Repositories; +using Bit.Core.Test.Auth.AutoFixture; +using Bit.Identity.IdentityServer; +using Bit.Identity.IdentityServer.RequestValidators; +using Bit.IntegrationTestCommon.Factories; +using Bit.Test.Common.AutoFixture.Attributes; +using Bit.Test.Common.Helpers; +using NSubstitute; +using Xunit; + +namespace Bit.Identity.IntegrationTest.Grants; + +/// +/// Integration tests for the refresh_token grant: redeeming a refresh token at /connect/token to +/// obtain a new access token. On this flow IdentityServer re-invokes the Profile Service, which rebuilds the +/// user's membership claims from the database, so a refreshed token always reflects the member's current state. +/// +public class RefreshTokenGrantTests : IClassFixture +{ + private static readonly KeysRequestModel TEST_ACCOUNT_KEYS = new KeysRequestModel + { + AccountKeys = null, + PublicKey = "public-key", + EncryptedPrivateKey = "encrypted-private-key", + }; + + private const int SecondsInMinute = 60; + private const int MinutesInHour = 60; + private const int SecondsInHour = SecondsInMinute * MinutesInHour; + private readonly IdentityApplicationFactory _factory; + + public RefreshTokenGrantTests(IdentityApplicationFactory factory) + { + _factory = factory; + + // Bypass client version gating to isolate refresh-token grant behavior. + _factory.SubstituteService(svc => + { + svc.Validate(Arg.Any(), Arg.Any()) + .Returns(true); + }); + + ReinitializeDbForTests(_factory); + } + + [Theory, BitAutoData, RegisterFinishRequestModelCustomize] + public async Task RefreshToken_Success(RegisterFinishRequestModel requestModel) + { + requestModel.UserAsymmetricKeys = TEST_ACCOUNT_KEYS; + var localFactory = new IdentityApplicationFactory(); + + var user = await localFactory.RegisterNewIdentityFactoryUserAsync(requestModel); + + var (_, refreshToken) = await localFactory.TokenFromPasswordAsync( + requestModel.Email, requestModel.MasterPasswordHash); + + var context = await localFactory.Server.PostAsync("/connect/token", + new FormUrlEncodedContent(new Dictionary + { + { "grant_type", "refresh_token" }, + { "client_id", "web" }, + { "refresh_token", refreshToken }, + })); + + using var body = await AssertDefaultTokenBodyAsync(context); + AssertRefreshTokenExists(body.RootElement); + } + + /// + /// Membership claims are rebuilt from the database on every token issuance, so a refreshed access token + /// reflects the member's current organization membership. When a user is removed from an organization, the + /// organization claim present on the earlier token is not re-issued on refresh. + /// + [Theory, BitAutoData, RegisterFinishRequestModelCustomize] + public async Task RefreshToken_ReflectsCurrentOrganizationMembership( + RegisterFinishRequestModel requestModel) + { + requestModel.UserAsymmetricKeys = TEST_ACCOUNT_KEYS; + var localFactory = new IdentityApplicationFactory(); + + var user = await localFactory.RegisterNewIdentityFactoryUserAsync(requestModel); + + // Give the user an organization membership so the issued token carries an organization claim. + var organizationId = Guid.NewGuid(); + await CreateOrganizationMembershipAsync(localFactory, organizationId, user.Email, OrganizationUserType.Owner); + + // Initial password login: the access token reflects the current (present) membership. + var (accessToken, refreshToken) = await localFactory.TokenFromPasswordAsync( + requestModel.Email, requestModel.MasterPasswordHash); + var initialClaims = ReadJwtPayload(accessToken); + Assert.True(JwtPayloadContainsClaimValue(initialClaims, Claims.OrganizationOwner, organizationId.ToString())); + + // The user is removed from the organization. + var organizationUserRepository = localFactory.Services.GetRequiredService(); + var membership = await organizationUserRepository.GetByOrganizationAsync(organizationId, user.Id); + Assert.NotNull(membership); + await organizationUserRepository.DeleteAsync(membership); + + // Refresh the token: the rebuilt token reflects current membership, so the organization claim is gone. + var context = await localFactory.Server.PostAsync("/connect/token", + new FormUrlEncodedContent(new Dictionary + { + { "grant_type", "refresh_token" }, + { "client_id", "web" }, + { "refresh_token", refreshToken }, + })); + + using var body = await AssertDefaultTokenBodyAsync(context); + var refreshedAccessToken = AssertAccessTokenExists(body.RootElement); + var refreshedClaims = ReadJwtPayload(refreshedAccessToken); + Assert.False(refreshedClaims.TryGetProperty(Claims.OrganizationOwner, out _)); + } + + private static async Task CreateOrganizationMembershipAsync( + IdentityApplicationFactory localFactory, + Guid organizationId, + string username, + OrganizationUserType organizationUserType) + { + var userRepository = localFactory.Services.GetRequiredService(); + var organizationRepository = localFactory.Services.GetRequiredService(); + var organizationUserRepository = localFactory.Services.GetRequiredService(); + + await organizationRepository.CreateAsync(new Organization + { + Id = organizationId, + Name = $"Org Name | {organizationId}", + Enabled = true, + Plan = "Enterprise", + BillingEmail = $"billing-email+{organizationId}@example.com", + }); + + var user = await userRepository.GetByEmailAsync(username); + await organizationUserRepository.CreateAsync(new OrganizationUser + { + OrganizationId = organizationId, + UserId = user.Id, + Status = OrganizationUserStatusType.Confirmed, + Type = organizationUserType + }); + } + + /// + /// Decodes the payload segment of a JWT into its JSON claims for inspection. + /// + private static JsonElement ReadJwtPayload(string jwt) + { + var payload = jwt.Split('.')[1].Replace('-', '+').Replace('_', '/'); + payload += (payload.Length % 4) switch + { + 2 => "==", + 3 => "=", + _ => string.Empty + }; + + var json = Encoding.UTF8.GetString(Convert.FromBase64String(payload)); + return JsonDocument.Parse(json).RootElement.Clone(); + } + + /// + /// A claim may be serialized on the token as a single value or as an array of values, so both shapes are checked. + /// + private static bool JwtPayloadContainsClaimValue(JsonElement payload, string claimType, string value) + { + if (!payload.TryGetProperty(claimType, out var claim)) + { + return false; + } + + return claim.ValueKind == JsonValueKind.Array + ? claim.EnumerateArray().Any(element => element.GetString() == value) + : claim.GetString() == value; + } + + private static async Task AssertDefaultTokenBodyAsync(HttpContext httpContext, string expectedScope = "api offline_access", int expectedExpiresIn = SecondsInHour * 1) + { + var body = await AssertHelper.AssertResponseTypeIs(httpContext); + var root = body.RootElement; + + Assert.Equal(JsonValueKind.Object, root.ValueKind); + AssertAccessTokenExists(root); + AssertExpiresIn(root, expectedExpiresIn); + AssertTokenType(root); + AssertScope(root, expectedScope); + return body; + } + + private static void AssertTokenType(JsonElement tokenResponse) + { + var tokenTypeProperty = AssertHelper.AssertJsonProperty(tokenResponse, "token_type", JsonValueKind.String).GetString(); + Assert.Equal("Bearer", tokenTypeProperty); + } + + private static int AssertExpiresIn(JsonElement tokenResponse, int expectedExpiresIn = 3600) + { + var expiresIn = AssertHelper.AssertJsonProperty(tokenResponse, "expires_in", JsonValueKind.Number).GetInt32(); + Assert.Equal(expectedExpiresIn, expiresIn); + return expiresIn; + } + + private static string AssertAccessTokenExists(JsonElement tokenResponse) + { + return AssertHelper.AssertJsonProperty(tokenResponse, "access_token", JsonValueKind.String).GetString(); + } + + private static string AssertRefreshTokenExists(JsonElement tokenResponse) + { + return AssertHelper.AssertJsonProperty(tokenResponse, "refresh_token", JsonValueKind.String).GetString(); + } + + private static string AssertScopeExists(JsonElement tokenResponse) + { + return AssertHelper.AssertJsonProperty(tokenResponse, "scope", JsonValueKind.String).GetString(); + } + + private static void AssertScope(JsonElement tokenResponse, string expectedScope) + { + var actualScope = AssertScopeExists(tokenResponse); + Assert.Equal(expectedScope, actualScope); + } + + private void ReinitializeDbForTests(IdentityApplicationFactory factory) + { + var databaseContext = factory.GetDatabaseContext(); + databaseContext.Policies.RemoveRange(databaseContext.Policies); + databaseContext.OrganizationUsers.RemoveRange(databaseContext.OrganizationUsers); + databaseContext.Organizations.RemoveRange(databaseContext.Organizations); + databaseContext.Users.RemoveRange(databaseContext.Users); + databaseContext.SaveChanges(); + } +} From 23ccfa0dbf9f567120d6e0d4d4dbc9442d5c8c5e Mon Sep 17 00:00:00 2001 From: Ike Kottlowski Date: Fri, 3 Jul 2026 13:52:46 -0400 Subject: [PATCH 3/5] test: update tests for profile service --- .../IdentityServer/ProfileServiceTests.cs | 108 ++++++++++++++++-- 1 file changed, 101 insertions(+), 7 deletions(-) diff --git a/test/Identity.Test/IdentityServer/ProfileServiceTests.cs b/test/Identity.Test/IdentityServer/ProfileServiceTests.cs index 3ff458c6c179..8b482819806f 100644 --- a/test/Identity.Test/IdentityServer/ProfileServiceTests.cs +++ b/test/Identity.Test/IdentityServer/ProfileServiceTests.cs @@ -430,10 +430,104 @@ public async Task GetProfileDataAsync_WithProviders_IncludesProviderClaims( } /// - /// Evaluates the happy path for the core session invalidation mechanism. - /// Critical events (e.g., password change) update the security stamp, and any subsequent request through - /// this service should expose the stamp as invalid. A found user and matching security stamp - /// prove out an active session. + /// Provider claims are membership-derived and, like organization claims, are rebuilt fresh on every token + /// issuance. A provider claim carried on the existing subject (e.g. from the refresh-token flow) is not + /// re-issued, so a refreshed token reflects the user's current provider membership. + /// + /// + [Theory] + [BitAutoData(BitwardenClient.Web)] + [BitAutoData(BitwardenClient.Browser)] + [BitAutoData(BitwardenClient.Cli)] + [BitAutoData(BitwardenClient.Desktop)] + [BitAutoData(BitwardenClient.Mobile)] + [BitAutoData(BitwardenClient.DirectoryConnector)] + public async Task GetProfileDataAsync_FiltersOutProviderClaimsFromExisting( + string client, + [AuthFixtures.ProfileDataRequestContext] + ProfileDataRequestContext context, + User user) + { + context.Client.ClientId = client; + user.Id = Guid.Parse(context.Subject.FindFirst("sub").Value); + + var existingClaims = new[] + { + new Claim(Claims.ProviderAdmin, Guid.NewGuid().ToString()), + new Claim(Claims.ProviderServiceUser, Guid.NewGuid().ToString()), new Claim("email", "test@example.com"), + new Claim("name", "Test User") + }; + context.Subject = new ClaimsPrincipal(new ClaimsIdentity(existingClaims)); + + _userService.GetUserByPrincipalAsync(context.Subject).Returns(user); + _licensingService.ValidateUserPremiumAsync(user).Returns(false); + _currentContext.OrganizationMembershipAsync(_organizationUserRepository, user.Id) + .Returns(new List()); + _currentContext.ProviderMembershipAsync(_providerUserRepository, user.Id) + .Returns(new List()); + + await _sut.GetProfileDataAsync(context); + + Assert.DoesNotContain(context.IssuedClaims, issuedClaim => issuedClaim.Type.StartsWith("provider")); + Assert.Contains(context.IssuedClaims, issuedClaim => issuedClaim.Type == "email"); + Assert.Contains(context.IssuedClaims, issuedClaim => issuedClaim.Type == "name"); + } + + /// + /// When a provider user's role changes (e.g. from ProviderAdmin to ServiceUser), the freshly issued token + /// reflects only their current role. A provider claim for the prior role carried on the existing subject is not + /// re-issued alongside the newly built claim, so the refreshed token matches current provider membership. + /// + /// + [Theory] + [BitAutoData(BitwardenClient.Web)] + [BitAutoData(BitwardenClient.Browser)] + [BitAutoData(BitwardenClient.Cli)] + [BitAutoData(BitwardenClient.Desktop)] + [BitAutoData(BitwardenClient.Mobile)] + [BitAutoData(BitwardenClient.DirectoryConnector)] + public async Task GetProfileDataAsync_ProviderRoleChange_ReflectsCurrentRole( + string client, + [AuthFixtures.ProfileDataRequestContext] + ProfileDataRequestContext context, + User user) + { + context.Client.ClientId = client; + user.Id = Guid.Parse(context.Subject.FindFirst("sub").Value); + + var providerId = Guid.NewGuid(); + var existingClaims = new[] + { + // The user was previously a ProviderAdmin for this provider. + new Claim(Claims.ProviderAdmin, providerId.ToString()) + }; + context.Subject = new ClaimsPrincipal(new ClaimsIdentity(existingClaims)); + + // The user has since been demoted to ServiceUser for the same provider. + var providerMemberships = new List + { + new() { Id = providerId, Type = ProviderUserType.ServiceUser } + }; + + _userService.GetUserByPrincipalAsync(context.Subject).Returns(user); + _licensingService.ValidateUserPremiumAsync(user).Returns(false); + _currentContext.OrganizationMembershipAsync(_organizationUserRepository, user.Id) + .Returns(new List()); + _currentContext.ProviderMembershipAsync(_providerUserRepository, user.Id) + .Returns(providerMemberships); + + await _sut.GetProfileDataAsync(context); + + Assert.DoesNotContain(context.IssuedClaims, + issuedClaim => issuedClaim.Type == Claims.ProviderAdmin && issuedClaim.Value == providerId.ToString()); + Assert.Contains(context.IssuedClaims, + issuedClaim => issuedClaim.Type == Claims.ProviderServiceUser && issuedClaim.Value == providerId.ToString()); + } + + /// + /// Evaluates the happy path for refresh-token validation via the security stamp. + /// Certain account changes (e.g., password change) update the security stamp, so a request whose stamp no + /// longer matches is treated as inactive. A found user and matching security stamp prove out an active session. /// [Theory] [BitAutoData(BitwardenClient.Web)] @@ -464,9 +558,9 @@ public async Task IsActiveAsync_SecurityStampMatches_ReturnsTrue( } /// - /// Critical events (e.g., password change) update the security stamp, and any subsequent request through - /// this service should expose the stamp as invalid. - /// See also examples for stamp invalidation (non-exhaustive): + /// Certain account changes (e.g., password change) update the security stamp, so a request whose stamp no + /// longer matches the user's current stamp is treated as inactive. + /// See also examples that update the security stamp (non-exhaustive): /// /// /// From 710abc0d570fa31ee13a73ab61359b2becfb6a02 Mon Sep 17 00:00:00 2001 From: Ike Kottlowski Date: Mon, 6 Jul 2026 21:40:35 -0400 Subject: [PATCH 4/5] test: address finding in test file from PR reviewer --- .../Grants/RefreshTokenGrantTests.cs | 142 +++++++++++++++--- 1 file changed, 123 insertions(+), 19 deletions(-) diff --git a/test/Identity.IntegrationTest/Grants/RefreshTokenGrantTests.cs b/test/Identity.IntegrationTest/Grants/RefreshTokenGrantTests.cs index 8c0a3bc806ee..6f2ce3277c93 100644 --- a/test/Identity.IntegrationTest/Grants/RefreshTokenGrantTests.cs +++ b/test/Identity.IntegrationTest/Grants/RefreshTokenGrantTests.cs @@ -5,6 +5,7 @@ using Bit.Core.Auth.Models.Api.Request.Accounts; using Bit.Core.Entities; using Bit.Core.Enums; +using Bit.Core.Models.Data; using Bit.Core.Repositories; using Bit.Core.Test.Auth.AutoFixture; using Bit.Identity.IdentityServer; @@ -56,18 +57,18 @@ public async Task RefreshToken_Success(RegisterFinishRequestModel requestModel) requestModel.UserAsymmetricKeys = TEST_ACCOUNT_KEYS; var localFactory = new IdentityApplicationFactory(); - var user = await localFactory.RegisterNewIdentityFactoryUserAsync(requestModel); + await localFactory.RegisterNewIdentityFactoryUserAsync(requestModel); var (_, refreshToken) = await localFactory.TokenFromPasswordAsync( requestModel.Email, requestModel.MasterPasswordHash); - var context = await localFactory.Server.PostAsync("/connect/token", - new FormUrlEncodedContent(new Dictionary - { - { "grant_type", "refresh_token" }, - { "client_id", "web" }, - { "refresh_token", refreshToken }, - })); + using var formContent = new FormUrlEncodedContent(new Dictionary + { + { "grant_type", "refresh_token" }, + { "client_id", "web" }, + { "refresh_token", refreshToken }, + }); + var context = await localFactory.Server.PostAsync("/connect/token", formContent); using var body = await AssertDefaultTokenBodyAsync(context); AssertRefreshTokenExists(body.RootElement); @@ -104,13 +105,13 @@ public async Task RefreshToken_ReflectsCurrentOrganizationMembership( await organizationUserRepository.DeleteAsync(membership); // Refresh the token: the rebuilt token reflects current membership, so the organization claim is gone. - var context = await localFactory.Server.PostAsync("/connect/token", - new FormUrlEncodedContent(new Dictionary - { - { "grant_type", "refresh_token" }, - { "client_id", "web" }, - { "refresh_token", refreshToken }, - })); + using var formContent = new FormUrlEncodedContent(new Dictionary + { + { "grant_type", "refresh_token" }, + { "client_id", "web" }, + { "refresh_token", refreshToken }, + }); + var context = await localFactory.Server.PostAsync("/connect/token", formContent); using var body = await AssertDefaultTokenBodyAsync(context); var refreshedAccessToken = AssertAccessTokenExists(body.RootElement); @@ -118,11 +119,106 @@ public async Task RefreshToken_ReflectsCurrentOrganizationMembership( Assert.False(refreshedClaims.TryGetProperty(Claims.OrganizationOwner, out _)); } + /// + /// Membership claims are rebuilt from the database on every token issuance. A Custom-role member's granted + /// permissions are carried as individual claims; when a permission is removed, the corresponding claim must + /// not be re-issued on refresh. + /// + [Theory, BitAutoData, RegisterFinishRequestModelCustomize] + public async Task RefreshToken_ReflectsRemovedCustomPermission( + RegisterFinishRequestModel requestModel) + { + requestModel.UserAsymmetricKeys = TEST_ACCOUNT_KEYS; + var localFactory = new IdentityApplicationFactory(); + + var user = await localFactory.RegisterNewIdentityFactoryUserAsync(requestModel); + + // Give the user a Custom membership with the ManageUsers permission granted. + var organizationId = Guid.NewGuid(); + await CreateOrganizationMembershipAsync(localFactory, organizationId, user.Email, OrganizationUserType.Custom, + permissions: new Permissions { ManageUsers = true }); + + // Initial password login: the access token reflects the granted permission. + var (accessToken, refreshToken) = await localFactory.TokenFromPasswordAsync( + requestModel.Email, requestModel.MasterPasswordHash); + var initialClaims = ReadJwtPayload(accessToken); + Assert.True(JwtPayloadContainsClaimValue(initialClaims, Claims.CustomPermissions.ManageUsers, organizationId.ToString())); + + // The ManageUsers permission is removed. + var organizationUserRepository = localFactory.Services.GetRequiredService(); + var membership = await organizationUserRepository.GetByOrganizationAsync(organizationId, user.Id); + Assert.NotNull(membership); + membership.SetPermissions(new Permissions { ManageUsers = false }); + await organizationUserRepository.ReplaceAsync(membership); + + // Refresh the token: the rebuilt token reflects current permissions, so the ManageUsers claim is gone. + using var formContent = new FormUrlEncodedContent(new Dictionary + { + { "grant_type", "refresh_token" }, + { "client_id", "web" }, + { "refresh_token", refreshToken }, + }); + var context = await localFactory.Server.PostAsync("/connect/token", formContent); + + using var body = await AssertDefaultTokenBodyAsync(context); + var refreshedAccessToken = AssertAccessTokenExists(body.RootElement); + var refreshedClaims = ReadJwtPayload(refreshedAccessToken); + Assert.False(refreshedClaims.TryGetProperty(Claims.CustomPermissions.ManageUsers, out _)); + } + + /// + /// Membership claims are rebuilt from the database on every token issuance. Secrets Manager access is granted + /// per-organization; when access is removed, the corresponding claim must not be re-issued on refresh. + /// + [Theory, BitAutoData, RegisterFinishRequestModelCustomize] + public async Task RefreshToken_ReflectsRemovedSecretsManagerAccess( + RegisterFinishRequestModel requestModel) + { + requestModel.UserAsymmetricKeys = TEST_ACCOUNT_KEYS; + var localFactory = new IdentityApplicationFactory(); + + var user = await localFactory.RegisterNewIdentityFactoryUserAsync(requestModel); + + // Give the user Secrets Manager access on the organization. + var organizationId = Guid.NewGuid(); + await CreateOrganizationMembershipAsync(localFactory, organizationId, user.Email, OrganizationUserType.User, + accessSecretsManager: true); + + // Initial password login: the access token reflects Secrets Manager access. + var (accessToken, refreshToken) = await localFactory.TokenFromPasswordAsync( + requestModel.Email, requestModel.MasterPasswordHash); + var initialClaims = ReadJwtPayload(accessToken); + Assert.True(JwtPayloadContainsClaimValue(initialClaims, Claims.SecretsManagerAccess, organizationId.ToString())); + + // Secrets Manager access is removed. + var organizationUserRepository = localFactory.Services.GetRequiredService(); + var membership = await organizationUserRepository.GetByOrganizationAsync(organizationId, user.Id); + Assert.NotNull(membership); + membership.AccessSecretsManager = false; + await organizationUserRepository.ReplaceAsync(membership); + + // Refresh the token: the rebuilt token reflects current access, so the Secrets Manager claim is gone. + using var formContent = new FormUrlEncodedContent(new Dictionary + { + { "grant_type", "refresh_token" }, + { "client_id", "web" }, + { "refresh_token", refreshToken }, + }); + var context = await localFactory.Server.PostAsync("/connect/token", formContent); + + using var body = await AssertDefaultTokenBodyAsync(context); + var refreshedAccessToken = AssertAccessTokenExists(body.RootElement); + var refreshedClaims = ReadJwtPayload(refreshedAccessToken); + Assert.False(refreshedClaims.TryGetProperty(Claims.SecretsManagerAccess, out _)); + } + private static async Task CreateOrganizationMembershipAsync( IdentityApplicationFactory localFactory, Guid organizationId, string username, - OrganizationUserType organizationUserType) + OrganizationUserType organizationUserType, + Permissions permissions = null, + bool accessSecretsManager = false) { var userRepository = localFactory.Services.GetRequiredService(); var organizationRepository = localFactory.Services.GetRequiredService(); @@ -135,16 +231,24 @@ await organizationRepository.CreateAsync(new Organization Enabled = true, Plan = "Enterprise", BillingEmail = $"billing-email+{organizationId}@example.com", + UseSecretsManager = accessSecretsManager, }); var user = await userRepository.GetByEmailAsync(username); - await organizationUserRepository.CreateAsync(new OrganizationUser + var organizationUser = new OrganizationUser { OrganizationId = organizationId, UserId = user.Id, Status = OrganizationUserStatusType.Confirmed, - Type = organizationUserType - }); + Type = organizationUserType, + AccessSecretsManager = accessSecretsManager, + }; + if (permissions != null) + { + organizationUser.SetPermissions(permissions); + } + + await organizationUserRepository.CreateAsync(organizationUser); } /// From 560999a5151bfb6343301d74e4e3247fbeba35c2 Mon Sep 17 00:00:00 2001 From: Ike Kottlowski Date: Mon, 6 Jul 2026 21:48:19 -0400 Subject: [PATCH 5/5] feat: change to allow list not deny list --- src/Core/Auth/Identity/Claims.cs | 61 ++++++++++--------- src/Identity/IdentityServer/ProfileService.cs | 12 ++-- 2 files changed, 37 insertions(+), 36 deletions(-) diff --git a/src/Core/Auth/Identity/Claims.cs b/src/Core/Auth/Identity/Claims.cs index f92dbec35404..c64b14eb7047 100644 --- a/src/Core/Auth/Identity/Claims.cs +++ b/src/Core/Auth/Identity/Claims.cs @@ -1,4 +1,6 @@ -namespace Bit.Core.Auth.Identity; +using Duende.IdentityModel; + +namespace Bit.Core.Auth.Identity; public static class Claims { @@ -41,39 +43,38 @@ public static class CustomPermissions } /// - /// The membership and authorization claim types that - /// is authoritative for. These are rebuilt - /// from the database on every token issuance, so the Identity Profile Service must not carry them over from the - /// existing subject when refreshing a token — a refreshed token should always reflect the member's current - /// organization and provider membership. + /// The claim types the Identity Profile Service will carry over from the existing subject when refreshing a + /// token. Everything not in this set is dropped by default — including membership and authorization claim + /// types produced by , which are rebuilt from + /// the database on every token issuance, so a refreshed token always reflects the member's current + /// organization and provider membership rather than a stale grant of access. /// - /// Keep this in sync with BuildIdentityClaims. It intentionally excludes user-identity claims - /// (, email, email_verified, name, ) and non-membership claims - /// (, , , , send_*), - /// which are overwritten or preserved separately. + /// This is an allowlist: a new claim type added to BuildIdentityClaims (or elsewhere) is dropped on + /// refresh by default unless it is deliberately added here, which fails closed. Only add a claim type here if + /// it is a user-identity claim that is safe to persist across a refresh regardless of the member's current + /// authorization state. /// /// - public static readonly IReadOnlySet MembershipClaimTypes = new HashSet + public static readonly IReadOnlySet UserIdentityClaimTypes = new HashSet { - OrganizationOwner, - OrganizationAdmin, - OrganizationUser, - OrganizationCustom, - ProviderAdmin, - ProviderServiceUser, - SecretsManagerAccess, - CustomPermissions.AccessEventLogs, - CustomPermissions.AccessImportExport, - CustomPermissions.AccessReports, - CustomPermissions.CreateNewCollections, - CustomPermissions.EditAnyCollection, - CustomPermissions.DeleteAnyCollection, - CustomPermissions.ManageGroups, - CustomPermissions.ManagePolicies, - CustomPermissions.ManageSso, - CustomPermissions.ManageUsers, - CustomPermissions.ManageResetPassword, - CustomPermissions.ManageScim, + // Added by Duende's GrantValidationResult when the subject is first authenticated; not produced by + // BuildIdentityClaims, but required for the refreshed token to keep resolving to the correct principal. + JwtClaimTypes.Subject, + JwtClaimTypes.AuthenticationMethod, + JwtClaimTypes.IdentityProvider, + JwtClaimTypes.AuthenticationTime, + + // User-identity claims rebuilt by BuildIdentityClaims on every issuance; safe to carry over as a fallback + // for issuances where the user cannot be looked up (see ProfileService.GetProfileDataAsync). + JwtClaimTypes.Email, + JwtClaimTypes.EmailVerified, + JwtClaimTypes.Name, + Premium, + SecurityStamp, + + // Device-binding claims added when the subject is first authenticated; not produced by BuildIdentityClaims. + Device, + DeviceType, }; public static class SendAccessClaims diff --git a/src/Identity/IdentityServer/ProfileService.cs b/src/Identity/IdentityServer/ProfileService.cs index c153dacb783d..a13e3010cb62 100644 --- a/src/Identity/IdentityServer/ProfileService.cs +++ b/src/Identity/IdentityServer/ProfileService.cs @@ -74,14 +74,14 @@ public async Task GetProfileDataAsync(ProfileDataRequestContext context) } // Determine which of the existingClaims to carry over onto the refreshed token. - // We do not carry over membership and authorization claims so each refresh token - // reflects the user's current membership and authorization state. + // Only known-safe user-identity claims are carried over; everything else (including membership and + // authorization claims) is dropped so each refresh token reflects the user's current membership and + // authorization state. var existingClaimsToKeep = existingClaims .Where(c => - // remove all membership claims so that the refreshed token reflects the user's current membership state - !Claims.MembershipClaimTypes.Contains(c.Type) && - // For all other (user-identity) claims: if we have no new claims, keep the existing claim; - // otherwise keep it only when it is not being overwritten by a freshly built claim of the same type. + // only allow user-identity claims to be carried over from the existing subject + Claims.UserIdentityClaimTypes.Contains(c.Type) && + // keep the existing claim only when it is not being overwritten by a freshly built claim of the same type (newClaims.Count == 0 || !newClaims.Any(nc => nc.Type == c.Type)) ).ToList();