From d29755de5a6776670977bcccf6cae53e89390207 Mon Sep 17 00:00:00 2001 From: Vincent Salucci <26154748+vincentsalucci@users.noreply.github.com> Date: Thu, 8 Feb 2024 07:44:36 -0600 Subject: [PATCH] [AC-1880] Public API - Deprecated properties (#3706) * feat: remove required for AccessAll and add xmldoc for usage restrictions, refs AC-1880 * feat: add validation for create group workflow wrt manage property, refs AC-1880 * feat: add validation for update group workflow wrt manage property, refs AC-1880 * feat: add validation for create and update member workflow wrt manage property, refs AC-1880 * feat: add validation for update collection workflow wrt manage property, refs AC-1880 * fix: flaky Public/GroupsControllerTests + more test coverage, refs AC-1880 --- .../Controllers/GroupsController.cs | 4 +- .../OrganizationUsersController.cs | 2 +- .../Public/Controllers/GroupsController.cs | 4 +- .../Public/Controllers/MembersController.cs | 12 +- .../AssociationWithPermissionsBaseModel.cs | 2 +- .../Public/Models/GroupBaseModel.cs | 4 +- .../Public/Models/MemberBaseModel.cs | 7 +- .../Controllers/CollectionsController.cs | 2 +- .../Groups/CreateGroupCommand.cs | 23 ++-- .../Groups/Interfaces/ICreateGroupCommand.cs | 4 +- .../Groups/Interfaces/IUpdateGroupCommand.cs | 4 +- .../Groups/UpdateGroupCommand.cs | 23 ++-- .../Services/IOrganizationService.cs | 4 +- .../Implementations/OrganizationService.cs | 25 +++- .../Implementations/CollectionService.cs | 24 ++-- .../Controllers/GroupsControllerTests.cs | 4 +- .../Controllers/GroupsControllerTests.cs | 119 +++++++++++++++++- .../Services/OrganizationServiceTests.cs | 22 ++-- 18 files changed, 221 insertions(+), 68 deletions(-) diff --git a/src/Api/AdminConsole/Controllers/GroupsController.cs b/src/Api/AdminConsole/Controllers/GroupsController.cs index 9fa392af78..026e0f03f9 100644 --- a/src/Api/AdminConsole/Controllers/GroupsController.cs +++ b/src/Api/AdminConsole/Controllers/GroupsController.cs @@ -125,7 +125,7 @@ public class GroupsController : Controller var organization = await _organizationRepository.GetByIdAsync(orgIdGuid); var group = model.ToGroup(orgIdGuid); - await _createGroupCommand.CreateGroupAsync(group, organization, model.Collections?.Select(c => c.ToSelectionReadOnly()), model.Users); + await _createGroupCommand.CreateGroupAsync(group, organization, model.Collections?.Select(c => c.ToSelectionReadOnly()).ToList(), model.Users); return new GroupResponseModel(group); } @@ -143,7 +143,7 @@ public class GroupsController : Controller var orgIdGuid = new Guid(orgId); var organization = await _organizationRepository.GetByIdAsync(orgIdGuid); - await _updateGroupCommand.UpdateGroupAsync(model.ToGroup(group), organization, model.Collections?.Select(c => c.ToSelectionReadOnly()), model.Users); + await _updateGroupCommand.UpdateGroupAsync(model.ToGroup(group), organization, model.Collections?.Select(c => c.ToSelectionReadOnly()).ToList(), model.Users); return new GroupResponseModel(group); } diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index 1eacab68b8..a6ef6e7b9f 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -308,7 +308,7 @@ public class OrganizationUsersController : Controller var userId = _userService.GetProperUserId(User); await _organizationService.SaveUserAsync(model.ToOrganizationUser(organizationUser), userId.Value, - model.Collections?.Select(c => c.ToSelectionReadOnly()), model.Groups); + model.Collections?.Select(c => c.ToSelectionReadOnly()).ToList(), model.Groups); } [HttpPut("{id}/groups")] diff --git a/src/Api/AdminConsole/Public/Controllers/GroupsController.cs b/src/Api/AdminConsole/Public/Controllers/GroupsController.cs index 6ced361771..373cf78e36 100644 --- a/src/Api/AdminConsole/Public/Controllers/GroupsController.cs +++ b/src/Api/AdminConsole/Public/Controllers/GroupsController.cs @@ -111,7 +111,7 @@ public class GroupsController : Controller { var group = model.ToGroup(_currentContext.OrganizationId.Value); var organization = await _organizationRepository.GetByIdAsync(_currentContext.OrganizationId.Value); - var associations = model.Collections?.Select(c => c.ToCollectionAccessSelection(organization.FlexibleCollections)); + var associations = model.Collections?.Select(c => c.ToCollectionAccessSelection(organization.FlexibleCollections)).ToList(); await _createGroupCommand.CreateGroupAsync(group, organization, associations); var response = new GroupResponseModel(group, associations); return new JsonResult(response); @@ -140,7 +140,7 @@ public class GroupsController : Controller var updatedGroup = model.ToGroup(existingGroup); var organization = await _organizationRepository.GetByIdAsync(_currentContext.OrganizationId.Value); - var associations = model.Collections?.Select(c => c.ToCollectionAccessSelection(organization.FlexibleCollections)); + var associations = model.Collections?.Select(c => c.ToCollectionAccessSelection(organization.FlexibleCollections)).ToList(); await _updateGroupCommand.UpdateGroupAsync(updatedGroup, organization, associations); var response = new GroupResponseModel(updatedGroup, associations); return new JsonResult(response); diff --git a/src/Api/AdminConsole/Public/Controllers/MembersController.cs b/src/Api/AdminConsole/Public/Controllers/MembersController.cs index 754c3130d0..f2bffb5189 100644 --- a/src/Api/AdminConsole/Public/Controllers/MembersController.cs +++ b/src/Api/AdminConsole/Public/Controllers/MembersController.cs @@ -5,7 +5,6 @@ using Bit.Api.Models.Public.Response; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.AdminConsole.Repositories; using Bit.Core.Context; -using Bit.Core.Models.Business; using Bit.Core.Repositories; using Bit.Core.Services; using Microsoft.AspNetCore.Authorization; @@ -123,14 +122,7 @@ public class MembersController : Controller public async Task Post([FromBody] MemberCreateRequestModel model) { var organizationAbility = await _applicationCacheService.GetOrganizationAbilityAsync(_currentContext.OrganizationId.Value); - var associations = model.Collections?.Select(c => c.ToCollectionAccessSelection(organizationAbility?.FlexibleCollections ?? false)); - var invite = new OrganizationUserInvite - { - Emails = new List { model.Email }, - Type = model.Type.Value, - AccessAll = model.AccessAll.Value, - Collections = associations - }; + var associations = model.Collections?.Select(c => c.ToCollectionAccessSelection(organizationAbility?.FlexibleCollections ?? false)).ToList(); var user = await _organizationService.InviteUserAsync(_currentContext.OrganizationId.Value, null, model.Email, model.Type.Value, model.AccessAll.Value, model.ExternalId, associations, model.Groups); var response = new MemberResponseModel(user, associations); @@ -159,7 +151,7 @@ public class MembersController : Controller } var updatedUser = model.ToOrganizationUser(existingUser); var organizationAbility = await _applicationCacheService.GetOrganizationAbilityAsync(_currentContext.OrganizationId.Value); - var associations = model.Collections?.Select(c => c.ToCollectionAccessSelection(organizationAbility?.FlexibleCollections ?? false)); + var associations = model.Collections?.Select(c => c.ToCollectionAccessSelection(organizationAbility?.FlexibleCollections ?? false)).ToList(); await _organizationService.SaveUserAsync(updatedUser, null, associations, model.Groups); MemberResponseModel response = null; if (existingUser.UserId.HasValue) diff --git a/src/Api/AdminConsole/Public/Models/AssociationWithPermissionsBaseModel.cs b/src/Api/AdminConsole/Public/Models/AssociationWithPermissionsBaseModel.cs index 72bbe87b92..8993118924 100644 --- a/src/Api/AdminConsole/Public/Models/AssociationWithPermissionsBaseModel.cs +++ b/src/Api/AdminConsole/Public/Models/AssociationWithPermissionsBaseModel.cs @@ -22,7 +22,7 @@ public abstract class AssociationWithPermissionsBaseModel public bool? HidePasswords { get; set; } /// /// When true, the manage permission allows a user to both edit the ciphers within a collection and edit the users/groups that are assigned to the collection. - /// This field will not affect behavior until the Flexible Collections functionality is released in Q1, 2024. + /// This field will not affect behavior until your organization is using the latest collection enhancements (Releasing Q1, 2024) /// public bool? Manage { get; set; } } diff --git a/src/Api/AdminConsole/Public/Models/GroupBaseModel.cs b/src/Api/AdminConsole/Public/Models/GroupBaseModel.cs index ff1813aff4..e5fa378f8c 100644 --- a/src/Api/AdminConsole/Public/Models/GroupBaseModel.cs +++ b/src/Api/AdminConsole/Public/Models/GroupBaseModel.cs @@ -13,9 +13,9 @@ public abstract class GroupBaseModel public string Name { get; set; } /// /// Determines if this group can access all collections within the organization, or only the associated - /// collections. If set to true, this option overrides any collection assignments. + /// collections. If set to true, this option overrides any collection assignments. If your organization is using + /// the latest collection enhancements, you will not be allowed to set this property to true. /// - [Required] public bool? AccessAll { get; set; } /// /// External identifier for reference or linking this group to another system, such as a user directory. diff --git a/src/Api/AdminConsole/Public/Models/MemberBaseModel.cs b/src/Api/AdminConsole/Public/Models/MemberBaseModel.cs index d7eaf98509..69bb0dc6f3 100644 --- a/src/Api/AdminConsole/Public/Models/MemberBaseModel.cs +++ b/src/Api/AdminConsole/Public/Models/MemberBaseModel.cs @@ -36,15 +36,16 @@ public abstract class MemberBaseModel } /// - /// The member's type (or role) within the organization. + /// The member's type (or role) within the organization. If your organization has is using the latest collection enhancements, + /// you will not be allowed to assign the Manager role (OrganizationUserType = 3). /// [Required] public OrganizationUserType? Type { get; set; } /// /// Determines if this member can access all collections within the organization, or only the associated - /// collections. If set to true, this option overrides any collection assignments. + /// collections. If set to true, this option overrides any collection assignments. If your organization is using + /// the latest collection enhancements, you will not be allowed to set this property to true. /// - [Required] public bool? AccessAll { get; set; } /// /// External identifier for reference or linking this member to another system, such as a user directory. diff --git a/src/Api/Public/Controllers/CollectionsController.cs b/src/Api/Public/Controllers/CollectionsController.cs index ecd84aa728..6ff9ada981 100644 --- a/src/Api/Public/Controllers/CollectionsController.cs +++ b/src/Api/Public/Controllers/CollectionsController.cs @@ -93,7 +93,7 @@ public class CollectionsController : Controller } var updatedCollection = model.ToCollection(existingCollection); var organizationAbility = await _applicationCacheService.GetOrganizationAbilityAsync(_currentContext.OrganizationId.Value); - var associations = model.Groups?.Select(c => c.ToCollectionAccessSelection(organizationAbility?.FlexibleCollections ?? false)); + var associations = model.Groups?.Select(c => c.ToCollectionAccessSelection(organizationAbility?.FlexibleCollections ?? false)).ToList(); await _collectionService.SaveAsync(updatedCollection, associations); var response = new CollectionResponseModel(updatedCollection, associations); return new JsonResult(response); diff --git a/src/Core/AdminConsole/OrganizationFeatures/Groups/CreateGroupCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/Groups/CreateGroupCommand.cs index 61321b2df2..af95f9fb36 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Groups/CreateGroupCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Groups/CreateGroupCommand.cs @@ -36,10 +36,10 @@ public class CreateGroupCommand : ICreateGroupCommand } public async Task CreateGroupAsync(Group group, Organization organization, - IEnumerable collections = null, + ICollection collections = null, IEnumerable users = null) { - Validate(organization, group); + Validate(organization, group, collections); await GroupRepositoryCreateGroupAsync(group, organization, collections); if (users != null) @@ -51,10 +51,10 @@ public class CreateGroupCommand : ICreateGroupCommand } public async Task CreateGroupAsync(Group group, Organization organization, EventSystemUser systemUser, - IEnumerable collections = null, + ICollection collections = null, IEnumerable users = null) { - Validate(organization, group); + Validate(organization, group, collections); await GroupRepositoryCreateGroupAsync(group, organization, collections); if (users != null) @@ -103,7 +103,7 @@ public class CreateGroupCommand : ICreateGroupCommand } } - private static void Validate(Organization organization, Group group) + private static void Validate(Organization organization, Group group, IEnumerable collections) { if (organization == null) { @@ -115,9 +115,18 @@ public class CreateGroupCommand : ICreateGroupCommand throw new BadRequestException("This organization cannot use groups."); } - if (organization.FlexibleCollections && group.AccessAll) + if (organization.FlexibleCollections) { - throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the group to collections instead."); + if (group.AccessAll) + { + throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the group to collections instead."); + } + + var invalidAssociations = collections?.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); + if (invalidAssociations?.Any() ?? false) + { + throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); + } } } } diff --git a/src/Core/AdminConsole/OrganizationFeatures/Groups/Interfaces/ICreateGroupCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/Groups/Interfaces/ICreateGroupCommand.cs index 428bd13462..b3ad06d9dc 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Groups/Interfaces/ICreateGroupCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Groups/Interfaces/ICreateGroupCommand.cs @@ -7,10 +7,10 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces; public interface ICreateGroupCommand { Task CreateGroupAsync(Group group, Organization organization, - IEnumerable collections = null, + ICollection collections = null, IEnumerable users = null); Task CreateGroupAsync(Group group, Organization organization, EventSystemUser systemUser, - IEnumerable collections = null, + ICollection collections = null, IEnumerable users = null); } diff --git a/src/Core/AdminConsole/OrganizationFeatures/Groups/Interfaces/IUpdateGroupCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/Groups/Interfaces/IUpdateGroupCommand.cs index fae217aad1..1cae4805f2 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Groups/Interfaces/IUpdateGroupCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Groups/Interfaces/IUpdateGroupCommand.cs @@ -7,10 +7,10 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces; public interface IUpdateGroupCommand { Task UpdateGroupAsync(Group group, Organization organization, - IEnumerable collections = null, + ICollection collections = null, IEnumerable users = null); Task UpdateGroupAsync(Group group, Organization organization, EventSystemUser systemUser, - IEnumerable collections = null, + ICollection collections = null, IEnumerable users = null); } diff --git a/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs index fecc06be8a..ba2aa2b8bd 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs @@ -26,10 +26,10 @@ public class UpdateGroupCommand : IUpdateGroupCommand } public async Task UpdateGroupAsync(Group group, Organization organization, - IEnumerable collections = null, + ICollection collections = null, IEnumerable userIds = null) { - Validate(organization, group); + Validate(organization, group, collections); await GroupRepositoryUpdateGroupAsync(group, collections); if (userIds != null) @@ -41,10 +41,10 @@ public class UpdateGroupCommand : IUpdateGroupCommand } public async Task UpdateGroupAsync(Group group, Organization organization, EventSystemUser systemUser, - IEnumerable collections = null, + ICollection collections = null, IEnumerable userIds = null) { - Validate(organization, group); + Validate(organization, group, collections); await GroupRepositoryUpdateGroupAsync(group, collections); if (userIds != null) @@ -97,7 +97,7 @@ public class UpdateGroupCommand : IUpdateGroupCommand } } - private static void Validate(Organization organization, Group group) + private static void Validate(Organization organization, Group group, IEnumerable collections) { if (organization == null) { @@ -109,9 +109,18 @@ public class UpdateGroupCommand : IUpdateGroupCommand throw new BadRequestException("This organization cannot use groups."); } - if (organization.FlexibleCollections && group.AccessAll) + if (organization.FlexibleCollections) { - throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the group to collections instead."); + if (group.AccessAll) + { + throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the group to collections instead."); + } + + var invalidAssociations = collections?.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); + if (invalidAssociations?.Any() ?? false) + { + throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); + } } } } diff --git a/src/Core/AdminConsole/Services/IOrganizationService.cs b/src/Core/AdminConsole/Services/IOrganizationService.cs index e03ac4936f..768edea9a3 100644 --- a/src/Core/AdminConsole/Services/IOrganizationService.cs +++ b/src/Core/AdminConsole/Services/IOrganizationService.cs @@ -36,7 +36,7 @@ public interface IOrganizationService Task> InviteUsersAsync(Guid organizationId, EventSystemUser systemUser, IEnumerable<(OrganizationUserInvite invite, string externalId)> invites); Task InviteUserAsync(Guid organizationId, Guid? invitingUserId, string email, - OrganizationUserType type, bool accessAll, string externalId, IEnumerable collections, IEnumerable groups); + OrganizationUserType type, bool accessAll, string externalId, ICollection collections, IEnumerable groups); Task InviteUserAsync(Guid organizationId, EventSystemUser systemUser, string email, OrganizationUserType type, bool accessAll, string externalId, IEnumerable collections, IEnumerable groups); Task>> ResendInvitesAsync(Guid organizationId, Guid? invitingUserId, IEnumerable organizationUsersId); @@ -45,7 +45,7 @@ public interface IOrganizationService Guid confirmingUserId, IUserService userService); Task>> ConfirmUsersAsync(Guid organizationId, Dictionary keys, Guid confirmingUserId, IUserService userService); - Task SaveUserAsync(OrganizationUser user, Guid? savingUserId, IEnumerable collections, IEnumerable groups); + Task SaveUserAsync(OrganizationUser user, Guid? savingUserId, ICollection collections, IEnumerable groups); [Obsolete("IDeleteOrganizationUserCommand should be used instead. To be removed by EC-607.")] Task DeleteUserAsync(Guid organizationId, Guid organizationUserId, Guid? deletingUserId); [Obsolete("IDeleteOrganizationUserCommand should be used instead. To be removed by EC-607.")] diff --git a/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs b/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs index fcd6c78e38..128486f8b0 100644 --- a/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs +++ b/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs @@ -1370,7 +1370,7 @@ public class OrganizationService : IOrganizationService } public async Task SaveUserAsync(OrganizationUser user, Guid? savingUserId, - IEnumerable collections, + ICollection collections, IEnumerable groups) { if (user.Id.Equals(default(Guid))) @@ -1408,6 +1408,15 @@ public class OrganizationService : IOrganizationService { throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the user to collections instead."); } + + if (organizationAbility?.FlexibleCollections == true && collections?.Any() == true) + { + var invalidAssociations = collections.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); + if (invalidAssociations.Any()) + { + throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); + } + } // End Flexible Collections // Only autoscale (if required) after all validation has passed so that we know it's a valid request before @@ -1625,9 +1634,20 @@ public class OrganizationService : IOrganizationService } public async Task InviteUserAsync(Guid organizationId, Guid? invitingUserId, string email, - OrganizationUserType type, bool accessAll, string externalId, IEnumerable collections, + OrganizationUserType type, bool accessAll, string externalId, ICollection collections, IEnumerable groups) { + // Validate Collection associations if org is using latest collection enhancements + var organizationAbility = await _applicationCacheService.GetOrganizationAbilityAsync(organizationId); + if (organizationAbility?.FlexibleCollections ?? false) + { + var invalidAssociations = collections?.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); + if (invalidAssociations?.Any() ?? false) + { + throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); + } + } + return await SaveUserSendInviteAsync(organizationId, invitingUserId, systemUser: null, email, type, accessAll, externalId, collections, groups); } @@ -1635,6 +1655,7 @@ public class OrganizationService : IOrganizationService OrganizationUserType type, bool accessAll, string externalId, IEnumerable collections, IEnumerable groups) { + // Collection associations validation not required as they are always an empty list - created via system user (scim) return await SaveUserSendInviteAsync(organizationId, invitingUserId: null, systemUser, email, type, accessAll, externalId, collections, groups); } diff --git a/src/Core/Services/Implementations/CollectionService.cs b/src/Core/Services/Implementations/CollectionService.cs index 26e59fd4d6..d8d2485264 100644 --- a/src/Core/Services/Implementations/CollectionService.cs +++ b/src/Core/Services/Implementations/CollectionService.cs @@ -55,15 +55,25 @@ public class CollectionService : ICollectionService var groupsList = groups?.ToList(); var usersList = users?.ToList(); - // If using Flexible Collections - a collection should always have someone with Can Manage permissions - if (org.FlexibleCollections && _featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1)) + if (org.FlexibleCollections) { - var groupHasManageAccess = groupsList?.Any(g => g.Manage) ?? false; - var userHasManageAccess = usersList?.Any(u => u.Manage) ?? false; - if (!groupHasManageAccess && !userHasManageAccess && !org.AllowAdminAccessToAllCollectionItems) + // Cannot use Manage with ReadOnly/HidePasswords permissions + var invalidAssociations = groupsList?.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); + if (invalidAssociations?.Any() ?? false) { - throw new BadRequestException( - "At least one member or group must have can manage permission."); + throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); + } + + // If using Flexible Collections V1 - a collection should always have someone with Can Manage permissions + if (_featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1)) + { + var groupHasManageAccess = groupsList?.Any(g => g.Manage) ?? false; + var userHasManageAccess = usersList?.Any(u => u.Manage) ?? false; + if (!groupHasManageAccess && !userHasManageAccess && !org.AllowAdminAccessToAllCollectionItems) + { + throw new BadRequestException( + "At least one member or group must have can manage permission."); + } } } diff --git a/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs index 4ddfeff646..2bedef7f65 100644 --- a/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs @@ -32,7 +32,7 @@ public class GroupsControllerTests g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name && g.AccessAll == groupRequestModel.AccessAll), organization, - Arg.Any>(), + Arg.Any>(), Arg.Any>()); Assert.Equal(groupRequestModel.Name, response.Name); Assert.Equal(organization.Id, response.OrganizationId); @@ -57,7 +57,7 @@ public class GroupsControllerTests g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name && g.AccessAll == groupRequestModel.AccessAll), Arg.Is(o => o.Id == organization.Id), - Arg.Any>(), + Arg.Any>(), Arg.Any>()); Assert.Equal(groupRequestModel.Name, response.Name); Assert.Equal(organization.Id, response.OrganizationId); diff --git a/test/Api.Test/AdminConsole/Public/Controllers/GroupsControllerTests.cs b/test/Api.Test/AdminConsole/Public/Controllers/GroupsControllerTests.cs index a3d5294fe2..6e24f44bd5 100644 --- a/test/Api.Test/AdminConsole/Public/Controllers/GroupsControllerTests.cs +++ b/test/Api.Test/AdminConsole/Public/Controllers/GroupsControllerTests.cs @@ -5,6 +5,7 @@ using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces; using Bit.Core.AdminConsole.Repositories; using Bit.Core.Context; +using Bit.Core.Exceptions; using Bit.Core.Models.Data; using Bit.Core.Repositories; using Bit.Test.Common.AutoFixture; @@ -21,8 +22,15 @@ public class GroupsControllerTests { [Theory] [BitAutoData] - public async Task Post_Success(Organization organization, GroupCreateUpdateRequestModel groupRequestModel, SutProvider sutProvider) + public async Task Post_Success_BeforeFlexibleCollectionMigration(Organization organization, GroupCreateUpdateRequestModel groupRequestModel, SutProvider sutProvider) { + // Organization has not migrated + organization.FlexibleCollections = false; + + // Permissions do not contain Manage property + var expectedPermissions = (groupRequestModel.Collections ?? []).Select(model => new AssociationWithPermissionsRequestModel { Id = model.Id, ReadOnly = model.ReadOnly, HidePasswords = model.HidePasswords.GetValueOrDefault() }); + groupRequestModel.Collections = expectedPermissions; + sutProvider.GetDependency().OrganizationId.Returns(organization.Id); sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); @@ -34,7 +42,7 @@ public class GroupsControllerTests g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name && g.AccessAll == groupRequestModel.AccessAll && g.ExternalId == groupRequestModel.ExternalId), organization, - Arg.Any>()); + Arg.Any>()); Assert.Equal(groupRequestModel.Name, responseValue.Name); Assert.Equal(groupRequestModel.AccessAll, responseValue.AccessAll); @@ -43,8 +51,32 @@ public class GroupsControllerTests [Theory] [BitAutoData] - public async Task Put_Success(Organization organization, Group group, GroupCreateUpdateRequestModel groupRequestModel, SutProvider sutProvider) + public async Task Post_Throws_BadRequestException_BeforeFlexibleCollectionMigration_Manage(Organization organization, GroupCreateUpdateRequestModel groupRequestModel, SutProvider sutProvider) { + // Organization has not migrated + organization.FlexibleCollections = false; + + // Contains at least one can manage + groupRequestModel.Collections.First().Manage = true; + + sutProvider.GetDependency().OrganizationId.Returns(organization.Id); + sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().CreateGroupAsync(default, default, default, default); + await Assert.ThrowsAsync(() => sutProvider.Sut.Post(groupRequestModel)); + } + + [Theory] + [BitAutoData] + public async Task Put_Success_BeforeFlexibleCollectionMigration(Organization organization, Group group, GroupCreateUpdateRequestModel groupRequestModel, SutProvider sutProvider) + { + // Organization has not migrated + organization.FlexibleCollections = false; + + // Permissions do not contain Manage property + var expectedPermissions = (groupRequestModel.Collections ?? []).Select(model => new AssociationWithPermissionsRequestModel { Id = model.Id, ReadOnly = model.ReadOnly, HidePasswords = model.HidePasswords.GetValueOrDefault() }); + groupRequestModel.Collections = expectedPermissions; + group.OrganizationId = organization.Id; sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); @@ -59,7 +91,86 @@ public class GroupsControllerTests g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name && g.AccessAll == groupRequestModel.AccessAll && g.ExternalId == groupRequestModel.ExternalId), Arg.Is(o => o.Id == organization.Id), - Arg.Any>()); + Arg.Any>()); + + Assert.Equal(groupRequestModel.Name, responseValue.Name); + Assert.Equal(groupRequestModel.AccessAll, responseValue.AccessAll); + Assert.Equal(groupRequestModel.ExternalId, responseValue.ExternalId); + } + + [Theory] + [BitAutoData] + public async Task Put_Throws_BadRequestException_BeforeFlexibleCollectionMigration_Manage(Organization organization, Group group, GroupCreateUpdateRequestModel groupRequestModel, SutProvider sutProvider) + { + // Organization has not migrated + organization.FlexibleCollections = false; + + // Contains at least one can manage + groupRequestModel.Collections.First().Manage = true; + + group.OrganizationId = organization.Id; + + sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); + sutProvider.GetDependency().GetByIdAsync(group.Id).Returns(group); + sutProvider.GetDependency().OrganizationId.Returns(organization.Id); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().UpdateGroupAsync(default, default, default, default); + await Assert.ThrowsAsync(() => sutProvider.Sut.Put(group.Id, groupRequestModel)); + } + + [Theory] + [BitAutoData] + public async Task Post_Success_AfterFlexibleCollectionMigration(Organization organization, GroupCreateUpdateRequestModel groupRequestModel, SutProvider sutProvider) + { + // Organization has migrated + organization.FlexibleCollections = true; + + // Contains at least one can manage + groupRequestModel.Collections.First().Manage = true; + + sutProvider.GetDependency().OrganizationId.Returns(organization.Id); + sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); + + var response = await sutProvider.Sut.Post(groupRequestModel) as JsonResult; + var responseValue = response.Value as GroupResponseModel; + + await sutProvider.GetDependency().Received(1).CreateGroupAsync( + Arg.Is(g => + g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name && + g.AccessAll == groupRequestModel.AccessAll && g.ExternalId == groupRequestModel.ExternalId), + organization, + Arg.Any>()); + + Assert.Equal(groupRequestModel.Name, responseValue.Name); + Assert.Equal(groupRequestModel.AccessAll, responseValue.AccessAll); + Assert.Equal(groupRequestModel.ExternalId, responseValue.ExternalId); + } + + [Theory] + [BitAutoData] + public async Task Put_Success_AfterFlexibleCollectionMigration(Organization organization, Group group, GroupCreateUpdateRequestModel groupRequestModel, SutProvider sutProvider) + { + // Organization has migrated + organization.FlexibleCollections = true; + + // Contains at least one can manage + groupRequestModel.Collections.First().Manage = true; + + group.OrganizationId = organization.Id; + + sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); + sutProvider.GetDependency().GetByIdAsync(group.Id).Returns(group); + sutProvider.GetDependency().OrganizationId.Returns(organization.Id); + + var response = await sutProvider.Sut.Put(group.Id, groupRequestModel) as JsonResult; + var responseValue = response.Value as GroupResponseModel; + + await sutProvider.GetDependency().Received(1).UpdateGroupAsync( + Arg.Is(g => + g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name && + g.AccessAll == groupRequestModel.AccessAll && g.ExternalId == groupRequestModel.ExternalId), + Arg.Is(o => o.Id == organization.Id), + Arg.Any>()); Assert.Equal(groupRequestModel.Name, responseValue.Name); Assert.Equal(groupRequestModel.AccessAll, responseValue.AccessAll); diff --git a/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs b/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs index 4c83e08287..86f11a278b 100644 --- a/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs +++ b/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs @@ -1089,7 +1089,7 @@ OrganizationUserInvite invite, SutProvider sutProvider) [Theory, BitAutoData] public async Task SaveUser_NoUserId_Throws(OrganizationUser user, Guid? savingUserId, - IEnumerable collections, IEnumerable groups, SutProvider sutProvider) + ICollection collections, IEnumerable groups, SutProvider sutProvider) { user.Id = default(Guid); var exception = await Assert.ThrowsAsync( @@ -1099,7 +1099,7 @@ OrganizationUserInvite invite, SutProvider sutProvider) [Theory, BitAutoData] public async Task SaveUser_NoChangeToData_Throws(OrganizationUser user, Guid? savingUserId, - IEnumerable collections, IEnumerable groups, SutProvider sutProvider) + ICollection collections, IEnumerable groups, SutProvider sutProvider) { var organizationUserRepository = sutProvider.GetDependency(); organizationUserRepository.GetByIdAsync(user.Id).Returns(user); @@ -1113,7 +1113,7 @@ OrganizationUserInvite invite, SutProvider sutProvider) Organization organization, OrganizationUser oldUserData, OrganizationUser newUserData, - IEnumerable collections, + ICollection collections, IEnumerable groups, Permissions permissions, [OrganizationUser(type: OrganizationUserType.Owner)] OrganizationUser savingUser, @@ -1145,7 +1145,7 @@ OrganizationUserInvite invite, SutProvider sutProvider) Organization organization, OrganizationUser oldUserData, [OrganizationUser(type: OrganizationUserType.Custom)] OrganizationUser newUserData, - IEnumerable collections, + ICollection collections, IEnumerable groups, [OrganizationUser(type: OrganizationUserType.Owner)] OrganizationUser savingUser, SutProvider sutProvider) @@ -1182,7 +1182,7 @@ OrganizationUserInvite invite, SutProvider sutProvider) Organization organization, OrganizationUser oldUserData, OrganizationUser newUserData, - IEnumerable collections, + ICollection collections, IEnumerable groups, Permissions permissions, [OrganizationUser(type: OrganizationUserType.Owner)] OrganizationUser savingUser, @@ -1217,7 +1217,7 @@ OrganizationUserInvite invite, SutProvider sutProvider) Organization organization, OrganizationUser oldUserData, [OrganizationUser(type: OrganizationUserType.Custom)] OrganizationUser newUserData, - IEnumerable collections, + ICollection collections, IEnumerable groups, Permissions permissions, [OrganizationUser(type: OrganizationUserType.Owner)] OrganizationUser savingUser, @@ -1251,7 +1251,7 @@ OrganizationUserInvite invite, SutProvider sutProvider) Organization organization, [OrganizationUser(type: OrganizationUserType.User)] OrganizationUser oldUserData, [OrganizationUser(type: OrganizationUserType.Custom)] OrganizationUser newUserData, - IEnumerable collections, + ICollection collections, IEnumerable groups, [OrganizationUser(type: OrganizationUserType.Custom)] OrganizationUser savingUser, [OrganizationUser(type: OrganizationUserType.Owner)] OrganizationUser organizationOwner, @@ -1295,7 +1295,7 @@ OrganizationUserInvite invite, SutProvider sutProvider) Organization organization, [OrganizationUser(type: OrganizationUserType.User)] OrganizationUser oldUserData, [OrganizationUser(type: OrganizationUserType.Custom)] OrganizationUser newUserData, - IEnumerable collections, + ICollection collections, IEnumerable groups, [OrganizationUser(type: OrganizationUserType.Custom)] OrganizationUser savingUser, SutProvider sutProvider) @@ -1330,7 +1330,7 @@ OrganizationUserInvite invite, SutProvider sutProvider) Organization organization, [OrganizationUser(type: OrganizationUserType.Custom)] OrganizationUser oldUserData, [OrganizationUser(type: OrganizationUserType.Admin)] OrganizationUser newUserData, - IEnumerable collections, + ICollection collections, IEnumerable groups, SutProvider sutProvider) { @@ -1365,7 +1365,7 @@ OrganizationUserInvite invite, SutProvider sutProvider) [OrganizationUser(type: OrganizationUserType.User)] OrganizationUser oldUserData, [OrganizationUser(type: OrganizationUserType.Manager)] OrganizationUser newUserData, [OrganizationUser(type: OrganizationUserType.Owner, status: OrganizationUserStatusType.Confirmed)] OrganizationUser savingUser, - IEnumerable collections, + ICollection collections, IEnumerable groups, SutProvider sutProvider) { @@ -1403,7 +1403,7 @@ OrganizationUserInvite invite, SutProvider sutProvider) [OrganizationUser(type: OrganizationUserType.User)] OrganizationUser oldUserData, [OrganizationUser(type: OrganizationUserType.User)] OrganizationUser newUserData, [OrganizationUser(type: OrganizationUserType.Owner, status: OrganizationUserStatusType.Confirmed)] OrganizationUser savingUser, - IEnumerable collections, + ICollection collections, IEnumerable groups, SutProvider sutProvider) {