From e513855941af982d1398d2c2063ec80b7c4a6037 Mon Sep 17 00:00:00 2001 From: Brandon Treston Date: Mon, 22 Jun 2026 10:31:30 -0400 Subject: [PATCH] fix broken auto confirm policyRequirement check for org scoped request (#7839) * fix broken auto confirm policyRequirement check for org scoped request * call push command unconditionally, move checks into command * add user role check to push notification command for auto confirm --- .../AcceptOrganizationMembershipValidator.cs | 11 +- .../OrganizationUsers/AcceptOrgUserCommand.cs | 5 +- .../IPushAutoConfirmNotificationCommand.cs | 12 ++ .../PushAutoConfirmNotificationCommand.cs | 30 +++- ...eptOrganizationMembershipValidatorTests.cs | 54 ++++++- ...PushAutoConfirmNotificationCommandTests.cs | 137 +++++++++++++++++- 6 files changed, 232 insertions(+), 17 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptMembership/AcceptOrganizationMembershipValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptMembership/AcceptOrganizationMembershipValidator.cs index e693ca3a1814..3ecbe019e3c6 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptMembership/AcceptOrganizationMembershipValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptMembership/AcceptOrganizationMembershipValidator.cs @@ -1,4 +1,5 @@ -using Bit.Core.AdminConsole.OrganizationFeatures.Policies; +using Bit.Core.AdminConsole.Enums; +using Bit.Core.AdminConsole.OrganizationFeatures.Policies; using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Enforcement.AutoConfirm; using Bit.Core.AdminConsole.OrganizationFeatures.Policies.PolicyRequirements; using Bit.Core.AdminConsole.Utilities.v2.Validation; @@ -11,7 +12,8 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.AcceptMem public class AcceptOrganizationMembershipValidator( IPolicyRequirementQuery policyRequirementQuery, IAutomaticUserConfirmationPolicyEnforcementHandler automaticUserConfirmationPolicyEnforcementHandler, - ITwoFactorIsEnabledQuery twoFactorIsEnabledQuery) + ITwoFactorIsEnabledQuery twoFactorIsEnabledQuery, + IPolicyQuery policyQuery) : IAcceptOrganizationMembershipValidator { public async Task> ValidateAsync( @@ -54,9 +56,12 @@ public async Task } } + var autoConfirmPolicyStatus = await policyQuery.RunAsync( + request.OrganizationId, PolicyType.AutomaticUserConfirmation); + return Valid(new AcceptOrganizationMembershipValidationResult { - AutoConfirmPolicyEnabled = autoConfirmRequirement.IsEnabled(request.OrganizationId) + AutoConfirmPolicyEnabled = autoConfirmPolicyStatus.Enabled }); } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommand.cs index 9cf68d7e334a..ca28aecde3b5 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommand.cs @@ -178,10 +178,7 @@ public async Task AcceptOrgUserAsync(OrganizationUser orgUser, await _mailService.SendOrganizationAcceptedEmailAsync(organization, user.Email, adminEmails); } - if (membershipValidationResult.AutoConfirmPolicyEnabled) - { - await _pushAutoConfirmNotificationCommand.PushAsync(user.Id, orgUser.OrganizationId); - } + await _pushAutoConfirmNotificationCommand.PushAsync(user.Id, orgUser.OrganizationId); return orgUser; } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IPushAutoConfirmNotificationCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IPushAutoConfirmNotificationCommand.cs index be70f4544f1f..050fc91f5a00 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IPushAutoConfirmNotificationCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IPushAutoConfirmNotificationCommand.cs @@ -2,5 +2,17 @@ public interface IPushAutoConfirmNotificationCommand { + /// + /// Sends auto-confirm push notifications to all admins and custom users with ManageUsers permission + /// for the given organization, prompting them to confirm the newly accepted user. + /// + /// + /// No notifications are sent if any of the following conditions are not met: + /// + /// The organization has the UseAutomaticUserConfirmation ability enabled. + /// The organization has the AutomaticUserConfirmation policy enabled. + /// The user being confirmed has the User role (owners, admins, and custom are excluded). + /// + /// Task PushAsync(Guid userId, Guid organizationId); } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/PushAutoConfirmNotificationCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/PushAutoConfirmNotificationCommand.cs index 95bf2c94b097..aee490f502b8 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/PushAutoConfirmNotificationCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/PushAutoConfirmNotificationCommand.cs @@ -1,8 +1,11 @@ -using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; +using Bit.Core.AdminConsole.Enums; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; +using Bit.Core.AdminConsole.OrganizationFeatures.Policies; using Bit.Core.Enums; using Bit.Core.Models; using Bit.Core.Platform.Push; using Bit.Core.Repositories; +using Bit.Core.Services; namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; @@ -10,23 +13,46 @@ public class PushAutoConfirmNotificationCommand : IPushAutoConfirmNotificationCo { private readonly IOrganizationUserRepository _organizationUserRepository; private readonly IPushNotificationService _pushNotificationService; + private readonly IApplicationCacheService _applicationCacheService; + private readonly IPolicyQuery _policyQuery; public PushAutoConfirmNotificationCommand( IOrganizationUserRepository organizationUserRepository, - IPushNotificationService pushNotificationService) + IPushNotificationService pushNotificationService, + IApplicationCacheService applicationCacheService, + IPolicyQuery policyQuery) { _organizationUserRepository = organizationUserRepository; _pushNotificationService = pushNotificationService; + _applicationCacheService = applicationCacheService; + _policyQuery = policyQuery; } public async Task PushAsync(Guid userId, Guid organizationId) { + var orgAbility = await _applicationCacheService.GetOrganizationAbilityAsync(organizationId); + if (orgAbility is not { UseAutomaticUserConfirmation: true }) + { + return; + } + + var policy = await _policyQuery.RunAsync(organizationId, PolicyType.AutomaticUserConfirmation); + if (!policy.Enabled) + { + return; + } + var organizationUser = await _organizationUserRepository.GetByOrganizationAsync(organizationId, userId); if (organizationUser == null) { throw new Exception("Organization user not found"); } + if (organizationUser.Type != OrganizationUserType.User) + { + return; + } + var admins = await _organizationUserRepository.GetManyByMinimumRoleAsync( organizationId, OrganizationUserType.Admin); diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptMembership/AcceptOrganizationMembershipValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptMembership/AcceptOrganizationMembershipValidatorTests.cs index 62ecbd989d72..55e72005fe14 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptMembership/AcceptOrganizationMembershipValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptMembership/AcceptOrganizationMembershipValidatorTests.cs @@ -14,6 +14,7 @@ using NSubstitute; using Xunit; using static Bit.Core.AdminConsole.Utilities.v2.Validation.ValidationResultHelpers; +using PolicyStatus = Bit.Core.AdminConsole.Models.Data.Organizations.Policies.PolicyStatus; namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.OrganizationUsers.AcceptMembership; @@ -160,12 +161,10 @@ public async Task ValidateAsync_WhenAutoConfirmEnabled_SetsAutoConfirmPolicyEnab { SetupHappyPath(organizationId, user, sutProvider); - sutProvider.GetDependency() - .GetAsync(user.Id) - .Returns(new AutomaticUserConfirmationPolicyRequirement( - [ - new PolicyDetails { OrganizationId = organizationId } - ])); + sutProvider.GetDependency() + .RunAsync(organizationId, PolicyType.AutomaticUserConfirmation) + .Returns(new PolicyStatus(organizationId, PolicyType.AutomaticUserConfirmation, + new Bit.Core.AdminConsole.Entities.Policy { Enabled = true })); var request = new AcceptOrganizationMembershipValidationRequest { @@ -180,6 +179,45 @@ public async Task ValidateAsync_WhenAutoConfirmEnabled_SetsAutoConfirmPolicyEnab Assert.True(result.Request.AutoConfirmPolicyEnabled); } + /// + /// JIT SSO-provisioned org users have Status=Invited, UserId=set, Email=null, which satisfies neither + /// the UserId-matched nor the Email-matched branch in the user-keyed policy details query. Using the + /// org-keyed IPolicyQuery instead avoids the broken user→OrgUser matching entirely. + /// + [Theory, BitAutoData] + public async Task ValidateAsync_WhenAutoConfirmEnabledAndUserHasNoMatchingPolicyDetails_SetsAutoConfirmPolicyEnabledTrue( + Guid organizationId, + SutProvider sutProvider) + { + // Simulate a JIT SSO user whose org user record has Status=Invited/UserId=set/Email=null, + // so the user-keyed requirement returns empty (neither UserId nor Email branch matches). + var jitUser = new User { Id = Guid.NewGuid(), Email = null! }; + SetupHappyPath(organizationId, jitUser, sutProvider); + + // User-keyed requirement returns empty — the broken state for JIT SSO users. + sutProvider.GetDependency() + .GetAsync(jitUser.Id) + .Returns(new AutomaticUserConfirmationPolicyRequirement([])); + + // But the org has auto-confirm enabled. + sutProvider.GetDependency() + .RunAsync(organizationId, PolicyType.AutomaticUserConfirmation) + .Returns(new PolicyStatus(organizationId, PolicyType.AutomaticUserConfirmation, + new Bit.Core.AdminConsole.Entities.Policy { Enabled = true })); + + var request = new AcceptOrganizationMembershipValidationRequest + { + OrganizationId = organizationId, + User = jitUser, + AllOrganizationMemberships = [], + }; + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsValid); + Assert.True(result.Request.AutoConfirmPolicyEnabled); + } + [Theory, BitAutoData] public async Task ValidateAsync_WhenAutoConfirmNotEnabled_SetsAutoConfirmPolicyEnabledFalse( User user, @@ -321,6 +359,10 @@ private static void SetupHappyPath( Arg.Any(), Arg.Any()) .Returns(callInfo => Valid(callInfo.Arg())); + + sutProvider.GetDependency() + .RunAsync(organizationId, PolicyType.AutomaticUserConfirmation) + .Returns(new PolicyStatus(organizationId, PolicyType.AutomaticUserConfirmation)); } [Theory, BitAutoData] diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/PushAutoConfirmNotificationCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/PushAutoConfirmNotificationCommandTests.cs index 09118c961812..0623f4719640 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/PushAutoConfirmNotificationCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/PushAutoConfirmNotificationCommandTests.cs @@ -1,10 +1,17 @@ -using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; +using Bit.Core.AdminConsole.Entities; +using Bit.Core.AdminConsole.Enums; +using Bit.Core.AdminConsole.Models.Data.Organizations.Policies; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; +using Bit.Core.AdminConsole.OrganizationFeatures.Policies; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Models; +using Bit.Core.Models.Data.Organizations; using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.Platform.Push; using Bit.Core.Repositories; +using Bit.Core.Services; +using Bit.Core.Test.AdminConsole.AutoFixture; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; using NSubstitute; @@ -15,6 +22,23 @@ namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.OrganizationUsers; [SutProviderCustomize] public class PushAutoConfirmNotificationCommandTests { + private static void SetupPassingGuards( + SutProvider sutProvider, + Guid organizationId, + OrganizationUser orgUser) + { + sutProvider.GetDependency() + .GetOrganizationAbilityAsync(organizationId) + .Returns(new OrganizationAbility { UseAutomaticUserConfirmation = true }); + + sutProvider.GetDependency() + .RunAsync(organizationId, PolicyType.AutomaticUserConfirmation) + .Returns(new PolicyStatus(organizationId, PolicyType.AutomaticUserConfirmation, + new Policy { Enabled = true })); + + orgUser.Type = OrganizationUserType.User; + } + [Theory] [BitAutoData] public async Task PushAsync_SendsNotificationToAdminsAndOwners( @@ -30,6 +54,7 @@ public async Task PushAsync_SendsNotificationToAdminsAndOwners( } orgUser.Id = Guid.NewGuid(); + SetupPassingGuards(sutProvider, organizationId, orgUser); sutProvider.GetDependency() .GetByOrganizationAsync(organizationId, userId) @@ -72,6 +97,7 @@ public async Task PushAsync_SendsNotificationToCustomUsersWithManageUsersPermiss } orgUser.Id = Guid.NewGuid(); + SetupPassingGuards(sutProvider, organizationId, orgUser); sutProvider.GetDependency() .GetByOrganizationAsync(organizationId, userId) @@ -114,6 +140,7 @@ public async Task PushAsync_DoesNotSendToCustomUsersWithoutManageUsersPermission } orgUser.Id = Guid.NewGuid(); + SetupPassingGuards(sutProvider, organizationId, orgUser); sutProvider.GetDependency() .GetByOrganizationAsync(organizationId, userId) @@ -163,6 +190,7 @@ public async Task PushAsync_SendsToAdminsAndCustomUsersWithManageUsers( } orgUser.Id = Guid.NewGuid(); + SetupPassingGuards(sutProvider, organizationId, orgUser); var allCustomUsers = customUsersWithPermission.Concat(customUsersWithoutPermission).ToList(); @@ -206,6 +234,7 @@ public async Task PushAsync_SkipsUsersWithoutUserId( admins[2].UserId = Guid.NewGuid(); orgUser.Id = Guid.NewGuid(); + SetupPassingGuards(sutProvider, organizationId, orgUser); sutProvider.GetDependency() .GetByOrganizationAsync(organizationId, userId) @@ -245,6 +274,7 @@ public async Task PushAsync_DeduplicatesUserIds( }; orgUser.Id = Guid.NewGuid(); + SetupPassingGuards(sutProvider, organizationId, orgUser); sutProvider.GetDependency() .GetByOrganizationAsync(organizationId, userId) @@ -271,8 +301,11 @@ await sutProvider.GetDependency() public async Task PushAsync_OrganizationUserNotFound_ThrowsException( SutProvider sutProvider, Guid userId, - Guid organizationId) + Guid organizationId, + OrganizationUser orgUser) { + SetupPassingGuards(sutProvider, organizationId, orgUser); + sutProvider.GetDependency() .GetByOrganizationAsync(organizationId, userId) .Returns((OrganizationUser)null); @@ -286,4 +319,104 @@ await sutProvider.GetDependency() .DidNotReceiveWithAnyArgs() .PushAsync(Arg.Any>()); } + + // Guard 1: org ability checks + + [Theory] + [BitAutoData] + public async Task PushAsync_OrgAbilityIsNull_ReturnsEarlyWithoutNotification( + SutProvider sutProvider, + Guid userId, + Guid organizationId) + { + sutProvider.GetDependency() + .GetOrganizationAbilityAsync(organizationId) + .Returns((OrganizationAbility?)null); + + await sutProvider.Sut.PushAsync(userId, organizationId); + + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .PushAsync(Arg.Any>()); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .GetByOrganizationAsync(Arg.Any(), Arg.Any()); + } + + [Theory] + [BitAutoData] + public async Task PushAsync_UseAutomaticUserConfirmationDisabled_ReturnsEarlyWithoutNotification( + SutProvider sutProvider, + Guid userId, + Guid organizationId) + { + sutProvider.GetDependency() + .GetOrganizationAbilityAsync(organizationId) + .Returns(new OrganizationAbility { UseAutomaticUserConfirmation = false }); + + await sutProvider.Sut.PushAsync(userId, organizationId); + + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .PushAsync(Arg.Any>()); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .GetByOrganizationAsync(Arg.Any(), Arg.Any()); + } + + // Guard 2: policy check + + [Theory, BitAutoData] + public async Task PushAsync_PolicyDisabled_ReturnsEarlyWithoutNotification( + Guid organizationId, + Guid userId, + [Policy(PolicyType.AutomaticUserConfirmation, false)] PolicyStatus disabledPolicy, + SutProvider sutProvider) + { + sutProvider.GetDependency() + .GetOrganizationAbilityAsync(organizationId) + .Returns(new OrganizationAbility { UseAutomaticUserConfirmation = true }); + + sutProvider.GetDependency() + .RunAsync(organizationId, PolicyType.AutomaticUserConfirmation) + .Returns(disabledPolicy); + + await sutProvider.Sut.PushAsync(userId, organizationId); + + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .PushAsync(Arg.Any>()); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .GetByOrganizationAsync(Arg.Any(), Arg.Any()); + } + + // Guard 3: target user role check + + [Theory] + [BitAutoData(OrganizationUserType.Admin)] + [BitAutoData(OrganizationUserType.Owner)] + [BitAutoData(OrganizationUserType.Custom)] + public async Task PushAsync_TargetUserIsNotMember_ReturnsEarlyWithoutNotification( + OrganizationUserType privilegedType, + SutProvider sutProvider, + Guid userId, + Guid organizationId, + OrganizationUser orgUser) + { + orgUser.Type = privilegedType; + SetupPassingGuards(sutProvider, organizationId, orgUser); + orgUser.Type = privilegedType; // override after SetupPassingGuards sets User + + sutProvider.GetDependency() + .GetByOrganizationAsync(organizationId, userId) + .Returns(orgUser); + + await sutProvider.Sut.PushAsync(userId, organizationId); + + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .PushAsync(Arg.Any>()); + } + }