Merge branch 'main' into billing/license-refactor

src/Api/Models/Public/Response/CollectionResponseModel.cs

@@ -1,6 +1,7 @@
 using System.ComponentModel.DataAnnotations;
 using Bit.Api.AdminConsole.Public.Models.Response;
 using Bit.Core.Entities;
+using Bit.Core.Enums;
 using Bit.Core.Models.Data;
 
 namespace Bit.Api.Models.Public.Response;
@@ -20,6 +21,7 @@ public class CollectionResponseModel : CollectionBaseModel, IResponseModel
         Id = collection.Id;
         ExternalId = collection.ExternalId;
         Groups = groups?.Select(c => new AssociationWithPermissionsResponseModel(c));
+        Type = collection.Type;
     }
 
     /// <summary>
@@ -38,4 +40,8 @@ public class CollectionResponseModel : CollectionBaseModel, IResponseModel
     /// The associated groups that this collection is assigned to.
     /// </summary>
     public IEnumerable<AssociationWithPermissionsResponseModel> Groups { get; set; }
+    /// <summary>
+    /// The type of this collection
+    /// </summary>
+    public CollectionType Type { get; set; }
 }

src/Api/Models/Response/CollectionResponseModel.cs

@@ -1,4 +1,5 @@
 using Bit.Core.Entities;
+using Bit.Core.Enums;
 using Bit.Core.Models.Api;
 using Bit.Core.Models.Data;
 
@@ -18,12 +19,14 @@ public class CollectionResponseModel : ResponseModel
         OrganizationId = collection.OrganizationId;
         Name = collection.Name;
         ExternalId = collection.ExternalId;
+        Type = collection.Type;
     }
 
     public Guid Id { get; set; }
     public Guid OrganizationId { get; set; }
     public string Name { get; set; }
     public string ExternalId { get; set; }
+    public CollectionType Type { get; set; }
 }
 
 /// <summary>

src/Core/AdminConsole/Entities/OrganizationUser.cs

@@ -1,4 +1,5 @@
 using System.ComponentModel.DataAnnotations;
+using Bit.Core.AdminConsole.Entities;
 using Bit.Core.AdminConsole.Interfaces;
 using Bit.Core.Enums;
 using Bit.Core.Models;
@@ -9,23 +10,75 @@ using Bit.Core.Utilities;
 
 namespace Bit.Core.Entities;
 
+/// <summary>
+/// An association table between one <see cref="User"/> and one <see cref="Organization"/>, representing that user's
+/// membership in the organization. "Member" refers to the OrganizationUser object.
+/// </summary>
 public class OrganizationUser : ITableObject<Guid>, IExternal, IOrganizationUser
 {
+    /// <summary>
+    /// A unique random identifier.
+    /// </summary>
     public Guid Id { get; set; }
+    /// <summary>
+    /// The ID of the Organization that the user is a member of.
+    /// </summary>
     public Guid OrganizationId { get; set; }
+    /// <summary>
+    /// The ID of the User that is the member. This is NULL if the Status is Invited (or Invited and then Revoked), because
+    /// it is not linked to a specific User yet.
+    /// </summary>
     public Guid? UserId { get; set; }
+    /// <summary>
+    /// The email address of the user invited to the organization. This is NULL if the Status is not Invited (or
+    /// Invited and then Revoked), because in that case the OrganizationUser is linked to a User
+    /// and the email is stored on the User object.
+    /// </summary>
     [MaxLength(256)]
     public string? Email { get; set; }
+    /// <summary>
+    /// The Organization symmetric key encrypted with the User's public key. NULL if the user is not in a Confirmed
+    /// (or Confirmed and then Revoked) status.
+    /// </summary>
     public string? Key { get; set; }
+    /// <summary>
+    /// The User's symmetric key encrypted with the Organization's public key. NULL if the OrganizationUser
+    /// is not enrolled in account recovery.
+    /// </summary>
     public string? ResetPasswordKey { get; set; }
+    /// <inheritdoc cref="OrganizationUserStatusType"/>
     public OrganizationUserStatusType Status { get; set; }
+    /// <summary>
+    /// The User's role in the Organization.
+    /// </summary>
     public OrganizationUserType Type { get; set; }
-
+    /// <summary>
+    /// An ID used to identify the OrganizationUser with an external directory service. Used by Directory Connector
+    /// and SCIM.
+    /// </summary>
     [MaxLength(300)]
     public string? ExternalId { get; set; }
+    /// <summary>
+    /// The date the OrganizationUser was created, i.e. when the User was first invited to the Organization.
+    /// </summary>
     public DateTime CreationDate { get; internal set; } = DateTime.UtcNow;
+    /// <summary>
+    /// The last date the OrganizationUser entry was updated.
+    /// </summary>
     public DateTime RevisionDate { get; internal set; } = DateTime.UtcNow;
+    /// <summary>
+    /// A json blob representing the <see cref="Bit.Core.Models.Data.Permissions"/> of the OrganizationUser if they
+    /// are a Custom user role (i.e. the <see cref="OrganizationUserType"/> is Custom). MAY be NULL if they are not
+    /// a custom user, but this is not guaranteed; do not use this to determine their role.
+    /// </summary>
+    /// <remarks>
+    /// Avoid using this property directly - instead use the <see cref="GetPermissions"/> and <see cref="SetPermissions"/>
+    /// helper methods.
+    /// </remarks>
     public string? Permissions { get; set; }
+    /// <summary>
+    /// True if the User has access to Secrets Manager for this Organization, false otherwise.
+    /// </summary>
     public bool AccessSecretsManager { get; set; }
 
     public void SetNewId()

src/Core/AdminConsole/Enums/OrganizationUserStatusType.cs

@@ -1,9 +1,34 @@
-namespace Bit.Core.Enums;
+using Bit.Core.Entities;
 
+namespace Bit.Core.Enums;
+
+/// <summary>
+/// Represents the different stages of a member's lifecycle in an organization.
+/// The <see cref="OrganizationUser"/> object is populated differently depending on their Status.
+/// </summary>
 public enum OrganizationUserStatusType : short
 {
+    /// <summary>
+    /// The OrganizationUser entry only represents an invitation to join the organization. It is not linked to a
+    /// specific User yet.
+    /// </summary>
     Invited = 0,
+    /// <summary>
+    /// The User has accepted the invitation and linked their User account to the OrganizationUser entry.
+    /// </summary>
     Accepted = 1,
+    /// <summary>
+    /// An administrator has granted the User access to the organization. This is the final step in the User becoming
+    /// a "full" member of the organization, including a key exchange so that they can decrypt organization data.
+    /// </summary>
     Confirmed = 2,
+    /// <summary>
+    /// The OrganizationUser has been revoked from the organization and cannot access organization data while in this state.
+    /// </summary>
+    /// <remarks>
+    /// An OrganizationUser may move into this status from any other status, and will move back to their original status
+    /// if restored. This allows an administrator to easily suspend and restore access without going through the
+    /// Invite flow again.
+    /// </remarks>
     Revoked = -1,
 }

src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs

@@ -104,8 +104,8 @@ public class TwoFactorAuthenticationPolicyValidator : IPolicyValidator
             throw new BadRequestException(string.Join(", ", commandResult.ErrorMessages));
         }
 
