From be72a77c72c932861d04bece5055eefd143df91c Mon Sep 17 00:00:00 2001 From: Brandon Treston Date: Wed, 11 Jun 2025 11:01:04 -0400 Subject: [PATCH 1/5] [PM-22099] add collection type (#5942) * add CollectionType enum * add CollectionType to CollectionResponseModels * cleanup * clean up * change types * add collection type to public API model * remove redundant statements --- src/Api/Models/Public/Response/CollectionResponseModel.cs | 6 ++++++ src/Api/Models/Response/CollectionResponseModel.cs | 3 +++ 2 files changed, 9 insertions(+) diff --git a/src/Api/Models/Public/Response/CollectionResponseModel.cs b/src/Api/Models/Public/Response/CollectionResponseModel.cs index 58968d4be7..d08db64290 100644 --- a/src/Api/Models/Public/Response/CollectionResponseModel.cs +++ b/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; } /// @@ -38,4 +40,8 @@ public class CollectionResponseModel : CollectionBaseModel, IResponseModel /// The associated groups that this collection is assigned to. /// public IEnumerable Groups { get; set; } + /// + /// The type of this collection + /// + public CollectionType Type { get; set; } } diff --git a/src/Api/Models/Response/CollectionResponseModel.cs b/src/Api/Models/Response/CollectionResponseModel.cs index d56ef5469a..5ce8310117 100644 --- a/src/Api/Models/Response/CollectionResponseModel.cs +++ b/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; } } /// From 821f66e99f822516351ccc8d8bdd01800db4699f Mon Sep 17 00:00:00 2001 From: Jimmy Vo Date: Wed, 11 Jun 2025 16:55:42 -0400 Subject: [PATCH 2/5] [PM-22205] Fix logic for sending out revoked email (#5933) --- .../TwoFactorAuthenticationPolicyValidator.cs | 4 +- ...actorAuthenticationPolicyValidatorTests.cs | 42 ++++++++++++++++--- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs index 13cc935eb9..5ce72df6c1 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs +++ b/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( diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs index 6a97f6bc1e..7b344d3b29 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs +++ b/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 sutProvider) { - policy.OrganizationId = organization.Id = policyUpdate.OrganizationId; + // Arrange + policy.OrganizationId = policyUpdate.OrganizationId; + organization.Id = policyUpdate.OrganizationId; + sutProvider.GetDependency().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() .GetManyDetailsByOrganizationAsync(policyUpdate.OrganizationId) - .Returns([orgUserDetailUserWithout2Fa]); + .Returns([nonCompliantUser, compliantUser]); sutProvider.GetDependency() .TwoFactorIsEnabledAsync(Arg.Any>()) .Returns(new List<(OrganizationUserUserDetails user, bool hasTwoFactor)>() { - (orgUserDetailUserWithout2Fa, false) + (nonCompliantUser, false), + (compliantUser, true) }); sutProvider.GetDependency() .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()) .Returns(new CommandResult()); + // Act await sutProvider.Sut.OnSaveSideEffectsAsync(policyUpdate, policy); + // Assert await sutProvider.GetDependency() .Received(1) .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()); + await sutProvider.GetDependency() + .Received(1) + .RevokeNonCompliantOrganizationUsersAsync(Arg.Is(req => + req.OrganizationId == policyUpdate.OrganizationId && + req.OrganizationUsers.SequenceEqual(new[] { nonCompliantUser }) + )); + await sutProvider.GetDependency() .Received(1) .SendOrganizationUserRevokedForTwoFactorPolicyEmailAsync(organization.DisplayName(), - "user3@test.com"); + nonCompliantUser.Email); + + // Did not send out an email for compliantUser + await sutProvider.GetDependency() + .Received(0) + .SendOrganizationUserRevokedForTwoFactorPolicyEmailAsync(organization.DisplayName(), + compliantUser.Email); } } From 6d36f636c45d6d4c89843ae600d0e0b8b552e417 Mon Sep 17 00:00:00 2001 From: SmithThe4th Date: Wed, 11 Jun 2025 17:46:21 -0400 Subject: [PATCH 3/5] Revert "replace fallback icon default with response (#5878)" (#5956) This reverts commit ed780d45d3f5e839bd1e390aecb2447ac33b006f. --- src/Icons/Controllers/IconsController.cs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/Icons/Controllers/IconsController.cs b/src/Icons/Controllers/IconsController.cs index b0d9306f94..871219b366 100644 --- a/src/Icons/Controllers/IconsController.cs +++ b/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); From 463dc1232d8097ac17b328259697c85c4a9fa038 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Thu, 12 Jun 2025 10:47:41 +1000 Subject: [PATCH 4/5] Add xmldoc for OrganizationUser (#5949) --- .../AdminConsole/Entities/OrganizationUser.cs | 55 ++++++++++++++++++- .../Enums/OrganizationUserStatusType.cs | 27 ++++++++- 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/src/Core/AdminConsole/Entities/OrganizationUser.cs b/src/Core/AdminConsole/Entities/OrganizationUser.cs index 9828482a7e..3166ebf3a8 100644 --- a/src/Core/AdminConsole/Entities/OrganizationUser.cs +++ b/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; +/// +/// An association table between one and one , representing that user's +/// membership in the organization. "Member" refers to the OrganizationUser object. +/// public class OrganizationUser : ITableObject, IExternal, IOrganizationUser { + /// + /// A unique random identifier. + /// public Guid Id { get; set; } + /// + /// The ID of the Organization that the user is a member of. + /// public Guid OrganizationId { get; set; } + /// + /// 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. + /// public Guid? UserId { get; set; } + /// + /// 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. + /// [MaxLength(256)] public string? Email { get; set; } + /// + /// 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. + /// public string? Key { get; set; } + /// + /// The User's symmetric key encrypted with the Organization's public key. NULL if the OrganizationUser + /// is not enrolled in account recovery. + /// public string? ResetPasswordKey { get; set; } + /// public OrganizationUserStatusType Status { get; set; } + /// + /// The User's role in the Organization. + /// public OrganizationUserType Type { get; set; } - + /// + /// An ID used to identify the OrganizationUser with an external directory service. Used by Directory Connector + /// and SCIM. + /// [MaxLength(300)] public string? ExternalId { get; set; } + /// + /// The date the OrganizationUser was created, i.e. when the User was first invited to the Organization. + /// public DateTime CreationDate { get; internal set; } = DateTime.UtcNow; + /// + /// The last date the OrganizationUser entry was updated. + /// public DateTime RevisionDate { get; internal set; } = DateTime.UtcNow; + /// + /// A json blob representing the of the OrganizationUser if they + /// are a Custom user role (i.e. the 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. + /// + /// + /// Avoid using this property directly - instead use the and + /// helper methods. + /// public string? Permissions { get; set; } + /// + /// True if the User has access to Secrets Manager for this Organization, false otherwise. + /// public bool AccessSecretsManager { get; set; } public void SetNewId() diff --git a/src/Core/AdminConsole/Enums/OrganizationUserStatusType.cs b/src/Core/AdminConsole/Enums/OrganizationUserStatusType.cs index 576e98ea74..3b4098715d 100644 --- a/src/Core/AdminConsole/Enums/OrganizationUserStatusType.cs +++ b/src/Core/AdminConsole/Enums/OrganizationUserStatusType.cs @@ -1,9 +1,34 @@ -namespace Bit.Core.Enums; +using Bit.Core.Entities; +namespace Bit.Core.Enums; + +/// +/// Represents the different stages of a member's lifecycle in an organization. +/// The object is populated differently depending on their Status. +/// public enum OrganizationUserStatusType : short { + /// + /// The OrganizationUser entry only represents an invitation to join the organization. It is not linked to a + /// specific User yet. + /// Invited = 0, + /// + /// The User has accepted the invitation and linked their User account to the OrganizationUser entry. + /// Accepted = 1, + /// + /// 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. + /// Confirmed = 2, + /// + /// The OrganizationUser has been revoked from the organization and cannot access organization data while in this state. + /// + /// + /// 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. + /// Revoked = -1, } From 64b288035caae240cda759a2a9f4b8aac6511fa4 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Thu, 12 Jun 2025 19:21:05 +1000 Subject: [PATCH 5/5] Chore: document SutProvider and clean up UserServiceTests (#5879) * UserServiceTests - use builder pattern for SutProvider to reduce boilerplate * SutProvider - add xmldoc --- .../Services/Implementations/UserService.cs | 19 +- test/Common/AutoFixture/SutProvider.cs | 97 ++++++-- .../Policies/SavePolicyCommandTests.cs | 2 +- test/Core.Test/Services/UserServiceTests.cs | 217 +++++++----------- 4 files changed, 169 insertions(+), 166 deletions(-) diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs index bf90190ee6..fe5a064c44 100644 --- a/src/Core/Services/Implementations/UserService.cs +++ b/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.Models; @@ -29,12 +28,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; @@ -44,12 +40,11 @@ using JsonSerializer = System.Text.Json.JsonSerializer; namespace Bit.Core.Services; -public class UserService : UserManager, IUserService, IDisposable +public class UserService : UserManager, 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; @@ -65,17 +60,14 @@ public class UserService : UserManager, 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 _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; @@ -83,7 +75,6 @@ public class UserService : UserManager, IUserService, IDisposable public UserService( IUserRepository userRepository, - ICipherRepository cipherRepository, IOrganizationUserRepository organizationUserRepository, IOrganizationRepository organizationRepository, IOrganizationDomainRepository organizationDomainRepository, @@ -101,7 +92,6 @@ public class UserService : UserManager, IUserService, IDisposable ILicensingService licenseService, IEventService eventService, IApplicationCacheService applicationCacheService, - IDataProtectionProvider dataProtectionProvider, IPaymentService paymentService, IPolicyRepository policyRepository, IPolicyService policyService, @@ -111,10 +101,8 @@ public class UserService : UserManager, IUserService, IDisposable IAcceptOrgUserCommand acceptOrgUserCommand, IProviderUserRepository providerUserRepository, IStripeSyncService stripeSyncService, - IDataProtectorTokenFactory orgUserInviteTokenDataFactory, IFeatureService featureService, IPremiumUserBillingService premiumUserBillingService, - IRemoveOrganizationUserCommand removeOrganizationUserCommand, IRevokeNonCompliantOrganizationUserCommand revokeNonCompliantOrganizationUserCommand, ITwoFactorIsEnabledQuery twoFactorIsEnabledQuery, IDistributedCache distributedCache, @@ -131,7 +119,6 @@ public class UserService : UserManager, IUserService, IDisposable logger) { _userRepository = userRepository; - _cipherRepository = cipherRepository; _organizationUserRepository = organizationUserRepository; _organizationRepository = organizationRepository; _organizationDomainRepository = organizationDomainRepository; @@ -147,18 +134,14 @@ public class UserService : UserManager, 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; diff --git a/test/Common/AutoFixture/SutProvider.cs b/test/Common/AutoFixture/SutProvider.cs index 4b6f268ac3..bdab622754 100644 --- a/test/Common/AutoFixture/SutProvider.cs +++ b/test/Common/AutoFixture/SutProvider.cs @@ -4,8 +4,19 @@ using AutoFixture.Kernel; namespace Bit.Test.Common.AutoFixture; +/// +/// 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. +/// +/// The concrete implementation of the class being tested. public class SutProvider : ISutProvider { + /// + /// 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. + /// private Dictionary> _dependencies; private readonly IFixture _fixture; private readonly ConstructorParameterRelay _constructorParameterRelay; @@ -23,9 +34,21 @@ public class SutProvider : ISutProvider _fixture.Customizations.Add(_constructorParameterRelay); } + /// + /// Registers a dependency to be injected when the sut is created. You must call after + /// this method to (re)create the sut with the dependency. + /// + /// The dependency to register. + /// An optional parameter name to disambiguate the dependency if there are multiple of the same type. You generally don't need this. + /// The type to register the dependency under - usually an interface. This should match the type expected by the sut's constructor. + /// public SutProvider SetDependency(T dependency, string parameterName = "") => SetDependency(typeof(T), dependency, parameterName); - public SutProvider SetDependency(Type dependencyType, object dependency, string parameterName = "") + + /// + /// An overload for which takes a runtime object rather than a compile-time type. + /// + private SutProvider SetDependency(Type dependencyType, object dependency, string parameterName = "") { if (_dependencies.TryGetValue(dependencyType, out var dependencyForType)) { @@ -39,45 +62,69 @@ public class SutProvider : ISutProvider return this; } + /// + /// Gets a dependency of the sut. Can only be called after the dependency has been set, either explicitly with + /// or automatically with . + /// 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. + /// + /// An optional parameter name to disambiguate the dependency if there are multiple of the same type. You generally don't need this. + /// The type of the dependency you want to get - usually an interface. + /// The dependency. public T GetDependency(string parameterName = "") => (T)GetDependency(typeof(T), parameterName); - public object GetDependency(Type dependencyType, string parameterName = "") + + /// + /// An overload for which takes a runtime object rather than a compile-time type. + /// + 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."); + + throw new ArgumentException(string.Concat($"Dependency of type {dependencyType.Name} and name ", + $"{parameterName} does not exist. Available dependency names are: ", + string.Join(", ", knownDependencies.Keys))); } + + throw new ArgumentException($"Dependency of type {dependencyType.Name} and name {parameterName} has not been set."); } + /// + /// Clear all the dependencies and the sut. This reverts the SutProvider back to a fully uninitialized state. + /// public void Reset() { _dependencies = new Dictionary>(); Sut = default; } + /// + /// Recreate a new sut with all new dependencies. This will reset all dependencies, including mocked return values + /// and any dependencies set with . + /// public void Recreate() { _dependencies = new Dictionary>(); Sut = _fixture.Create(); } + /// > ISutProvider ISutProvider.Create() => Create(); + + /// + /// Creates the sut, injecting any dependencies configured via and falling back to + /// NSubstitute mocks for any dependencies that have not been explicitly configured. + /// + /// public SutProvider Create() { Sut = _fixture.Create(); @@ -89,6 +136,19 @@ public class SutProvider : ISutProvider private object GetDefault(Type type) => type.IsValueType ? Activator.CreateInstance(type) : null; + /// + /// A specimen builder which tells Autofixture to use the dependency registered in + /// when creating test data. If no matching dependency exists in , it creates + /// an NSubstitute mock and registers it using + /// so it can be retrieved later. + /// This is the link between and Autofixture. + /// + /// + /// 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 and register it with the in + /// to provide that instruction. + /// + /// The type of the sut. private class ConstructorParameterRelay : ISpecimenBuilder { private readonly SutProvider _sutProvider; @@ -102,6 +162,7 @@ public class SutProvider : 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 : 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, but no overload for // Create(Type type) exists. var dependency = new SpecimenContext(_fixture).Resolve(new SeededRequest(parameterInfo.ParameterType, diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/SavePolicyCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/SavePolicyCommandTests.cs index 3ca7004e70..426389f33c 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/SavePolicyCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/SavePolicyCommandTests.cs @@ -288,7 +288,7 @@ public class SavePolicyCommandTests { return new SutProvider() .WithFakeTimeProvider() - .SetDependency(typeof(IEnumerable), policyValidators ?? []) + .SetDependency(policyValidators ?? []) .Create(); } diff --git a/test/Core.Test/Services/UserServiceTests.cs b/test/Core.Test/Services/UserServiceTests.cs index 9252d28588..0589753dd7 100644 --- a/test/Core.Test/Services/UserServiceTests.cs +++ b/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 sutProvider, User user) + public async Task SendNewDeviceVerificationEmailAsync_DeviceMatches(DeviceType deviceType, string deviceTypeName, + User user) { - SetupFakeTokenProvider(sutProvider, user); + var sutProvider = new SutProvider() + .CreateWithUserServiceCustomizations(user); + var context = sutProvider.GetDependency(); context.DeviceType = deviceType; context.IpAddress = "1.1.1.1"; @@ -194,9 +187,11 @@ public class UserServiceTests } [Theory, BitAutoData] - public async Task SendNewDeviceVerificationEmailAsync_NullDeviceTypeShouldSendUnkownBrowserType(SutProvider sutProvider, User user) + public async Task SendNewDeviceVerificationEmailAsync_NullDeviceTypeShouldSendUnkownBrowserType(User user) { - SetupFakeTokenProvider(sutProvider, user); + var sutProvider = new SutProvider() + .CreateWithUserServiceCustomizations(user); + var context = sutProvider.GetDependency(); 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 sutProvider, User user) // AutoFixture injected data + User user) // AutoFixture injected data { // Arrange - var tokenProvider = SetupFakeTokenProvider(sutProvider, user); SetupUserAndDevice(user, shouldHavePassword); + var sutProvider = new SutProvider() + .CreateWithUserServiceCustomizations(user); + // Setup the fake password verification - var substitutedUserPasswordStore = Substitute.For>(); - substitutedUserPasswordStore + sutProvider.GetDependency>() .GetPasswordHashAsync(user, Arg.Any()) - .Returns((ci) => - { - return Task.FromResult("hashed_test_password"); - }); + .Returns(Task.FromResult("hashed_test_password")); - sutProvider.SetDependency>(substitutedUserPasswordStore, "store"); - - sutProvider.GetDependency>("passwordHasher") + sutProvider.GetDependency>() .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(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency>(), - sutProvider.GetDependency>(), - sutProvider.GetDependency>(), - sutProvider.GetDependency>>(), - sutProvider.GetDependency>>(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency>>(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - new FakeDataProtectorTokenFactory(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency() - ); - - var actualIsVerified = await sut.VerifySecretAsync(user, secret); + var actualIsVerified = await sutProvider.Sut.VerifySecretAsync(user, secret); Assert.Equal(expectedIsVerified, actualIsVerified); - await tokenProvider + await sutProvider.GetDependency>() .Received(shouldCheck.HasFlag(ShouldCheck.OTP) ? 1 : 0) .ValidateAsync(Arg.Any(), secret, Arg.Any>(), user); @@ -661,26 +608,25 @@ public class UserServiceTests } [Theory, BitAutoData] - public async Task ResendNewDeviceVerificationEmail_SendsToken_Success( - SutProvider 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() + .CreateWithUserServiceCustomizations(user); + // Setup the fake password verification - var substitutedUserPasswordStore = Substitute.For>(); - substitutedUserPasswordStore + sutProvider + .GetDependency>() .GetPasswordHashAsync(user, Arg.Any()) .Returns((ci) => { return Task.FromResult("hashed_test_password"); }); - sutProvider.SetDependency>(substitutedUserPasswordStore, "store"); - - sutProvider.GetDependency>("passwordHasher") + sutProvider.GetDependency>() .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() .Received(1) @@ -842,8 +785,15 @@ public class UserServiceTests user.MasterPassword = null; } } +} - private static IUserTwoFactorTokenProvider SetupFakeTokenProvider(SutProvider sutProvider, User user) +public static class UserServiceSutProviderExtensions +{ + /// + /// Arranges a fake token provider. Must call as part of a builder pattern that ends in Create(), as it modifies + /// the SutProvider build chain. + /// + private static SutProvider SetFakeTokenProvider(this SutProvider sutProvider, User user) { var fakeUserTwoFactorProvider = Substitute.For>(); @@ -859,8 +809,11 @@ public class UserServiceTests .ValidateAsync(Arg.Any(), "otp_token", Arg.Any>(), user) .Returns(true); - sutProvider.GetDependency>() - .Value.Returns(new IdentityOptions + var fakeIdentityOptions = Substitute.For>(); + + 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 sutProvider) + /// + /// Properly registers IUserPasswordStore as IUserStore so it's injected when the sut is initialized. + /// + /// + /// + private static SutProvider SetUserPasswordStore(this SutProvider sutProvider) { - return new UserService( - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency>(), - sutProvider.GetDependency>(), - sutProvider.GetDependency>(), - sutProvider.GetDependency>>(), - sutProvider.GetDependency>>(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency>>(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - new FakeDataProtectorTokenFactory(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency(), - sutProvider.GetDependency() - ); + var substitutedUserPasswordStore = Substitute.For>(); + + // IUserPasswordStore must be registered under the IUserStore parameter to be properly injected + // because this is what the constructor expects + sutProvider.SetDependency>(substitutedUserPasswordStore); + + // Also store it under its own type for retrieval and configuration + sutProvider.SetDependency(substitutedUserPasswordStore); + + return sutProvider; } + + /// + /// 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(). + /// + private static SutProvider FixPasswordHasherBug(this SutProvider sutProvider) + { + // Get the configured sutProvider mock and assign it back to the public property in the base class + sutProvider.Sut.PasswordHasher = sutProvider.GetDependency>(); + return sutProvider; + } + + /// + /// A helper that combines all SutProvider configuration usually required for UserService. + /// Call this instead of SutProvider.Create, after any additional configuration your test needs. + /// + public static SutProvider CreateWithUserServiceCustomizations(this SutProvider sutProvider, User user) + => sutProvider + .SetUserPasswordStore() + .SetFakeTokenProvider(user) + .Create() + .FixPasswordHasherBug(); + }