From 95106376465f5a706fe32d60262da1955701867a Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Wed, 17 Sep 2025 13:17:00 -0600 Subject: [PATCH 01/34] feat(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Added in logic to block existing sso org users who are not in the confirmed or accepted state. --- .../src/Sso/Controllers/AccountController.cs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index 98a581e8caed..90ff57005cbe 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -266,6 +266,23 @@ public async Task ExternalCallback() // We will now sign the Bitwarden user in. if (user != null) { + // Block sign-in if the user's organization membership is revoked + if (Guid.TryParse(provider, out var organizationId)) + { + var orgUser = await _organizationUserRepository.GetByOrganizationAsync(organizationId, user.Id); + var organization = await _organizationRepository.GetByIdAsync(organizationId); + + if (orgUser != null && organization != null) + { + EnsureOrgUserStatusAllowed(orgUser.Status, organization.DisplayName(), + allowedStatuses: [OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed]); + } + else + { + _logger.LogError("Organization user or organization not found for user ID: {UserId} and organization ID: {OrganizationId}", user.Id, organizationId); + } + } + // This allows us to collect any additional claims or properties // for the specific protocols used and store them in the local auth cookie. // this is typically used to store data needed for signout from those protocols. From 864fafe0a6645ea1bcccb61225185777a756e92d Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Thu, 18 Sep 2025 13:38:11 -0600 Subject: [PATCH 02/34] feat(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Added extra log statement --- bitwarden_license/src/Sso/Controllers/AccountController.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index 90ff57005cbe..0ca6e1b30f46 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -282,6 +282,10 @@ public async Task ExternalCallback() _logger.LogError("Organization user or organization not found for user ID: {UserId} and organization ID: {OrganizationId}", user.Id, organizationId); } } + else + { + _logger.LogError("Failed to parse organization ID: {OrganizationId}", organizationId); + } // This allows us to collect any additional claims or properties // for the specific protocols used and store them in the local auth cookie. From ea048ef2e7eba1175efe64364ed42b63012833ab Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Thu, 25 Sep 2025 15:18:25 -0400 Subject: [PATCH 03/34] test(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Adding tests, second attempt to do it properly with xunit --- bitwarden-server.sln | 7 + .../src/Sso/Controllers/AccountController.cs | 118 ++++++-- src/Core/Constants.cs | 9 +- .../Controllers/AccountControllerTest.cs | 276 ++++++++++++++++++ test/SSO.Test/SSO.Test.csproj | 29 ++ 5 files changed, 417 insertions(+), 22 deletions(-) create mode 100644 test/SSO.Test/Controllers/AccountControllerTest.cs create mode 100644 test/SSO.Test/SSO.Test.csproj diff --git a/bitwarden-server.sln b/bitwarden-server.sln index dbc37372a1ae..6dc8ba7a139e 100644 --- a/bitwarden-server.sln +++ b/bitwarden-server.sln @@ -135,6 +135,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "DbSeederUtility", "util\DbS EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "SharedWeb.Test", "test\SharedWeb.Test\SharedWeb.Test.csproj", "{AD59537D-5259-4B7A-948F-0CF58E80B359}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "SSO.Test", "test\SSO.Test\SSO.Test.csproj", "{7D98784C-C253-43FB-9873-25B65C6250D6}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -343,6 +345,10 @@ Global {AD59537D-5259-4B7A-948F-0CF58E80B359}.Debug|Any CPU.Build.0 = Debug|Any CPU {AD59537D-5259-4B7A-948F-0CF58E80B359}.Release|Any CPU.ActiveCfg = Release|Any CPU {AD59537D-5259-4B7A-948F-0CF58E80B359}.Release|Any CPU.Build.0 = Release|Any CPU + {7D98784C-C253-43FB-9873-25B65C6250D6}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {7D98784C-C253-43FB-9873-25B65C6250D6}.Debug|Any CPU.Build.0 = Debug|Any CPU + {7D98784C-C253-43FB-9873-25B65C6250D6}.Release|Any CPU.ActiveCfg = Release|Any CPU + {7D98784C-C253-43FB-9873-25B65C6250D6}.Release|Any CPU.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -398,6 +404,7 @@ Global {9A612EBA-1C0E-42B8-982B-62F0EE81000A} = {DD5BD056-4AAE-43EF-BBD2-0B569B8DA84E} {17A89266-260A-4A03-81AE-C0468C6EE06E} = {DD5BD056-4AAE-43EF-BBD2-0B569B8DA84E} {AD59537D-5259-4B7A-948F-0CF58E80B359} = {DD5BD056-4AAE-43EF-BBD2-0B569B8DA84F} + {7D98784C-C253-43FB-9873-25B65C6250D6} = {287CFF34-BBDB-4BC4-AF88-1E19A5A4679B} EndGlobalSection GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {E01CBF68-2E20-425F-9EDB-E0A6510CA92F} diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index 0ca6e1b30f46..b980f380e481 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -57,6 +57,7 @@ public class AccountController : Controller private readonly IDataProtectorTokenFactory _dataProtector; private readonly IOrganizationDomainRepository _organizationDomainRepository; private readonly IRegisterUserCommand _registerUserCommand; + private readonly IFeatureService _featureService; public AccountController( IAuthenticationSchemeProvider schemeProvider, @@ -77,7 +78,8 @@ public AccountController( Core.Services.IEventService eventService, IDataProtectorTokenFactory dataProtector, IOrganizationDomainRepository organizationDomainRepository, - IRegisterUserCommand registerUserCommand) + IRegisterUserCommand registerUserCommand, + IFeatureService featureService) { _schemeProvider = schemeProvider; _clientStore = clientStore; @@ -98,6 +100,7 @@ public AccountController( _dataProtector = dataProtector; _organizationDomainRepository = organizationDomainRepository; _registerUserCommand = registerUserCommand; + _featureService = featureService; } [HttpGet] @@ -251,6 +254,15 @@ public async Task ExternalCallback() // This is signified by the user existing in the User table and the SSOUser table for the SSO provider they're using. var (user, provider, providerUserId, claims, ssoConfigData) = await FindUserFromExternalProviderAsync(result); + // Feature-flag switch for preventing SSO on non-compliant existing users + var preventNonCompliant = _featureService.IsEnabled(FeatureFlagKeys.PM24579_PreventSsoOnExistingNonCompliantUsers); + + // Resolve organization and membership up-front (feature-flagged) + // When the flag is off, we skip pre-resolution entirely + var (organization, orgUser) = preventNonCompliant + ? await ResolveOrganizationAndMembershipAsync(provider, user, claims, ssoConfigData) + : (null, null); + // The user has not authenticated with this SSO provider before. // They could have an existing Bitwarden account in the User table though. if (user == null) @@ -258,7 +270,21 @@ public async Task ExternalCallback() // If we're manually linking to SSO, the user's external identifier will be passed as query string parameter. var userIdentifier = result.Properties.Items.Keys.Contains("user_identifier") ? result.Properties.Items["user_identifier"] : null; - user = await AutoProvisionUserAsync(provider, providerUserId, claims, userIdentifier, ssoConfigData); + // Pass any pre-resolved org/orgUser only when the flag is enabled + user = await AutoProvisionUserAsync( + provider, + providerUserId, + claims, + userIdentifier, + ssoConfigData, + preventNonCompliant ? organization : null, + preventNonCompliant ? orgUser : null); + + // If enabled, ensure we have orgUser after provisioning in case it was created/linked + if (preventNonCompliant && organization != null) + { + orgUser ??= await _organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id); + } } // Either the user already authenticated with the SSO provider, or we've just provisioned them. @@ -266,26 +292,32 @@ public async Task ExternalCallback() // We will now sign the Bitwarden user in. if (user != null) { - // Block sign-in if the user's organization membership is revoked - if (Guid.TryParse(provider, out var organizationId)) + // Feature-flagged enforcement: block sign-in for revoked/non-compliant org membership + if (preventNonCompliant) { - var orgUser = await _organizationUserRepository.GetByOrganizationAsync(organizationId, user.Id); - var organization = await _organizationRepository.GetByIdAsync(organizationId); - - if (orgUser != null && organization != null) + if (organization != null) { - EnsureOrgUserStatusAllowed(orgUser.Status, organization.DisplayName(), - allowedStatuses: [OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed]); + orgUser ??= await _organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id); + if (orgUser != null) + { + EnsureOrgUserStatusAllowed( + orgUser.Status, + organization.DisplayName(), + allowedStatuses: [OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed]); + } + else + { + _logger.LogError( + "Organization user not found for user ID: {UserId} and organization ID: {OrganizationId}", + user.Id, + organization?.Id); + } } else { - _logger.LogError("Organization user or organization not found for user ID: {UserId} and organization ID: {OrganizationId}", user.Id, organizationId); + _logger.LogError("Organization not found for provider: {Provider}", provider); } } - else - { - _logger.LogError("Failed to parse organization ID: {OrganizationId}", organizationId); - } // This allows us to collect any additional claims or properties // for the specific protocols used and store them in the local auth cookie. @@ -440,10 +472,13 @@ static bool nameIdIsNotTransient(Claim c) => c.Type == ClaimTypes.NameIdentifier /// The claims from the external IdP. /// The user identifier used for manual SSO linking. /// The SSO configuration for the organization. + /// + /// /// The User to sign in. /// An exception if the user cannot be provisioned as requested. private async Task AutoProvisionUserAsync(string provider, string providerUserId, - IEnumerable claims, string userIdentifier, SsoConfigurationData config) + IEnumerable claims, string userIdentifier, SsoConfigurationData config, + Organization organization, OrganizationUser orgUser) { var name = GetName(claims, config.GetAdditionalNameClaimTypes()); var email = GetEmailAddress(claims, config.GetAdditionalEmailClaimTypes()); @@ -472,8 +507,19 @@ private async Task AutoProvisionUserAsync(string provider, string provider existingUser = await GetUserFromManualLinkingData(userIdentifier); } - // Try to find the OrganizationUser if it exists. - var (organization, orgUser) = await FindOrganizationUser(existingUser, email, orgId); + // Try to find the OrganizationUser if it exists. Use preloaded values if provided. + if (organization == null || orgUser == null) + { + var (foundOrganization, foundOrgUser) = await FindOrganizationUser(existingUser, email, orgId); + if (organization == null) + { + organization = foundOrganization; + } + if (orgUser == null) + { + orgUser = foundOrgUser; + } + } //---------------------------------------------------- // Scenario 1: We've found the user in the User table @@ -605,6 +651,40 @@ private async Task AutoProvisionUserAsync(string provider, string provider return user; } + private async Task<(Organization organization, OrganizationUser orgUser)> ResolveOrganizationAndMembershipAsync( + string provider, User user, IEnumerable claims, SsoConfigurationData ssoConfigData) + { + Organization organization = null; + OrganizationUser orgUser = null; + + if (!Guid.TryParse(provider, out var organizationId)) + { + _logger.LogError("Failed to parse organization ID from provider: {Provider}", provider); + return (null, null); + } + + organization = await _organizationRepository.GetByIdAsync(organizationId); + if (organization == null) + { + _logger.LogError("Organization not found for provider: {Provider}", provider); + return (null, null); + } + + if (user != null) + { + orgUser = await _organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id); + return (organization, orgUser); + } + + var emailFromClaims = GetEmailAddress(claims, ssoConfigData.GetAdditionalEmailClaimTypes()); + if (!string.IsNullOrWhiteSpace(emailFromClaims)) + { + orgUser = await _organizationUserRepository.GetByOrganizationEmailAsync(organization.Id, emailFromClaims); + } + + return (organization, orgUser); + } + private async Task GetUserFromManualLinkingData(string userIdentifier) { User user = null; @@ -635,7 +715,7 @@ private async Task GetUserFromManualLinkingData(string userIdentifier) return user; } - private async Task<(Organization, OrganizationUser)> FindOrganizationUser(User existingUser, string email, Guid orgId) + private async Task<(Organization organization, OrganizationUser orgUser)> FindOrganizationUser(User existingUser, string email, Guid orgId) { OrganizationUser orgUser = null; var organization = await _organizationRepository.GetByIdAsync(orgId); diff --git a/src/Core/Constants.cs b/src/Core/Constants.cs index 43bba121df82..d88ed76b53fe 100644 --- a/src/Core/Constants.cs +++ b/src/Core/Constants.cs @@ -134,8 +134,6 @@ public static class FeatureFlagKeys public const string PM23845_VNextApplicationCache = "pm-24957-refactor-memory-application-cache"; public const string CipherRepositoryBulkResourceCreation = "pm-24951-cipher-repository-bulk-resource-creation-service"; public const string CollectionVaultRefactor = "pm-25030-resolve-ts-upgrade-errors"; - public const string DeleteClaimedUserAccountRefactor = "pm-25094-refactor-delete-managed-organization-user-command"; - public const string InviteEmailImprovements = "pm-25644-update-join-organization-subject-line"; /* Auth Team */ public const string TwoFactorExtensionDataPersistence = "pm-9115-two-factor-extension-data-persistence"; @@ -145,6 +143,8 @@ public static class FeatureFlagKeys public const string ChangeExistingPasswordRefactor = "pm-16117-change-existing-password-refactor"; public const string Otp6Digits = "pm-18612-otp-6-digits"; public const string FailedTwoFactorEmail = "pm-24425-send-2fa-failed-email"; + public const string PM24579_PreventSsoOnExistingNonCompliantUsers = "pm-24579-prevent-sso-on-existing-non-compliant-users"; + public const string DisableAlternateLoginMethods = "pm-22110-disable-alternate-login-methods"; /* Autofill Team */ public const string IdpAutoSubmitLogin = "idp-auto-submit-login"; @@ -169,7 +169,6 @@ public static class FeatureFlagKeys public const string PM17772_AdminInitiatedSponsorships = "pm-17772-admin-initiated-sponsorships"; public const string UsePricingService = "use-pricing-service"; public const string PM19422_AllowAutomaticTaxUpdates = "pm-19422-allow-automatic-tax-updates"; - public const string UseOrganizationWarningsService = "use-organization-warnings-service"; public const string PM21881_ManagePaymentDetailsOutsideCheckout = "pm-21881-manage-payment-details-outside-checkout"; public const string PM21821_ProviderPortalTakeover = "pm-21821-provider-portal-takeover"; public const string PM22415_TaxIDWarnings = "pm-22415-tax-id-warnings"; @@ -186,6 +185,7 @@ public static class FeatureFlagKeys public const string PM17987_BlockType0 = "pm-17987-block-type-0"; public const string ForceUpdateKDFSettings = "pm-18021-force-update-kdf-settings"; public const string UnlockWithMasterPasswordUnlockData = "pm-23246-unlock-with-master-password-unlock-data"; + public const string WindowsBiometricsV2 = "pm-25373-windows-biometrics-v2"; /* Mobile Team */ public const string NativeCarouselFlow = "native-carousel-flow"; @@ -235,6 +235,9 @@ public static class FeatureFlagKeys /* Innovation Team */ public const string ArchiveVaultItems = "pm-19148-innovation-archive"; + /* DIRT Team */ + public const string PM22887_RiskInsightsActivityTab = "pm-22887-risk-insights-activity-tab"; + public static List GetAllKeys() { return typeof(FeatureFlagKeys).GetFields(BindingFlags.Public | BindingFlags.Static | BindingFlags.FlattenHierarchy) diff --git a/test/SSO.Test/Controllers/AccountControllerTest.cs b/test/SSO.Test/Controllers/AccountControllerTest.cs new file mode 100644 index 000000000000..441856ea9375 --- /dev/null +++ b/test/SSO.Test/Controllers/AccountControllerTest.cs @@ -0,0 +1,276 @@ +using System.Reflection; +using System.Security.Claims; +using Bit.Core; +using Bit.Core.AdminConsole.Entities; +using Bit.Core.AdminConsole.Repositories; +using Bit.Core.Auth.Entities; +using Bit.Core.Auth.Models.Business.Tokenables; +using Bit.Core.Auth.Models.Data; +using Bit.Core.Auth.Repositories; +using Bit.Core.Auth.UserFeatures.Registration; +using Bit.Core.Entities; +using Bit.Core.Enums; +using Bit.Core.Repositories; +using Bit.Core.Services; +using Bit.Core.Settings; +using Bit.Core.Tokens; +using Bit.Sso.Controllers; +using Duende.IdentityModel; +using Duende.IdentityServer.Models; +using Duende.IdentityServer.Services; +using Duende.IdentityServer.Stores; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Identity; +using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using NSubstitute; + +namespace Bit.SSO.Test.Controllers; + +public class AccountControllerTest +{ + private static AccountController CreateController( + IAuthenticationService authenticationService, + out ISsoConfigRepository ssoConfigRepository, + out IUserRepository userRepository, + out IOrganizationRepository organizationRepository, + out IOrganizationUserRepository organizationUserRepository, + out IIdentityServerInteractionService interactionService, + out II18nService i18nService, + out ISsoUserRepository ssoUserRepository, + out Core.Services.IEventService eventService, + out IFeatureService featureService) + { + var schemeProvider = Substitute.For(); + var clientStore = Substitute.For(); + interactionService = Substitute.For(); + var logger = Substitute.For>(); + organizationRepository = Substitute.For(); + organizationUserRepository = Substitute.For(); + var organizationService = Substitute.For(); + ssoConfigRepository = Substitute.For(); + ssoUserRepository = Substitute.For(); + userRepository = Substitute.For(); + var policyRepository = Substitute.For(); + var userService = Substitute.For(); + i18nService = Substitute.For(); + i18nService.T(Arg.Any(), Arg.Any()).Returns(ci => (string)ci[0]!); + + // Minimal UserManager setup (not used in tested code paths, but required by ctor) + var userStore = Substitute.For>(); + var identityOptions = Microsoft.Extensions.Options.Options.Create(new IdentityOptions()); + var passwordHasher = Substitute.For>(); + var userValidators = Array.Empty>(); + var passwordValidators = Array.Empty>(); + var lookupNormalizer = Substitute.For(); + var errorDescriber = new IdentityErrorDescriber(); + var userLogger = Substitute.For>>(); + var userManager = new UserManager( + userStore, + identityOptions, + passwordHasher, + userValidators, + passwordValidators, + lookupNormalizer, + errorDescriber, + new ServiceCollection().BuildServiceProvider(), + userLogger); + + var globalSettings = Substitute.For(); + eventService = Substitute.For(); + var dataProtector = Substitute.For>(); + var organizationDomainRepository = Substitute.For(); + var registerUserCommand = Substitute.For(); + featureService = Substitute.For(); + + var controller = new AccountController( + schemeProvider, + clientStore, + interactionService, + logger, + organizationRepository, + organizationUserRepository, + organizationService, + ssoConfigRepository, + ssoUserRepository, + userRepository, + policyRepository, + userService, + i18nService, + userManager, + globalSettings, + eventService, + dataProtector, + organizationDomainRepository, + registerUserCommand, + featureService); + + var services = new ServiceCollection(); + services.AddSingleton(authenticationService); + var sp = services.BuildServiceProvider(); + + controller.ControllerContext = new ControllerContext + { + HttpContext = new DefaultHttpContext + { + RequestServices = sp + } + }; + + return controller; + } + + [Fact] + public async Task ExternalCallback_WithExistingUserAndAcceptedMembership_RedirectsToReturnUrl() + { + var orgId = Guid.NewGuid(); + var providerUserId = "ext-123"; + var user = new User { Id = Guid.NewGuid(), Email = "user@example.com" }; + var organization = new Organization { Id = orgId, Name = "Test Org" }; + var orgUser = new OrganizationUser + { + OrganizationId = orgId, + UserId = user.Id, + Status = OrganizationUserStatusType.Accepted, + Type = OrganizationUserType.User + }; + + // Prepare authenticate result that AccountController expects to read + var claims = new[] + { + new Claim(JwtClaimTypes.Subject, providerUserId), + new Claim(JwtClaimTypes.Email, user.Email!) + }; + var principal = new ClaimsPrincipal(new ClaimsIdentity(claims, "External")); + var properties = new AuthenticationProperties + { + Items = + { + ["scheme"] = orgId.ToString(), + ["return_url"] = "~/", + ["state"] = "state", + ["user_identifier"] = string.Empty + } + }; + var ticket = new AuthenticationTicket(principal, properties, AuthenticationSchemes.BitwardenExternalCookieAuthenticationScheme); + var authenticateResult = AuthenticateResult.Success(ticket); + + var authService = Substitute.For(); + authService.AuthenticateAsync( + Arg.Any(), + AuthenticationSchemes.BitwardenExternalCookieAuthenticationScheme) + .Returns(authenticateResult); + + var controller = CreateController( + authService, + out var ssoConfigRepository, + out var userRepository, + out var organizationRepository, + out var organizationUserRepository, + out var interactionService, + out var i18nService, + out var ssoUserRepository, + out var eventService, + out var featureService); + + // Configure dependencies used by FindUserFromExternalProviderAsync and enforcement + var ssoConfig = new SsoConfig { OrganizationId = orgId, Enabled = true }; + var ssoData = new SsoConfigurationData(); + ssoConfig.SetData(ssoData); + ssoConfigRepository.GetByOrganizationIdAsync(orgId).Returns(ssoConfig); + + userRepository.GetBySsoUserAsync(providerUserId, orgId).Returns(user); + organizationRepository.GetByIdAsync(orgId).Returns(organization); + organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id).Returns(orgUser); + + featureService.IsEnabled(Arg.Any()).Returns(true); + interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + + var result = await controller.ExternalCallback(); + + var redirect = Assert.IsType(result); + Assert.Equal("~/", redirect.Url); + + await authService.Received().SignInAsync( + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any()); + + await authService.Received().SignOutAsync( + Arg.Any(), + AuthenticationSchemes.BitwardenExternalCookieAuthenticationScheme, + Arg.Any()); + } + + [Fact] + public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLinkAndReturnsUser() + { + var orgId = Guid.NewGuid(); + var providerUserId = "ext-456"; + var email = "jit@example.com"; + var existingUser = new User { Id = Guid.NewGuid(), Email = email }; + var organization = new Organization { Id = orgId, Name = "Org" }; + var orgUser = new OrganizationUser + { + OrganizationId = orgId, + UserId = existingUser.Id, + Status = OrganizationUserStatusType.Accepted, + Type = OrganizationUserType.User + }; + + var authService = Substitute.For(); + var controller = CreateController( + authService, + out var ssoConfigRepository, + out var userRepository, + out var organizationRepository, + out var organizationUserRepository, + out var interactionService, + out var i18nService, + out var ssoUserRepository, + out var eventService, + out var featureService); + + // Arrange repository expectations for the flow + userRepository.GetByEmailAsync(email).Returns(existingUser); + + // No existing SSO link so first SSO login event is logged + ssoUserRepository.GetByUserIdOrganizationIdAsync(orgId, existingUser.Id).Returns((SsoUser?)null); + + var claims = new[] + { + new Claim(JwtClaimTypes.Email, email), + new Claim(JwtClaimTypes.Name, "Jit User") + } as IEnumerable; + var config = new SsoConfigurationData(); + + var method = typeof(AccountController).GetMethod( + "AutoProvisionUserAsync", + BindingFlags.Instance | BindingFlags.NonPublic); + Assert.NotNull(method); + + var task = (Task)method!.Invoke(controller, new object[] + { + orgId.ToString(), + providerUserId, + claims, + null!, + config, + organization, + orgUser + })!; + + var returnedUser = await task; + Assert.Equal(existingUser.Id, returnedUser.Id); + + await ssoUserRepository.Received().CreateAsync(Arg.Is(s => + s.OrganizationId == orgId && s.UserId == existingUser.Id && s.ExternalId == providerUserId)); + + await eventService.Received().LogOrganizationUserEventAsync( + orgUser, + EventType.OrganizationUser_FirstSsoLogin); + } +} diff --git a/test/SSO.Test/SSO.Test.csproj b/test/SSO.Test/SSO.Test.csproj new file mode 100644 index 000000000000..e6a2ccf9af0d --- /dev/null +++ b/test/SSO.Test/SSO.Test.csproj @@ -0,0 +1,29 @@ + + + + net8.0 + enable + enable + + false + true + + + + + + + + + + + + + + + + + + + + From 395bdb66d1869fa37f3a9e8cb286458683a26801 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Thu, 25 Sep 2025 15:46:45 -0400 Subject: [PATCH 04/34] test(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Fixed up more tests. --- .../Controllers/AccountControllerTest.cs | 124 +++++++++++++++++- test/SSO.Test/SSO.Test.csproj | 1 - 2 files changed, 121 insertions(+), 4 deletions(-) diff --git a/test/SSO.Test/Controllers/AccountControllerTest.cs b/test/SSO.Test/Controllers/AccountControllerTest.cs index 441856ea9375..e59e8a384a9c 100644 --- a/test/SSO.Test/Controllers/AccountControllerTest.cs +++ b/test/SSO.Test/Controllers/AccountControllerTest.cs @@ -16,6 +16,7 @@ using Bit.Core.Tokens; using Bit.Sso.Controllers; using Duende.IdentityModel; +using Duende.IdentityServer.Configuration; using Duende.IdentityServer.Models; using Duende.IdentityServer.Services; using Duende.IdentityServer.Stores; @@ -26,6 +27,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using NSubstitute; +using AuthenticationOptions = Duende.IdentityServer.Configuration.AuthenticationOptions; namespace Bit.SSO.Test.Controllers; @@ -44,6 +46,8 @@ private static AccountController CreateController( out IFeatureService featureService) { var schemeProvider = Substitute.For(); + schemeProvider.GetDefaultAuthenticateSchemeAsync() + .Returns(new AuthenticationScheme("idsrv", "idsrv", typeof(IAuthenticationHandler))); var clientStore = Substitute.For(); interactionService = Substitute.For(); var logger = Substitute.For>(); @@ -109,6 +113,14 @@ private static AccountController CreateController( var services = new ServiceCollection(); services.AddSingleton(authenticationService); + services.AddSingleton(schemeProvider); + services.AddSingleton(new IdentityServerOptions + { + Authentication = new AuthenticationOptions + { + CookieAuthenticationScheme = "idsrv" + } + }); var sp = services.BuildServiceProvider(); controller.ControllerContext = new ControllerContext @@ -122,6 +134,113 @@ private static AccountController CreateController( return controller; } + private static void InvokeEnsureOrgUserStatusAllowed( + AccountController controller, + OrganizationUserStatusType status, + params OrganizationUserStatusType[] allowed) + { + var method = typeof(AccountController).GetMethod( + "EnsureOrgUserStatusAllowed", + BindingFlags.Instance | BindingFlags.NonPublic); + Assert.NotNull(method); + method.Invoke(controller, [status, "Org", allowed]); + } + + [Fact] + public void EnsureOrgUserStatusAllowed_AllowsAcceptedAndConfirmed() + { + var authService = Substitute.For(); + var controller = CreateController( + authService, + out var ssoConfigRepository, + out var userRepository, + out var organizationRepository, + out var organizationUserRepository, + out var interactionService, + out var i18nService, + out var ssoUserRepository, + out var eventService, + out var featureService); + + InvokeEnsureOrgUserStatusAllowed(controller, OrganizationUserStatusType.Accepted, + OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed); + InvokeEnsureOrgUserStatusAllowed(controller, OrganizationUserStatusType.Confirmed, + OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed); + } + + [Fact] + public void EnsureOrgUserStatusAllowed_Invited_ThrowsAcceptInvite() + { + var authService = Substitute.For(); + var controller = CreateController( + authService, + out var ssoConfigRepository, + out var userRepository, + out var organizationRepository, + out var organizationUserRepository, + out var interactionService, + out var i18nService, + out var ssoUserRepository, + out var eventService, + out var featureService); + + var ex = Assert.Throws(() => + InvokeEnsureOrgUserStatusAllowed(controller, OrganizationUserStatusType.Invited, + OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); + + Assert.IsType(ex.InnerException); + Assert.Equal("AcceptInviteBeforeUsingSSO", ex.InnerException!.Message); + } + + [Fact] + public void EnsureOrgUserStatusAllowed_Revoked_ThrowsAccessRevoked() + { + var authService = Substitute.For(); + var controller = CreateController( + authService, + out var ssoConfigRepository, + out var userRepository, + out var organizationRepository, + out var organizationUserRepository, + out var interactionService, + out var i18nService, + out var ssoUserRepository, + out var eventService, + out var featureService); + + var ex = Assert.Throws(() => + InvokeEnsureOrgUserStatusAllowed(controller, OrganizationUserStatusType.Revoked, + OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); + + Assert.IsType(ex.InnerException); + Assert.Equal("OrganizationUserAccessRevoked", ex.InnerException!.Message); + } + + [Fact] + public void EnsureOrgUserStatusAllowed_UnknownStatus_ThrowsUnknown() + { + var authService = Substitute.For(); + var controller = CreateController( + authService, + out var ssoConfigRepository, + out var userRepository, + out var organizationRepository, + out var organizationUserRepository, + out var interactionService, + out var i18nService, + out var ssoUserRepository, + out var eventService, + out var featureService); + + var unknown = (OrganizationUserStatusType)999; + var ex = Assert.Throws(() => + InvokeEnsureOrgUserStatusAllowed(controller, unknown, + OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); + + Assert.IsType(ex.InnerException); + Assert.Equal("OrganizationUserUnknownStatus", ex.InnerException!.Message); + } + [Fact] public async Task ExternalCallback_WithExistingUserAndAcceptedMembership_RedirectsToReturnUrl() { @@ -252,8 +371,7 @@ public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLink BindingFlags.Instance | BindingFlags.NonPublic); Assert.NotNull(method); - var task = (Task)method!.Invoke(controller, new object[] - { + var task = (Task)method.Invoke(controller, [ orgId.ToString(), providerUserId, claims, @@ -261,7 +379,7 @@ public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLink config, organization, orgUser - })!; + ])!; var returnedUser = await task; Assert.Equal(existingUser.Id, returnedUser.Id); diff --git a/test/SSO.Test/SSO.Test.csproj b/test/SSO.Test/SSO.Test.csproj index e6a2ccf9af0d..eb636c44ee2d 100644 --- a/test/SSO.Test/SSO.Test.csproj +++ b/test/SSO.Test/SSO.Test.csproj @@ -22,7 +22,6 @@ - From 4d707b325f376a9a801121703860a0e41b426861 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Thu, 25 Sep 2025 15:55:10 -0400 Subject: [PATCH 05/34] test(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Fixed csproj to use variable version numbers. --- test/SSO.Test/SSO.Test.csproj | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/test/SSO.Test/SSO.Test.csproj b/test/SSO.Test/SSO.Test.csproj index eb636c44ee2d..baf5bfa82b56 100644 --- a/test/SSO.Test/SSO.Test.csproj +++ b/test/SSO.Test/SSO.Test.csproj @@ -10,11 +10,17 @@ - - - - - + + + + runtime; build; native; contentfiles; analyzers; buildtransitive + all + + + runtime; build; native; contentfiles; analyzers; buildtransitive + all + + From 0a6a2f2e553d4594c8e96c91040e0edd5497fae0 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Thu, 25 Sep 2025 16:21:48 -0400 Subject: [PATCH 06/34] test(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Made tests more dry --- .../Controllers/AccountControllerTest.cs | 193 ++++++++++++++---- 1 file changed, 153 insertions(+), 40 deletions(-) diff --git a/test/SSO.Test/Controllers/AccountControllerTest.cs b/test/SSO.Test/Controllers/AccountControllerTest.cs index e59e8a384a9c..2a642ef74c7e 100644 --- a/test/SSO.Test/Controllers/AccountControllerTest.cs +++ b/test/SSO.Test/Controllers/AccountControllerTest.cs @@ -113,7 +113,7 @@ private static AccountController CreateController( var services = new ServiceCollection(); services.AddSingleton(authenticationService); - services.AddSingleton(schemeProvider); + services.AddSingleton(schemeProvider); services.AddSingleton(new IdentityServerOptions { Authentication = new AuthenticationOptions @@ -146,6 +146,94 @@ private static void InvokeEnsureOrgUserStatusAllowed( method.Invoke(controller, [status, "Org", allowed]); } + private static AuthenticateResult BuildSuccessfulExternalAuth(Guid orgId, string providerUserId, string email) + { + var claims = new[] + { + new Claim(JwtClaimTypes.Subject, providerUserId), + new Claim(JwtClaimTypes.Email, email) + }; + var principal = new ClaimsPrincipal(new ClaimsIdentity(claims, "External")); + var properties = new AuthenticationProperties + { + Items = + { + ["scheme"] = orgId.ToString(), + ["return_url"] = "~/", + ["state"] = "state", + ["user_identifier"] = string.Empty + } + }; + var ticket = new AuthenticationTicket(principal, properties, AuthenticationSchemes.BitwardenExternalCookieAuthenticationScheme); + return AuthenticateResult.Success(ticket); + } + + private static AccountController CreateControllerWithAuth( + AuthenticateResult authResult, + out IAuthenticationService authService, + out ISsoConfigRepository ssoConfigRepository, + out IUserRepository userRepository, + out IOrganizationRepository organizationRepository, + out IOrganizationUserRepository organizationUserRepository, + out IIdentityServerInteractionService interactionService, + out II18nService i18nService, + out ISsoUserRepository ssoUserRepository, + out Core.Services.IEventService eventService, + out IFeatureService featureService) + { + var authServiceSub = Substitute.For(); + authServiceSub.AuthenticateAsync( + Arg.Any(), + AuthenticationSchemes.BitwardenExternalCookieAuthenticationScheme) + .Returns(authResult); + authService = authServiceSub; + return CreateController( + authServiceSub, + out ssoConfigRepository, + out userRepository, + out organizationRepository, + out organizationUserRepository, + out interactionService, + out i18nService, + out ssoUserRepository, + out eventService, + out featureService); + } + + private static void ConfigureSsoAndUser( + ISsoConfigRepository ssoConfigRepository, + IUserRepository userRepository, + IOrganizationRepository organizationRepository, + IOrganizationUserRepository organizationUserRepository, + Guid orgId, + string providerUserId, + User user, + Organization? organization = null, + OrganizationUser? orgUser = null) + { + var ssoConfig = new SsoConfig { OrganizationId = orgId, Enabled = true }; + var ssoData = new SsoConfigurationData(); + ssoConfig.SetData(ssoData); + ssoConfigRepository.GetByOrganizationIdAsync(orgId).Returns(ssoConfig); + + userRepository.GetBySsoUserAsync(providerUserId, orgId).Returns(user); + + if (organization != null) + { + organizationRepository.GetByIdAsync(orgId).Returns(organization); + } + if (organization != null && orgUser != null) + { + organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id).Returns(orgUser); + } + } + + private static void SetPreventNonCompliant(IFeatureService featureService, bool enabled) + => featureService.IsEnabled(Arg.Any()).Returns(enabled); + + private static void SetDefaultReturnContext(IIdentityServerInteractionService interactionService) + => interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + [Fact] public void EnsureOrgUserStatusAllowed_AllowsAcceptedAndConfirmed() { @@ -256,34 +344,10 @@ public async Task ExternalCallback_WithExistingUserAndAcceptedMembership_Redirec Type = OrganizationUserType.User }; - // Prepare authenticate result that AccountController expects to read - var claims = new[] - { - new Claim(JwtClaimTypes.Subject, providerUserId), - new Claim(JwtClaimTypes.Email, user.Email!) - }; - var principal = new ClaimsPrincipal(new ClaimsIdentity(claims, "External")); - var properties = new AuthenticationProperties - { - Items = - { - ["scheme"] = orgId.ToString(), - ["return_url"] = "~/", - ["state"] = "state", - ["user_identifier"] = string.Empty - } - }; - var ticket = new AuthenticationTicket(principal, properties, AuthenticationSchemes.BitwardenExternalCookieAuthenticationScheme); - var authenticateResult = AuthenticateResult.Success(ticket); - - var authService = Substitute.For(); - authService.AuthenticateAsync( - Arg.Any(), - AuthenticationSchemes.BitwardenExternalCookieAuthenticationScheme) - .Returns(authenticateResult); - - var controller = CreateController( - authService, + var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); + var controller = CreateControllerWithAuth( + authResult, + out var authService, out var ssoConfigRepository, out var userRepository, out var organizationRepository, @@ -294,18 +358,19 @@ public async Task ExternalCallback_WithExistingUserAndAcceptedMembership_Redirec out var eventService, out var featureService); - // Configure dependencies used by FindUserFromExternalProviderAsync and enforcement - var ssoConfig = new SsoConfig { OrganizationId = orgId, Enabled = true }; - var ssoData = new SsoConfigurationData(); - ssoConfig.SetData(ssoData); - ssoConfigRepository.GetByOrganizationIdAsync(orgId).Returns(ssoConfig); - - userRepository.GetBySsoUserAsync(providerUserId, orgId).Returns(user); - organizationRepository.GetByIdAsync(orgId).Returns(organization); - organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id).Returns(orgUser); + ConfigureSsoAndUser( + ssoConfigRepository, + userRepository, + organizationRepository, + organizationUserRepository, + orgId, + providerUserId, + user, + organization, + orgUser); - featureService.IsEnabled(Arg.Any()).Returns(true); - interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + SetPreventNonCompliant(featureService, true); + SetDefaultReturnContext(interactionService); var result = await controller.ExternalCallback(); @@ -324,6 +389,54 @@ await authService.Received().SignOutAsync( Arg.Any()); } + [Fact] + public async Task ExternalCallback_PreventNonCompliantFalse_SkipsOrgLookupAndSignsIn() + { + var orgId = Guid.NewGuid(); + var providerUserId = "ext-flag-off"; + var user = new User { Id = Guid.NewGuid(), Email = "flagoff@example.com" }; + + var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); + var controller = CreateControllerWithAuth( + authResult, + out var authService, + out var ssoConfigRepository, + out var userRepository, + out var organizationRepository, + out var organizationUserRepository, + out var interactionService, + out var i18nService, + out var ssoUserRepository, + out var eventService, + out var featureService); + + ConfigureSsoAndUser( + ssoConfigRepository, + userRepository, + organizationRepository, + organizationUserRepository, + orgId, + providerUserId, + user); + + SetPreventNonCompliant(featureService, false); + SetDefaultReturnContext(interactionService); + + var result = await controller.ExternalCallback(); + + var redirect = Assert.IsType(result); + Assert.Equal("~/", redirect.Url); + + await authService.Received().SignInAsync( + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any()); + + // When flag is off, controller does not require org or orgUser; ensure repo not called for orgUser + await organizationUserRepository.DidNotReceiveWithAnyArgs().GetByOrganizationAsync(Guid.Empty, Guid.Empty); + } + [Fact] public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLinkAndReturnsUser() { From dddf8e94bf6bd59c352b36268811eb3adcf93091 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 29 Sep 2025 10:07:46 -0400 Subject: [PATCH 07/34] test(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Removed unneeded test code. --- test/SSO.Test/Controllers/AccountControllerTest.cs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/test/SSO.Test/Controllers/AccountControllerTest.cs b/test/SSO.Test/Controllers/AccountControllerTest.cs index 2a642ef74c7e..612a81cfc983 100644 --- a/test/SSO.Test/Controllers/AccountControllerTest.cs +++ b/test/SSO.Test/Controllers/AccountControllerTest.cs @@ -228,12 +228,6 @@ private static void ConfigureSsoAndUser( } } - private static void SetPreventNonCompliant(IFeatureService featureService, bool enabled) - => featureService.IsEnabled(Arg.Any()).Returns(enabled); - - private static void SetDefaultReturnContext(IIdentityServerInteractionService interactionService) - => interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); - [Fact] public void EnsureOrgUserStatusAllowed_AllowsAcceptedAndConfirmed() { @@ -369,8 +363,8 @@ public async Task ExternalCallback_WithExistingUserAndAcceptedMembership_Redirec organization, orgUser); - SetPreventNonCompliant(featureService, true); - SetDefaultReturnContext(interactionService); + featureService.IsEnabled(Arg.Any()).Returns(true); + interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); var result = await controller.ExternalCallback(); @@ -419,8 +413,8 @@ public async Task ExternalCallback_PreventNonCompliantFalse_SkipsOrgLookupAndSig providerUserId, user); - SetPreventNonCompliant(featureService, false); - SetDefaultReturnContext(interactionService); + featureService.IsEnabled(Arg.Any()).Returns(false); + interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); var result = await controller.ExternalCallback(); From c80097ac1f231f7262f149c5100c5a23d6d9069f Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 29 Sep 2025 10:12:58 -0400 Subject: [PATCH 08/34] test(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Made it clear on arrange act assert --- .../Controllers/AccountControllerTest.cs | 36 ++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/test/SSO.Test/Controllers/AccountControllerTest.cs b/test/SSO.Test/Controllers/AccountControllerTest.cs index 612a81cfc983..3642b6ce0c8c 100644 --- a/test/SSO.Test/Controllers/AccountControllerTest.cs +++ b/test/SSO.Test/Controllers/AccountControllerTest.cs @@ -231,6 +231,7 @@ private static void ConfigureSsoAndUser( [Fact] public void EnsureOrgUserStatusAllowed_AllowsAcceptedAndConfirmed() { + // Arrange var authService = Substitute.For(); var controller = CreateController( authService, @@ -244,15 +245,23 @@ public void EnsureOrgUserStatusAllowed_AllowsAcceptedAndConfirmed() out var eventService, out var featureService); - InvokeEnsureOrgUserStatusAllowed(controller, OrganizationUserStatusType.Accepted, - OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed); - InvokeEnsureOrgUserStatusAllowed(controller, OrganizationUserStatusType.Confirmed, - OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed); + // Act + var ex1 = Record.Exception(() => + InvokeEnsureOrgUserStatusAllowed(controller, OrganizationUserStatusType.Accepted, + OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); + var ex2 = Record.Exception(() => + InvokeEnsureOrgUserStatusAllowed(controller, OrganizationUserStatusType.Confirmed, + OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); + + // Assert + Assert.Null(ex1); + Assert.Null(ex2); } [Fact] public void EnsureOrgUserStatusAllowed_Invited_ThrowsAcceptInvite() { + // Arrange var authService = Substitute.For(); var controller = CreateController( authService, @@ -266,10 +275,12 @@ public void EnsureOrgUserStatusAllowed_Invited_ThrowsAcceptInvite() out var eventService, out var featureService); + // Act var ex = Assert.Throws(() => InvokeEnsureOrgUserStatusAllowed(controller, OrganizationUserStatusType.Invited, OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); + // Assert Assert.IsType(ex.InnerException); Assert.Equal("AcceptInviteBeforeUsingSSO", ex.InnerException!.Message); } @@ -277,6 +288,7 @@ public void EnsureOrgUserStatusAllowed_Invited_ThrowsAcceptInvite() [Fact] public void EnsureOrgUserStatusAllowed_Revoked_ThrowsAccessRevoked() { + // Arrange var authService = Substitute.For(); var controller = CreateController( authService, @@ -290,10 +302,12 @@ public void EnsureOrgUserStatusAllowed_Revoked_ThrowsAccessRevoked() out var eventService, out var featureService); + // Act var ex = Assert.Throws(() => InvokeEnsureOrgUserStatusAllowed(controller, OrganizationUserStatusType.Revoked, OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); + // Assert Assert.IsType(ex.InnerException); Assert.Equal("OrganizationUserAccessRevoked", ex.InnerException!.Message); } @@ -301,6 +315,7 @@ public void EnsureOrgUserStatusAllowed_Revoked_ThrowsAccessRevoked() [Fact] public void EnsureOrgUserStatusAllowed_UnknownStatus_ThrowsUnknown() { + // Arrange var authService = Substitute.For(); var controller = CreateController( authService, @@ -315,10 +330,13 @@ public void EnsureOrgUserStatusAllowed_UnknownStatus_ThrowsUnknown() out var featureService); var unknown = (OrganizationUserStatusType)999; + + // Act var ex = Assert.Throws(() => InvokeEnsureOrgUserStatusAllowed(controller, unknown, OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); + // Assert Assert.IsType(ex.InnerException); Assert.Equal("OrganizationUserUnknownStatus", ex.InnerException!.Message); } @@ -326,6 +344,7 @@ public void EnsureOrgUserStatusAllowed_UnknownStatus_ThrowsUnknown() [Fact] public async Task ExternalCallback_WithExistingUserAndAcceptedMembership_RedirectsToReturnUrl() { + // Arrange var orgId = Guid.NewGuid(); var providerUserId = "ext-123"; var user = new User { Id = Guid.NewGuid(), Email = "user@example.com" }; @@ -366,8 +385,10 @@ public async Task ExternalCallback_WithExistingUserAndAcceptedMembership_Redirec featureService.IsEnabled(Arg.Any()).Returns(true); interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + // Act var result = await controller.ExternalCallback(); + // Assert var redirect = Assert.IsType(result); Assert.Equal("~/", redirect.Url); @@ -386,6 +407,7 @@ await authService.Received().SignOutAsync( [Fact] public async Task ExternalCallback_PreventNonCompliantFalse_SkipsOrgLookupAndSignsIn() { + // Arrange var orgId = Guid.NewGuid(); var providerUserId = "ext-flag-off"; var user = new User { Id = Guid.NewGuid(), Email = "flagoff@example.com" }; @@ -416,8 +438,10 @@ public async Task ExternalCallback_PreventNonCompliantFalse_SkipsOrgLookupAndSig featureService.IsEnabled(Arg.Any()).Returns(false); interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + // Act var result = await controller.ExternalCallback(); + // Assert var redirect = Assert.IsType(result); Assert.Equal("~/", redirect.Url); @@ -434,6 +458,7 @@ await authService.Received().SignInAsync( [Fact] public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLinkAndReturnsUser() { + // Arrange var orgId = Guid.NewGuid(); var providerUserId = "ext-456"; var email = "jit@example.com"; @@ -478,6 +503,7 @@ public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLink BindingFlags.Instance | BindingFlags.NonPublic); Assert.NotNull(method); + // Act var task = (Task)method.Invoke(controller, [ orgId.ToString(), providerUserId, @@ -489,6 +515,8 @@ public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLink ])!; var returnedUser = await task; + + // Assert Assert.Equal(existingUser.Id, returnedUser.Id); await ssoUserRepository.Received().CreateAsync(Arg.Is(s => From 1077d735bc665b027b91f850202aa0e07f95a550 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 29 Sep 2025 12:22:12 -0400 Subject: [PATCH 09/34] test(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Fixed up tests and code to make sure there is the correct number of repo calls. --- .../src/Sso/Controllers/AccountController.cs | 66 ++- .../Controllers/AccountControllerTest.cs | 533 +++++++++++++++++- 2 files changed, 571 insertions(+), 28 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index b980f380e481..bcf4ed9f51f3 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -254,14 +254,13 @@ public async Task ExternalCallback() // This is signified by the user existing in the User table and the SSOUser table for the SSO provider they're using. var (user, provider, providerUserId, claims, ssoConfigData) = await FindUserFromExternalProviderAsync(result); - // Feature-flag switch for preventing SSO on non-compliant existing users + // Feature flag (PM-24579): Prevent SSO on existing non-compliant users. + // When removing this feature flag, delete this check and always run the enforcement logic below. var preventNonCompliant = _featureService.IsEnabled(FeatureFlagKeys.PM24579_PreventSsoOnExistingNonCompliantUsers); - // Resolve organization and membership up-front (feature-flagged) - // When the flag is off, we skip pre-resolution entirely - var (organization, orgUser) = preventNonCompliant - ? await ResolveOrganizationAndMembershipAsync(provider, user, claims, ssoConfigData) - : (null, null); + // Defer organization and membership resolution to when needed (lazy resolution) + Organization organization = null; + OrganizationUser orgUser = null; // The user has not authenticated with this SSO provider before. // They could have an existing Bitwarden account in the User table though. @@ -271,7 +270,10 @@ public async Task ExternalCallback() var userIdentifier = result.Properties.Items.Keys.Contains("user_identifier") ? result.Properties.Items["user_identifier"] : null; // Pass any pre-resolved org/orgUser only when the flag is enabled - user = await AutoProvisionUserAsync( + // PM-24579: After removing the feature flag, just call AutoProvisionUserAsync + // and always use the returned organization/orgUser. The conditional arguments + // can be collapsed away at that time. + var provision = await AutoProvisionUserAsync( provider, providerUserId, claims, @@ -279,11 +281,12 @@ public async Task ExternalCallback() ssoConfigData, preventNonCompliant ? organization : null, preventNonCompliant ? orgUser : null); - - // If enabled, ensure we have orgUser after provisioning in case it was created/linked - if (preventNonCompliant && organization != null) + user = provision.user; + // PM-24579: After removing the flag, assign these unconditionally and remove this if block. + if (preventNonCompliant) { - orgUser ??= await _organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id); + organization ??= provision.organization; + orgUser ??= provision.orgUser; } } @@ -293,10 +296,22 @@ public async Task ExternalCallback() if (user != null) { // Feature-flagged enforcement: block sign-in for revoked/non-compliant org membership + // PM-24579: After removing the feature flag, delete the 'if' and always run the enforcement body. if (preventNonCompliant) { - if (organization != null) + // Lazily resolve organization if not already known + if (organization == null && Guid.TryParse(provider, out var organizationId)) + { + organization = await _organizationRepository.GetByIdAsync(organizationId); + } + + if (organization == null) + { + _logger.LogError("Organization not found for provider: {Provider}", provider); + } + else { + // Lazily resolve orgUser only when we have an organization and a user orgUser ??= await _organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id); if (orgUser != null) { @@ -313,10 +328,6 @@ public async Task ExternalCallback() organization?.Id); } } - else - { - _logger.LogError("Organization not found for provider: {Provider}", provider); - } } // This allows us to collect any additional claims or properties @@ -472,13 +483,18 @@ static bool nameIdIsNotTransient(Claim c) => c.Type == ClaimTypes.NameIdentifier /// The claims from the external IdP. /// The user identifier used for manual SSO linking. /// The SSO configuration for the organization. - /// - /// + /// The organization to find the user in, can be null. + /// The org user pair, can be null. /// The User to sign in. /// An exception if the user cannot be provisioned as requested. - private async Task AutoProvisionUserAsync(string provider, string providerUserId, - IEnumerable claims, string userIdentifier, SsoConfigurationData config, - Organization organization, OrganizationUser orgUser) + private async Task<(User user, Organization organization, OrganizationUser orgUser)> AutoProvisionUserAsync( + string provider, + string providerUserId, + IEnumerable claims, + string userIdentifier, + SsoConfigurationData config, + Organization organization, + OrganizationUser orgUser) { var name = GetName(claims, config.GetAdditionalNameClaimTypes()); var email = GetEmailAddress(claims, config.GetAdditionalEmailClaimTypes()); @@ -549,7 +565,7 @@ private async Task AutoProvisionUserAsync(string provider, string provider // We've verified that the user is Accepted or Confnirmed, so we can create an SsoUser link and proceed // with authentication. await CreateSsoUserRecord(providerUserId, existingUser.Id, orgId, orgUser); - return existingUser; + return (existingUser, organization, orgUser); } // Before any user creation - if Org User doesn't exist at this point - make sure there are enough seats to add one @@ -648,10 +664,10 @@ private async Task AutoProvisionUserAsync(string provider, string provider // Create the SsoUser record to link the user to the SSO provider. await CreateSsoUserRecord(providerUserId, user.Id, orgId, orgUser); - return user; + return (user, organization, orgUser); } - private async Task<(Organization organization, OrganizationUser orgUser)> ResolveOrganizationAndMembershipAsync( + private async Task<(Organization, OrganizationUser)> ResolveOrganizationAndMembershipAsync( string provider, User user, IEnumerable claims, SsoConfigurationData ssoConfigData) { Organization organization = null; @@ -715,7 +731,7 @@ private async Task GetUserFromManualLinkingData(string userIdentifier) return user; } - private async Task<(Organization organization, OrganizationUser orgUser)> FindOrganizationUser(User existingUser, string email, Guid orgId) + private async Task<(Organization, OrganizationUser)> FindOrganizationUser(User existingUser, string email, Guid orgId) { OrganizationUser orgUser = null; var organization = await _organizationRepository.GetByIdAsync(orgId); diff --git a/test/SSO.Test/Controllers/AccountControllerTest.cs b/test/SSO.Test/Controllers/AccountControllerTest.cs index 3642b6ce0c8c..12af2a8f9c1a 100644 --- a/test/SSO.Test/Controllers/AccountControllerTest.cs +++ b/test/SSO.Test/Controllers/AccountControllerTest.cs @@ -27,12 +27,19 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using NSubstitute; +using Xunit.Abstractions; using AuthenticationOptions = Duende.IdentityServer.Configuration.AuthenticationOptions; namespace Bit.SSO.Test.Controllers; public class AccountControllerTest { + private readonly ITestOutputHelper _output; + + public AccountControllerTest(ITestOutputHelper output) + { + _output = output; + } private static AccountController CreateController( IAuthenticationService authenticationService, out ISsoConfigRepository ssoConfigRepository, @@ -228,6 +235,104 @@ private static void ConfigureSsoAndUser( } } + private enum MeasurementScenario + { + ExistingSsoLinkedAccepted, + ExistingUserNoOrgUser, + JitProvision + } + + private sealed class LookupCounts + { + public int UserGetBySso { get; init; } + public int UserGetByEmail { get; init; } + public int OrgGetById { get; init; } + public int OrgUserGetByOrg { get; init; } + public int OrgUserGetByEmail { get; init; } + } + + private async Task MeasureCountsForScenarioAsync(MeasurementScenario scenario, bool preventNonCompliant) + { + var orgId = Guid.NewGuid(); + var providerUserId = $"meas-{scenario}-{(preventNonCompliant ? "on" : "off")}"; + var email = scenario == MeasurementScenario.JitProvision + ? "jit.compare@example.com" + : "existing.compare@example.com"; + + var organization = new Organization { Id = orgId, Name = "Org" }; + var user = new User { Id = Guid.NewGuid(), Email = email }; + + var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, email); + var controller = CreateControllerWithAuth( + authResult, + out var authService, + out var ssoConfigRepository, + out var userRepository, + out var organizationRepository, + out var organizationUserRepository, + out var interactionService, + out var i18nService, + out var ssoUserRepository, + out var eventService, + out var featureService); + + // SSO config present + var ssoConfig = new SsoConfig { OrganizationId = orgId, Enabled = true }; + var ssoData = new SsoConfigurationData(); + ssoConfig.SetData(ssoData); + ssoConfigRepository.GetByOrganizationIdAsync(orgId).Returns(ssoConfig); + + switch (scenario) + { + case MeasurementScenario.ExistingSsoLinkedAccepted: + userRepository.GetBySsoUserAsync(providerUserId, orgId).Returns(user); + organizationRepository.GetByIdAsync(orgId).Returns(organization); + organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id) + .Returns(new OrganizationUser + { + OrganizationId = orgId, + UserId = user.Id, + Status = OrganizationUserStatusType.Accepted, + Type = OrganizationUserType.User + }); + break; + case MeasurementScenario.ExistingUserNoOrgUser: + userRepository.GetBySsoUserAsync(providerUserId, orgId).Returns(user); + organizationRepository.GetByIdAsync(orgId).Returns(organization); + organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id) + .Returns((OrganizationUser?)null); + break; + case MeasurementScenario.JitProvision: + userRepository.GetBySsoUserAsync(providerUserId, orgId).Returns((User?)null); + userRepository.GetByEmailAsync(email).Returns((User?)null); + organizationRepository.GetByIdAsync(orgId).Returns(organization); + organizationUserRepository.GetByOrganizationEmailAsync(orgId, email) + .Returns((OrganizationUser?)null); + break; + } + + featureService.IsEnabled(Arg.Any()).Returns(preventNonCompliant); + interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + + try + { + _ = await controller.ExternalCallback(); + } + catch + { + // Ignore exceptions for measurement; some flows can throw based on status enforcement + } + + return new LookupCounts + { + UserGetBySso = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetBySsoUserAsync)), + UserGetByEmail = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetByEmailAsync)), + OrgGetById = organizationRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationRepository.GetByIdAsync)), + OrgUserGetByOrg = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationAsync)), + OrgUserGetByEmail = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationEmailAsync)), + }; + } + [Fact] public void EnsureOrgUserStatusAllowed_AllowsAcceptedAndConfirmed() { @@ -404,6 +509,9 @@ await authService.Received().SignOutAsync( Arg.Any()); } + /// + /// PM-24579: Temporary test, remove with feature flag. + /// [Fact] public async Task ExternalCallback_PreventNonCompliantFalse_SkipsOrgLookupAndSignsIn() { @@ -455,6 +563,391 @@ await authService.Received().SignInAsync( await organizationUserRepository.DidNotReceiveWithAnyArgs().GetByOrganizationAsync(Guid.Empty, Guid.Empty); } + /// + /// PM-24579: Permanent test, remove the True in PreventNonCompliantTrue and remove the configure for the feature + /// flag. + /// + [Fact] + public async Task ExternalCallback_PreventNonCompliantTrue_ExistingSsoLinkedAccepted_MeasureLookups() + { + // Arrange + var orgId = Guid.NewGuid(); + var providerUserId = "ext-measure-existing"; + var user = new User { Id = Guid.NewGuid(), Email = "existing@example.com" }; + var organization = new Organization { Id = orgId, Name = "Org" }; + var orgUser = new OrganizationUser + { + OrganizationId = orgId, + UserId = user.Id, + Status = OrganizationUserStatusType.Accepted, + Type = OrganizationUserType.User + }; + + var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); + var controller = CreateControllerWithAuth( + authResult, + out var authService, + out var ssoConfigRepository, + out var userRepository, + out var organizationRepository, + out var organizationUserRepository, + out var interactionService, + out var i18nService, + out var ssoUserRepository, + out var eventService, + out var featureService); + + ConfigureSsoAndUser( + ssoConfigRepository, + userRepository, + organizationRepository, + organizationUserRepository, + orgId, + providerUserId, + user, + organization, + orgUser); + + featureService.IsEnabled(Arg.Any()).Returns(true); + interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + + // Act + try + { + _ = await controller.ExternalCallback(); + } + catch + { + // ignore for measurement only + } + + // Assert (measurement only - no asserts on counts) + var userGetBySso = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetBySsoUserAsync)); + var userGetByEmail = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetByEmailAsync)); + var orgGet = organizationRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationRepository.GetByIdAsync)); + var orgUserGetByOrg = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationAsync)); + var orgUserGetByEmail = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationEmailAsync)); + + _output.WriteLine($"GetBySsoUserAsync: {userGetBySso}"); + _output.WriteLine($"GetByEmailAsync: {userGetByEmail}"); + _output.WriteLine($"GetByIdAsync (Org): {orgGet}"); + _output.WriteLine($"GetByOrganizationAsync (OrgUser): {orgUserGetByOrg}"); + _output.WriteLine($"GetByOrganizationEmailAsync (OrgUser): {orgUserGetByEmail}"); + + // Snapshot assertions + Assert.Equal(1, userGetBySso); + Assert.Equal(0, userGetByEmail); + Assert.Equal(1, orgGet); + Assert.Equal(1, orgUserGetByOrg); + Assert.Equal(0, orgUserGetByEmail); + } + + /// + /// PM-24579: Permanent test, remove the True in PreventNonCompliantTrue and remove the configure for the feature + /// flag. + /// + [Fact] + public async Task ExternalCallback_PreventNonCompliantTrue_JitProvision_MeasureLookups() + { + // Arrange + var orgId = Guid.NewGuid(); + var providerUserId = "ext-measure-jit"; + var email = "jit.measure@example.com"; + var organization = new Organization { Id = orgId, Name = "Org", Seats = null }; + + var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, email); + var controller = CreateControllerWithAuth( + authResult, + out var authService, + out var ssoConfigRepository, + out var userRepository, + out var organizationRepository, + out var organizationUserRepository, + out var interactionService, + out var i18nService, + out var ssoUserRepository, + out var eventService, + out var featureService); + + // Configure SSO config and ensure there is NO existing SSO link or user (JIT) + var ssoConfig = new SsoConfig { OrganizationId = orgId, Enabled = true }; + var ssoData = new SsoConfigurationData(); + ssoConfig.SetData(ssoData); + ssoConfigRepository.GetByOrganizationIdAsync(orgId).Returns(ssoConfig); + + userRepository.GetBySsoUserAsync(providerUserId, orgId).Returns((User?)null); + userRepository.GetByEmailAsync(email).Returns((User?)null); + organizationRepository.GetByIdAsync(orgId).Returns(organization); + organizationUserRepository.GetByOrganizationEmailAsync(orgId, email).Returns((OrganizationUser?)null); + + featureService.IsEnabled(Arg.Any()).Returns(true); + interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + + // Act + try + { + _ = await controller.ExternalCallback(); + } + catch + { + // JIT path may throw due to Invited status under enforcement; ignore for measurement + } + + // Assert (measurement only - no asserts on counts) + var userGetBySso = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetBySsoUserAsync)); + var userGetByEmail = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetByEmailAsync)); + var orgGet = organizationRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationRepository.GetByIdAsync)); + var orgUserGetByOrg = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationAsync)); + var orgUserGetByEmail = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationEmailAsync)); + + _output.WriteLine($"GetBySsoUserAsync: {userGetBySso}"); + _output.WriteLine($"GetByEmailAsync: {userGetByEmail}"); + _output.WriteLine($"GetByIdAsync (Org): {orgGet}"); + _output.WriteLine($"GetByOrganizationAsync (OrgUser): {orgUserGetByOrg}"); + _output.WriteLine($"GetByOrganizationEmailAsync (OrgUser): {orgUserGetByEmail}"); + + // Snapshot assertions + Assert.Equal(1, userGetBySso); + Assert.Equal(1, userGetByEmail); + Assert.Equal(1, orgGet); + Assert.Equal(0, orgUserGetByOrg); + Assert.Equal(1, orgUserGetByEmail); + } + + /// + /// PM-24579: Permanent test, remove the True in PreventNonCompliantTrue and remove the configure for the feature + /// flag. + /// + [Fact] + public async Task ExternalCallback_PreventNonCompliantTrue_ExistingUser_NoOrgUser_MeasureLookups() + { + // Arrange + var orgId = Guid.NewGuid(); + var providerUserId = "ext-measure-existing-no-orguser"; + var user = new User { Id = Guid.NewGuid(), Email = "existing2@example.com" }; + var organization = new Organization { Id = orgId, Name = "Org" }; + + var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); + var controller = CreateControllerWithAuth( + authResult, + out var authService, + out var ssoConfigRepository, + out var userRepository, + out var organizationRepository, + out var organizationUserRepository, + out var interactionService, + out var i18nService, + out var ssoUserRepository, + out var eventService, + out var featureService); + + ConfigureSsoAndUser( + ssoConfigRepository, + userRepository, + organizationRepository, + organizationUserRepository, + orgId, + providerUserId, + user, + organization, + orgUser: null); + + // Ensure orgUser lookup returns null + organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id).Returns((OrganizationUser?)null); + + featureService.IsEnabled(Arg.Any()).Returns(true); + interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + + // Act + try + { + _ = await controller.ExternalCallback(); + } + catch + { + // ignore for measurement only + } + + // Assert (measurement only - no asserts on counts) + var userGetBySso = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetBySsoUserAsync)); + var userGetByEmail = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetByEmailAsync)); + var orgGet = organizationRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationRepository.GetByIdAsync)); + var orgUserGetByOrg = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationAsync)); + var orgUserGetByEmail = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationEmailAsync)); + + _output.WriteLine($"GetBySsoUserAsync: {userGetBySso}"); + _output.WriteLine($"GetByEmailAsync: {userGetByEmail}"); + _output.WriteLine($"GetByIdAsync (Org): {orgGet}"); + _output.WriteLine($"GetByOrganizationAsync (OrgUser): {orgUserGetByOrg}"); + _output.WriteLine($"GetByOrganizationEmailAsync (OrgUser): {orgUserGetByEmail}"); + + // Snapshot assertions + Assert.Equal(1, userGetBySso); + Assert.Equal(0, userGetByEmail); + Assert.Equal(1, orgGet); + Assert.Equal(1, orgUserGetByOrg); + Assert.Equal(0, orgUserGetByEmail); + } + + /// + /// PM-24579: Temporary test, remove with feature flag. + /// + [Fact] + public async Task ExternalCallback_PreventNonCompliantFalse_ExistingSsoLinkedAccepted_MeasureLookups() + { + // Arrange + var orgId = Guid.NewGuid(); + var providerUserId = "ext-measure-existing-flagoff"; + var user = new User { Id = Guid.NewGuid(), Email = "existing.flagoff@example.com" }; + + var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); + var controller = CreateControllerWithAuth( + authResult, + out var authService, + out var ssoConfigRepository, + out var userRepository, + out var organizationRepository, + out var organizationUserRepository, + out var interactionService, + out var i18nService, + out var ssoUserRepository, + out var eventService, + out var featureService); + + var ssoConfig = new SsoConfig { OrganizationId = orgId, Enabled = true }; + var ssoData = new SsoConfigurationData(); + ssoConfig.SetData(ssoData); + ssoConfigRepository.GetByOrganizationIdAsync(orgId).Returns(ssoConfig); + userRepository.GetBySsoUserAsync(providerUserId, orgId).Returns(user); + + featureService.IsEnabled(Arg.Any()).Returns(false); + interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + + // Act + try { _ = await controller.ExternalCallback(); } catch { } + + // Assert (measurement) + var userGetBySso = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetBySsoUserAsync)); + var userGetByEmail = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetByEmailAsync)); + var orgGet = organizationRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationRepository.GetByIdAsync)); + var orgUserGetByOrg = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationAsync)); + var orgUserGetByEmail = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationEmailAsync)); + + _output.WriteLine($"[flag off] GetBySsoUserAsync: {userGetBySso}"); + _output.WriteLine($"[flag off] GetByEmailAsync: {userGetByEmail}"); + _output.WriteLine($"[flag off] GetByIdAsync (Org): {orgGet}"); + _output.WriteLine($"[flag off] GetByOrganizationAsync (OrgUser): {orgUserGetByOrg}"); + _output.WriteLine($"[flag off] GetByOrganizationEmailAsync (OrgUser): {orgUserGetByEmail}"); + } + + /// + /// PM-24579: Temporary test, remove with feature flag. + /// + [Fact] + public async Task ExternalCallback_PreventNonCompliantFalse_ExistingUser_NoOrgUser_MeasureLookups() + { + // Arrange + var orgId = Guid.NewGuid(); + var providerUserId = "ext-measure-existing-no-orguser-flagoff"; + var user = new User { Id = Guid.NewGuid(), Email = "existing2.flagoff@example.com" }; + + var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); + var controller = CreateControllerWithAuth( + authResult, + out var authService, + out var ssoConfigRepository, + out var userRepository, + out var organizationRepository, + out var organizationUserRepository, + out var interactionService, + out var i18nService, + out var ssoUserRepository, + out var eventService, + out var featureService); + + var ssoConfig = new SsoConfig { OrganizationId = orgId, Enabled = true }; + var ssoData = new SsoConfigurationData(); + ssoConfig.SetData(ssoData); + ssoConfigRepository.GetByOrganizationIdAsync(orgId).Returns(ssoConfig); + userRepository.GetBySsoUserAsync(providerUserId, orgId).Returns(user); + + featureService.IsEnabled(Arg.Any()).Returns(false); + interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + + // Act + try { _ = await controller.ExternalCallback(); } catch { } + + // Assert (measurement) + var userGetBySso = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetBySsoUserAsync)); + var userGetByEmail = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetByEmailAsync)); + var orgGet = organizationRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationRepository.GetByIdAsync)); + var orgUserGetByOrg = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationAsync)); + var orgUserGetByEmail = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationEmailAsync)); + + _output.WriteLine($"[flag off] GetBySsoUserAsync: {userGetBySso}"); + _output.WriteLine($"[flag off] GetByEmailAsync: {userGetByEmail}"); + _output.WriteLine($"[flag off] GetByIdAsync (Org): {orgGet}"); + _output.WriteLine($"[flag off] GetByOrganizationAsync (OrgUser): {orgUserGetByOrg}"); + _output.WriteLine($"[flag off] GetByOrganizationEmailAsync (OrgUser): {orgUserGetByEmail}"); + } + + /// + /// PM-24579: Temporary test, remove with feature flag. + /// + [Fact] + public async Task ExternalCallback_PreventNonCompliantFalse_JitProvision_MeasureLookups() + { + // Arrange + var orgId = Guid.NewGuid(); + var providerUserId = "ext-measure-jit-flagoff"; + var email = "jit.flagoff@example.com"; + var organization = new Organization { Id = orgId, Name = "Org", Seats = null }; + + var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, email); + var controller = CreateControllerWithAuth( + authResult, + out var authService, + out var ssoConfigRepository, + out var userRepository, + out var organizationRepository, + out var organizationUserRepository, + out var interactionService, + out var i18nService, + out var ssoUserRepository, + out var eventService, + out var featureService); + + var ssoConfig = new SsoConfig { OrganizationId = orgId, Enabled = true }; + var ssoData = new SsoConfigurationData(); + ssoConfig.SetData(ssoData); + ssoConfigRepository.GetByOrganizationIdAsync(orgId).Returns(ssoConfig); + + // JIT (no existing user or sso link) + userRepository.GetBySsoUserAsync(providerUserId, orgId).Returns((User?)null); + userRepository.GetByEmailAsync(email).Returns((User?)null); + organizationRepository.GetByIdAsync(orgId).Returns(organization); + organizationUserRepository.GetByOrganizationEmailAsync(orgId, email).Returns((OrganizationUser?)null); + + featureService.IsEnabled(Arg.Any()).Returns(false); + interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + + // Act + try { _ = await controller.ExternalCallback(); } catch { } + + // Assert (measurement) + var userGetBySso = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetBySsoUserAsync)); + var userGetByEmail = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetByEmailAsync)); + var orgGet = organizationRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationRepository.GetByIdAsync)); + var orgUserGetByOrg = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationAsync)); + var orgUserGetByEmail = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationEmailAsync)); + + _output.WriteLine($"[flag off] GetBySsoUserAsync: {userGetBySso}"); + _output.WriteLine($"[flag off] GetByEmailAsync: {userGetByEmail}"); + _output.WriteLine($"[flag off] GetByIdAsync (Org): {orgGet}"); + _output.WriteLine($"[flag off] GetByOrganizationAsync (OrgUser): {orgUserGetByOrg}"); + _output.WriteLine($"[flag off] GetByOrganizationEmailAsync (OrgUser): {orgUserGetByEmail}"); + } + [Fact] public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLinkAndReturnsUser() { @@ -504,7 +997,7 @@ public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLink Assert.NotNull(method); // Act - var task = (Task)method.Invoke(controller, [ + var task = (Task<(User user, Organization organization, OrganizationUser orgUser)>)method.Invoke(controller, [ orgId.ToString(), providerUserId, claims, @@ -514,10 +1007,10 @@ public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLink orgUser ])!; - var returnedUser = await task; + var returned = await task; // Assert - Assert.Equal(existingUser.Id, returnedUser.Id); + Assert.Equal(existingUser.Id, returned.user.Id); await ssoUserRepository.Received().CreateAsync(Arg.Is(s => s.OrganizationId == orgId && s.UserId == existingUser.Id && s.ExternalId == providerUserId)); @@ -526,4 +1019,38 @@ await eventService.Received().LogOrganizationUserEventAsync( orgUser, EventType.OrganizationUser_FirstSsoLogin); } + + /// + /// PM-24579: Temporary comparison test to ensure the feature flag ON does not + /// regress lookup counts compared to OFF. When removing the flag, delete this + /// comparison test and keep the specific scenario snapshot tests if desired. + /// + [Fact] + public async Task ExternalCallback_Measurements_FlagOnVsOff_Comparisons() + { + // Arrange + var scenarios = new[] + { + MeasurementScenario.ExistingSsoLinkedAccepted, + MeasurementScenario.ExistingUserNoOrgUser, + MeasurementScenario.JitProvision + }; + + foreach (var scenario in scenarios) + { + // Act + var onCounts = await MeasureCountsForScenarioAsync(scenario, preventNonCompliant: true); + var offCounts = await MeasureCountsForScenarioAsync(scenario, preventNonCompliant: false); + + // Assert: off should not exceed on in any measured lookup type + Assert.True(offCounts.UserGetBySso <= onCounts.UserGetBySso, $"{scenario}: off UserGetBySso={offCounts.UserGetBySso} > on {onCounts.UserGetBySso}"); + Assert.True(offCounts.UserGetByEmail <= onCounts.UserGetByEmail, $"{scenario}: off UserGetByEmail={offCounts.UserGetByEmail} > on {onCounts.UserGetByEmail}"); + Assert.True(offCounts.OrgGetById <= onCounts.OrgGetById, $"{scenario}: off OrgGetById={offCounts.OrgGetById} > on {onCounts.OrgGetById}"); + Assert.True(offCounts.OrgUserGetByOrg <= onCounts.OrgUserGetByOrg, $"{scenario}: off OrgUserGetByOrg={offCounts.OrgUserGetByOrg} > on {onCounts.OrgUserGetByOrg}"); + Assert.True(offCounts.OrgUserGetByEmail <= onCounts.OrgUserGetByEmail, $"{scenario}: off OrgUserGetByEmail={offCounts.OrgUserGetByEmail} > on {onCounts.OrgUserGetByEmail}"); + + _output.WriteLine($"Scenario={scenario} | ON: SSO={onCounts.UserGetBySso}, Email={onCounts.UserGetByEmail}, Org={onCounts.OrgGetById}, OrgUserByOrg={onCounts.OrgUserGetByOrg}, OrgUserByEmail={onCounts.OrgUserGetByEmail}"); + _output.WriteLine($"Scenario={scenario} | OFF: SSO={offCounts.UserGetBySso}, Email={offCounts.UserGetByEmail}, Org={offCounts.OrgGetById}, OrgUserByOrg={offCounts.OrgUserGetByOrg}, OrgUserByEmail={offCounts.OrgUserGetByEmail}"); + } + } } From 6a74289f5b23ecab1bf7684923153fb6cf4a4dc8 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 29 Sep 2025 12:39:52 -0400 Subject: [PATCH 10/34] test(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Removed unnessessary code --- .../src/Sso/Controllers/AccountController.cs | 29 ++++++------------- .../Controllers/AccountControllerTest.cs | 13 +++++---- 2 files changed, 17 insertions(+), 25 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index bcf4ed9f51f3..30c9415c32e6 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -269,18 +269,16 @@ public async Task ExternalCallback() // If we're manually linking to SSO, the user's external identifier will be passed as query string parameter. var userIdentifier = result.Properties.Items.Keys.Contains("user_identifier") ? result.Properties.Items["user_identifier"] : null; - // Pass any pre-resolved org/orgUser only when the flag is enabled // PM-24579: After removing the feature flag, just call AutoProvisionUserAsync // and always use the returned organization/orgUser. The conditional arguments // can be collapsed away at that time. + // Pass current organization/orgUser (likely null) and let provisioning populate them once. var provision = await AutoProvisionUserAsync( provider, providerUserId, claims, userIdentifier, - ssoConfigData, - preventNonCompliant ? organization : null, - preventNonCompliant ? orgUser : null); + ssoConfigData); user = provision.user; // PM-24579: After removing the flag, assign these unconditionally and remove this if block. if (preventNonCompliant) @@ -492,9 +490,7 @@ static bool nameIdIsNotTransient(Claim c) => c.Type == ClaimTypes.NameIdentifier string providerUserId, IEnumerable claims, string userIdentifier, - SsoConfigurationData config, - Organization organization, - OrganizationUser orgUser) + SsoConfigurationData config) { var name = GetName(claims, config.GetAdditionalNameClaimTypes()); var email = GetEmailAddress(claims, config.GetAdditionalEmailClaimTypes()); @@ -523,19 +519,12 @@ static bool nameIdIsNotTransient(Claim c) => c.Type == ClaimTypes.NameIdentifier existingUser = await GetUserFromManualLinkingData(userIdentifier); } - // Try to find the OrganizationUser if it exists. Use preloaded values if provided. - if (organization == null || orgUser == null) - { - var (foundOrganization, foundOrgUser) = await FindOrganizationUser(existingUser, email, orgId); - if (organization == null) - { - organization = foundOrganization; - } - if (orgUser == null) - { - orgUser = foundOrgUser; - } - } + // Find organization and orgUser (none are preloaded). + Organization organization = null; + OrganizationUser orgUser = null; + var (foundOrganization, foundOrgUser) = await FindOrganizationUser(existingUser, email, orgId); + organization ??= foundOrganization; + orgUser ??= foundOrgUser; //---------------------------------------------------- // Scenario 1: We've found the user in the User table diff --git a/test/SSO.Test/Controllers/AccountControllerTest.cs b/test/SSO.Test/Controllers/AccountControllerTest.cs index 12af2a8f9c1a..ffa167f8a122 100644 --- a/test/SSO.Test/Controllers/AccountControllerTest.cs +++ b/test/SSO.Test/Controllers/AccountControllerTest.cs @@ -980,6 +980,10 @@ public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLink // Arrange repository expectations for the flow userRepository.GetByEmailAsync(email).Returns(existingUser); + organizationRepository.GetByIdAsync(orgId).Returns(organization); + organizationUserRepository.GetManyByUserAsync(existingUser.Id) + .Returns(new List { orgUser }); + organizationUserRepository.GetByOrganizationEmailAsync(orgId, email).Returns(orgUser); // No existing SSO link so first SSO login event is logged ssoUserRepository.GetByUserIdOrganizationIdAsync(orgId, existingUser.Id).Returns((SsoUser?)null); @@ -997,15 +1001,14 @@ public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLink Assert.NotNull(method); // Act - var task = (Task<(User user, Organization organization, OrganizationUser orgUser)>)method.Invoke(controller, [ + var task = (Task<(User user, Organization organization, OrganizationUser orgUser)>)method!.Invoke(controller, new object[] + { orgId.ToString(), providerUserId, claims, null!, - config, - organization, - orgUser - ])!; + config + })!; var returned = await task; From 597eb690412834e9661983f55cf994ca85b6dda5 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 29 Sep 2025 12:51:11 -0400 Subject: [PATCH 11/34] test(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Removed more unneeded code --- .../src/Sso/Controllers/AccountController.cs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index 30c9415c32e6..ec0fd2c87c2c 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -269,10 +269,6 @@ public async Task ExternalCallback() // If we're manually linking to SSO, the user's external identifier will be passed as query string parameter. var userIdentifier = result.Properties.Items.Keys.Contains("user_identifier") ? result.Properties.Items["user_identifier"] : null; - // PM-24579: After removing the feature flag, just call AutoProvisionUserAsync - // and always use the returned organization/orgUser. The conditional arguments - // can be collapsed away at that time. - // Pass current organization/orgUser (likely null) and let provisioning populate them once. var provision = await AutoProvisionUserAsync( provider, providerUserId, @@ -520,11 +516,7 @@ static bool nameIdIsNotTransient(Claim c) => c.Type == ClaimTypes.NameIdentifier } // Find organization and orgUser (none are preloaded). - Organization organization = null; - OrganizationUser orgUser = null; - var (foundOrganization, foundOrgUser) = await FindOrganizationUser(existingUser, email, orgId); - organization ??= foundOrganization; - orgUser ??= foundOrgUser; + var (organization, orgUser) = await FindOrganizationUser(existingUser, email, orgId); //---------------------------------------------------- // Scenario 1: We've found the user in the User table From f06c88932b67bad4714d3f62bfa22af99e8ddb7a Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 29 Sep 2025 12:54:35 -0400 Subject: [PATCH 12/34] test(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Removed even more unneeded code. --- .../src/Sso/Controllers/AccountController.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index ec0fd2c87c2c..4d9c6ece3b1b 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -279,8 +279,8 @@ public async Task ExternalCallback() // PM-24579: After removing the flag, assign these unconditionally and remove this if block. if (preventNonCompliant) { - organization ??= provision.organization; - orgUser ??= provision.orgUser; + organization = provision.foundOrganization; + orgUser = provision.foundOrgUser; } } @@ -481,7 +481,7 @@ static bool nameIdIsNotTransient(Claim c) => c.Type == ClaimTypes.NameIdentifier /// The org user pair, can be null. /// The User to sign in. /// An exception if the user cannot be provisioned as requested. - private async Task<(User user, Organization organization, OrganizationUser orgUser)> AutoProvisionUserAsync( + private async Task<(User user, Organization foundOrganization, OrganizationUser foundOrgUser)> AutoProvisionUserAsync( string provider, string providerUserId, IEnumerable claims, @@ -515,7 +515,7 @@ static bool nameIdIsNotTransient(Claim c) => c.Type == ClaimTypes.NameIdentifier existingUser = await GetUserFromManualLinkingData(userIdentifier); } - // Find organization and orgUser (none are preloaded). + // Try to find the OrganizationUser if it exists. var (organization, orgUser) = await FindOrganizationUser(existingUser, email, orgId); //---------------------------------------------------- From 811eb0b30b63be43437a59e2fe09cb7dbbd69c0b Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 29 Sep 2025 13:01:20 -0400 Subject: [PATCH 13/34] test(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Removed even even more unneeded code. --- .../src/Sso/Controllers/AccountController.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index 4d9c6ece3b1b..8b458c43e4a7 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -275,7 +275,6 @@ public async Task ExternalCallback() claims, userIdentifier, ssoConfigData); - user = provision.user; // PM-24579: After removing the flag, assign these unconditionally and remove this if block. if (preventNonCompliant) { @@ -477,16 +476,15 @@ static bool nameIdIsNotTransient(Claim c) => c.Type == ClaimTypes.NameIdentifier /// The claims from the external IdP. /// The user identifier used for manual SSO linking. /// The SSO configuration for the organization. - /// The organization to find the user in, can be null. - /// The org user pair, can be null. - /// The User to sign in. + /// The User to sign in as well as the found organization and org user. /// An exception if the user cannot be provisioned as requested. private async Task<(User user, Organization foundOrganization, OrganizationUser foundOrgUser)> AutoProvisionUserAsync( string provider, string providerUserId, IEnumerable claims, string userIdentifier, - SsoConfigurationData config) + SsoConfigurationData config + ) { var name = GetName(claims, config.GetAdditionalNameClaimTypes()); var email = GetEmailAddress(claims, config.GetAdditionalEmailClaimTypes()); From b173cd3cfa67ae071df6541740ba334a5495ed31 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 29 Sep 2025 13:08:29 -0400 Subject: [PATCH 14/34] fix(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Whoops, added back in user being assigned. --- bitwarden_license/src/Sso/Controllers/AccountController.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index 8b458c43e4a7..5a1f89cf38dc 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -275,6 +275,7 @@ public async Task ExternalCallback() claims, userIdentifier, ssoConfigData); + user = provision.user; // PM-24579: After removing the flag, assign these unconditionally and remove this if block. if (preventNonCompliant) { From 5845ca048938d82d4d8e8e058018769981c502e8 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 29 Sep 2025 13:16:38 -0400 Subject: [PATCH 15/34] fix(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Removed uneeded function. --- .../src/Sso/Controllers/AccountController.cs | 34 ------------------- 1 file changed, 34 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index 5a1f89cf38dc..d3b87990ecf7 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -647,40 +647,6 @@ SsoConfigurationData config return (user, organization, orgUser); } - private async Task<(Organization, OrganizationUser)> ResolveOrganizationAndMembershipAsync( - string provider, User user, IEnumerable claims, SsoConfigurationData ssoConfigData) - { - Organization organization = null; - OrganizationUser orgUser = null; - - if (!Guid.TryParse(provider, out var organizationId)) - { - _logger.LogError("Failed to parse organization ID from provider: {Provider}", provider); - return (null, null); - } - - organization = await _organizationRepository.GetByIdAsync(organizationId); - if (organization == null) - { - _logger.LogError("Organization not found for provider: {Provider}", provider); - return (null, null); - } - - if (user != null) - { - orgUser = await _organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id); - return (organization, orgUser); - } - - var emailFromClaims = GetEmailAddress(claims, ssoConfigData.GetAdditionalEmailClaimTypes()); - if (!string.IsNullOrWhiteSpace(emailFromClaims)) - { - orgUser = await _organizationUserRepository.GetByOrganizationEmailAsync(organization.Id, emailFromClaims); - } - - return (organization, orgUser); - } - private async Task GetUserFromManualLinkingData(string userIdentifier) { User user = null; From 26f0b668c7dcfdebc5ebac993d04852523c02565 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 6 Oct 2025 16:15:45 -0400 Subject: [PATCH 16/34] fix(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Converted to using bit auto data and sut provider. --- .../Controllers/AccountControllerTest.cs | 588 ++++++------------ test/SSO.Test/SSO.Test.csproj | 1 + 2 files changed, 200 insertions(+), 389 deletions(-) diff --git a/test/SSO.Test/Controllers/AccountControllerTest.cs b/test/SSO.Test/Controllers/AccountControllerTest.cs index ffa167f8a122..61bdf4abd037 100644 --- a/test/SSO.Test/Controllers/AccountControllerTest.cs +++ b/test/SSO.Test/Controllers/AccountControllerTest.cs @@ -2,36 +2,34 @@ using System.Security.Claims; using Bit.Core; using Bit.Core.AdminConsole.Entities; -using Bit.Core.AdminConsole.Repositories; using Bit.Core.Auth.Entities; -using Bit.Core.Auth.Models.Business.Tokenables; using Bit.Core.Auth.Models.Data; using Bit.Core.Auth.Repositories; -using Bit.Core.Auth.UserFeatures.Registration; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Repositories; using Bit.Core.Services; -using Bit.Core.Settings; -using Bit.Core.Tokens; using Bit.Sso.Controllers; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; using Duende.IdentityModel; using Duende.IdentityServer.Configuration; using Duende.IdentityServer.Models; using Duende.IdentityServer.Services; -using Duende.IdentityServer.Stores; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Identity; using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.Logging; using NSubstitute; using Xunit.Abstractions; using AuthenticationOptions = Duende.IdentityServer.Configuration.AuthenticationOptions; namespace Bit.SSO.Test.Controllers; +[ + ControllerCustomize(typeof(AccountController)), + SutProviderCustomize +] public class AccountControllerTest { private readonly ITestOutputHelper _output; @@ -40,86 +38,24 @@ public AccountControllerTest(ITestOutputHelper output) { _output = output; } - private static AccountController CreateController( - IAuthenticationService authenticationService, - out ISsoConfigRepository ssoConfigRepository, - out IUserRepository userRepository, - out IOrganizationRepository organizationRepository, - out IOrganizationUserRepository organizationUserRepository, - out IIdentityServerInteractionService interactionService, - out II18nService i18nService, - out ISsoUserRepository ssoUserRepository, - out Core.Services.IEventService eventService, - out IFeatureService featureService) + + private static IAuthenticationService SetupHttpContextWithAuth( + SutProvider sutProvider, + AuthenticateResult authResult, + IAuthenticationService? authService = null) { var schemeProvider = Substitute.For(); schemeProvider.GetDefaultAuthenticateSchemeAsync() .Returns(new AuthenticationScheme("idsrv", "idsrv", typeof(IAuthenticationHandler))); - var clientStore = Substitute.For(); - interactionService = Substitute.For(); - var logger = Substitute.For>(); - organizationRepository = Substitute.For(); - organizationUserRepository = Substitute.For(); - var organizationService = Substitute.For(); - ssoConfigRepository = Substitute.For(); - ssoUserRepository = Substitute.For(); - userRepository = Substitute.For(); - var policyRepository = Substitute.For(); - var userService = Substitute.For(); - i18nService = Substitute.For(); - i18nService.T(Arg.Any(), Arg.Any()).Returns(ci => (string)ci[0]!); - - // Minimal UserManager setup (not used in tested code paths, but required by ctor) - var userStore = Substitute.For>(); - var identityOptions = Microsoft.Extensions.Options.Options.Create(new IdentityOptions()); - var passwordHasher = Substitute.For>(); - var userValidators = Array.Empty>(); - var passwordValidators = Array.Empty>(); - var lookupNormalizer = Substitute.For(); - var errorDescriber = new IdentityErrorDescriber(); - var userLogger = Substitute.For>>(); - var userManager = new UserManager( - userStore, - identityOptions, - passwordHasher, - userValidators, - passwordValidators, - lookupNormalizer, - errorDescriber, - new ServiceCollection().BuildServiceProvider(), - userLogger); - - var globalSettings = Substitute.For(); - eventService = Substitute.For(); - var dataProtector = Substitute.For>(); - var organizationDomainRepository = Substitute.For(); - var registerUserCommand = Substitute.For(); - featureService = Substitute.For(); - - var controller = new AccountController( - schemeProvider, - clientStore, - interactionService, - logger, - organizationRepository, - organizationUserRepository, - organizationService, - ssoConfigRepository, - ssoUserRepository, - userRepository, - policyRepository, - userService, - i18nService, - userManager, - globalSettings, - eventService, - dataProtector, - organizationDomainRepository, - registerUserCommand, - featureService); + + var resolvedAuthService = authService ?? Substitute.For(); + resolvedAuthService.AuthenticateAsync( + Arg.Any(), + AuthenticationSchemes.BitwardenExternalCookieAuthenticationScheme) + .Returns(authResult); var services = new ServiceCollection(); - services.AddSingleton(authenticationService); + services.AddSingleton(resolvedAuthService); services.AddSingleton(schemeProvider); services.AddSingleton(new IdentityServerOptions { @@ -130,7 +66,7 @@ private static AccountController CreateController( }); var sp = services.BuildServiceProvider(); - controller.ControllerContext = new ControllerContext + sutProvider.Sut.ControllerContext = new ControllerContext { HttpContext = new DefaultHttpContext { @@ -138,7 +74,7 @@ private static AccountController CreateController( } }; - return controller; + return resolvedAuthService; } private static void InvokeEnsureOrgUserStatusAllowed( @@ -175,49 +111,19 @@ private static AuthenticateResult BuildSuccessfulExternalAuth(Guid orgId, string return AuthenticateResult.Success(ticket); } - private static AccountController CreateControllerWithAuth( - AuthenticateResult authResult, - out IAuthenticationService authService, - out ISsoConfigRepository ssoConfigRepository, - out IUserRepository userRepository, - out IOrganizationRepository organizationRepository, - out IOrganizationUserRepository organizationUserRepository, - out IIdentityServerInteractionService interactionService, - out II18nService i18nService, - out ISsoUserRepository ssoUserRepository, - out Core.Services.IEventService eventService, - out IFeatureService featureService) - { - var authServiceSub = Substitute.For(); - authServiceSub.AuthenticateAsync( - Arg.Any(), - AuthenticationSchemes.BitwardenExternalCookieAuthenticationScheme) - .Returns(authResult); - authService = authServiceSub; - return CreateController( - authServiceSub, - out ssoConfigRepository, - out userRepository, - out organizationRepository, - out organizationUserRepository, - out interactionService, - out i18nService, - out ssoUserRepository, - out eventService, - out featureService); - } - private static void ConfigureSsoAndUser( - ISsoConfigRepository ssoConfigRepository, - IUserRepository userRepository, - IOrganizationRepository organizationRepository, - IOrganizationUserRepository organizationUserRepository, + SutProvider sutProvider, Guid orgId, string providerUserId, User user, Organization? organization = null, OrganizationUser? orgUser = null) { + var ssoConfigRepository = sutProvider.GetDependency(); + var userRepository = sutProvider.GetDependency(); + var organizationRepository = sutProvider.GetDependency(); + var organizationUserRepository = sutProvider.GetDependency(); + var ssoConfig = new SsoConfig { OrganizationId = orgId, Enabled = true }; var ssoData = new SsoConfigurationData(); ssoConfig.SetData(ssoData); @@ -251,7 +157,10 @@ private sealed class LookupCounts public int OrgUserGetByEmail { get; init; } } - private async Task MeasureCountsForScenarioAsync(MeasurementScenario scenario, bool preventNonCompliant) + private async Task MeasureCountsForScenarioAsync( + SutProvider sutProvider, + MeasurementScenario scenario, + bool preventNonCompliant) { var orgId = Guid.NewGuid(); var providerUserId = $"meas-{scenario}-{(preventNonCompliant ? "on" : "off")}"; @@ -263,20 +172,16 @@ private async Task MeasureCountsForScenarioAsync(MeasurementScenar var user = new User { Id = Guid.NewGuid(), Email = email }; var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, email); - var controller = CreateControllerWithAuth( - authResult, - out var authService, - out var ssoConfigRepository, - out var userRepository, - out var organizationRepository, - out var organizationUserRepository, - out var interactionService, - out var i18nService, - out var ssoUserRepository, - out var eventService, - out var featureService); + SetupHttpContextWithAuth(sutProvider, authResult); // SSO config present + var ssoConfigRepository = sutProvider.GetDependency(); + var userRepository = sutProvider.GetDependency(); + var organizationRepository = sutProvider.GetDependency(); + var organizationUserRepository = sutProvider.GetDependency(); + var featureService = sutProvider.GetDependency(); + var interactionService = sutProvider.GetDependency(); + var ssoConfig = new SsoConfig { OrganizationId = orgId, Enabled = true }; var ssoData = new SsoConfigurationData(); ssoConfig.SetData(ssoData); @@ -316,14 +221,14 @@ private async Task MeasureCountsForScenarioAsync(MeasurementScenar try { - _ = await controller.ExternalCallback(); + _ = await sutProvider.Sut.ExternalCallback(); } catch { // Ignore exceptions for measurement; some flows can throw based on status enforcement } - return new LookupCounts + var counts = new LookupCounts { UserGetBySso = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetBySsoUserAsync)), UserGetByEmail = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetByEmailAsync)), @@ -331,31 +236,29 @@ private async Task MeasureCountsForScenarioAsync(MeasurementScenar OrgUserGetByOrg = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationAsync)), OrgUserGetByEmail = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationEmailAsync)), }; + + userRepository.ClearReceivedCalls(); + organizationRepository.ClearReceivedCalls(); + organizationUserRepository.ClearReceivedCalls(); + + return counts; } - [Fact] - public void EnsureOrgUserStatusAllowed_AllowsAcceptedAndConfirmed() + [Theory, BitAutoData] + public void EnsureOrgUserStatusAllowed_AllowsAcceptedAndConfirmed( + SutProvider sutProvider) { // Arrange - var authService = Substitute.For(); - var controller = CreateController( - authService, - out var ssoConfigRepository, - out var userRepository, - out var organizationRepository, - out var organizationUserRepository, - out var interactionService, - out var i18nService, - out var ssoUserRepository, - out var eventService, - out var featureService); + sutProvider.GetDependency() + .T(Arg.Any(), Arg.Any()) + .Returns(ci => (string)ci[0]!); // Act var ex1 = Record.Exception(() => - InvokeEnsureOrgUserStatusAllowed(controller, OrganizationUserStatusType.Accepted, + InvokeEnsureOrgUserStatusAllowed(sutProvider.Sut, OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); var ex2 = Record.Exception(() => - InvokeEnsureOrgUserStatusAllowed(controller, OrganizationUserStatusType.Confirmed, + InvokeEnsureOrgUserStatusAllowed(sutProvider.Sut, OrganizationUserStatusType.Confirmed, OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); // Assert @@ -363,26 +266,18 @@ public void EnsureOrgUserStatusAllowed_AllowsAcceptedAndConfirmed() Assert.Null(ex2); } - [Fact] - public void EnsureOrgUserStatusAllowed_Invited_ThrowsAcceptInvite() + [Theory, BitAutoData] + public void EnsureOrgUserStatusAllowed_Invited_ThrowsAcceptInvite( + SutProvider sutProvider) { // Arrange - var authService = Substitute.For(); - var controller = CreateController( - authService, - out var ssoConfigRepository, - out var userRepository, - out var organizationRepository, - out var organizationUserRepository, - out var interactionService, - out var i18nService, - out var ssoUserRepository, - out var eventService, - out var featureService); + sutProvider.GetDependency() + .T(Arg.Any(), Arg.Any()) + .Returns(ci => (string)ci[0]!); // Act var ex = Assert.Throws(() => - InvokeEnsureOrgUserStatusAllowed(controller, OrganizationUserStatusType.Invited, + InvokeEnsureOrgUserStatusAllowed(sutProvider.Sut, OrganizationUserStatusType.Invited, OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); // Assert @@ -390,26 +285,18 @@ public void EnsureOrgUserStatusAllowed_Invited_ThrowsAcceptInvite() Assert.Equal("AcceptInviteBeforeUsingSSO", ex.InnerException!.Message); } - [Fact] - public void EnsureOrgUserStatusAllowed_Revoked_ThrowsAccessRevoked() + [Theory, BitAutoData] + public void EnsureOrgUserStatusAllowed_Revoked_ThrowsAccessRevoked( + SutProvider sutProvider) { // Arrange - var authService = Substitute.For(); - var controller = CreateController( - authService, - out var ssoConfigRepository, - out var userRepository, - out var organizationRepository, - out var organizationUserRepository, - out var interactionService, - out var i18nService, - out var ssoUserRepository, - out var eventService, - out var featureService); + sutProvider.GetDependency() + .T(Arg.Any(), Arg.Any()) + .Returns(ci => (string)ci[0]!); // Act var ex = Assert.Throws(() => - InvokeEnsureOrgUserStatusAllowed(controller, OrganizationUserStatusType.Revoked, + InvokeEnsureOrgUserStatusAllowed(sutProvider.Sut, OrganizationUserStatusType.Revoked, OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); // Assert @@ -417,28 +304,20 @@ public void EnsureOrgUserStatusAllowed_Revoked_ThrowsAccessRevoked() Assert.Equal("OrganizationUserAccessRevoked", ex.InnerException!.Message); } - [Fact] - public void EnsureOrgUserStatusAllowed_UnknownStatus_ThrowsUnknown() + [Theory, BitAutoData] + public void EnsureOrgUserStatusAllowed_UnknownStatus_ThrowsUnknown( + SutProvider sutProvider) { // Arrange - var authService = Substitute.For(); - var controller = CreateController( - authService, - out var ssoConfigRepository, - out var userRepository, - out var organizationRepository, - out var organizationUserRepository, - out var interactionService, - out var i18nService, - out var ssoUserRepository, - out var eventService, - out var featureService); + sutProvider.GetDependency() + .T(Arg.Any(), Arg.Any()) + .Returns(ci => (string)ci[0]!); var unknown = (OrganizationUserStatusType)999; // Act var ex = Assert.Throws(() => - InvokeEnsureOrgUserStatusAllowed(controller, unknown, + InvokeEnsureOrgUserStatusAllowed(sutProvider.Sut, unknown, OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); // Assert @@ -446,8 +325,9 @@ public void EnsureOrgUserStatusAllowed_UnknownStatus_ThrowsUnknown() Assert.Equal("OrganizationUserUnknownStatus", ex.InnerException!.Message); } - [Fact] - public async Task ExternalCallback_WithExistingUserAndAcceptedMembership_RedirectsToReturnUrl() + [Theory, BitAutoData] + public async Task ExternalCallback_WithExistingUserAndAcceptedMembership_RedirectsToReturnUrl( + SutProvider sutProvider) { // Arrange var orgId = Guid.NewGuid(); @@ -463,35 +343,22 @@ public async Task ExternalCallback_WithExistingUserAndAcceptedMembership_Redirec }; var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); - var controller = CreateControllerWithAuth( - authResult, - out var authService, - out var ssoConfigRepository, - out var userRepository, - out var organizationRepository, - out var organizationUserRepository, - out var interactionService, - out var i18nService, - out var ssoUserRepository, - out var eventService, - out var featureService); + var authService = SetupHttpContextWithAuth(sutProvider, authResult); ConfigureSsoAndUser( - ssoConfigRepository, - userRepository, - organizationRepository, - organizationUserRepository, + sutProvider, orgId, providerUserId, user, organization, orgUser); - featureService.IsEnabled(Arg.Any()).Returns(true); - interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + sutProvider.GetDependency().IsEnabled(Arg.Any()).Returns(true); + sutProvider.GetDependency() + .GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); // Act - var result = await controller.ExternalCallback(); + var result = await sutProvider.Sut.ExternalCallback(); // Assert var redirect = Assert.IsType(result); @@ -512,8 +379,9 @@ await authService.Received().SignOutAsync( /// /// PM-24579: Temporary test, remove with feature flag. /// - [Fact] - public async Task ExternalCallback_PreventNonCompliantFalse_SkipsOrgLookupAndSignsIn() + [Theory, BitAutoData] + public async Task ExternalCallback_PreventNonCompliantFalse_SkipsOrgLookupAndSignsIn( + SutProvider sutProvider) { // Arrange var orgId = Guid.NewGuid(); @@ -521,33 +389,20 @@ public async Task ExternalCallback_PreventNonCompliantFalse_SkipsOrgLookupAndSig var user = new User { Id = Guid.NewGuid(), Email = "flagoff@example.com" }; var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); - var controller = CreateControllerWithAuth( - authResult, - out var authService, - out var ssoConfigRepository, - out var userRepository, - out var organizationRepository, - out var organizationUserRepository, - out var interactionService, - out var i18nService, - out var ssoUserRepository, - out var eventService, - out var featureService); + var authService = SetupHttpContextWithAuth(sutProvider, authResult); ConfigureSsoAndUser( - ssoConfigRepository, - userRepository, - organizationRepository, - organizationUserRepository, + sutProvider, orgId, providerUserId, user); - featureService.IsEnabled(Arg.Any()).Returns(false); - interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + sutProvider.GetDependency().IsEnabled(Arg.Any()).Returns(false); + sutProvider.GetDependency() + .GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); // Act - var result = await controller.ExternalCallback(); + var result = await sutProvider.Sut.ExternalCallback(); // Assert var redirect = Assert.IsType(result); @@ -559,16 +414,17 @@ await authService.Received().SignInAsync( Arg.Any(), Arg.Any()); - // When flag is off, controller does not require org or orgUser; ensure repo not called for orgUser - await organizationUserRepository.DidNotReceiveWithAnyArgs().GetByOrganizationAsync(Guid.Empty, Guid.Empty); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .GetByOrganizationAsync(Guid.Empty, Guid.Empty); } /// /// PM-24579: Permanent test, remove the True in PreventNonCompliantTrue and remove the configure for the feature /// flag. /// - [Fact] - public async Task ExternalCallback_PreventNonCompliantTrue_ExistingSsoLinkedAccepted_MeasureLookups() + [Theory, BitAutoData] + public async Task ExternalCallback_PreventNonCompliantTrue_ExistingSsoLinkedAccepted_MeasureLookups( + SutProvider sutProvider) { // Arrange var orgId = Guid.NewGuid(); @@ -584,37 +440,24 @@ public async Task ExternalCallback_PreventNonCompliantTrue_ExistingSsoLinkedAcce }; var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); - var controller = CreateControllerWithAuth( - authResult, - out var authService, - out var ssoConfigRepository, - out var userRepository, - out var organizationRepository, - out var organizationUserRepository, - out var interactionService, - out var i18nService, - out var ssoUserRepository, - out var eventService, - out var featureService); + var authService = SetupHttpContextWithAuth(sutProvider, authResult); ConfigureSsoAndUser( - ssoConfigRepository, - userRepository, - organizationRepository, - organizationUserRepository, + sutProvider, orgId, providerUserId, user, organization, orgUser); - featureService.IsEnabled(Arg.Any()).Returns(true); - interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + sutProvider.GetDependency().IsEnabled(Arg.Any()).Returns(true); + sutProvider.GetDependency() + .GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); // Act try { - _ = await controller.ExternalCallback(); + _ = await sutProvider.Sut.ExternalCallback(); } catch { @@ -622,6 +465,10 @@ public async Task ExternalCallback_PreventNonCompliantTrue_ExistingSsoLinkedAcce } // Assert (measurement only - no asserts on counts) + var userRepository = sutProvider.GetDependency(); + var organizationRepository = sutProvider.GetDependency(); + var organizationUserRepository = sutProvider.GetDependency(); + var userGetBySso = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetBySsoUserAsync)); var userGetByEmail = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetByEmailAsync)); var orgGet = organizationRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationRepository.GetByIdAsync)); @@ -646,8 +493,9 @@ public async Task ExternalCallback_PreventNonCompliantTrue_ExistingSsoLinkedAcce /// PM-24579: Permanent test, remove the True in PreventNonCompliantTrue and remove the configure for the feature /// flag. /// - [Fact] - public async Task ExternalCallback_PreventNonCompliantTrue_JitProvision_MeasureLookups() + [Theory, BitAutoData] + public async Task ExternalCallback_PreventNonCompliantTrue_JitProvision_MeasureLookups( + SutProvider sutProvider) { // Arrange var orgId = Guid.NewGuid(); @@ -656,37 +504,32 @@ public async Task ExternalCallback_PreventNonCompliantTrue_JitProvision_MeasureL var organization = new Organization { Id = orgId, Name = "Org", Seats = null }; var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, email); - var controller = CreateControllerWithAuth( - authResult, - out var authService, - out var ssoConfigRepository, - out var userRepository, - out var organizationRepository, - out var organizationUserRepository, - out var interactionService, - out var i18nService, - out var ssoUserRepository, - out var eventService, - out var featureService); - - // Configure SSO config and ensure there is NO existing SSO link or user (JIT) + SetupHttpContextWithAuth(sutProvider, authResult); + + var ssoConfigRepository = sutProvider.GetDependency(); + var userRepository = sutProvider.GetDependency(); + var organizationRepository = sutProvider.GetDependency(); + var organizationUserRepository = sutProvider.GetDependency(); + var ssoConfig = new SsoConfig { OrganizationId = orgId, Enabled = true }; var ssoData = new SsoConfigurationData(); ssoConfig.SetData(ssoData); ssoConfigRepository.GetByOrganizationIdAsync(orgId).Returns(ssoConfig); + // JIT (no existing user or sso link) userRepository.GetBySsoUserAsync(providerUserId, orgId).Returns((User?)null); userRepository.GetByEmailAsync(email).Returns((User?)null); organizationRepository.GetByIdAsync(orgId).Returns(organization); organizationUserRepository.GetByOrganizationEmailAsync(orgId, email).Returns((OrganizationUser?)null); - featureService.IsEnabled(Arg.Any()).Returns(true); - interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + sutProvider.GetDependency().IsEnabled(Arg.Any()).Returns(true); + sutProvider.GetDependency() + .GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); // Act try { - _ = await controller.ExternalCallback(); + _ = await sutProvider.Sut.ExternalCallback(); } catch { @@ -718,8 +561,9 @@ public async Task ExternalCallback_PreventNonCompliantTrue_JitProvision_MeasureL /// PM-24579: Permanent test, remove the True in PreventNonCompliantTrue and remove the configure for the feature /// flag. /// - [Fact] - public async Task ExternalCallback_PreventNonCompliantTrue_ExistingUser_NoOrgUser_MeasureLookups() + [Theory, BitAutoData] + public async Task ExternalCallback_PreventNonCompliantTrue_ExistingUser_NoOrgUser_MeasureLookups( + SutProvider sutProvider) { // Arrange var orgId = Guid.NewGuid(); @@ -728,24 +572,10 @@ public async Task ExternalCallback_PreventNonCompliantTrue_ExistingUser_NoOrgUse var organization = new Organization { Id = orgId, Name = "Org" }; var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); - var controller = CreateControllerWithAuth( - authResult, - out var authService, - out var ssoConfigRepository, - out var userRepository, - out var organizationRepository, - out var organizationUserRepository, - out var interactionService, - out var i18nService, - out var ssoUserRepository, - out var eventService, - out var featureService); + SetupHttpContextWithAuth(sutProvider, authResult); ConfigureSsoAndUser( - ssoConfigRepository, - userRepository, - organizationRepository, - organizationUserRepository, + sutProvider, orgId, providerUserId, user, @@ -753,15 +583,17 @@ public async Task ExternalCallback_PreventNonCompliantTrue_ExistingUser_NoOrgUse orgUser: null); // Ensure orgUser lookup returns null - organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id).Returns((OrganizationUser?)null); + sutProvider.GetDependency() + .GetByOrganizationAsync(organization.Id, user.Id).Returns((OrganizationUser?)null); - featureService.IsEnabled(Arg.Any()).Returns(true); - interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + sutProvider.GetDependency().IsEnabled(Arg.Any()).Returns(true); + sutProvider.GetDependency() + .GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); // Act try { - _ = await controller.ExternalCallback(); + _ = await sutProvider.Sut.ExternalCallback(); } catch { @@ -769,6 +601,10 @@ public async Task ExternalCallback_PreventNonCompliantTrue_ExistingUser_NoOrgUse } // Assert (measurement only - no asserts on counts) + var userRepository = sutProvider.GetDependency(); + var organizationRepository = sutProvider.GetDependency(); + var organizationUserRepository = sutProvider.GetDependency(); + var userGetBySso = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetBySsoUserAsync)); var userGetByEmail = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetByEmailAsync)); var orgGet = organizationRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationRepository.GetByIdAsync)); @@ -792,8 +628,9 @@ public async Task ExternalCallback_PreventNonCompliantTrue_ExistingUser_NoOrgUse /// /// PM-24579: Temporary test, remove with feature flag. /// - [Fact] - public async Task ExternalCallback_PreventNonCompliantFalse_ExistingSsoLinkedAccepted_MeasureLookups() + [Theory, BitAutoData] + public async Task ExternalCallback_PreventNonCompliantFalse_ExistingSsoLinkedAccepted_MeasureLookups( + SutProvider sutProvider) { // Arrange var orgId = Guid.NewGuid(); @@ -801,32 +638,26 @@ public async Task ExternalCallback_PreventNonCompliantFalse_ExistingSsoLinkedAcc var user = new User { Id = Guid.NewGuid(), Email = "existing.flagoff@example.com" }; var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); - var controller = CreateControllerWithAuth( - authResult, - out var authService, - out var ssoConfigRepository, - out var userRepository, - out var organizationRepository, - out var organizationUserRepository, - out var interactionService, - out var i18nService, - out var ssoUserRepository, - out var eventService, - out var featureService); + SetupHttpContextWithAuth(sutProvider, authResult); var ssoConfig = new SsoConfig { OrganizationId = orgId, Enabled = true }; var ssoData = new SsoConfigurationData(); ssoConfig.SetData(ssoData); - ssoConfigRepository.GetByOrganizationIdAsync(orgId).Returns(ssoConfig); - userRepository.GetBySsoUserAsync(providerUserId, orgId).Returns(user); + sutProvider.GetDependency().GetByOrganizationIdAsync(orgId).Returns(ssoConfig); + sutProvider.GetDependency().GetBySsoUserAsync(providerUserId, orgId).Returns(user); - featureService.IsEnabled(Arg.Any()).Returns(false); - interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + sutProvider.GetDependency().IsEnabled(Arg.Any()).Returns(false); + sutProvider.GetDependency() + .GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); // Act - try { _ = await controller.ExternalCallback(); } catch { } + try { _ = await sutProvider.Sut.ExternalCallback(); } catch { } // Assert (measurement) + var userRepository = sutProvider.GetDependency(); + var organizationRepository = sutProvider.GetDependency(); + var organizationUserRepository = sutProvider.GetDependency(); + var userGetBySso = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetBySsoUserAsync)); var userGetByEmail = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetByEmailAsync)); var orgGet = organizationRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationRepository.GetByIdAsync)); @@ -843,8 +674,9 @@ public async Task ExternalCallback_PreventNonCompliantFalse_ExistingSsoLinkedAcc /// /// PM-24579: Temporary test, remove with feature flag. /// - [Fact] - public async Task ExternalCallback_PreventNonCompliantFalse_ExistingUser_NoOrgUser_MeasureLookups() + [Theory, BitAutoData] + public async Task ExternalCallback_PreventNonCompliantFalse_ExistingUser_NoOrgUser_MeasureLookups( + SutProvider sutProvider) { // Arrange var orgId = Guid.NewGuid(); @@ -852,32 +684,26 @@ public async Task ExternalCallback_PreventNonCompliantFalse_ExistingUser_NoOrgUs var user = new User { Id = Guid.NewGuid(), Email = "existing2.flagoff@example.com" }; var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); - var controller = CreateControllerWithAuth( - authResult, - out var authService, - out var ssoConfigRepository, - out var userRepository, - out var organizationRepository, - out var organizationUserRepository, - out var interactionService, - out var i18nService, - out var ssoUserRepository, - out var eventService, - out var featureService); + SetupHttpContextWithAuth(sutProvider, authResult); var ssoConfig = new SsoConfig { OrganizationId = orgId, Enabled = true }; var ssoData = new SsoConfigurationData(); ssoConfig.SetData(ssoData); - ssoConfigRepository.GetByOrganizationIdAsync(orgId).Returns(ssoConfig); - userRepository.GetBySsoUserAsync(providerUserId, orgId).Returns(user); + sutProvider.GetDependency().GetByOrganizationIdAsync(orgId).Returns(ssoConfig); + sutProvider.GetDependency().GetBySsoUserAsync(providerUserId, orgId).Returns(user); - featureService.IsEnabled(Arg.Any()).Returns(false); - interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + sutProvider.GetDependency().IsEnabled(Arg.Any()).Returns(false); + sutProvider.GetDependency() + .GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); // Act - try { _ = await controller.ExternalCallback(); } catch { } + try { _ = await sutProvider.Sut.ExternalCallback(); } catch { } // Assert (measurement) + var userRepository = sutProvider.GetDependency(); + var organizationRepository = sutProvider.GetDependency(); + var organizationUserRepository = sutProvider.GetDependency(); + var userGetBySso = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetBySsoUserAsync)); var userGetByEmail = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetByEmailAsync)); var orgGet = organizationRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationRepository.GetByIdAsync)); @@ -894,8 +720,9 @@ public async Task ExternalCallback_PreventNonCompliantFalse_ExistingUser_NoOrgUs /// /// PM-24579: Temporary test, remove with feature flag. /// - [Fact] - public async Task ExternalCallback_PreventNonCompliantFalse_JitProvision_MeasureLookups() + [Theory, BitAutoData] + public async Task ExternalCallback_PreventNonCompliantFalse_JitProvision_MeasureLookups( + SutProvider sutProvider) { // Arrange var orgId = Guid.NewGuid(); @@ -904,37 +731,31 @@ public async Task ExternalCallback_PreventNonCompliantFalse_JitProvision_Measure var organization = new Organization { Id = orgId, Name = "Org", Seats = null }; var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, email); - var controller = CreateControllerWithAuth( - authResult, - out var authService, - out var ssoConfigRepository, - out var userRepository, - out var organizationRepository, - out var organizationUserRepository, - out var interactionService, - out var i18nService, - out var ssoUserRepository, - out var eventService, - out var featureService); + SetupHttpContextWithAuth(sutProvider, authResult); var ssoConfig = new SsoConfig { OrganizationId = orgId, Enabled = true }; var ssoData = new SsoConfigurationData(); ssoConfig.SetData(ssoData); - ssoConfigRepository.GetByOrganizationIdAsync(orgId).Returns(ssoConfig); + sutProvider.GetDependency().GetByOrganizationIdAsync(orgId).Returns(ssoConfig); // JIT (no existing user or sso link) - userRepository.GetBySsoUserAsync(providerUserId, orgId).Returns((User?)null); - userRepository.GetByEmailAsync(email).Returns((User?)null); - organizationRepository.GetByIdAsync(orgId).Returns(organization); - organizationUserRepository.GetByOrganizationEmailAsync(orgId, email).Returns((OrganizationUser?)null); + sutProvider.GetDependency().GetBySsoUserAsync(providerUserId, orgId).Returns((User?)null); + sutProvider.GetDependency().GetByEmailAsync(email).Returns((User?)null); + sutProvider.GetDependency().GetByIdAsync(orgId).Returns(organization); + sutProvider.GetDependency().GetByOrganizationEmailAsync(orgId, email).Returns((OrganizationUser?)null); - featureService.IsEnabled(Arg.Any()).Returns(false); - interactionService.GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + sutProvider.GetDependency().IsEnabled(Arg.Any()).Returns(false); + sutProvider.GetDependency() + .GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); // Act - try { _ = await controller.ExternalCallback(); } catch { } + try { _ = await sutProvider.Sut.ExternalCallback(); } catch { } // Assert (measurement) + var userRepository = sutProvider.GetDependency(); + var organizationRepository = sutProvider.GetDependency(); + var organizationUserRepository = sutProvider.GetDependency(); + var userGetBySso = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetBySsoUserAsync)); var userGetByEmail = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetByEmailAsync)); var orgGet = organizationRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationRepository.GetByIdAsync)); @@ -948,8 +769,9 @@ public async Task ExternalCallback_PreventNonCompliantFalse_JitProvision_Measure _output.WriteLine($"[flag off] GetByOrganizationEmailAsync (OrgUser): {orgUserGetByEmail}"); } - [Fact] - public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLinkAndReturnsUser() + [Theory, BitAutoData] + public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLinkAndReturnsUser( + SutProvider sutProvider) { // Arrange var orgId = Guid.NewGuid(); @@ -965,28 +787,15 @@ public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLink Type = OrganizationUserType.User }; - var authService = Substitute.For(); - var controller = CreateController( - authService, - out var ssoConfigRepository, - out var userRepository, - out var organizationRepository, - out var organizationUserRepository, - out var interactionService, - out var i18nService, - out var ssoUserRepository, - out var eventService, - out var featureService); - // Arrange repository expectations for the flow - userRepository.GetByEmailAsync(email).Returns(existingUser); - organizationRepository.GetByIdAsync(orgId).Returns(organization); - organizationUserRepository.GetManyByUserAsync(existingUser.Id) + sutProvider.GetDependency().GetByEmailAsync(email).Returns(existingUser); + sutProvider.GetDependency().GetByIdAsync(orgId).Returns(organization); + sutProvider.GetDependency().GetManyByUserAsync(existingUser.Id) .Returns(new List { orgUser }); - organizationUserRepository.GetByOrganizationEmailAsync(orgId, email).Returns(orgUser); + sutProvider.GetDependency().GetByOrganizationEmailAsync(orgId, email).Returns(orgUser); // No existing SSO link so first SSO login event is logged - ssoUserRepository.GetByUserIdOrganizationIdAsync(orgId, existingUser.Id).Returns((SsoUser?)null); + sutProvider.GetDependency().GetByUserIdOrganizationIdAsync(orgId, existingUser.Id).Returns((SsoUser?)null); var claims = new[] { @@ -1001,7 +810,7 @@ public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLink Assert.NotNull(method); // Act - var task = (Task<(User user, Organization organization, OrganizationUser orgUser)>)method!.Invoke(controller, new object[] + var task = (Task<(User user, Organization organization, OrganizationUser orgUser)>)method!.Invoke(sutProvider.Sut, new object[] { orgId.ToString(), providerUserId, @@ -1015,10 +824,10 @@ public async Task AutoProvisionUserAsync_WithExistingAcceptedUser_CreatesSsoLink // Assert Assert.Equal(existingUser.Id, returned.user.Id); - await ssoUserRepository.Received().CreateAsync(Arg.Is(s => + await sutProvider.GetDependency().Received().CreateAsync(Arg.Is(s => s.OrganizationId == orgId && s.UserId == existingUser.Id && s.ExternalId == providerUserId)); - await eventService.Received().LogOrganizationUserEventAsync( + await sutProvider.GetDependency().Received().LogOrganizationUserEventAsync( orgUser, EventType.OrganizationUser_FirstSsoLogin); } @@ -1028,8 +837,9 @@ await eventService.Received().LogOrganizationUserEventAsync( /// regress lookup counts compared to OFF. When removing the flag, delete this /// comparison test and keep the specific scenario snapshot tests if desired. /// - [Fact] - public async Task ExternalCallback_Measurements_FlagOnVsOff_Comparisons() + [Theory, BitAutoData] + public async Task ExternalCallback_Measurements_FlagOnVsOff_Comparisons( + SutProvider sutProvider) { // Arrange var scenarios = new[] @@ -1042,8 +852,8 @@ public async Task ExternalCallback_Measurements_FlagOnVsOff_Comparisons() foreach (var scenario in scenarios) { // Act - var onCounts = await MeasureCountsForScenarioAsync(scenario, preventNonCompliant: true); - var offCounts = await MeasureCountsForScenarioAsync(scenario, preventNonCompliant: false); + var onCounts = await MeasureCountsForScenarioAsync(sutProvider, scenario, preventNonCompliant: true); + var offCounts = await MeasureCountsForScenarioAsync(sutProvider, scenario, preventNonCompliant: false); // Assert: off should not exceed on in any measured lookup type Assert.True(offCounts.UserGetBySso <= onCounts.UserGetBySso, $"{scenario}: off UserGetBySso={offCounts.UserGetBySso} > on {onCounts.UserGetBySso}"); diff --git a/test/SSO.Test/SSO.Test.csproj b/test/SSO.Test/SSO.Test.csproj index baf5bfa82b56..cccc5e5d1c50 100644 --- a/test/SSO.Test/SSO.Test.csproj +++ b/test/SSO.Test/SSO.Test.csproj @@ -29,6 +29,7 @@ + From 401253ca98c76032e840fb1864f16adfd359948d Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 6 Oct 2025 16:39:25 -0400 Subject: [PATCH 17/34] fix(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Now throws when no org found --- .../src/Sso/Controllers/AccountController.cs | 41 ++++++++++--------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index d3b87990ecf7..7076e36f2fc3 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -294,33 +294,36 @@ public async Task ExternalCallback() if (preventNonCompliant) { // Lazily resolve organization if not already known - if (organization == null && Guid.TryParse(provider, out var organizationId)) + if (organization == null) { + if (!Guid.TryParse(provider, out var organizationId)) + { + throw new Exception(_i18nService.T("SSOProviderIsNotAnOrgId", provider)); + } + organization = await _organizationRepository.GetByIdAsync(organizationId); + + if (organization == null) + { + throw new Exception(_i18nService.T("CouldNotFindOrganization", organizationId)); + } } - if (organization == null) + // Lazily resolve orgUser only when we have an organization and a user + orgUser ??= await _organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id); + if (orgUser != null) { - _logger.LogError("Organization not found for provider: {Provider}", provider); + EnsureOrgUserStatusAllowed( + orgUser.Status, + organization.DisplayName(), + allowedStatuses: [OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed]); } else { - // Lazily resolve orgUser only when we have an organization and a user - orgUser ??= await _organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id); - if (orgUser != null) - { - EnsureOrgUserStatusAllowed( - orgUser.Status, - organization.DisplayName(), - allowedStatuses: [OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed]); - } - else - { - _logger.LogError( - "Organization user not found for user ID: {UserId} and organization ID: {OrganizationId}", - user.Id, - organization?.Id); - } + _logger.LogError( + "Organization user not found for user ID: {UserId} and organization ID: {OrganizationId}", + user.Id, + organization?.Id); } } From f2f538afdbffc897de1fb416914251322ad862b8 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Tue, 7 Oct 2025 12:56:20 -0400 Subject: [PATCH 18/34] fix(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Moved test directory. --- bitwarden-server.sln | 2 +- bitwarden_license/test/Bitwarden.License.Tests.proj | 2 +- .../test}/SSO.Test/Controllers/AccountControllerTest.cs | 0 {test => bitwarden_license/test}/SSO.Test/SSO.Test.csproj | 4 ++-- 4 files changed, 4 insertions(+), 4 deletions(-) rename {test => bitwarden_license/test}/SSO.Test/Controllers/AccountControllerTest.cs (100%) rename {test => bitwarden_license/test}/SSO.Test/SSO.Test.csproj (89%) diff --git a/bitwarden-server.sln b/bitwarden-server.sln index 6dc8ba7a139e..4f375d79a609 100644 --- a/bitwarden-server.sln +++ b/bitwarden-server.sln @@ -135,7 +135,7 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "DbSeederUtility", "util\DbS EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "SharedWeb.Test", "test\SharedWeb.Test\SharedWeb.Test.csproj", "{AD59537D-5259-4B7A-948F-0CF58E80B359}" EndProject -Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "SSO.Test", "test\SSO.Test\SSO.Test.csproj", "{7D98784C-C253-43FB-9873-25B65C6250D6}" +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "SSO.Test", "bitwarden_license\test\SSO.Test\SSO.Test.csproj", "{7D98784C-C253-43FB-9873-25B65C6250D6}" EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution diff --git a/bitwarden_license/test/Bitwarden.License.Tests.proj b/bitwarden_license/test/Bitwarden.License.Tests.proj index 94da2116ce8b..b35e78f93f58 100644 --- a/bitwarden_license/test/Bitwarden.License.Tests.proj +++ b/bitwarden_license/test/Bitwarden.License.Tests.proj @@ -1,5 +1,5 @@ - + diff --git a/test/SSO.Test/Controllers/AccountControllerTest.cs b/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs similarity index 100% rename from test/SSO.Test/Controllers/AccountControllerTest.cs rename to bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs diff --git a/test/SSO.Test/SSO.Test.csproj b/bitwarden_license/test/SSO.Test/SSO.Test.csproj similarity index 89% rename from test/SSO.Test/SSO.Test.csproj rename to bitwarden_license/test/SSO.Test/SSO.Test.csproj index cccc5e5d1c50..4b509c9a50a3 100644 --- a/test/SSO.Test/SSO.Test.csproj +++ b/bitwarden_license/test/SSO.Test/SSO.Test.csproj @@ -28,8 +28,8 @@ - - + + From 14ac00291bb49b3b2f2751a18e3b2ca590a0cf54 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Tue, 7 Oct 2025 16:48:29 -0400 Subject: [PATCH 19/34] fix(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Fixed slash direction change. --- bitwarden_license/test/Bitwarden.License.Tests.proj | 2 +- .../test/SSO.Test/Controllers/AccountControllerTest.cs | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/bitwarden_license/test/Bitwarden.License.Tests.proj b/bitwarden_license/test/Bitwarden.License.Tests.proj index b35e78f93f58..94da2116ce8b 100644 --- a/bitwarden_license/test/Bitwarden.License.Tests.proj +++ b/bitwarden_license/test/Bitwarden.License.Tests.proj @@ -1,5 +1,5 @@ - + diff --git a/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs b/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs index 61bdf4abd037..0f9edd5482cc 100644 --- a/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs +++ b/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs @@ -26,10 +26,7 @@ namespace Bit.SSO.Test.Controllers; -[ - ControllerCustomize(typeof(AccountController)), - SutProviderCustomize -] +[ControllerCustomize(typeof(AccountController)), SutProviderCustomize] public class AccountControllerTest { private readonly ITestOutputHelper _output; From dc624dc8335e88cb193b29b37479ff3e1f8fe490 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Wed, 22 Oct 2025 18:44:28 -0400 Subject: [PATCH 20/34] fix(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Shuffled around functions and added more tests. --- .../src/Sso/Controllers/AccountController.cs | 183 ++++++++++-------- .../Controllers/AccountControllerTest.cs | 167 +++++++++++++++- src/Core/Resources/SharedResources.en.resx | 3 + 3 files changed, 274 insertions(+), 79 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index 7076e36f2fc3..a49520586d94 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -104,7 +104,7 @@ public AccountController( } [HttpGet] - public async Task PreValidate(string domainHint) + public async Task PreValidateAsync(string domainHint) { try { @@ -163,7 +163,7 @@ public async Task PreValidate(string domainHint) } [HttpGet] - public async Task Login(string returnUrl) + public async Task LoginAsync(string returnUrl) { var context = await _interaction.GetAuthorizationContextAsync(returnUrl); @@ -238,26 +238,33 @@ public IActionResult ExternalChallenge(string scheme, string returnUrl, string s [HttpGet] public async Task ExternalCallback() { + // Feature flag (PM-24579): Prevent SSO on existing non-compliant users. + // When removing this feature flag, delete this check and always run the enforcement logic below. + var preventNonCompliant = _featureService.IsEnabled(FeatureFlagKeys.PM24579_PreventSsoOnExistingNonCompliantUsers); + // Read external identity from the temporary cookie var result = await HttpContext.AuthenticateAsync( AuthenticationSchemes.BitwardenExternalCookieAuthenticationScheme); - if (result?.Succeeded != true) + + if (preventNonCompliant) { - throw new Exception(_i18nService.T("ExternalAuthenticationError")); + if (!result.Succeeded) + { + throw new Exception(_i18nService.T("ExternalAuthenticationError")); + } + } + else + { + if (result?.Succeeded != true) + { + throw new Exception(_i18nService.T("ExternalAuthenticationError")); + } } - - // Debugging - var externalClaims = result.Principal.Claims.Select(c => $"{c.Type}: {c.Value}"); - _logger.LogDebug("External claims: {@claims}", externalClaims); // See if the user has logged in with this SSO provider before and has already been provisioned. // This is signified by the user existing in the User table and the SSOUser table for the SSO provider they're using. var (user, provider, providerUserId, claims, ssoConfigData) = await FindUserFromExternalProviderAsync(result); - // Feature flag (PM-24579): Prevent SSO on existing non-compliant users. - // When removing this feature flag, delete this check and always run the enforcement logic below. - var preventNonCompliant = _featureService.IsEnabled(FeatureFlagKeys.PM24579_PreventSsoOnExistingNonCompliantUsers); - // Defer organization and membership resolution to when needed (lazy resolution) Organization organization = null; OrganizationUser orgUser = null; @@ -269,63 +276,37 @@ public async Task ExternalCallback() // If we're manually linking to SSO, the user's external identifier will be passed as query string parameter. var userIdentifier = result.Properties.Items.Keys.Contains("user_identifier") ? result.Properties.Items["user_identifier"] : null; - var provision = await AutoProvisionUserAsync( + + var (provisionedUser, foundOrganization, foundOrCreatedOrgUser) = await AutoProvisionUserAsync( provider, providerUserId, claims, userIdentifier, ssoConfigData); - user = provision.user; + + user = provisionedUser; + // PM-24579: After removing the flag, assign these unconditionally and remove this if block. if (preventNonCompliant) { - organization = provision.foundOrganization; - orgUser = provision.foundOrgUser; + organization = foundOrganization; + orgUser = foundOrCreatedOrgUser; } } - // Either the user already authenticated with the SSO provider, or we've just provisioned them. - // Either way, we have associated the SSO login with a Bitwarden user. - // We will now sign the Bitwarden user in. - if (user != null) + // Feature-flagged enforcement: block sign-in for revoked/non-compliant org membership + // PM-24579: After removing the feature flag, delete if + if (preventNonCompliant) { - // Feature-flagged enforcement: block sign-in for revoked/non-compliant org membership - // PM-24579: After removing the feature flag, delete the 'if' and always run the enforcement body. - if (preventNonCompliant) - { - // Lazily resolve organization if not already known - if (organization == null) - { - if (!Guid.TryParse(provider, out var organizationId)) - { - throw new Exception(_i18nService.T("SSOProviderIsNotAnOrgId", provider)); - } - - organization = await _organizationRepository.GetByIdAsync(organizationId); - - if (organization == null) - { - throw new Exception(_i18nService.T("CouldNotFindOrganization", organizationId)); - } - } + // Call the guarded here because the user could have been not null in which case we need to retrieve + // the organization and orgUser. + await GuardedEnsureUserStatusAsync(user, organization, orgUser, provider); + } - // Lazily resolve orgUser only when we have an organization and a user - orgUser ??= await _organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id); - if (orgUser != null) - { - EnsureOrgUserStatusAllowed( - orgUser.Status, - organization.DisplayName(), - allowedStatuses: [OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed]); - } - else - { - _logger.LogError( - "Organization user not found for user ID: {UserId} and organization ID: {OrganizationId}", - user.Id, - organization?.Id); - } - } + if (preventNonCompliant) + { + // Same as else block without the if check, it should not be necessary. The user should _always_ + // be defined at this point either by auto provisioned or they existed to begin with. // This allows us to collect any additional claims or properties // for the specific protocols used and store them in the local auth cookie. @@ -346,6 +327,35 @@ await HttpContext.SignInAsync(new IdentityServerUser(user.Id.ToString()) AdditionalClaims = additionalLocalClaims.ToArray() }, localSignInProps); } + else + { + // Either the user already authenticated with the SSO provider, or we've just provisioned them. + // Either way, we have associated the SSO login with a Bitwarden user. + // We will now sign the Bitwarden user in. + if (user != null) + { + // This allows us to collect any additional claims or properties + // for the specific protocols used and store them in the local auth cookie. + // this is typically used to store data needed for signout from those protocols. + var additionalLocalClaims = new List(); + var localSignInProps = new AuthenticationProperties + { + IsPersistent = true, + ExpiresUtc = DateTimeOffset.UtcNow.AddMinutes(1) + }; + ProcessLoginCallback(result, additionalLocalClaims, localSignInProps); + + // Issue authentication cookie for user + await HttpContext.SignInAsync(new IdentityServerUser(user.Id.ToString()) + { + DisplayName = user.Email, + IdentityProvider = provider, + AdditionalClaims = additionalLocalClaims.ToArray() + }, localSignInProps); + } + } + + // Delete temporary cookie used during external authentication await HttpContext.SignOutAsync(AuthenticationSchemes.BitwardenExternalCookieAuthenticationScheme); @@ -480,7 +490,7 @@ static bool nameIdIsNotTransient(Claim c) => c.Type == ClaimTypes.NameIdentifier /// The claims from the external IdP. /// The user identifier used for manual SSO linking. /// The SSO configuration for the organization. - /// The User to sign in as well as the found organization and org user. + /// Guaranteed to return the user to sign in as well as the found organization and org user. /// An exception if the user cannot be provisioned as requested. private async Task<(User user, Organization foundOrganization, OrganizationUser foundOrgUser)> AutoProvisionUserAsync( string provider, @@ -514,11 +524,11 @@ SsoConfigurationData config } else { - existingUser = await GetUserFromManualLinkingData(userIdentifier); + existingUser = await GetUserFromManualLinkingDataAsync(userIdentifier); } // Try to find the OrganizationUser if it exists. - var (organization, orgUser) = await FindOrganizationUser(existingUser, email, orgId); + var (organization, orgUser) = await TryToFindOrganizationUserAsync(existingUser, email, orgId); //---------------------------------------------------- // Scenario 1: We've found the user in the User table @@ -542,12 +552,12 @@ SsoConfigurationData config EnsureOrgUserStatusAllowed(orgUser.Status, organization.DisplayName(), allowedStatuses: [OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed]); - // Since we're in the auto-provisioning logic, this means that the user exists, but they have not // authenticated with the org's SSO provider before now (otherwise we wouldn't be auto-provisioning them). // We've verified that the user is Accepted or Confnirmed, so we can create an SsoUser link and proceed // with authentication. - await CreateSsoUserRecord(providerUserId, existingUser.Id, orgId, orgUser); + await CreateSsoUserRecordAsync(providerUserId, existingUser.Id, orgId, orgUser); + return (existingUser, organization, orgUser); } @@ -645,12 +655,43 @@ SsoConfigurationData config } // Create the SsoUser record to link the user to the SSO provider. - await CreateSsoUserRecord(providerUserId, user.Id, orgId, orgUser); + await CreateSsoUserRecordAsync(providerUserId, user.Id, orgId, orgUser); return (user, organization, orgUser); } - private async Task GetUserFromManualLinkingData(string userIdentifier) + private async Task GuardedEnsureUserStatusAsync(User user, Organization organization, OrganizationUser orgUser, string provider) + { + // Lazily resolve organization if not already known + if (organization == null) + { + if (!Guid.TryParse(provider, out var organizationId)) + { + throw new Exception(_i18nService.T("SSOProviderIsNotAnOrgId", provider)); + } + + organization = await _organizationRepository.GetByIdAsync(organizationId); + + if (organization == null) + { + throw new Exception(_i18nService.T("CouldNotFindOrganization", organizationId)); + } + } + + // Use fallback to email if not found by id. Use find OrganizationUser + orgUser ??= await _organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id); + if (orgUser != null) + { + EnsureOrgUserStatusAllowed(orgUser.Status, organization.DisplayName(), + allowedStatuses: [OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed]); + } + else + { + throw new Exception(_i18nService.T("CouldNotFindOrganizationUser", user.Id, organization.Id)); + } + } + + private async Task GetUserFromManualLinkingDataAsync(string userIdentifier) { User user = null; var split = userIdentifier.Split(","); @@ -680,7 +721,7 @@ private async Task GetUserFromManualLinkingData(string userIdentifier) return user; } - private async Task<(Organization, OrganizationUser)> FindOrganizationUser(User existingUser, string email, Guid orgId) + private async Task<(Organization, OrganizationUser)> TryToFindOrganizationUserAsync(User existingUser, string email, Guid orgId) { OrganizationUser orgUser = null; var organization = await _organizationRepository.GetByIdAsync(orgId); @@ -791,7 +832,7 @@ private string GetName(IEnumerable claims, IEnumerable additional return null; } - private async Task CreateSsoUserRecord(string providerUserId, Guid userId, Guid orgId, OrganizationUser orgUser) + private async Task CreateSsoUserRecordAsync(string providerUserId, Guid userId, Guid orgId, OrganizationUser orgUser) { // Delete existing SsoUser (if any) - avoids error if providerId has changed and the sso link is stale var existingSsoUser = await _ssoUserRepository.GetByUserIdOrganizationIdAsync(orgId, userId); @@ -835,18 +876,6 @@ private void ProcessLoginCallback(AuthenticateResult externalResult, } } - private async Task GetProviderAsync(string returnUrl) - { - var context = await _interaction.GetAuthorizationContextAsync(returnUrl); - if (context?.IdP != null && await _schemeProvider.GetSchemeAsync(context.IdP) != null) - { - return context.IdP; - } - var schemes = await _schemeProvider.GetAllSchemesAsync(); - var providers = schemes.Select(x => x.Name).ToList(); - return providers.FirstOrDefault(); - } - private async Task<(string, string, string)> GetLoggedOutDataAsync(string logoutId) { // Get context information (client name, post logout redirect URI and iframe for federated signout) diff --git a/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs b/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs index 0f9edd5482cc..4247a8b4e001 100644 --- a/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs +++ b/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs @@ -322,6 +322,169 @@ public void EnsureOrgUserStatusAllowed_UnknownStatus_ThrowsUnknown( Assert.Equal("OrganizationUserUnknownStatus", ex.InnerException!.Message); } + [Theory, BitAutoData] + public async Task ExternalCallback_PreventNonCompliantTrue_ExistingUser_NoOrgUser_ThrowsCouldNotFindOrganizationUser( + SutProvider sutProvider) + { + // Arrange + var orgId = Guid.NewGuid(); + var providerUserId = "ext-missing-orguser"; + var user = new User { Id = Guid.NewGuid(), Email = "missing.orguser@example.com" }; + var organization = new Organization { Id = orgId, Name = "Org" }; + + var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); + SetupHttpContextWithAuth(sutProvider, authResult); + + // i18n returns the key so we can assert on message contents + sutProvider.GetDependency() + .T(Arg.Any(), Arg.Any()) + .Returns(ci => (string)ci[0]!); + + // SSO config + user link exists, but no org user membership + ConfigureSsoAndUser( + sutProvider, + orgId, + providerUserId, + user, + organization, + orgUser: null); + + sutProvider.GetDependency() + .GetByOrganizationAsync(organization.Id, user.Id).Returns((OrganizationUser?)null); + + sutProvider.GetDependency().IsEnabled(Arg.Any()).Returns(true); + sutProvider.GetDependency() + .GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + + // Act + Assert + var ex = await Assert.ThrowsAsync(() => sutProvider.Sut.ExternalCallback()); + Assert.Equal("CouldNotFindOrganizationUser", ex.Message); + } + + [Theory, BitAutoData] + public async Task ExternalCallback_PreventNonCompliantTrue_ExistingUser_OrgUserInvited_ThrowsAcceptInvite( + SutProvider sutProvider) + { + // Arrange + var orgId = Guid.NewGuid(); + var providerUserId = "ext-invited-orguser"; + var user = new User { Id = Guid.NewGuid(), Email = "invited.orguser@example.com" }; + var organization = new Organization { Id = orgId, Name = "Org" }; + var orgUser = new OrganizationUser + { + OrganizationId = orgId, + UserId = user.Id, + Status = OrganizationUserStatusType.Invited, + Type = OrganizationUserType.User + }; + + var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); + SetupHttpContextWithAuth(sutProvider, authResult); + + sutProvider.GetDependency() + .T(Arg.Any(), Arg.Any()) + .Returns(ci => (string)ci[0]!); + + ConfigureSsoAndUser( + sutProvider, + orgId, + providerUserId, + user, + organization, + orgUser); + + sutProvider.GetDependency().IsEnabled(Arg.Any()).Returns(true); + sutProvider.GetDependency() + .GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + + // Act + Assert + var ex = await Assert.ThrowsAsync(() => sutProvider.Sut.ExternalCallback()); + Assert.Equal("AcceptInviteBeforeUsingSSO", ex.Message); + } + + [Theory, BitAutoData] + public async Task ExternalCallback_PreventNonCompliantTrue_ExistingUser_OrgUserRevoked_ThrowsAccessRevoked( + SutProvider sutProvider) + { + // Arrange + var orgId = Guid.NewGuid(); + var providerUserId = "ext-revoked-orguser"; + var user = new User { Id = Guid.NewGuid(), Email = "revoked.orguser@example.com" }; + var organization = new Organization { Id = orgId, Name = "Org" }; + var orgUser = new OrganizationUser + { + OrganizationId = orgId, + UserId = user.Id, + Status = OrganizationUserStatusType.Revoked, + Type = OrganizationUserType.User + }; + + var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); + SetupHttpContextWithAuth(sutProvider, authResult); + + sutProvider.GetDependency() + .T(Arg.Any(), Arg.Any()) + .Returns(ci => (string)ci[0]!); + + ConfigureSsoAndUser( + sutProvider, + orgId, + providerUserId, + user, + organization, + orgUser); + + sutProvider.GetDependency().IsEnabled(Arg.Any()).Returns(true); + sutProvider.GetDependency() + .GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + + // Act + Assert + var ex = await Assert.ThrowsAsync(() => sutProvider.Sut.ExternalCallback()); + Assert.Equal("OrganizationUserAccessRevoked", ex.Message); + } + + [Theory, BitAutoData] + public async Task ExternalCallback_PreventNonCompliantTrue_ExistingUser_OrgUserUnknown_ThrowsUnknown( + SutProvider sutProvider) + { + // Arrange + var orgId = Guid.NewGuid(); + var providerUserId = "ext-unknown-orguser"; + var user = new User { Id = Guid.NewGuid(), Email = "unknown.orguser@example.com" }; + var organization = new Organization { Id = orgId, Name = "Org" }; + var unknownStatus = (OrganizationUserStatusType)999; + var orgUser = new OrganizationUser + { + OrganizationId = orgId, + UserId = user.Id, + Status = unknownStatus, + Type = OrganizationUserType.User + }; + + var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); + SetupHttpContextWithAuth(sutProvider, authResult); + + sutProvider.GetDependency() + .T(Arg.Any(), Arg.Any()) + .Returns(ci => (string)ci[0]!); + + ConfigureSsoAndUser( + sutProvider, + orgId, + providerUserId, + user, + organization, + orgUser); + + sutProvider.GetDependency().IsEnabled(Arg.Any()).Returns(true); + sutProvider.GetDependency() + .GetAuthorizationContextAsync("~/").Returns((AuthorizationRequest?)null); + + // Act + Assert + var ex = await Assert.ThrowsAsync(() => sutProvider.Sut.ExternalCallback()); + Assert.Equal("OrganizationUserUnknownStatus", ex.Message); + } + [Theory, BitAutoData] public async Task ExternalCallback_WithExistingUserAndAcceptedMembership_RedirectsToReturnUrl( SutProvider sutProvider) @@ -436,8 +599,8 @@ public async Task ExternalCallback_PreventNonCompliantTrue_ExistingSsoLinkedAcce Type = OrganizationUserType.User }; - var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email!); - var authService = SetupHttpContextWithAuth(sutProvider, authResult); + var authResult = BuildSuccessfulExternalAuth(orgId, providerUserId, user.Email); + SetupHttpContextWithAuth(sutProvider, authResult); ConfigureSsoAndUser( sutProvider, diff --git a/src/Core/Resources/SharedResources.en.resx b/src/Core/Resources/SharedResources.en.resx index 28ae70ca9604..db41dcc43d7e 100644 --- a/src/Core/Resources/SharedResources.en.resx +++ b/src/Core/Resources/SharedResources.en.resx @@ -511,6 +511,9 @@ Could not find organization for '{0}' + + Could not find organization user for user '{0}' organization '{1}' + No seats available for organization, '{0}' From 615ec4b7461299caeb8b7d867618364164608713 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Wed, 22 Oct 2025 18:46:43 -0400 Subject: [PATCH 21/34] fix(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Small changes. --- bitwarden_license/src/Sso/Controllers/AccountController.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index a49520586d94..35f8f4bea66e 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -286,7 +286,6 @@ public async Task ExternalCallback() user = provisionedUser; - // PM-24579: After removing the flag, assign these unconditionally and remove this if block. if (preventNonCompliant) { organization = foundOrganization; @@ -294,8 +293,6 @@ public async Task ExternalCallback() } } - // Feature-flagged enforcement: block sign-in for revoked/non-compliant org membership - // PM-24579: After removing the feature flag, delete if if (preventNonCompliant) { // Call the guarded here because the user could have been not null in which case we need to retrieve From 4bffbae7292a6a72bd60de3e1f6df85c51d3752b Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Fri, 24 Oct 2025 13:40:48 -0400 Subject: [PATCH 22/34] fix(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Changes to parameters being passed. Good state. --- .../src/Sso/Controllers/AccountController.cs | 60 ++++++++++++------- .../Controllers/AccountControllerTest.cs | 12 +++- 2 files changed, 48 insertions(+), 24 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index 35f8f4bea66e..0b3b89e1d982 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -239,7 +239,6 @@ public IActionResult ExternalChallenge(string scheme, string returnUrl, string s public async Task ExternalCallback() { // Feature flag (PM-24579): Prevent SSO on existing non-compliant users. - // When removing this feature flag, delete this check and always run the enforcement logic below. var preventNonCompliant = _featureService.IsEnabled(FeatureFlagKeys.PM24579_PreventSsoOnExistingNonCompliantUsers); // Read external identity from the temporary cookie @@ -297,13 +296,15 @@ public async Task ExternalCallback() { // Call the guarded here because the user could have been not null in which case we need to retrieve // the organization and orgUser. - await GuardedEnsureUserStatusAsync(user, organization, orgUser, provider); + var fallbackEmailFromClaimForInvitedUser = GetEmailAddress(claims, ssoConfigData.GetAdditionalEmailClaimTypes()); + await GuardedEnsureUserStatusAsync(user, organization, orgUser, provider, fallbackEmailFromClaimForInvitedUser); } if (preventNonCompliant) { // Same as else block without the if check, it should not be necessary. The user should _always_ // be defined at this point either by auto provisioned or they existed to begin with. + // PM-24579 When removing the feature flag, delete the above comment block. // This allows us to collect any additional claims or properties // for the specific protocols used and store them in the local auth cookie. @@ -352,8 +353,6 @@ await HttpContext.SignInAsync(new IdentityServerUser(user.Id.ToString()) } } - - // Delete temporary cookie used during external authentication await HttpContext.SignOutAsync(AuthenticationSchemes.BitwardenExternalCookieAuthenticationScheme); @@ -378,7 +377,7 @@ await HttpContext.SignInAsync(new IdentityServerUser(user.Id.ToString()) } [HttpGet] - public async Task Logout(string logoutId) + public async Task LogoutAsync(string logoutId) { // Build a model so the logged out page knows what to display var (updatedLogoutId, redirectUri, externalAuthenticationScheme) = await GetLoggedOutDataAsync(logoutId); @@ -525,7 +524,7 @@ SsoConfigurationData config } // Try to find the OrganizationUser if it exists. - var (organization, orgUser) = await TryToFindOrganizationUserAsync(existingUser, email, orgId); + var (organization, orgUser) = await TryToFindOrganizationUserAsync(existingUser, orgId, email); //---------------------------------------------------- // Scenario 1: We've found the user in the User table @@ -657,16 +656,21 @@ SsoConfigurationData config return (user, organization, orgUser); } - private async Task GuardedEnsureUserStatusAsync(User user, Organization organization, OrganizationUser orgUser, string provider) + private async Task GuardedEnsureUserStatusAsync( + User user, + Organization organization, + OrganizationUser orgUser, + string provider, + string emailToUserForInvitedUsers) { + if (!Guid.TryParse(provider, out var organizationId)) + { + throw new Exception(_i18nService.T("SSOProviderIsNotAnOrgId", provider)); + } + // Lazily resolve organization if not already known if (organization == null) { - if (!Guid.TryParse(provider, out var organizationId)) - { - throw new Exception(_i18nService.T("SSOProviderIsNotAnOrgId", provider)); - } - organization = await _organizationRepository.GetByIdAsync(organizationId); if (organization == null) @@ -675,8 +679,8 @@ private async Task GuardedEnsureUserStatusAsync(User user, Organization organiza } } - // Use fallback to email if not found by id. Use find OrganizationUser - orgUser ??= await _organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id); + // Lazily load the org user if we do not already have it provided. + orgUser ??= (await TryToFindOrganizationUserAsync(user, organizationId, emailToUserForInvitedUsers, organization)).Item2; if (orgUser != null) { EnsureOrgUserStatusAllowed(orgUser.Status, organization.DisplayName(), @@ -718,26 +722,40 @@ private async Task GetUserFromManualLinkingDataAsync(string userIdentifier return user; } - private async Task<(Organization, OrganizationUser)> TryToFindOrganizationUserAsync(User existingUser, string email, Guid orgId) + /// + /// This will try to retrieve the organization user. + /// + /// + /// + /// + /// + /// + /// + private async Task<(Organization, OrganizationUser)> TryToFindOrganizationUserAsync( + User user, + Guid organizationId, + string emailToUseForInvitedUsers, + Organization organization = null) { OrganizationUser orgUser = null; - var organization = await _organizationRepository.GetByIdAsync(orgId); + // Reduce calls to the database by checking if we have an organization we can already provide. + organization ??= await _organizationRepository.GetByIdAsync(organizationId); if (organization == null) { - throw new Exception(_i18nService.T("CouldNotFindOrganization", orgId)); + throw new Exception(_i18nService.T("CouldNotFindOrganization", organizationId)); } // Try to find OrgUser via existing User Id. // This covers any OrganizationUser state after they have accepted an invite. - if (existingUser != null) + if (user != null) { - var orgUsersByUserId = await _organizationUserRepository.GetManyByUserAsync(existingUser.Id); - orgUser = orgUsersByUserId.SingleOrDefault(u => u.OrganizationId == orgId); + var orgUsersByUserId = await _organizationUserRepository.GetManyByUserAsync(user.Id); + orgUser = orgUsersByUserId.SingleOrDefault(u => u.OrganizationId == organizationId); } // If no Org User found by Existing User Id - search all the organization's users via email. // This covers users who are Invited but haven't accepted their invite yet. - orgUser ??= await _organizationUserRepository.GetByOrganizationEmailAsync(orgId, email); + orgUser ??= await _organizationUserRepository.GetByOrganizationEmailAsync(organizationId, emailToUseForInvitedUsers); return (organization, orgUser); } diff --git a/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs b/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs index 4247a8b4e001..8d5bfb0c0c66 100644 --- a/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs +++ b/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs @@ -135,6 +135,7 @@ private static void ConfigureSsoAndUser( if (organization != null && orgUser != null) { organizationUserRepository.GetByOrganizationAsync(organization.Id, user.Id).Returns(orgUser); + organizationUserRepository.GetManyByUserAsync(user.Id).Returns([orgUser]); } } @@ -632,7 +633,8 @@ public async Task ExternalCallback_PreventNonCompliantTrue_ExistingSsoLinkedAcce var userGetBySso = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetBySsoUserAsync)); var userGetByEmail = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetByEmailAsync)); var orgGet = organizationRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationRepository.GetByIdAsync)); - var orgUserGetByOrg = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationAsync)); + var orgUserGetByOrg = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationAsync)) + + organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetManyByUserAsync)); var orgUserGetByEmail = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationEmailAsync)); _output.WriteLine($"GetBySsoUserAsync: {userGetBySso}"); @@ -720,6 +722,9 @@ public async Task ExternalCallback_PreventNonCompliantTrue_JitProvision_MeasureL /// /// PM-24579: Permanent test, remove the True in PreventNonCompliantTrue and remove the configure for the feature /// flag. + /// + /// This test will trigger both the GetByOrganizationAsync and the fallback attempt to get by email + /// GetByOrganizationEmailAsync. /// [Theory, BitAutoData] public async Task ExternalCallback_PreventNonCompliantTrue_ExistingUser_NoOrgUser_MeasureLookups( @@ -768,7 +773,8 @@ public async Task ExternalCallback_PreventNonCompliantTrue_ExistingUser_NoOrgUse var userGetBySso = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetBySsoUserAsync)); var userGetByEmail = userRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IUserRepository.GetByEmailAsync)); var orgGet = organizationRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationRepository.GetByIdAsync)); - var orgUserGetByOrg = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationAsync)); + var orgUserGetByOrg = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationAsync)) + + organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetManyByUserAsync)); var orgUserGetByEmail = organizationUserRepository.ReceivedCalls().Count(c => c.GetMethodInfo().Name == nameof(IOrganizationUserRepository.GetByOrganizationEmailAsync)); _output.WriteLine($"GetBySsoUserAsync: {userGetBySso}"); @@ -782,7 +788,7 @@ public async Task ExternalCallback_PreventNonCompliantTrue_ExistingUser_NoOrgUse Assert.Equal(0, userGetByEmail); Assert.Equal(1, orgGet); Assert.Equal(1, orgUserGetByOrg); - Assert.Equal(0, orgUserGetByEmail); + Assert.Equal(1, orgUserGetByEmail); } /// From 78a3e1d758877389e39c05db85d0f5772bea02fc Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Fri, 24 Oct 2025 13:49:49 -0400 Subject: [PATCH 23/34] docs(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Added more comments. --- .../src/Sso/Controllers/AccountController.cs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index 0b3b89e1d982..ea3a02689826 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -723,14 +723,17 @@ private async Task GetUserFromManualLinkingDataAsync(string userIdentifier } /// - /// This will try to retrieve the organization user. + /// This will try to retrieve the organization user. If there is no org user it will return a null value. + /// Throws on not being able to find the organization. /// - /// - /// - /// - /// - /// - /// + /// User used to retrieve from the org user join table. + /// Organization id from the provider data. + /// Email to use as a fallback in case of an invited user not in the Users + /// table yet. + /// Organization so that we can optimize the number of calls to the database. If we provided + /// the database hit will be skipped. + /// Guaranteed to return organization and optional org user. + /// Throws if there is no found organization. private async Task<(Organization, OrganizationUser)> TryToFindOrganizationUserAsync( User user, Guid organizationId, From 346c5b4ee2361832686d75ed346c1b113919a931 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 27 Oct 2025 08:26:29 -0400 Subject: [PATCH 24/34] feat(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Jared's ideas + some tweaks and touchups. --- .../src/Sso/Controllers/AccountController.cs | 268 ++++++++++-------- 1 file changed, 149 insertions(+), 119 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index ea3a02689826..8146507f547c 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -239,13 +239,14 @@ public IActionResult ExternalChallenge(string scheme, string returnUrl, string s public async Task ExternalCallback() { // Feature flag (PM-24579): Prevent SSO on existing non-compliant users. - var preventNonCompliant = _featureService.IsEnabled(FeatureFlagKeys.PM24579_PreventSsoOnExistingNonCompliantUsers); + var preventOrgUserLoginIfStatusInvalid = + _featureService.IsEnabled(FeatureFlagKeys.PM24579_PreventSsoOnExistingNonCompliantUsers); // Read external identity from the temporary cookie var result = await HttpContext.AuthenticateAsync( AuthenticationSchemes.BitwardenExternalCookieAuthenticationScheme); - if (preventNonCompliant) + if (preventOrgUserLoginIfStatusInvalid) { if (!result.Succeeded) { @@ -264,7 +265,7 @@ public async Task ExternalCallback() // This is signified by the user existing in the User table and the SSOUser table for the SSO provider they're using. var (user, provider, providerUserId, claims, ssoConfigData) = await FindUserFromExternalProviderAsync(result); - // Defer organization and membership resolution to when needed (lazy resolution) + // We will look these up as required (lazy resolution) to avoid multiple DB hits. Organization organization = null; OrganizationUser orgUser = null; @@ -273,34 +274,33 @@ public async Task ExternalCallback() if (user == null) { // If we're manually linking to SSO, the user's external identifier will be passed as query string parameter. - var userIdentifier = result.Properties.Items.Keys.Contains("user_identifier") ? - result.Properties.Items["user_identifier"] : null; - - var (provisionedUser, foundOrganization, foundOrCreatedOrgUser) = await AutoProvisionUserAsync( - provider, - providerUserId, - claims, - userIdentifier, - ssoConfigData); + var userIdentifier = result.Properties.Items.Keys.Contains("user_identifier") + ? result.Properties.Items["user_identifier"] + : null; + + var (provisionedUser, foundOrganization, foundOrCreatedOrgUser) = + await AutoProvisionUserAsync( + provider, + providerUserId, + claims, + userIdentifier, + ssoConfigData); user = provisionedUser; - if (preventNonCompliant) + if (preventOrgUserLoginIfStatusInvalid) { organization = foundOrganization; orgUser = foundOrCreatedOrgUser; } } - if (preventNonCompliant) + if (preventOrgUserLoginIfStatusInvalid) { - // Call the guarded here because the user could have been not null in which case we need to retrieve - // the organization and orgUser. - var fallbackEmailFromClaimForInvitedUser = GetEmailAddress(claims, ssoConfigData.GetAdditionalEmailClaimTypes()); - await GuardedEnsureUserStatusAsync(user, organization, orgUser, provider, fallbackEmailFromClaimForInvitedUser); + await PreventOrgUserLoginIfStatusInvalidAsync(organization, provider, orgUser, user); } - if (preventNonCompliant) + if (preventOrgUserLoginIfStatusInvalid) { // Same as else block without the if check, it should not be necessary. The user should _always_ // be defined at this point either by auto provisioned or they existed to begin with. @@ -318,12 +318,13 @@ public async Task ExternalCallback() ProcessLoginCallback(result, additionalLocalClaims, localSignInProps); // Issue authentication cookie for user - await HttpContext.SignInAsync(new IdentityServerUser(user.Id.ToString()) - { - DisplayName = user.Email, - IdentityProvider = provider, - AdditionalClaims = additionalLocalClaims.ToArray() - }, localSignInProps); + await HttpContext.SignInAsync( + new IdentityServerUser(user.Id.ToString()) + { + DisplayName = user.Email, + IdentityProvider = provider, + AdditionalClaims = additionalLocalClaims.ToArray() + }, localSignInProps); } else { @@ -344,12 +345,13 @@ await HttpContext.SignInAsync(new IdentityServerUser(user.Id.ToString()) ProcessLoginCallback(result, additionalLocalClaims, localSignInProps); // Issue authentication cookie for user - await HttpContext.SignInAsync(new IdentityServerUser(user.Id.ToString()) - { - DisplayName = user.Email, - IdentityProvider = provider, - AdditionalClaims = additionalLocalClaims.ToArray() - }, localSignInProps); + await HttpContext.SignInAsync( + new IdentityServerUser(user.Id.ToString()) + { + DisplayName = user.Email, + IdentityProvider = provider, + AdditionalClaims = additionalLocalClaims.ToArray() + }, localSignInProps); } } @@ -400,6 +402,7 @@ public async Task LogoutAsync(string logoutId) // This triggers a redirect to the external provider for sign-out return SignOut(new AuthenticationProperties { RedirectUri = url }, externalAuthenticationScheme); } + if (redirectUri != null) { return View("Redirect", new RedirectViewModel { RedirectUrl = redirectUri }); @@ -414,7 +417,8 @@ public async Task LogoutAsync(string logoutId) /// Attempts to map the external identity to a Bitwarden user, through the SsoUser table, which holds the `externalId`. /// The claims on the external identity are used to determine an `externalId`, and that is used to find the appropriate `SsoUser` and `User` records. /// - private async Task<(User user, string provider, string providerUserId, IEnumerable claims, SsoConfigurationData config)> + private async Task<(User user, string provider, string providerUserId, IEnumerable claims, + SsoConfigurationData config)> FindUserFromExternalProviderAsync(AuthenticateResult result) { var provider = result.Properties.Items["scheme"]; @@ -441,9 +445,10 @@ public async Task LogoutAsync(string logoutId) // Ensure the NameIdentifier used is not a transient name ID, if so, we need a different attribute // for the user identifier. static bool nameIdIsNotTransient(Claim c) => c.Type == ClaimTypes.NameIdentifier - && (c.Properties == null - || !c.Properties.TryGetValue(SamlPropertyKeys.ClaimFormat, out var claimFormat) - || claimFormat != SamlNameIdFormats.Transient); + && (c.Properties == null + || !c.Properties.TryGetValue(SamlPropertyKeys.ClaimFormat, + out var claimFormat) + || claimFormat != SamlNameIdFormats.Transient); // Try to determine the unique id of the external user (issued by the provider) // the most common claim type for that are the sub claim and the NameIdentifier @@ -485,29 +490,20 @@ static bool nameIdIsNotTransient(Claim c) => c.Type == ClaimTypes.NameIdentifier /// The external identity provider's user identifier. /// The claims from the external IdP. /// The user identifier used for manual SSO linking. - /// The SSO configuration for the organization. + /// The SSO configuration for the organization. /// Guaranteed to return the user to sign in as well as the found organization and org user. /// An exception if the user cannot be provisioned as requested. - private async Task<(User user, Organization foundOrganization, OrganizationUser foundOrgUser)> AutoProvisionUserAsync( - string provider, - string providerUserId, - IEnumerable claims, - string userIdentifier, - SsoConfigurationData config + private async Task<(User user, Organization foundOrganization, OrganizationUser foundOrgUser)> + AutoProvisionUserAsync( + string provider, + string providerUserId, + IEnumerable claims, + string userIdentifier, + SsoConfigurationData ssoConfigData ) { - var name = GetName(claims, config.GetAdditionalNameClaimTypes()); - var email = GetEmailAddress(claims, config.GetAdditionalEmailClaimTypes()); - if (string.IsNullOrWhiteSpace(email) && providerUserId.Contains("@")) - { - email = providerUserId; - } - - if (!Guid.TryParse(provider, out var orgId)) - { - // TODO: support non-org (server-wide) SSO in the future? - throw new Exception(_i18nService.T("SSOProviderIsNotAnOrgId", provider)); - } + var name = GetName(claims, ssoConfigData.GetAdditionalNameClaimTypes()); + var email = TryGetEmailAddressAsync(claims, ssoConfigData, providerUserId); User existingUser = null; if (string.IsNullOrWhiteSpace(userIdentifier)) @@ -516,6 +512,7 @@ SsoConfigurationData config { throw new Exception(_i18nService.T("CannotFindEmailClaim")); } + existingUser = await _userRepository.GetByEmailAsync(email); } else @@ -523,8 +520,11 @@ SsoConfigurationData config existingUser = await GetUserFromManualLinkingDataAsync(userIdentifier); } - // Try to find the OrganizationUser if it exists. - var (organization, orgUser) = await TryToFindOrganizationUserAsync(existingUser, orgId, email); + // Try to find the org (we error if we can't find an org) + var organization = await TryGetOrganizationByProviderAsync(provider); + + // Try to find an org user (null org user possible here) + var orgUser = await TryGetOrganizationUserByUserAndOrgOrEmail(existingUser, organization.Id, email); //---------------------------------------------------- // Scenario 1: We've found the user in the User table @@ -552,7 +552,7 @@ SsoConfigurationData config // authenticated with the org's SSO provider before now (otherwise we wouldn't be auto-provisioning them). // We've verified that the user is Accepted or Confnirmed, so we can create an SsoUser link and proceed // with authentication. - await CreateSsoUserRecordAsync(providerUserId, existingUser.Id, orgId, orgUser); + await CreateSsoUserRecordAsync(providerUserId, existingUser.Id, organization.Id, orgUser); return (existingUser, organization, orgUser); } @@ -560,7 +560,8 @@ SsoConfigurationData config // Before any user creation - if Org User doesn't exist at this point - make sure there are enough seats to add one if (orgUser == null && organization.Seats.HasValue) { - var occupiedSeats = await _organizationRepository.GetOccupiedSeatCountByOrganizationIdAsync(organization.Id); + var occupiedSeats = + await _organizationRepository.GetOccupiedSeatCountByOrganizationIdAsync(organization.Id); var initialSeatCount = organization.Seats.Value; var availableSeats = initialSeatCount - occupiedSeats.Total; if (availableSeats < 1) @@ -578,8 +579,10 @@ SsoConfigurationData config { if (organization.Seats.Value != initialSeatCount) { - await _organizationService.AdjustSeatsAsync(orgId, initialSeatCount - organization.Seats.Value); + await _organizationService.AdjustSeatsAsync(organization.Id, + initialSeatCount - organization.Seats.Value); } + _logger.LogInformation(e, "SSO auto provisioning failed"); throw new Exception(_i18nService.T("NoSeatsAvailable", organization.DisplayName())); } @@ -591,7 +594,8 @@ SsoConfigurationData config var emailDomain = CoreHelpers.GetEmailDomain(email); if (!string.IsNullOrWhiteSpace(emailDomain)) { - var organizationDomain = await _organizationDomainRepository.GetDomainByOrgIdAndDomainNameAsync(orgId, emailDomain); + var organizationDomain = + await _organizationDomainRepository.GetDomainByOrgIdAndDomainNameAsync(organization.Id, emailDomain); emailVerified = organizationDomain?.VerifiedDate.HasValue ?? false; } @@ -609,7 +613,7 @@ SsoConfigurationData config // If the organization has 2fa policy enabled, make sure to default jit user 2fa to email var twoFactorPolicy = - await _policyRepository.GetByOrganizationIdTypeAsync(orgId, PolicyType.TwoFactorAuthentication); + await _policyRepository.GetByOrganizationIdTypeAsync(organization.Id, PolicyType.TwoFactorAuthentication); if (twoFactorPolicy != null && twoFactorPolicy.Enabled) { user.SetTwoFactorProviders(new Dictionary @@ -632,7 +636,7 @@ SsoConfigurationData config { orgUser = new OrganizationUser { - OrganizationId = orgId, + OrganizationId = organization.Id, UserId = user.Id, Type = OrganizationUserType.User, Status = OrganizationUserStatusType.Invited @@ -651,36 +655,31 @@ SsoConfigurationData config } // Create the SsoUser record to link the user to the SSO provider. - await CreateSsoUserRecordAsync(providerUserId, user.Id, orgId, orgUser); + await CreateSsoUserRecordAsync(providerUserId, user.Id, organization.Id, orgUser); return (user, organization, orgUser); } - private async Task GuardedEnsureUserStatusAsync( - User user, - Organization organization, - OrganizationUser orgUser, - string provider, - string emailToUserForInvitedUsers) + /// + /// Prevents an Organization user from logging in if in an invalid status like revoked or invited. + /// + /// TODO + /// TODO + /// TODO + /// Either an existing user that has logged in before or a newly provisioned user + /// TODO + private async Task PreventOrgUserLoginIfStatusInvalidAsync(Organization organization, string provider, + OrganizationUser orgUser, User user) { - if (!Guid.TryParse(provider, out var organizationId)) - { - throw new Exception(_i18nService.T("SSOProviderIsNotAnOrgId", provider)); - } + // Lazily get organization if not already known + organization ??= await TryGetOrganizationByProviderAsync(provider); - // Lazily resolve organization if not already known - if (organization == null) - { - organization = await _organizationRepository.GetByIdAsync(organizationId); - - if (organization == null) - { - throw new Exception(_i18nService.T("CouldNotFindOrganization", organizationId)); - } - } + // Lazily get the org user if not already known + orgUser ??= await TryGetOrganizationUserByUserAndOrgOrEmail( + user, + organization.Id, + user.Email); - // Lazily load the org user if we do not already have it provided. - orgUser ??= (await TryToFindOrganizationUserAsync(user, organizationId, emailToUserForInvitedUsers, organization)).Item2; if (orgUser != null) { EnsureOrgUserStatusAllowed(orgUser.Status, organization.DisplayName(), @@ -688,7 +687,7 @@ private async Task GuardedEnsureUserStatusAsync( } else { - throw new Exception(_i18nService.T("CouldNotFindOrganizationUser", user.Id, organization.Id)); + throw new Exception(_i18nService.T("CouldNotFindOrganizationUser", user?.Id, organization.Id)); } } @@ -700,6 +699,7 @@ private async Task GetUserFromManualLinkingDataAsync(string userIdentifier { throw new Exception(_i18nService.T("InvalidUserIdentifier")); } + var userId = split[0]; var token = split[1]; @@ -719,34 +719,49 @@ private async Task GetUserFromManualLinkingDataAsync(string userIdentifier throw new Exception(_i18nService.T("UserIdAndTokenMismatch")); } } + return user; } /// - /// This will try to retrieve the organization user. If there is no org user it will return a null value. - /// Throws on not being able to find the organization. + /// Tries to get the organization by the provider which is org id for us as we use the scheme + /// to identify organizations - not identity providers. + /// + /// Org id string from SSO scheme property + /// Errors if the provider string is not a valid org id guid or if the org cannot be found by the id. + private async Task TryGetOrganizationByProviderAsync(string provider) + { + if (!Guid.TryParse(provider, out var organizationId)) + { + // TODO: support non-org (server-wide) SSO in the future? + throw new Exception(_i18nService.T("SSOProviderIsNotAnOrgId", provider)); + } + + var organization = await _organizationRepository.GetByIdAsync(organizationId); + + if (organization == null) + { + throw new Exception(_i18nService.T("CouldNotFindOrganization", organizationId)); + } + + return organization; + } + + /// + /// Attempts to get an for a given organization + /// by first checking for an existing user relationship, and if none is found, + /// by looking up an invited user via their email address. /// - /// User used to retrieve from the org user join table. + /// The existing user entity to be looked up in OrganizationUsers table. /// Organization id from the provider data. - /// Email to use as a fallback in case of an invited user not in the Users + /// Email to use as a fallback in case of an invited user not in the Org Users /// table yet. - /// Organization so that we can optimize the number of calls to the database. If we provided - /// the database hit will be skipped. - /// Guaranteed to return organization and optional org user. - /// Throws if there is no found organization. - private async Task<(Organization, OrganizationUser)> TryToFindOrganizationUserAsync( + private async Task TryGetOrganizationUserByUserAndOrgOrEmail( User user, Guid organizationId, - string emailToUseForInvitedUsers, - Organization organization = null) + string email) { OrganizationUser orgUser = null; - // Reduce calls to the database by checking if we have an organization we can already provide. - organization ??= await _organizationRepository.GetByIdAsync(organizationId); - if (organization == null) - { - throw new Exception(_i18nService.T("CouldNotFindOrganization", organizationId)); - } // Try to find OrgUser via existing User Id. // This covers any OrganizationUser state after they have accepted an invite. @@ -758,9 +773,9 @@ private async Task GetUserFromManualLinkingDataAsync(string userIdentifier // If no Org User found by Existing User Id - search all the organization's users via email. // This covers users who are Invited but haven't accepted their invite yet. - orgUser ??= await _organizationUserRepository.GetByOrganizationEmailAsync(organizationId, emailToUseForInvitedUsers); + orgUser ??= await _organizationUserRepository.GetByOrganizationEmailAsync(organizationId, email); - return (organization, orgUser); + return orgUser; } private void EnsureOrgUserStatusAllowed( @@ -792,7 +807,6 @@ private void EnsureOrgUserStatusAllowed( } } - private IActionResult InvalidJson(string errorMessageKey, Exception ex = null) { Response.StatusCode = ex == null ? 400 : 500; @@ -804,13 +818,13 @@ private IActionResult InvalidJson(string errorMessageKey, Exception ex = null) }); } - private string GetEmailAddress(IEnumerable claims, IEnumerable additionalClaimTypes) + private string TryGetEmailAddressFromClaims(IEnumerable claims, IEnumerable additionalClaimTypes) { var filteredClaims = claims.Where(c => !string.IsNullOrWhiteSpace(c.Value) && c.Value.Contains("@")); var email = filteredClaims.GetFirstMatch(additionalClaimTypes.ToArray()) ?? - filteredClaims.GetFirstMatch(JwtClaimTypes.Email, ClaimTypes.Email, - SamlClaimTypes.Email, "mail", "emailaddress"); + filteredClaims.GetFirstMatch(JwtClaimTypes.Email, ClaimTypes.Email, + SamlClaimTypes.Email, "mail", "emailaddress"); if (!string.IsNullOrWhiteSpace(email)) { return email; @@ -831,8 +845,8 @@ private string GetName(IEnumerable claims, IEnumerable additional var filteredClaims = claims.Where(c => !string.IsNullOrWhiteSpace(c.Value)); var name = filteredClaims.GetFirstMatch(additionalClaimTypes.ToArray()) ?? - filteredClaims.GetFirstMatch(JwtClaimTypes.Name, ClaimTypes.Name, - SamlClaimTypes.DisplayName, SamlClaimTypes.CommonName, "displayname", "cn"); + filteredClaims.GetFirstMatch(JwtClaimTypes.Name, ClaimTypes.Name, + SamlClaimTypes.DisplayName, SamlClaimTypes.CommonName, "displayname", "cn"); if (!string.IsNullOrWhiteSpace(name)) { return name; @@ -850,7 +864,8 @@ private string GetName(IEnumerable claims, IEnumerable additional return null; } - private async Task CreateSsoUserRecordAsync(string providerUserId, Guid userId, Guid orgId, OrganizationUser orgUser) + private async Task CreateSsoUserRecordAsync(string providerUserId, Guid userId, Guid orgId, + OrganizationUser orgUser) { // Delete existing SsoUser (if any) - avoids error if providerId has changed and the sso link is stale var existingSsoUser = await _ssoUserRepository.GetByUserIdOrganizationIdAsync(orgId, userId); @@ -865,12 +880,7 @@ private async Task CreateSsoUserRecordAsync(string providerUserId, Guid userId, await _eventService.LogOrganizationUserEventAsync(orgUser, EventType.OrganizationUser_FirstSsoLogin); } - var ssoUser = new SsoUser - { - ExternalId = providerUserId, - UserId = userId, - OrganizationId = orgId, - }; + var ssoUser = new SsoUser { ExternalId = providerUserId, UserId = userId, OrganizationId = orgId, }; await _ssoUserRepository.CreateAsync(ssoUser); } @@ -925,9 +935,29 @@ private void ProcessLoginCallback(AuthenticateResult externalResult, return (logoutId, logout?.PostLogoutRedirectUri, externalAuthenticationScheme); } + /** + * Tries to get a user's email from the claims and SSO configuration data or the provider user id if + * the claims email extraction returns null. + */ + private string TryGetEmailAddressAsync( + IEnumerable claims, + SsoConfigurationData config, + string providerUserId) + { + var email = TryGetEmailAddressFromClaims(claims, config.GetAdditionalEmailClaimTypes()); + + // If email isn't populated from claims and providerUserId has @, assume it is the email. + if (string.IsNullOrWhiteSpace(email) && providerUserId.Contains("@")) + { + email = providerUserId; + } + + return email; + } + public bool IsNativeClient(DIM.AuthorizationRequest context) { return !context.RedirectUri.StartsWith("https", StringComparison.Ordinal) - && !context.RedirectUri.StartsWith("http", StringComparison.Ordinal); + && !context.RedirectUri.StartsWith("http", StringComparison.Ordinal); } } From 196062120bd754275a72ddda14b2ca12ff5b436b Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 27 Oct 2025 10:16:33 -0400 Subject: [PATCH 25/34] fix(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Added new check and throw for user. --- bitwarden_license/src/Sso/Controllers/AccountController.cs | 7 +++---- src/Core/Resources/SharedResources.en.resx | 3 +++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index 8146507f547c..954eac6886d3 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -302,9 +302,7 @@ await AutoProvisionUserAsync( if (preventOrgUserLoginIfStatusInvalid) { - // Same as else block without the if check, it should not be necessary. The user should _always_ - // be defined at this point either by auto provisioned or they existed to begin with. - // PM-24579 When removing the feature flag, delete the above comment block. + if (user == null) throw new Exception(_i18nService.T("UserShouldBeFound")); // This allows us to collect any additional claims or properties // for the specific protocols used and store them in the local auth cookie. @@ -523,7 +521,7 @@ SsoConfigurationData ssoConfigData // Try to find the org (we error if we can't find an org) var organization = await TryGetOrganizationByProviderAsync(provider); - // Try to find an org user (null org user possible here) + // Try to find an org user (null org user possible and valid here) var orgUser = await TryGetOrganizationUserByUserAndOrgOrEmail(existingUser, organization.Id, email); //---------------------------------------------------- @@ -643,6 +641,7 @@ await _organizationService.AdjustSeatsAsync(organization.Id, }; await _organizationUserRepository.CreateAsync(orgUser); } + //----------------------------------------------------------------- // Scenario 3: There is already an existing OrganizationUser // That was established through an invitation. We just need to diff --git a/src/Core/Resources/SharedResources.en.resx b/src/Core/Resources/SharedResources.en.resx index db41dcc43d7e..ca150f2106dd 100644 --- a/src/Core/Resources/SharedResources.en.resx +++ b/src/Core/Resources/SharedResources.en.resx @@ -508,6 +508,9 @@ Supplied userId and token did not match. + + User should have been defined by this point. + Could not find organization for '{0}' From 41bb741322b16c51237fdba79975f75abcfaa358 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 27 Oct 2025 10:39:17 -0400 Subject: [PATCH 26/34] fix(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Added docs as well as made clear what statuses are permissible. --- .../src/Sso/Controllers/AccountController.cs | 34 +++++++++++-------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index 954eac6886d3..5cd096124ed9 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -543,8 +543,7 @@ SsoConfigurationData ssoConfigData throw new Exception(_i18nService.T("UserAlreadyExistsInviteProcess")); } - EnsureOrgUserStatusAllowed(orgUser.Status, organization.DisplayName(), - allowedStatuses: [OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed]); + EnsureOrgUserStatusAllowed(orgUser.Status, organization.DisplayName()); // Since we're in the auto-provisioning logic, this means that the user exists, but they have not // authenticated with the org's SSO provider before now (otherwise we wouldn't be auto-provisioning them). @@ -660,15 +659,20 @@ await _organizationService.AdjustSeatsAsync(organization.Id, } /// - /// Prevents an Organization user from logging in if in an invalid status like revoked or invited. + /// Validates an organization user is allowed to log in via SSO and blocks invalid statuses. + /// Lazily resolves the organization and organization user if not provided. /// - /// TODO - /// TODO - /// TODO - /// Either an existing user that has logged in before or a newly provisioned user - /// TODO - private async Task PreventOrgUserLoginIfStatusInvalidAsync(Organization organization, string provider, - OrganizationUser orgUser, User user) + /// The target organization; if null, resolved from provider. + /// The SSO scheme provider value (organization id as a GUID string). + /// The organization-user record; if null, looked up by user/org or user email for invited users. + /// The user attempting to sign in (existing or newly provisioned). + /// Thrown if the organization cannot be resolved from provider; + /// the organization user cannot be found; or the organization user status is not allowed. + private async Task PreventOrgUserLoginIfStatusInvalidAsync( + Organization organization, + string provider, + OrganizationUser orgUser, + User user) { // Lazily get organization if not already known organization ??= await TryGetOrganizationByProviderAsync(provider); @@ -681,8 +685,7 @@ private async Task PreventOrgUserLoginIfStatusInvalidAsync(Organization organiza if (orgUser != null) { - EnsureOrgUserStatusAllowed(orgUser.Status, organization.DisplayName(), - allowedStatuses: [OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed]); + EnsureOrgUserStatusAllowed(orgUser.Status, organization.DisplayName()); } else { @@ -779,9 +782,12 @@ private async Task TryGetOrganizationUserByUserAndOrgOrEmail( private void EnsureOrgUserStatusAllowed( OrganizationUserStatusType status, - string organizationDisplayName, - params OrganizationUserStatusType[] allowedStatuses) + string organizationDisplayName) { + // The only permissible org user statuses allowed. + OrganizationUserStatusType[] allowedStatuses = + [OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed]; + // if this status is one of the allowed ones, just return if (allowedStatuses.Contains(status)) { From e5f7d4b8750af90536155994e90c68780ddd578d Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 27 Oct 2025 10:43:53 -0400 Subject: [PATCH 27/34] fix(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Changed function name. --- bitwarden_license/src/Sso/Controllers/AccountController.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index 5cd096124ed9..71b776170884 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -543,7 +543,7 @@ SsoConfigurationData ssoConfigData throw new Exception(_i18nService.T("UserAlreadyExistsInviteProcess")); } - EnsureOrgUserStatusAllowed(orgUser.Status, organization.DisplayName()); + EnsureAcceptedOrConfirmedOrgUserStatus(orgUser.Status, organization.DisplayName()); // Since we're in the auto-provisioning logic, this means that the user exists, but they have not // authenticated with the org's SSO provider before now (otherwise we wouldn't be auto-provisioning them). @@ -685,7 +685,7 @@ private async Task PreventOrgUserLoginIfStatusInvalidAsync( if (orgUser != null) { - EnsureOrgUserStatusAllowed(orgUser.Status, organization.DisplayName()); + EnsureAcceptedOrConfirmedOrgUserStatus(orgUser.Status, organization.DisplayName()); } else { @@ -780,7 +780,7 @@ private async Task TryGetOrganizationUserByUserAndOrgOrEmail( return orgUser; } - private void EnsureOrgUserStatusAllowed( + private void EnsureAcceptedOrConfirmedOrgUserStatus( OrganizationUserStatusType status, string organizationDisplayName) { From b3eade0c791060eae7b2cfb9c1345a442be3116d Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 27 Oct 2025 11:01:45 -0400 Subject: [PATCH 28/34] test(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Fixed tests. --- .../Controllers/AccountControllerTest.cs | 22 +++++++------------ 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs b/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs index 8d5bfb0c0c66..7dbc98d2619c 100644 --- a/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs +++ b/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs @@ -76,14 +76,13 @@ private static IAuthenticationService SetupHttpContextWithAuth( private static void InvokeEnsureOrgUserStatusAllowed( AccountController controller, - OrganizationUserStatusType status, - params OrganizationUserStatusType[] allowed) + OrganizationUserStatusType status) { var method = typeof(AccountController).GetMethod( - "EnsureOrgUserStatusAllowed", + "EnsureAcceptedOrConfirmedOrgUserStatus", BindingFlags.Instance | BindingFlags.NonPublic); Assert.NotNull(method); - method.Invoke(controller, [status, "Org", allowed]); + method.Invoke(controller, [status, "Org"]); } private static AuthenticateResult BuildSuccessfulExternalAuth(Guid orgId, string providerUserId, string email) @@ -253,11 +252,9 @@ public void EnsureOrgUserStatusAllowed_AllowsAcceptedAndConfirmed( // Act var ex1 = Record.Exception(() => - InvokeEnsureOrgUserStatusAllowed(sutProvider.Sut, OrganizationUserStatusType.Accepted, - OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); + InvokeEnsureOrgUserStatusAllowed(sutProvider.Sut, OrganizationUserStatusType.Accepted)); var ex2 = Record.Exception(() => - InvokeEnsureOrgUserStatusAllowed(sutProvider.Sut, OrganizationUserStatusType.Confirmed, - OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); + InvokeEnsureOrgUserStatusAllowed(sutProvider.Sut, OrganizationUserStatusType.Confirmed)); // Assert Assert.Null(ex1); @@ -275,8 +272,7 @@ public void EnsureOrgUserStatusAllowed_Invited_ThrowsAcceptInvite( // Act var ex = Assert.Throws(() => - InvokeEnsureOrgUserStatusAllowed(sutProvider.Sut, OrganizationUserStatusType.Invited, - OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); + InvokeEnsureOrgUserStatusAllowed(sutProvider.Sut, OrganizationUserStatusType.Invited)); // Assert Assert.IsType(ex.InnerException); @@ -294,8 +290,7 @@ public void EnsureOrgUserStatusAllowed_Revoked_ThrowsAccessRevoked( // Act var ex = Assert.Throws(() => - InvokeEnsureOrgUserStatusAllowed(sutProvider.Sut, OrganizationUserStatusType.Revoked, - OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); + InvokeEnsureOrgUserStatusAllowed(sutProvider.Sut, OrganizationUserStatusType.Revoked)); // Assert Assert.IsType(ex.InnerException); @@ -315,8 +310,7 @@ public void EnsureOrgUserStatusAllowed_UnknownStatus_ThrowsUnknown( // Act var ex = Assert.Throws(() => - InvokeEnsureOrgUserStatusAllowed(sutProvider.Sut, unknown, - OrganizationUserStatusType.Accepted, OrganizationUserStatusType.Confirmed)); + InvokeEnsureOrgUserStatusAllowed(sutProvider.Sut, unknown)); // Assert Assert.IsType(ex.InnerException); From 434709cb83b8884401c74f64aa05b6906e9916ab Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 27 Oct 2025 11:36:20 -0400 Subject: [PATCH 29/34] fix(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Moved null check higher. --- .../src/Sso/Controllers/AccountController.cs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index 71b776170884..db355937bebd 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -295,15 +295,12 @@ await AutoProvisionUserAsync( } } - if (preventOrgUserLoginIfStatusInvalid) - { - await PreventOrgUserLoginIfStatusInvalidAsync(organization, provider, orgUser, user); - } - if (preventOrgUserLoginIfStatusInvalid) { if (user == null) throw new Exception(_i18nService.T("UserShouldBeFound")); + await PreventOrgUserLoginIfStatusInvalidAsync(organization, provider, orgUser, user); + // This allows us to collect any additional claims or properties // for the specific protocols used and store them in the local auth cookie. // this is typically used to store data needed for signout from those protocols. @@ -689,7 +686,7 @@ private async Task PreventOrgUserLoginIfStatusInvalidAsync( } else { - throw new Exception(_i18nService.T("CouldNotFindOrganizationUser", user?.Id, organization.Id)); + throw new Exception(_i18nService.T("CouldNotFindOrganizationUser", user.Id, organization.Id)); } } From 01eb9180b05ce12f26d264b16ddb2e6891c3f073 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 27 Oct 2025 12:15:43 -0400 Subject: [PATCH 30/34] fix(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Fixed nullish checking --- .../src/Sso/Controllers/AccountController.cs | 77 +++++++++---------- src/Core/Resources/SharedResources.en.resx | 12 +++ 2 files changed, 49 insertions(+), 40 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index db355937bebd..441ce9685c31 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -1,7 +1,4 @@ -// FIXME: Update this file to be null safe and then delete the line below -#nullable disable - -using System.Security.Claims; +using System.Security.Claims; using Bit.Core; using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; @@ -167,7 +164,7 @@ public async Task LoginAsync(string returnUrl) { var context = await _interaction.GetAuthorizationContextAsync(returnUrl); - if (!context.Parameters.AllKeys.Contains("domain_hint") || + if (context!.Parameters.AllKeys.Contains("domain_hint") || string.IsNullOrWhiteSpace(context.Parameters["domain_hint"])) { throw new Exception(_i18nService.T("NoDomainHintProvided")); @@ -181,7 +178,7 @@ public async Task LoginAsync(string returnUrl) } var domainHint = context.Parameters["domain_hint"]; - var organization = await _organizationRepository.GetByIdentifierAsync(domainHint); + var organization = await _organizationRepository.GetByIdentifierAsync(domainHint ?? throw new Exception(_i18nService.T("NoDomainHintFromAuthorizationContext"))); if (organization == null) { @@ -266,15 +263,15 @@ public async Task ExternalCallback() var (user, provider, providerUserId, claims, ssoConfigData) = await FindUserFromExternalProviderAsync(result); // We will look these up as required (lazy resolution) to avoid multiple DB hits. - Organization organization = null; - OrganizationUser orgUser = null; + Organization? organization = null; + OrganizationUser? orgUser = null; // The user has not authenticated with this SSO provider before. // They could have an existing Bitwarden account in the User table though. if (user == null) { // If we're manually linking to SSO, the user's external identifier will be passed as query string parameter. - var userIdentifier = result.Properties.Items.Keys.Contains("user_identifier") + string? userIdentifier = result.Properties!.Items.Keys.Contains("user_identifier") ? result.Properties.Items["user_identifier"] : null; @@ -354,7 +351,7 @@ await HttpContext.SignInAsync( await HttpContext.SignOutAsync(AuthenticationSchemes.BitwardenExternalCookieAuthenticationScheme); // Retrieve return URL - var returnUrl = result.Properties.Items["return_url"] ?? "~/"; + var returnUrl = result.Properties!.Items["return_url"] ?? "~/"; // Check if external login is in the context of an OIDC request var context = await _interaction.GetAuthorizationContextAsync(returnUrl); @@ -379,7 +376,7 @@ public async Task LogoutAsync(string logoutId) // Build a model so the logged out page knows what to display var (updatedLogoutId, redirectUri, externalAuthenticationScheme) = await GetLoggedOutDataAsync(logoutId); - if (User?.Identity.IsAuthenticated == true) + if (User.Identity!.IsAuthenticated) { // Delete local authentication cookie await HttpContext.SignOutAsync(); @@ -412,12 +409,10 @@ public async Task LogoutAsync(string logoutId) /// Attempts to map the external identity to a Bitwarden user, through the SsoUser table, which holds the `externalId`. /// The claims on the external identity are used to determine an `externalId`, and that is used to find the appropriate `SsoUser` and `User` records. /// - private async Task<(User user, string provider, string providerUserId, IEnumerable claims, - SsoConfigurationData config)> - FindUserFromExternalProviderAsync(AuthenticateResult result) + private async Task<(User? user, string provider, string providerUserId, List claims, SsoConfigurationData ssoConfigData)> FindUserFromExternalProviderAsync(AuthenticateResult result) { - var provider = result.Properties.Items["scheme"]; - var orgId = new Guid(provider); + var provider = result.Properties!.Items["scheme"]; + var orgId = new Guid(provider ?? throw new Exception(_i18nService.T("NoProviderOnAuthenticateResult"))); var ssoConfig = await _ssoConfigRepository.GetByOrganizationIdAsync(orgId); if (ssoConfig == null || !ssoConfig.Enabled) { @@ -430,7 +425,7 @@ public async Task LogoutAsync(string logoutId) // Validate acr claim against expectation before going further if (!string.IsNullOrWhiteSpace(ssoConfigData.ExpectedReturnAcrValue)) { - var acrClaim = externalUser.FindFirst(JwtClaimTypes.AuthenticationContextClassReference); + var acrClaim = externalUser!.FindFirst(JwtClaimTypes.AuthenticationContextClassReference); if (acrClaim?.Value != ssoConfigData.ExpectedReturnAcrValue) { throw new Exception(_i18nService.T("AcrMissingOrInvalid")); @@ -449,7 +444,7 @@ static bool nameIdIsNotTransient(Claim c) => c.Type == ClaimTypes.NameIdentifier // the most common claim type for that are the sub claim and the NameIdentifier // depending on the external provider, some other claim type might be used var customUserIdClaimTypes = ssoConfigData.GetAdditionalUserIdClaimTypes(); - var userIdClaim = externalUser.FindFirst(c => customUserIdClaimTypes.Contains(c.Type)) ?? + var userIdClaim = externalUser!.FindFirst(c => customUserIdClaimTypes.Contains(c.Type)) ?? externalUser.FindFirst(JwtClaimTypes.Subject) ?? externalUser.FindFirst(nameIdIsNotTransient) ?? // Some SAML providers may use the `uid` attribute for this @@ -493,14 +488,14 @@ static bool nameIdIsNotTransient(Claim c) => c.Type == ClaimTypes.NameIdentifier string provider, string providerUserId, IEnumerable claims, - string userIdentifier, + string? userIdentifier, SsoConfigurationData ssoConfigData ) { var name = GetName(claims, ssoConfigData.GetAdditionalNameClaimTypes()); - var email = TryGetEmailAddressAsync(claims, ssoConfigData, providerUserId); + string? email = TryGetEmailAddress(claims, ssoConfigData, providerUserId); - User existingUser = null; + User? existingUser; if (string.IsNullOrWhiteSpace(userIdentifier)) { if (string.IsNullOrWhiteSpace(email)) @@ -585,7 +580,7 @@ await _organizationService.AdjustSeatsAsync(organization.Id, // If the email domain is verified, we can mark the email as verified var emailVerified = false; - var emailDomain = CoreHelpers.GetEmailDomain(email); + var emailDomain = CoreHelpers.GetEmailDomain(email ?? throw new Exception(_i18nService.T(""))); if (!string.IsNullOrWhiteSpace(emailDomain)) { var organizationDomain = @@ -666,9 +661,9 @@ await _organizationService.AdjustSeatsAsync(organization.Id, /// Thrown if the organization cannot be resolved from provider; /// the organization user cannot be found; or the organization user status is not allowed. private async Task PreventOrgUserLoginIfStatusInvalidAsync( - Organization organization, + Organization? organization, string provider, - OrganizationUser orgUser, + OrganizationUser? orgUser, User user) { // Lazily get organization if not already known @@ -690,9 +685,9 @@ private async Task PreventOrgUserLoginIfStatusInvalidAsync( } } - private async Task GetUserFromManualLinkingDataAsync(string userIdentifier) + private async Task GetUserFromManualLinkingDataAsync(string userIdentifier) { - User user = null; + User? user = null; var split = userIdentifier.Split(","); if (split.Length < 2) { @@ -755,12 +750,12 @@ private async Task TryGetOrganizationByProviderAsync(string provid /// Organization id from the provider data. /// Email to use as a fallback in case of an invited user not in the Org Users /// table yet. - private async Task TryGetOrganizationUserByUserAndOrgOrEmail( - User user, + private async Task TryGetOrganizationUserByUserAndOrgOrEmail( + User? user, Guid organizationId, - string email) + string? email) { - OrganizationUser orgUser = null; + OrganizationUser? orgUser = null; // Try to find OrgUser via existing User Id. // This covers any OrganizationUser state after they have accepted an invite. @@ -772,6 +767,8 @@ private async Task TryGetOrganizationUserByUserAndOrgOrEmail( // If no Org User found by Existing User Id - search all the organization's users via email. // This covers users who are Invited but haven't accepted their invite yet. + if (email == null) throw new Exception(_i18nService.T("NoEmailFoundWhenFallingBackToEmailForOrgUserLookup")); + orgUser ??= await _organizationUserRepository.GetByOrganizationEmailAsync(organizationId, email); return orgUser; @@ -809,7 +806,7 @@ private void EnsureAcceptedOrConfirmedOrgUserStatus( } } - private IActionResult InvalidJson(string errorMessageKey, Exception ex = null) + private IActionResult InvalidJson(string errorMessageKey, Exception? ex = null) { Response.StatusCode = ex == null ? 400 : 500; return Json(new ErrorResponseModel(_i18nService.T(errorMessageKey)) @@ -820,7 +817,7 @@ private IActionResult InvalidJson(string errorMessageKey, Exception ex = null) }); } - private string TryGetEmailAddressFromClaims(IEnumerable claims, IEnumerable additionalClaimTypes) + private string? TryGetEmailAddressFromClaims(IEnumerable claims, IEnumerable additionalClaimTypes) { var filteredClaims = claims.Where(c => !string.IsNullOrWhiteSpace(c.Value) && c.Value.Contains("@")); @@ -842,7 +839,7 @@ private string TryGetEmailAddressFromClaims(IEnumerable claims, IEnumerab return null; } - private string GetName(IEnumerable claims, IEnumerable additionalClaimTypes) + private string? GetName(IEnumerable claims, IEnumerable additionalClaimTypes) { var filteredClaims = claims.Where(c => !string.IsNullOrWhiteSpace(c.Value)); @@ -891,14 +888,14 @@ private void ProcessLoginCallback(AuthenticateResult externalResult, { // If the external system sent a session id claim, copy it over // so we can use it for single sign-out - var sid = externalResult.Principal.Claims.FirstOrDefault(x => x.Type == JwtClaimTypes.SessionId); + var sid = externalResult.Principal!.Claims.FirstOrDefault(x => x.Type == JwtClaimTypes.SessionId); if (sid != null) { localClaims.Add(new Claim(JwtClaimTypes.SessionId, sid.Value)); } // If the external provider issued an idToken, we'll keep it for signout - var idToken = externalResult.Properties.GetTokenValue("id_token"); + var idToken = externalResult.Properties!.GetTokenValue("id_token"); if (idToken != null) { localSignInProps.StoreTokens( @@ -906,12 +903,12 @@ private void ProcessLoginCallback(AuthenticateResult externalResult, } } - private async Task<(string, string, string)> GetLoggedOutDataAsync(string logoutId) + private async Task<(string? logoutId, string? PostLogoutRedirectUri, string? externalAuthenticationScheme)> GetLoggedOutDataAsync(string? logoutId) { // Get context information (client name, post logout redirect URI and iframe for federated signout) var logout = await _interaction.GetLogoutContextAsync(logoutId); - string externalAuthenticationScheme = null; - if (User?.Identity.IsAuthenticated == true) + string? externalAuthenticationScheme = null; + if (User.Identity!.IsAuthenticated) { var idp = User.FindFirst(JwtClaimTypes.IdentityProvider)?.Value; if (idp != null && idp != IdentityServerConstants.LocalIdentityProvider) @@ -941,12 +938,12 @@ private void ProcessLoginCallback(AuthenticateResult externalResult, * Tries to get a user's email from the claims and SSO configuration data or the provider user id if * the claims email extraction returns null. */ - private string TryGetEmailAddressAsync( + private string? TryGetEmailAddress( IEnumerable claims, SsoConfigurationData config, string providerUserId) { - var email = TryGetEmailAddressFromClaims(claims, config.GetAdditionalEmailClaimTypes()); + string? email = TryGetEmailAddressFromClaims(claims, config.GetAdditionalEmailClaimTypes()); // If email isn't populated from claims and providerUserId has @, assume it is the email. if (string.IsNullOrWhiteSpace(email) && providerUserId.Contains("@")) diff --git a/src/Core/Resources/SharedResources.en.resx b/src/Core/Resources/SharedResources.en.resx index ca150f2106dd..2c014b14e97f 100644 --- a/src/Core/Resources/SharedResources.en.resx +++ b/src/Core/Resources/SharedResources.en.resx @@ -511,6 +511,18 @@ User should have been defined by this point. + + Tried to use email as a fallback when searrching for org user, but there was no email found in claims. + + + While trying to auto provision the user there was no email to use to get the domain of to mark the email as verified. + + + Could not find a domain_hint value from hte authorization context. + + + No provider found in the scheme property of the authenticate result. + Could not find organization for '{0}' From 969720fd47e95696521fdc82141fdc2d24016a61 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Mon, 27 Oct 2025 12:47:06 -0400 Subject: [PATCH 31/34] fix(prevent-bad-existing-sso-user): [PM-24579] Precent Existing Non Confirmed and Accepted SSO Users - Renaming of users to make code more clear. --- .../src/Sso/Controllers/AccountController.cs | 18 +++++++++--------- src/Core/Repositories/IUserRepository.cs | 2 -- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index 441ce9685c31..f90db41e532c 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -591,29 +591,29 @@ await _organizationService.AdjustSeatsAsync(organization.Id, //-------------------------------------------------- // Scenarios 2 and 3: We need to register a new user //-------------------------------------------------- - var user = new User + var newUser = new User { Name = name, Email = email, EmailVerified = emailVerified, ApiKey = CoreHelpers.SecureRandomString(30) }; - await _registerUserCommand.RegisterUser(user); + await _registerUserCommand.RegisterUser(newUser); // If the organization has 2fa policy enabled, make sure to default jit user 2fa to email var twoFactorPolicy = await _policyRepository.GetByOrganizationIdTypeAsync(organization.Id, PolicyType.TwoFactorAuthentication); if (twoFactorPolicy != null && twoFactorPolicy.Enabled) { - user.SetTwoFactorProviders(new Dictionary + newUser.SetTwoFactorProviders(new Dictionary { [TwoFactorProviderType.Email] = new TwoFactorProvider { - MetaData = new Dictionary { ["Email"] = user.Email.ToLowerInvariant() }, + MetaData = new Dictionary { ["Email"] = newUser.Email.ToLowerInvariant() }, Enabled = true } }); - await _userService.UpdateTwoFactorProviderAsync(user, TwoFactorProviderType.Email); + await _userService.UpdateTwoFactorProviderAsync(newUser, TwoFactorProviderType.Email); } //----------------------------------------------------------------- @@ -626,7 +626,7 @@ await _organizationService.AdjustSeatsAsync(organization.Id, orgUser = new OrganizationUser { OrganizationId = organization.Id, - UserId = user.Id, + UserId = newUser.Id, Type = OrganizationUserType.User, Status = OrganizationUserStatusType.Invited }; @@ -640,14 +640,14 @@ await _organizationService.AdjustSeatsAsync(organization.Id, //----------------------------------------------------------------- else { - orgUser.UserId = user.Id; + orgUser.UserId = newUser.Id; await _organizationUserRepository.ReplaceAsync(orgUser); } // Create the SsoUser record to link the user to the SSO provider. - await CreateSsoUserRecordAsync(providerUserId, user.Id, organization.Id, orgUser); + await CreateSsoUserRecordAsync(providerUserId, newUser.Id, organization.Id, orgUser); - return (user, organization, orgUser); + return (newUser, organization, orgUser); } /// diff --git a/src/Core/Repositories/IUserRepository.cs b/src/Core/Repositories/IUserRepository.cs index 22effb4329d5..2fbefe1fbd01 100644 --- a/src/Core/Repositories/IUserRepository.cs +++ b/src/Core/Repositories/IUserRepository.cs @@ -2,8 +2,6 @@ using Bit.Core.KeyManagement.UserKey; using Bit.Core.Models.Data; -#nullable enable - namespace Bit.Core.Repositories; public interface IUserRepository : IRepository From e1064016574c69c4b04d97947efd93030b18b0b6 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Tue, 28 Oct 2025 10:15:46 -0400 Subject: [PATCH 32/34] fix(prevent-null) [PM-27510] AccountController Prevent Null Values - Fixed null values. --- .../src/Sso/Controllers/AccountController.cs | 21 +++++++++++++------ src/Core/Resources/SharedResources.en.resx | 3 +++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index f90db41e532c..3c3a71d8b420 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -164,7 +164,9 @@ public async Task LoginAsync(string returnUrl) { var context = await _interaction.GetAuthorizationContextAsync(returnUrl); - if (context!.Parameters.AllKeys.Contains("domain_hint") || + if (context == null) throw new Exception(_i18nService.T("AuthorizationContextMissing")); + + if (context.Parameters.AllKeys.Contains("domain_hint") || string.IsNullOrWhiteSpace(context.Parameters["domain_hint"])) { throw new Exception(_i18nService.T("NoDomainHintProvided")); @@ -271,9 +273,10 @@ public async Task ExternalCallback() if (user == null) { // If we're manually linking to SSO, the user's external identifier will be passed as query string parameter. - string? userIdentifier = result.Properties!.Items.Keys.Contains("user_identifier") - ? result.Properties.Items["user_identifier"] - : null; + string? userIdentifier = + result.Properties?.Items != null && result.Properties.Items.ContainsKey("user_identifier") + ? result.Properties.Items["user_identifier"] + : null; var (provisionedUser, foundOrganization, foundOrCreatedOrgUser) = await AutoProvisionUserAsync( @@ -351,7 +354,13 @@ await HttpContext.SignInAsync( await HttpContext.SignOutAsync(AuthenticationSchemes.BitwardenExternalCookieAuthenticationScheme); // Retrieve return URL - var returnUrl = result.Properties!.Items["return_url"] ?? "~/"; + var returnUrl = "~/"; + if (result.Properties?.Items != null + && result.Properties.Items.TryGetValue("return_url", out var storedReturnUrl) + && !string.IsNullOrWhiteSpace(storedReturnUrl)) + { + returnUrl = storedReturnUrl; + } // Check if external login is in the context of an OIDC request var context = await _interaction.GetAuthorizationContextAsync(returnUrl); @@ -580,7 +589,7 @@ await _organizationService.AdjustSeatsAsync(organization.Id, // If the email domain is verified, we can mark the email as verified var emailVerified = false; - var emailDomain = CoreHelpers.GetEmailDomain(email ?? throw new Exception(_i18nService.T(""))); + var emailDomain = CoreHelpers.GetEmailDomain(email ?? throw new Exception(_i18nService.T("NoEmailFoundWhenMarkingDomainAsVerified"))); if (!string.IsNullOrWhiteSpace(emailDomain)) { var organizationDomain = diff --git a/src/Core/Resources/SharedResources.en.resx b/src/Core/Resources/SharedResources.en.resx index 2c014b14e97f..b7780f4f0f54 100644 --- a/src/Core/Resources/SharedResources.en.resx +++ b/src/Core/Resources/SharedResources.en.resx @@ -697,4 +697,7 @@ Invalid SSO identifier + + Authorization context was not found for this request. + From 4a9b2f31776d23a1c876fa7e6a4b478cb5afcbb7 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Tue, 28 Oct 2025 10:28:49 -0400 Subject: [PATCH 33/34] fix(prevent-null) [PM-27510] AccountController Prevent Null Values - Fixed tiny changes. --- bitwarden_license/src/Sso/Controllers/AccountController.cs | 2 +- src/Core/Resources/SharedResources.en.resx | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index 3c3a71d8b420..e22529b74f28 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -166,7 +166,7 @@ public async Task LoginAsync(string returnUrl) if (context == null) throw new Exception(_i18nService.T("AuthorizationContextMissing")); - if (context.Parameters.AllKeys.Contains("domain_hint") || + if (!context.Parameters.AllKeys.Contains("domain_hint") || string.IsNullOrWhiteSpace(context.Parameters["domain_hint"])) { throw new Exception(_i18nService.T("NoDomainHintProvided")); diff --git a/src/Core/Resources/SharedResources.en.resx b/src/Core/Resources/SharedResources.en.resx index b7780f4f0f54..94dd83d2de58 100644 --- a/src/Core/Resources/SharedResources.en.resx +++ b/src/Core/Resources/SharedResources.en.resx @@ -512,13 +512,13 @@ User should have been defined by this point. - Tried to use email as a fallback when searrching for org user, but there was no email found in claims. + Tried to use email as a fallback when searching for org user, but there was no email found in claims. While trying to auto provision the user there was no email to use to get the domain of to mark the email as verified. - Could not find a domain_hint value from hte authorization context. + Could not find a domain_hint value from the authorization context. No provider found in the scheme property of the authenticate result. From 90db2fc76f8ee43961beb8058dbf08635ef5396e Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Tue, 28 Oct 2025 13:29:24 -0400 Subject: [PATCH 34/34] fix(prevent-null) [PM-27510] AccountController Prevent Null Values - Added tests to get more coverage. --- .../src/Sso/Controllers/AccountController.cs | 10 +- .../Controllers/AccountControllerTest.cs | 110 ++++++++++++++++++ 2 files changed, 118 insertions(+), 2 deletions(-) diff --git a/bitwarden_license/src/Sso/Controllers/AccountController.cs b/bitwarden_license/src/Sso/Controllers/AccountController.cs index e22529b74f28..e517adeb082e 100644 --- a/bitwarden_license/src/Sso/Controllers/AccountController.cs +++ b/bitwarden_license/src/Sso/Controllers/AccountController.cs @@ -420,8 +420,13 @@ public async Task LogoutAsync(string logoutId) /// private async Task<(User? user, string provider, string providerUserId, List claims, SsoConfigurationData ssoConfigData)> FindUserFromExternalProviderAsync(AuthenticateResult result) { - var provider = result.Properties!.Items["scheme"]; - var orgId = new Guid(provider ?? throw new Exception(_i18nService.T("NoProviderOnAuthenticateResult"))); + if (result.Properties?.Items == null || + !result.Properties.Items.TryGetValue("scheme", out var provider) || + string.IsNullOrWhiteSpace(provider)) + { + throw new Exception(_i18nService.T("NoProviderOnAuthenticateResult")); + } + var orgId = new Guid(provider); var ssoConfig = await _ssoConfigRepository.GetByOrganizationIdAsync(orgId); if (ssoConfig == null || !ssoConfig.Enabled) { @@ -589,6 +594,7 @@ await _organizationService.AdjustSeatsAsync(organization.Id, // If the email domain is verified, we can mark the email as verified var emailVerified = false; + // We cannot reach this code with email currently being null so there is no way to provoke this exception. var emailDomain = CoreHelpers.GetEmailDomain(email ?? throw new Exception(_i18nService.T("NoEmailFoundWhenMarkingDomainAsVerified"))); if (!string.IsNullOrWhiteSpace(emailDomain)) { diff --git a/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs b/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs index 7dbc98d2619c..d2dcfd69d0ea 100644 --- a/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs +++ b/bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs @@ -36,6 +36,116 @@ public AccountControllerTest(ITestOutputHelper output) _output = output; } + [Theory, BitAutoData] + public async Task LoginAsync_WhenAuthorizationContextIsNull_ThrowsAuthorizationContextMissing( + SutProvider sutProvider) + { + // Arrange + sutProvider.GetDependency() + .T(Arg.Any(), Arg.Any()) + .Returns(ci => (string)ci[0]!); + + sutProvider.GetDependency() + .GetAuthorizationContextAsync(Arg.Any()) + .Returns((AuthorizationRequest?)null); + + // Act + Assert + var ex = await Assert.ThrowsAsync(() => sutProvider.Sut.LoginAsync("~/")); + Assert.Equal("AuthorizationContextMissing", ex.Message); + } + + [Theory, BitAutoData] + public async Task LoginAsync_WhenDomainHintMissingOrNull_ThrowsNoDomainHintProvided( + SutProvider sutProvider) + { + // Arrange + sutProvider.GetDependency() + .T(Arg.Any(), Arg.Any()) + .Returns(ci => (string)ci[0]!); + + var ctx = (AuthorizationRequest)Activator.CreateInstance(typeof(AuthorizationRequest), nonPublic: true)!; + // Ensure parameters start empty + ctx.Parameters.Clear(); + // domain_hint missing entirely triggers NoDomainHintProvided + sutProvider.GetDependency() + .GetAuthorizationContextAsync(Arg.Any()) + .Returns(ctx); + + // Act + Assert + var ex1 = await Assert.ThrowsAsync(() => sutProvider.Sut.LoginAsync("~/")); + Assert.Equal("NoDomainHintProvided", ex1.Message); + + // Now add the key but null value to also trigger NoDomainHintProvided + ctx.Parameters.Add("domain_hint", null); + var ex2 = await Assert.ThrowsAsync(() => sutProvider.Sut.LoginAsync("~/")); + Assert.Equal("NoDomainHintProvided", ex2.Message); + } + + [Theory, BitAutoData] + public async Task ExternalCallback_WhenSchemePropertyMissing_ThrowsNoProviderOnAuthenticateResult( + SutProvider sutProvider) + { + // Arrange + sutProvider.GetDependency() + .T(Arg.Any(), Arg.Any()) + .Returns(ci => (string)ci[0]!); + + // Build auth result without the required "scheme" property + var claims = new[] + { + new Claim(JwtClaimTypes.Subject, "provider-user"), + new Claim(JwtClaimTypes.Email, "user@example.com") + }; + var principal = new ClaimsPrincipal(new ClaimsIdentity(claims, "External")); + var properties = new AuthenticationProperties + { + Items = + { + ["return_url"] = "~/", + ["state"] = "state" + } + }; + var ticket = new AuthenticationTicket(principal, properties, AuthenticationSchemes.BitwardenExternalCookieAuthenticationScheme); + var authResult = AuthenticateResult.Success(ticket); + SetupHttpContextWithAuth(sutProvider, authResult); + + // Feature flag on/off does not matter; it fails before org lookup + sutProvider.GetDependency().IsEnabled(Arg.Any()).Returns(true); + + // Act + Assert + var ex = await Assert.ThrowsAsync(() => sutProvider.Sut.ExternalCallback()); + Assert.Equal("NoProviderOnAuthenticateResult", ex.Message); + } + + [Theory, BitAutoData] + public async Task TryGetOrganizationUserByUserAndOrgOrEmail_EmailNull_Throws( + SutProvider sutProvider) + { + // Arrange + sutProvider.GetDependency() + .T(Arg.Any(), Arg.Any()) + .Returns(ci => (string)ci[0]!); + + var orgId = Guid.NewGuid(); + var existingUser = new User { Id = Guid.NewGuid(), Email = "ignored@example.com" }; + + var method = typeof(AccountController).GetMethod( + "TryGetOrganizationUserByUserAndOrgOrEmail", + BindingFlags.Instance | BindingFlags.NonPublic); + Assert.NotNull(method); + + // Act + var task = (Task)method.Invoke(sutProvider.Sut, [ + existingUser, + orgId, + null + ])!; + + // Assert + var ex = await Assert.ThrowsAsync(async () => await task); + Assert.Equal("NoEmailFoundWhenFallingBackToEmailForOrgUserLookup", ex.Message); + } + private static IAuthenticationService SetupHttpContextWithAuth( SutProvider sutProvider, AuthenticateResult authResult,