-        await Task.WhenAll(currentActiveRevocableOrganizationUsers.Select(x =>
-            _mailService.SendOrganizationUserRevokedForTwoFactorPolicyEmailAsync(organization.DisplayName(), x.Email)));
+        await Task.WhenAll(nonCompliantUsers.Select(nonCompliantUser =>
+            _mailService.SendOrganizationUserRevokedForTwoFactorPolicyEmailAsync(organization.DisplayName(), nonCompliantUser.user.Email)));
     }
 
     private static bool MembersWithNoMasterPasswordWillLoseAccess(

src/Core/Services/Implementations/UserService.cs

@@ -12,7 +12,6 @@ using Bit.Core.AdminConsole.Repositories;
 using Bit.Core.AdminConsole.Services;
 using Bit.Core.Auth.Enums;
 using Bit.Core.Auth.Models;
-using Bit.Core.Auth.Models.Business.Tokenables;
 using Bit.Core.Auth.UserFeatures.TwoFactorAuth.Interfaces;
 using Bit.Core.Billing.Constants;
 using Bit.Core.Billing.Licenses.Extensions;
@@ -30,12 +29,9 @@ using Bit.Core.OrganizationFeatures.OrganizationUsers.Interfaces;
 using Bit.Core.Platform.Push;
 using Bit.Core.Repositories;
 using Bit.Core.Settings;
-using Bit.Core.Tokens;
 using Bit.Core.Utilities;
-using Bit.Core.Vault.Repositories;
 using Fido2NetLib;
 using Fido2NetLib.Objects;
-using Microsoft.AspNetCore.DataProtection;
 using Microsoft.AspNetCore.Identity;
 using Microsoft.Extensions.Caching.Distributed;
 using Microsoft.Extensions.Logging;
@@ -45,12 +41,11 @@ using JsonSerializer = System.Text.Json.JsonSerializer;
 
 namespace Bit.Core.Services;
 
-public class UserService : UserManager<User>, IUserService, IDisposable
+public class UserService : UserManager<User>, IUserService
 {
     private const string PremiumPlanId = "premium-annually";
 
     private readonly IUserRepository _userRepository;
-    private readonly ICipherRepository _cipherRepository;
     private readonly IOrganizationUserRepository _organizationUserRepository;
     private readonly IOrganizationRepository _organizationRepository;
     private readonly IOrganizationDomainRepository _organizationDomainRepository;
@@ -66,17 +61,14 @@ public class UserService : UserManager<User>, IUserService, IDisposable
     private readonly IPaymentService _paymentService;
     private readonly IPolicyRepository _policyRepository;
     private readonly IPolicyService _policyService;
-    private readonly IDataProtector _organizationServiceDataProtector;
     private readonly IFido2 _fido2;
     private readonly ICurrentContext _currentContext;
     private readonly IGlobalSettings _globalSettings;
     private readonly IAcceptOrgUserCommand _acceptOrgUserCommand;
     private readonly IProviderUserRepository _providerUserRepository;
     private readonly IStripeSyncService _stripeSyncService;
-    private readonly IDataProtectorTokenFactory<OrgUserInviteTokenable> _orgUserInviteTokenDataFactory;
     private readonly IFeatureService _featureService;
     private readonly IPremiumUserBillingService _premiumUserBillingService;
-    private readonly IRemoveOrganizationUserCommand _removeOrganizationUserCommand;
     private readonly IRevokeNonCompliantOrganizationUserCommand _revokeNonCompliantOrganizationUserCommand;
     private readonly ITwoFactorIsEnabledQuery _twoFactorIsEnabledQuery;
     private readonly IDistributedCache _distributedCache;
@@ -84,7 +76,6 @@ public class UserService : UserManager<User>, IUserService, IDisposable
 
     public UserService(
         IUserRepository userRepository,
-        ICipherRepository cipherRepository,
         IOrganizationUserRepository organizationUserRepository,
         IOrganizationRepository organizationRepository,
         IOrganizationDomainRepository organizationDomainRepository,
@@ -102,7 +93,6 @@ public class UserService : UserManager<User>, IUserService, IDisposable
         ILicensingService licenseService,
         IEventService eventService,
         IApplicationCacheService applicationCacheService,
-        IDataProtectionProvider dataProtectionProvider,
         IPaymentService paymentService,
         IPolicyRepository policyRepository,
         IPolicyService policyService,
@@ -112,10 +102,8 @@ public class UserService : UserManager<User>, IUserService, IDisposable
         IAcceptOrgUserCommand acceptOrgUserCommand,
         IProviderUserRepository providerUserRepository,
         IStripeSyncService stripeSyncService,
-        IDataProtectorTokenFactory<OrgUserInviteTokenable> orgUserInviteTokenDataFactory,
         IFeatureService featureService,
         IPremiumUserBillingService premiumUserBillingService,
-        IRemoveOrganizationUserCommand removeOrganizationUserCommand,
         IRevokeNonCompliantOrganizationUserCommand revokeNonCompliantOrganizationUserCommand,
         ITwoFactorIsEnabledQuery twoFactorIsEnabledQuery,
         IDistributedCache distributedCache,
@@ -132,7 +120,6 @@ public class UserService : UserManager<User>, IUserService, IDisposable
               logger)
     {
         _userRepository = userRepository;
-        _cipherRepository = cipherRepository;
         _organizationUserRepository = organizationUserRepository;
         _organizationRepository = organizationRepository;
         _organizationDomainRepository = organizationDomainRepository;
@@ -148,18 +135,14 @@ public class UserService : UserManager<User>, IUserService, IDisposable
         _paymentService = paymentService;
         _policyRepository = policyRepository;
         _policyService = policyService;
-        _organizationServiceDataProtector = dataProtectionProvider.CreateProtector(
-            "OrganizationServiceDataProtector");
         _fido2 = fido2;
         _currentContext = currentContext;
         _globalSettings = globalSettings;
         _acceptOrgUserCommand = acceptOrgUserCommand;
         _providerUserRepository = providerUserRepository;
         _stripeSyncService = stripeSyncService;
-        _orgUserInviteTokenDataFactory = orgUserInviteTokenDataFactory;
         _featureService = featureService;
         _premiumUserBillingService = premiumUserBillingService;
-        _removeOrganizationUserCommand = removeOrganizationUserCommand;
         _revokeNonCompliantOrganizationUserCommand = revokeNonCompliantOrganizationUserCommand;
         _twoFactorIsEnabledQuery = twoFactorIsEnabledQuery;
         _distributedCache = distributedCache;

src/Icons/Controllers/IconsController.cs

@@ -8,6 +8,16 @@ namespace Bit.Icons.Controllers;
 [Route("")]
 public class IconsController : Controller
 {
+    // Basic bwi-globe icon
+    private static readonly byte[] _notFoundImage = Convert.FromBase64String("iVBORw0KGgoAAAANSUhEUg" +
+        "AAABMAAAATCAQAAADYWf5HAAABu0lEQVR42nXSvWuTURTH8R+t0heI9Y04aJycdBLNJNrBFBU7OFgUER3q21I0bXK+JwZ" +
+        "pXISm/QdcRB3EgqBBsNihsUbbgODQQSKCuKSDOApJuuhj8tCYQj/jvYfD795z1MZ+nBKrNKhSwrMxbZTrtRnqlEjZkB/x" +
+        "C/xmhZrlc71qS0Up8yVzTCGucFNKD1JhORVd70SZNU4okNx5d4+U2UXRIpJFWLClsR79YzN88wQvLWNzzPKEeS/wkQGpW" +
+        "VhhqhW8TtDJD3Mm1x/23zLSrZCdpBY8BueTNjHSbc+8wC9HlHgU5Aj5AW5zPdcVdpq0UcknWBSr/pjixO4gfp899Kd23p" +
+        "M2qQCH7LkCnqAqGh73OK/8NPOcaibr90LrW/yWAnaUhqjaOSl9nFR2r5rsqo22ypn1B5IN8VOUMHVgOnNQIX+d62plcz6" +
+        "rg1/jskK8CMb4we4pG6OWHtR/LBJkC2E4a7ZPkuX5ntumAOM2xxveclEhLvGH6XCmLPs735Eetrw63NnOgr9P9q1viC3x" +
+        "lRUGOjImqFDuOBvrYYoaZU9z1uPpYae5NfdvbNVG2ZjDIlXq/oMi46lo++4vjjPBl2Dlg00AAAAASUVORK5CYII=");
+
     private readonly IMemoryCache _memoryCache;
     private readonly IDomainMappingService _domainMappingService;
     private readonly IIconFetchingService _iconFetchingService;
@@ -89,7 +99,7 @@ public class IconsController : Controller
 
         if (icon == null)
         {
-            return new NotFoundResult();
+            return new FileContentResult(_notFoundImage, "image/png");
         }
 
         return new FileContentResult(icon.Image, icon.Format);

test/Common/AutoFixture/SutProvider.cs

@@ -4,8 +4,19 @@ using AutoFixture.Kernel;
 
 namespace Bit.Test.Common.AutoFixture;
 
+/// <summary>
+/// A utility class that encapsulates a system under test (sut) and its dependencies.
+/// By default, all dependencies are initialized as mocks using the NSubstitute library.
+/// SutProvider provides an interface for accessing these dependencies in the arrange and assert stages of your tests.
+/// </summary>
+/// <typeparam name="TSut">The concrete implementation of the class being tested.</typeparam>
 public class SutProvider<TSut> : ISutProvider
 {
+    /// <summary>
+    /// A record of the configured dependencies (constructor parameters). The outer Dictionary is keyed by the dependency's
+    /// type, and the inner dictionary is keyed by the parameter name (optionally used to disambiguate parameters with the same type).
+    /// The inner dictionary value is the dependency.
+    /// </summary>
     private Dictionary<Type, Dictionary<string, object>> _dependencies;
     private readonly IFixture _fixture;
     private readonly ConstructorParameterRelay<TSut> _constructorParameterRelay;
@@ -23,9 +34,21 @@ public class SutProvider<TSut> : ISutProvider
         _fixture.Customizations.Add(_constructorParameterRelay);
     }
 
+    /// <summary>
+    /// Registers a dependency to be injected when the sut is created. You must call <see cref="Create"/> after
+    /// this method to (re)create the sut with the dependency.
+    /// </summary>
+    /// <param name="dependency">The dependency to register.</param>
+    /// <param name="parameterName">An optional parameter name to disambiguate the dependency if there are multiple of the same type. You generally don't need this.</param>
+    /// <typeparam name="T">The type to register the dependency under - usually an interface. This should match the type expected by the sut's constructor.</typeparam>
+    /// <returns></returns>
     public SutProvider<TSut> SetDependency<T>(T dependency, string parameterName = "")
         => SetDependency(typeof(T), dependency, parameterName);
-    public SutProvider<TSut> SetDependency(Type dependencyType, object dependency, string parameterName = "")
+
+    /// <summary>
+    /// An overload for <see cref="SetDependency{T}"/> which takes a runtime <see cref="Type"/> object rather than a compile-time type.
+    /// </summary>
+    private SutProvider<TSut> SetDependency(Type dependencyType, object dependency, string parameterName = "")
     {
         if (_dependencies.TryGetValue(dependencyType, out var dependencyForType))
         {
@@ -39,45 +62,69 @@ public class SutProvider<TSut> : ISutProvider
         return this;
     }
 
+    /// <summary>
+    /// Gets a dependency of the sut. Can only be called after the dependency has been set, either explicitly with
+    /// <see cref="SetDependency{T}"/> or automatically with <see cref="Create"/>.
+    /// As dependencies are initialized with NSubstitute mocks by default, this is often used to retrieve those mocks in order to
+    /// configure them during the arrange stage, or check received calls in the assert stage.
+    /// </summary>
+    /// <param name="parameterName">An optional parameter name to disambiguate the dependency if there are multiple of the same type. You generally don't need this.</param>
+    /// <typeparam name="T">The type of the dependency you want to get - usually an interface.</typeparam>
+    /// <returns>The dependency.</returns>
     public T GetDependency<T>(string parameterName = "") => (T)GetDependency(typeof(T), parameterName);
-    public object GetDependency(Type dependencyType, string parameterName = "")
+
+    /// <summary>
+    /// An overload for <see cref="GetDependency{T}"/> which takes a runtime <see cref="Type"/> object rather than a compile-time type.
+    /// </summary>
+    private object GetDependency(Type dependencyType, string parameterName = "")
     {
         if (DependencyIsSet(dependencyType, parameterName))
         {
             return _dependencies[dependencyType][parameterName];
         }
-        else if (_dependencies.TryGetValue(dependencyType, out var knownDependencies))
+
+        if (_dependencies.TryGetValue(dependencyType, out var knownDependencies))
         {
             if (knownDependencies.Values.Count == 1)
             {
                 return knownDependencies.Values.Single();
             }
-            else
-            {
+
             throw new ArgumentException(string.Concat($"Dependency of type {dependencyType.Name} and name ",
                 $"{parameterName} does not exist. Available dependency names are: ",
                 string.Join(", ", knownDependencies.Keys)));
         }
-        }
-        else
-        {
+
         throw new ArgumentException($"Dependency of type {dependencyType.Name} and name {parameterName} has not been set.");
     }
-    }
 
+    /// <summary>
+    /// Clear all the dependencies and the sut. This reverts the SutProvider back to a fully uninitialized state.
+    /// </summary>
     public void Reset()
     {
         _dependencies = new Dictionary<Type, Dictionary<string, object>>();
         Sut = default;
     }
 
+    /// <summary>
+    /// Recreate a new sut with all new dependencies. This will reset all dependencies, including mocked return values
+    /// and any dependencies set with <see cref="SetDependency{T}"/>.
+    /// </summary>
     public void Recreate()
     {
         _dependencies = new Dictionary<Type, Dictionary<string, object>>();
         Sut = _fixture.Create<TSut>();
     }
 
+    /// <inheritdoc cref="Create()"/>>
     ISutProvider ISutProvider.Create() => Create();
+
+    /// <summary>
+    /// Creates the sut, injecting any dependencies configured via <see cref="SetDependency{T}"/> and falling back to
+    /// NSubstitute mocks for any dependencies that have not been explicitly configured.
+    /// </summary>
+    /// <returns></returns>
     public SutProvider<TSut> Create()
     {
         Sut = _fixture.Create<TSut>();
@@ -89,6 +136,19 @@ public class SutProvider<TSut> : ISutProvider
 
     private object GetDefault(Type type) => type.IsValueType ? Activator.CreateInstance(type) : null;
 
+    /// <summary>
+    /// A specimen builder which tells Autofixture to use the dependency registered in <see cref="SutProvider{T}"/>
+    /// when creating test data. If no matching dependency exists in <see cref="SutProvider{TSut}"/>, it creates
+    /// an NSubstitute mock and registers it using <see cref="SutProvider{TSut}.SetDependency{T}"/>
+    /// so it can be retrieved later.
+    /// This is the link between <see cref="SutProvider{T}"/> and Autofixture.
+    /// </summary>
+    /// <remarks>
+    /// Autofixture knows how to create sample data of simple types (such as an int or string) but not more complex classes.
+    /// We create our own <see cref="ISpecimenBuilder"/> and register it with the <see cref="Fixture"/> in
+    /// <see cref="SutProvider{TSut}"/> to provide that instruction.
+    /// </remarks>
+    /// <typeparam name="T">The type of the sut.</typeparam>
     private class ConstructorParameterRelay<T> : ISpecimenBuilder
     {
         private readonly SutProvider<T> _sutProvider;
@@ -102,6 +162,7 @@ public class SutProvider<TSut> : ISutProvider
 
         public object Create(object request, ISpecimenContext context)
         {
+            // Basic checks to filter out irrelevant requests from Autofixture
             if (context == null)
             {
                 throw new ArgumentNullException(nameof(context));
@@ -116,16 +177,22 @@ public class SutProvider<TSut> : ISutProvider
                 return new NoSpecimen();
             }
 
+            // Use the dependency set under this parameter name, if any
             if (_sutProvider.DependencyIsSet(parameterInfo.ParameterType, parameterInfo.Name))
             {
                 return _sutProvider.GetDependency(parameterInfo.ParameterType, parameterInfo.Name);
             }
-            // Return default type if set
-            else if (_sutProvider.DependencyIsSet(parameterInfo.ParameterType, ""))
+
+            // Use the default dependency set for this type, if any (i.e. no parameter name has been specified)
+            if (_sutProvider.DependencyIsSet(parameterInfo.ParameterType, ""))
             {
                 return _sutProvider.GetDependency(parameterInfo.ParameterType, "");
             }
 
+            // Fallback: pass the request down the chain. This lets another fixture customization populate the value.
+            // If you haven't added any customizations, this should be an NSubstitute mock.
+            // It is registered with SetDependency so you can retrieve it later.
+
             // This is the equivalent of _fixture.Create<parameterInfo.ParameterType>, but no overload for
             // Create(Type type) exists.
             var dependency = new SpecimenContext(_fixture).Resolve(new SeededRequest(parameterInfo.ParameterType,

test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs

@@ -60,16 +60,19 @@ public class TwoFactorAuthenticationPolicyValidatorTests
     }
 
     [Theory, BitAutoData]
-    public async Task OnSaveSideEffectsAsync_RevokesNonCompliantUsers(
+    public async Task OnSaveSideEffectsAsync_RevokesOnlyNonCompliantUsers(
         Organization organization,
         [PolicyUpdate(PolicyType.TwoFactorAuthentication)] PolicyUpdate policyUpdate,
         [Policy(PolicyType.TwoFactorAuthentication, false)] Policy policy,
         SutProvider<TwoFactorAuthenticationPolicyValidator> sutProvider)
     {
-        policy.OrganizationId = organization.Id = policyUpdate.OrganizationId;
+        // Arrange
+        policy.OrganizationId = policyUpdate.OrganizationId;
+        organization.Id = policyUpdate.OrganizationId;
+
         sutProvider.GetDependency<IOrganizationRepository>().GetByIdAsync(organization.Id).Returns(organization);
 
-        var orgUserDetailUserWithout2Fa = new OrganizationUserUserDetails
+        var nonCompliantUser = new OrganizationUserUserDetails
         {
             Id = Guid.NewGuid(),
             Status = OrganizationUserStatusType.Confirmed,
@@ -80,30 +83,57 @@ public class TwoFactorAuthenticationPolicyValidatorTests
             HasMasterPassword = true
         };
 
+        var compliantUser = new OrganizationUserUserDetails
+        {
+            Id = Guid.NewGuid(),
+            Status = OrganizationUserStatusType.Confirmed,
+            Type = OrganizationUserType.User,
+            Email = "user4@test.com",
+            Name = "TEST",
+            UserId = Guid.NewGuid(),
+            HasMasterPassword = true
+        };
+
         sutProvider.GetDependency<IOrganizationUserRepository>()
             .GetManyDetailsByOrganizationAsync(policyUpdate.OrganizationId)
-            .Returns([orgUserDetailUserWithout2Fa]);
+            .Returns([nonCompliantUser, compliantUser]);
 
         sutProvider.GetDependency<ITwoFactorIsEnabledQuery>()
             .TwoFactorIsEnabledAsync(Arg.Any<IEnumerable<OrganizationUserUserDetails>>())
             .Returns(new List<(OrganizationUserUserDetails user, bool hasTwoFactor)>()
             {
-                (orgUserDetailUserWithout2Fa, false)
+                (nonCompliantUser, false),
+                (compliantUser, true)
             });
 
         sutProvider.GetDependency<IRevokeNonCompliantOrganizationUserCommand>()
             .RevokeNonCompliantOrganizationUsersAsync(Arg.Any<RevokeOrganizationUsersRequest>())
             .Returns(new CommandResult());
 
+        // Act
         await sutProvider.Sut.OnSaveSideEffectsAsync(policyUpdate, policy);
 
+        // Assert
         await sutProvider.GetDependency<IRevokeNonCompliantOrganizationUserCommand>()
             .Received(1)
             .RevokeNonCompliantOrganizationUsersAsync(Arg.Any<RevokeOrganizationUsersRequest>());
 
+        await sutProvider.GetDependency<IRevokeNonCompliantOrganizationUserCommand>()
+            .Received(1)
+            .RevokeNonCompliantOrganizationUsersAsync(Arg.Is<RevokeOrganizationUsersRequest>(req =>
+                    req.OrganizationId == policyUpdate.OrganizationId &&
+                    req.OrganizationUsers.SequenceEqual(new[] { nonCompliantUser })
+            ));
+
         await sutProvider.GetDependency<IMailService>()
             .Received(1)
             .SendOrganizationUserRevokedForTwoFactorPolicyEmailAsync(organization.DisplayName(),
-                "user3@test.com");
+                nonCompliantUser.Email);
+
+        // Did not send out an email for compliantUser
+        await sutProvider.GetDependency<IMailService>()
+            .Received(0)
+            .SendOrganizationUserRevokedForTwoFactorPolicyEmailAsync(organization.DisplayName(),
+                compliantUser.Email);
     }
 }

test/Core.Test/AdminConsole/OrganizationFeatures/Policies/SavePolicyCommandTests.cs

@@ -288,7 +288,7 @@ public class SavePolicyCommandTests
     {
         return new SutProvider<SavePolicyCommand>()
             .WithFakeTimeProvider()
-            .SetDependency(typeof(IEnumerable<IPolicyValidator>), policyValidators ?? [])
+            .SetDependency(policyValidators ?? [])
             .Create();
     }
 

test/Core.Test/Services/UserServiceTests.cs

@@ -7,13 +7,10 @@ using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces;
 using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests;
 using Bit.Core.AdminConsole.OrganizationFeatures.Policies;
 using Bit.Core.AdminConsole.OrganizationFeatures.Policies.PolicyRequirements;
-using Bit.Core.AdminConsole.Repositories;
 using Bit.Core.AdminConsole.Services;
 using Bit.Core.Auth.Enums;
 using Bit.Core.Auth.Models;
-using Bit.Core.Auth.Models.Business.Tokenables;
 using Bit.Core.Auth.UserFeatures.TwoFactorAuth.Interfaces;
-using Bit.Core.Billing.Services;
 using Bit.Core.Context;
 using Bit.Core.Entities;
 using Bit.Core.Enums;
@@ -21,22 +18,15 @@ using Bit.Core.Exceptions;
 using Bit.Core.Models.Business;
 using Bit.Core.Models.Data.Organizations;
 using Bit.Core.Models.Data.Organizations.OrganizationUsers;
-using Bit.Core.OrganizationFeatures.OrganizationUsers.Interfaces;
-using Bit.Core.Platform.Push;
 using Bit.Core.Repositories;
 using Bit.Core.Services;
 using Bit.Core.Settings;
 using Bit.Core.Utilities;
-using Bit.Core.Vault.Repositories;
 using Bit.Test.Common.AutoFixture;
 using Bit.Test.Common.AutoFixture.Attributes;
-using Bit.Test.Common.Fakes;
 using Bit.Test.Common.Helpers;
-using Fido2NetLib;
-using Microsoft.AspNetCore.DataProtection;
 using Microsoft.AspNetCore.Identity;
 using Microsoft.Extensions.Caching.Distributed;
-using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using NSubstitute;
 using Xunit;
@@ -179,9 +169,12 @@ public class UserServiceTests
     [Theory]
     [BitAutoData(DeviceType.UnknownBrowser, "Unknown Browser")]
     [BitAutoData(DeviceType.Android, "Android")]
-    public async Task SendNewDeviceVerificationEmailAsync_DeviceMatches(DeviceType deviceType, string deviceTypeName, SutProvider<UserService> sutProvider, User user)
+    public async Task SendNewDeviceVerificationEmailAsync_DeviceMatches(DeviceType deviceType, string deviceTypeName,
+        User user)
     {
-        SetupFakeTokenProvider(sutProvider, user);
+        var sutProvider = new SutProvider<UserService>()
+            .CreateWithUserServiceCustomizations(user);
+
         var context = sutProvider.GetDependency<ICurrentContext>();
         context.DeviceType = deviceType;
         context.IpAddress = "1.1.1.1";
@@ -194,9 +187,11 @@ public class UserServiceTests
     }
 
     [Theory, BitAutoData]
-    public async Task SendNewDeviceVerificationEmailAsync_NullDeviceTypeShouldSendUnkownBrowserType(SutProvider<UserService> sutProvider, User user)
+    public async Task SendNewDeviceVerificationEmailAsync_NullDeviceTypeShouldSendUnkownBrowserType(User user)
     {
-        SetupFakeTokenProvider(sutProvider, user);
+        var sutProvider = new SutProvider<UserService>()
+            .CreateWithUserServiceCustomizations(user);
+
         var context = sutProvider.GetDependency<ICurrentContext>();
         context.DeviceType = null;
         context.IpAddress = "1.1.1.1";
@@ -266,76 +261,28 @@ public class UserServiceTests
     [BitAutoData(true, "bad_test_password", false, ShouldCheck.Password | ShouldCheck.OTP)]
     public async Task VerifySecretAsync_Works(
         bool shouldHavePassword, string secret, bool expectedIsVerified, ShouldCheck shouldCheck, // inline theory data
-        SutProvider<UserService> sutProvider, User user) // AutoFixture injected data
+        User user) // AutoFixture injected data
     {
         // Arrange
-        var tokenProvider = SetupFakeTokenProvider(sutProvider, user);
         SetupUserAndDevice(user, shouldHavePassword);
 
+        var sutProvider = new SutProvider<UserService>()
+            .CreateWithUserServiceCustomizations(user);
+
         // Setup the fake password verification
-        var substitutedUserPasswordStore = Substitute.For<IUserPasswordStore<User>>();
-        substitutedUserPasswordStore
+        sutProvider.GetDependency<IUserPasswordStore<User>>()
             .GetPasswordHashAsync(user, Arg.Any<CancellationToken>())
-            .Returns((ci) =>
-            {
-                return Task.FromResult("hashed_test_password");
-            });
+            .Returns(Task.FromResult("hashed_test_password"));
 
-        sutProvider.SetDependency<IUserStore<User>>(substitutedUserPasswordStore, "store");
-
-        sutProvider.GetDependency<IPasswordHasher<User>>("passwordHasher")
+        sutProvider.GetDependency<IPasswordHasher<User>>()
             .VerifyHashedPassword(user, "hashed_test_password", "test_password")
-            .Returns((ci) =>
-            {
-                return PasswordVerificationResult.Success;
-            });
+            .Returns(PasswordVerificationResult.Success);
 
-        // HACK: SutProvider is being weird about not injecting the IPasswordHasher that I configured
-        var sut = new UserService(
-            sutProvider.GetDependency<IUserRepository>(),
-            sutProvider.GetDependency<ICipherRepository>(),
-            sutProvider.GetDependency<IOrganizationUserRepository>(),
-            sutProvider.GetDependency<IOrganizationRepository>(),
-            sutProvider.GetDependency<IOrganizationDomainRepository>(),
-            sutProvider.GetDependency<IMailService>(),
-            sutProvider.GetDependency<IPushNotificationService>(),
-            sutProvider.GetDependency<IUserStore<User>>(),
-            sutProvider.GetDependency<IOptions<IdentityOptions>>(),
-            sutProvider.GetDependency<IPasswordHasher<User>>(),
-            sutProvider.GetDependency<IEnumerable<IUserValidator<User>>>(),
-            sutProvider.GetDependency<IEnumerable<IPasswordValidator<User>>>(),
-            sutProvider.GetDependency<ILookupNormalizer>(),
-            sutProvider.GetDependency<IdentityErrorDescriber>(),
-            sutProvider.GetDependency<IServiceProvider>(),
-            sutProvider.GetDependency<ILogger<UserManager<User>>>(),
-            sutProvider.GetDependency<ILicensingService>(),
-            sutProvider.GetDependency<IEventService>(),
-            sutProvider.GetDependency<IApplicationCacheService>(),
-            sutProvider.GetDependency<IDataProtectionProvider>(),
-            sutProvider.GetDependency<IPaymentService>(),
-            sutProvider.GetDependency<IPolicyRepository>(),
-            sutProvider.GetDependency<IPolicyService>(),
-            sutProvider.GetDependency<IFido2>(),
-            sutProvider.GetDependency<ICurrentContext>(),
-            sutProvider.GetDependency<IGlobalSettings>(),
-            sutProvider.GetDependency<IAcceptOrgUserCommand>(),
-            sutProvider.GetDependency<IProviderUserRepository>(),
-            sutProvider.GetDependency<IStripeSyncService>(),
-            new FakeDataProtectorTokenFactory<OrgUserInviteTokenable>(),
-            sutProvider.GetDependency<IFeatureService>(),
-            sutProvider.GetDependency<IPremiumUserBillingService>(),
-            sutProvider.GetDependency<IRemoveOrganizationUserCommand>(),
-            sutProvider.GetDependency<IRevokeNonCompliantOrganizationUserCommand>(),
-            sutProvider.GetDependency<ITwoFactorIsEnabledQuery>(),
-            sutProvider.GetDependency<IDistributedCache>(),
-            sutProvider.GetDependency<IPolicyRequirementQuery>()
-            );
-
-        var actualIsVerified = await sut.VerifySecretAsync(user, secret);
+        var actualIsVerified = await sutProvider.Sut.VerifySecretAsync(user, secret);
 
         Assert.Equal(expectedIsVerified, actualIsVerified);
 
-        await tokenProvider
+        await sutProvider.GetDependency<IUserTwoFactorTokenProvider<User>>()
             .Received(shouldCheck.HasFlag(ShouldCheck.OTP) ? 1 : 0)
             .ValidateAsync(Arg.Any<string>(), secret, Arg.Any<UserManager<User>>(), user);
 
@@ -661,26 +608,25 @@ public class UserServiceTests
     }
 
     [Theory, BitAutoData]
-    public async Task ResendNewDeviceVerificationEmail_SendsToken_Success(
-        SutProvider<UserService> sutProvider, User user)
+    public async Task ResendNewDeviceVerificationEmail_SendsToken_Success(User user)
     {
         // Arrange
         var testPassword = "test_password";
-        var tokenProvider = SetupFakeTokenProvider(sutProvider, user);
         SetupUserAndDevice(user, true);
 
+        var sutProvider = new SutProvider<UserService>()
+            .CreateWithUserServiceCustomizations(user);
+
         // Setup the fake password verification
-        var substitutedUserPasswordStore = Substitute.For<IUserPasswordStore<User>>();
-        substitutedUserPasswordStore
+        sutProvider
+            .GetDependency<IUserPasswordStore<User>>()
             .GetPasswordHashAsync(user, Arg.Any<CancellationToken>())
             .Returns((ci) =>
             {
                 return Task.FromResult("hashed_test_password");
             });
 
-        sutProvider.SetDependency<IUserStore<User>>(substitutedUserPasswordStore, "store");
-
-        sutProvider.GetDependency<IPasswordHasher<User>>("passwordHasher")
+        sutProvider.GetDependency<IPasswordHasher<User>>()
             .VerifyHashedPassword(user, "hashed_test_password", testPassword)
             .Returns((ci) =>
             {
@@ -695,10 +641,7 @@ public class UserServiceTests
         context.DeviceType = DeviceType.Android;
         context.IpAddress = "1.1.1.1";
 
-        // HACK: SutProvider is being weird about not injecting the IPasswordHasher that I configured
-        var sut = RebuildSut(sutProvider);
-
-        await sut.ResendNewDeviceVerificationEmail(user.Email, testPassword);
+        await sutProvider.Sut.ResendNewDeviceVerificationEmail(user.Email, testPassword);
 
         await sutProvider.GetDependency<IMailService>()
             .Received(1)
@@ -842,8 +785,15 @@ public class UserServiceTests
             user.MasterPassword = null;
         }
     }
+}
 
-    private static IUserTwoFactorTokenProvider<User> SetupFakeTokenProvider(SutProvider<UserService> sutProvider, User user)
+public static class UserServiceSutProviderExtensions
+{
+    /// <summary>
+    /// Arranges a fake token provider. Must call as part of a builder pattern that ends in Create(), as it modifies
+    /// the SutProvider build chain.
+    /// </summary>
+    private static SutProvider<UserService> SetFakeTokenProvider(this SutProvider<UserService> sutProvider, User user)
     {
         var fakeUserTwoFactorProvider = Substitute.For<IUserTwoFactorTokenProvider<User>>();
 
@@ -859,8 +809,11 @@ public class UserServiceTests
             .ValidateAsync(Arg.Any<string>(), "otp_token", Arg.Any<UserManager<User>>(), user)
             .Returns(true);
 
-        sutProvider.GetDependency<IOptions<IdentityOptions>>()
-            .Value.Returns(new IdentityOptions
+        var fakeIdentityOptions = Substitute.For<IOptions<IdentityOptions>>();
+
+        fakeIdentityOptions
+            .Value
+            .Returns(new IdentityOptions
             {
                 Tokens = new TokenOptions
                 {
@@ -874,54 +827,54 @@ public class UserServiceTests
                 }
             });
 
-        // The above arranging of dependencies is used in the constructor of UserManager
-        // ref: https://github.com/dotnet/aspnetcore/blob/bfeb3bf9005c36b081d1e48725531ee0e15a9dfb/src/Identity/Extensions.Core/src/UserManager.cs#L103-L120
-        // since the constructor of the Sut has ran already (when injected) I need to recreate it to get it to run again
-        sutProvider.Create();
+        sutProvider.SetDependency(fakeIdentityOptions);
+        // Also set the fake provider dependency so that we can retrieve it easily via GetDependency
+        sutProvider.SetDependency(fakeUserTwoFactorProvider);
 
-        return fakeUserTwoFactorProvider;
+        return sutProvider;
     }
 
-    private IUserService RebuildSut(SutProvider<UserService> sutProvider)
+    /// <summary>
+    /// Properly registers IUserPasswordStore as IUserStore so it's injected when the sut is initialized.
+    /// </summary>
+    /// <param name="sutProvider"></param>
+    /// <returns></returns>
+    private static SutProvider<UserService> SetUserPasswordStore(this SutProvider<UserService> sutProvider)
     {
-        return new UserService(
-            sutProvider.GetDependency<IUserRepository>(),
-            sutProvider.GetDependency<ICipherRepository>(),
-            sutProvider.GetDependency<IOrganizationUserRepository>(),
-            sutProvider.GetDependency<IOrganizationRepository>(),
-            sutProvider.GetDependency<IOrganizationDomainRepository>(),
-            sutProvider.GetDependency<IMailService>(),
-            sutProvider.GetDependency<IPushNotificationService>(),
-            sutProvider.GetDependency<IUserStore<User>>(),
-            sutProvider.GetDependency<IOptions<IdentityOptions>>(),
-            sutProvider.GetDependency<IPasswordHasher<User>>(),
-            sutProvider.GetDependency<IEnumerable<IUserValidator<User>>>(),
-            sutProvider.GetDependency<IEnumerable<IPasswordValidator<User>>>(),
-            sutProvider.GetDependency<ILookupNormalizer>(),
-            sutProvider.GetDependency<IdentityErrorDescriber>(),
-            sutProvider.GetDependency<IServiceProvider>(),
-            sutProvider.GetDependency<ILogger<UserManager<User>>>(),
-            sutProvider.GetDependency<ILicensingService>(),
-            sutProvider.GetDependency<IEventService>(),
-            sutProvider.GetDependency<IApplicationCacheService>(),
-            sutProvider.GetDependency<IDataProtectionProvider>(),
-            sutProvider.GetDependency<IPaymentService>(),
-            sutProvider.GetDependency<IPolicyRepository>(),
-            sutProvider.GetDependency<IPolicyService>(),
-            sutProvider.GetDependency<IFido2>(),
-            sutProvider.GetDependency<ICurrentContext>(),
-            sutProvider.GetDependency<IGlobalSettings>(),
-            sutProvider.GetDependency<IAcceptOrgUserCommand>(),
-            sutProvider.GetDependency<IProviderUserRepository>(),
-            sutProvider.GetDependency<IStripeSyncService>(),
-            new FakeDataProtectorTokenFactory<OrgUserInviteTokenable>(),
-            sutProvider.GetDependency<IFeatureService>(),
-            sutProvider.GetDependency<IPremiumUserBillingService>(),
-            sutProvider.GetDependency<IRemoveOrganizationUserCommand>(),
-            sutProvider.GetDependency<IRevokeNonCompliantOrganizationUserCommand>(),
-            sutProvider.GetDependency<ITwoFactorIsEnabledQuery>(),
-            sutProvider.GetDependency<IDistributedCache>(),
-            sutProvider.GetDependency<IPolicyRequirementQuery>()
-            );
+        var substitutedUserPasswordStore = Substitute.For<IUserPasswordStore<User>>();
+
+        // IUserPasswordStore must be registered under the IUserStore parameter to be properly injected
+        // because this is what the constructor expects
+        sutProvider.SetDependency<IUserStore<User>>(substitutedUserPasswordStore);
+
+        // Also store it under its own type for retrieval and configuration
+        sutProvider.SetDependency(substitutedUserPasswordStore);
+
+        return sutProvider;
     }
+
+    /// <summary>
+    /// This is a hack: when autofixture initializes the sut in sutProvider, it overwrites the public
+    /// PasswordHasher property with a new substitute, so it loses the configured sutProvider mock.
+    /// This doesn't usually happen because our dependencies are not usually public.
+    /// Call this AFTER SutProvider.Create().
+    /// </summary>
+    private static SutProvider<UserService> FixPasswordHasherBug(this SutProvider<UserService> sutProvider)
+    {
+        // Get the configured sutProvider mock and assign it back to the public property in the base class
+        sutProvider.Sut.PasswordHasher = sutProvider.GetDependency<IPasswordHasher<User>>();
+        return sutProvider;
+    }
+
+    /// <summary>
+    /// A helper that combines all SutProvider configuration usually required for UserService.
+    /// Call this instead of SutProvider.Create, after any additional configuration your test needs.
+    /// </summary>
+    public static SutProvider<UserService> CreateWithUserServiceCustomizations(this SutProvider<UserService> sutProvider, User user)
+        => sutProvider
+            .SetUserPasswordStore()
+            .SetFakeTokenProvider(user)
+            .Create()
+            .FixPasswordHasherBug();
+
 }