Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AC-2646] Remove FC MVP dead code from Core #4281

Merged
merged 17 commits into from
Jul 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
85d75c4
chore: remove fc refs in CreateGroup and UpdateGroup commands, refs Aโ€ฆ
vincentsalucci Jun 10, 2024
6c683c0
chore: remove fc refs and update interface to represent usage/get ridโ€ฆ
vincentsalucci Jun 10, 2024
e5b003d
chore: remove org/provider service fc callers, refs AC-2646
vincentsalucci Jun 12, 2024
66b8bf0
chore: remove collection service fc callers, refs AC-2646
vincentsalucci Jun 12, 2024
7c1a36f
chore: remove cipher service import ciphers fc callers, refs AC-2646
vincentsalucci Jun 12, 2024
0ef8776
fix: UpdateOrganizationUserCommandTests collections to list, refs AC-โ€ฆ
vincentsalucci Jun 12, 2024
7c3bb84
fix: update CreateGroupCommandTests, refs AC-2646
vincentsalucci Jun 14, 2024
ef382e9
fix: adjust UpdateGroupCommandTests, refs AC-2646
vincentsalucci Jun 15, 2024
84cedba
fix: adjust UpdateOrganizationUserCommandTests for FC always true, reโ€ฆ
vincentsalucci Jun 15, 2024
f8a3b75
fix: update CollectionServiceTests, refs AC-2646
vincentsalucci Jun 25, 2024
04feb17
fix: remove unnecessary test with fc disabled, refs AC-2646
vincentsalucci Jun 25, 2024
1c94d44
Merge branch 'main' into ac/ac-2646/remove-fc-mvp-code
vincentsalucci Jul 3, 2024
bdc471a
Merge branch 'main' into ac/ac-2646/remove-fc-mvp-code
vincentsalucci Jul 5, 2024
a1f6a48
fix: update tests to account for AccessAll removal and Manager removaโ€ฆ
vincentsalucci Jul 5, 2024
4942598
chore: remove dependence on FC flag for tests, refs AC-2646
vincentsalucci Jul 5, 2024
b9b66a5
Merge branch 'main' into ac/ac-2646/remove-fc-mvp-code
vincentsalucci Jul 8, 2024
7a25b93
Merge branch 'main' into ac/ac-2646/remove-fc-mvp-code
vincentsalucci Jul 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -544,9 +544,9 @@ private static string GetEnumDisplayName(Enum value)
await _providerOrganizationRepository.CreateAsync(providerOrganization);
await _eventService.LogProviderOrganizationEventAsync(providerOrganization, EventType.ProviderOrganization_Created);

// If using Flexible Collections, give the owner Can Manage access over the default collection
// Give the owner Can Manage access over the default collection
// The orgUser is not available when the org is created so we have to do it here as part of the invite
var defaultOwnerAccess = organization.FlexibleCollections && defaultCollection != null
var defaultOwnerAccess = defaultCollection != null
?
[
new CollectionAccessSelection
Expand All @@ -566,10 +566,6 @@ private static string GetEnumDisplayName(Enum value)
new OrganizationUserInvite
{
Emails = new[] { clientOwnerEmail },

// If using Flexible Collections, AccessAll is deprecated and set to false.
// If not using Flexible Collections, set AccessAll to true (previous behavior)
AccessAll = !organization.FlexibleCollections,
Type = OrganizationUserType.Owner,
Permissions = null,
Collections = defaultOwnerAccess,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ public OrganizationUserInvite ToOrganizationUserInvite(ScimProviderType scimProv

// Permissions cannot be set via SCIM so we use default values
Type = OrganizationUserType.User,
AccessAll = false,
Collections = new List<CollectionAccessSelection>(),
Groups = new List<Guid>()
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -637,8 +637,7 @@ await sutProvider.GetDependency<IOrganizationService>()
t.First().Item1.Emails.Count() == 1 &&
t.First().Item1.Emails.First() == clientOwnerEmail &&
t.First().Item1.Type == OrganizationUserType.Owner &&
t.First().Item1.AccessAll &&
!t.First().Item1.Collections.Any() &&
t.First().Item1.Collections.Count() == 1 &&
t.First().Item2 == null));
}

Expand Down Expand Up @@ -717,13 +716,12 @@ await sutProvider.GetDependency<IOrganizationService>()
t.First().Item1.Emails.Count() == 1 &&
t.First().Item1.Emails.First() == clientOwnerEmail &&
t.First().Item1.Type == OrganizationUserType.Owner &&
t.First().Item1.AccessAll &&
!t.First().Item1.Collections.Any() &&
t.First().Item1.Collections.Count() == 1 &&
t.First().Item2 == null));
}

[Theory, OrganizationCustomize(FlexibleCollections = true), BitAutoData]
public async Task CreateOrganizationAsync_WithFlexibleCollections_SetsAccessAllToFalse
[Theory, OrganizationCustomize, BitAutoData]
public async Task CreateOrganizationAsync_SetsAccessAllToFalse
(Provider provider, OrganizationSignup organizationSignup, Organization organization, string clientOwnerEmail,
User user, SutProvider<ProviderService> sutProvider, Collection defaultCollection)
{
Expand All @@ -747,7 +745,6 @@ await sutProvider.GetDependency<IOrganizationService>()
t.First().Item1.Emails.Count() == 1 &&
t.First().Item1.Emails.First() == clientOwnerEmail &&
t.First().Item1.Type == OrganizationUserType.Owner &&
t.First().Item1.AccessAll == false &&
t.First().Item1.Collections.Single().Id == defaultCollection.Id &&
!t.First().Item1.Collections.Single().HidePasswords &&
!t.First().Item1.Collections.Single().ReadOnly &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ public async Task PostUser_Success(SutProvider<PostUserCommand> sutProvider, str
Arg.Is<OrganizationUserInvite>(i =>
i.Emails.Single().Equals(scimUserRequestModel.PrimaryEmail.ToLowerInvariant()) &&
i.Type == OrganizationUserType.User &&
!i.AccessAll &&
!i.Collections.Any() &&
!i.Groups.Any() &&
i.AccessSecretsManager), externalId)
Expand All @@ -56,7 +55,6 @@ public async Task PostUser_Success(SutProvider<PostUserCommand> sutProvider, str
Arg.Is<OrganizationUserInvite>(i =>
i.Emails.Single().Equals(scimUserRequestModel.PrimaryEmail.ToLowerInvariant()) &&
i.Type == OrganizationUserType.User &&
!i.AccessAll &&
!i.Collections.Any() &&
!i.Groups.Any() &&
i.AccessSecretsManager), externalId);
Expand Down
6 changes: 5 additions & 1 deletion src/Core/AdminConsole/Entities/OrganizationUser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ public class OrganizationUser : ITableObject<Guid>, IExternal
public string? ResetPasswordKey { get; set; }
public OrganizationUserStatusType Status { get; set; }
public OrganizationUserType Type { get; set; }
public bool AccessAll { get; set; }

/// <summary>
/// AccessAll is deprecated and should always be left as false. Scheduled for removal.
/// </summary>
public bool AccessAll { get; set; } = false;
[MaxLength(300)]
public string? ExternalId { get; set; }
public DateTime CreationDate { get; internal set; } = DateTime.UtcNow;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ public class OrganizationUserInvite
{
public IEnumerable<string> Emails { get; set; }
public Enums.OrganizationUserType? Type { get; set; }
public bool AccessAll { get; set; }
public bool AccessSecretsManager { get; set; }
public Permissions Permissions { get; set; }
public IEnumerable<CollectionAccessSelection> Collections { get; set; }
Expand All @@ -19,7 +18,6 @@ public OrganizationUserInvite(OrganizationUserInviteData requestModel)
{
Emails = requestModel.Emails;
Type = requestModel.Type;
AccessAll = requestModel.AccessAll;
AccessSecretsManager = requestModel.AccessSecretsManager;
Collections = requestModel.Collections;
Groups = requestModel.Groups;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ public class OrganizationUserInviteData
{
public IEnumerable<string> Emails { get; set; }
public OrganizationUserType? Type { get; set; }
public bool AccessAll { get; set; }
public bool AccessSecretsManager { get; set; }
public IEnumerable<CollectionAccessSelection> Collections { get; set; }
public IEnumerable<Guid> Groups { get; set; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,18 +115,15 @@
throw new BadRequestException("This organization cannot use groups.");
}

if (organization.FlexibleCollections)
if (group.AccessAll)
{
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.");
}
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.");

Check warning on line 126 in src/Core/AdminConsole/OrganizationFeatures/Groups/CreateGroupCommand.cs

View check run for this annotation

Codecov / codecov/patch

src/Core/AdminConsole/OrganizationFeatures/Groups/CreateGroupCommand.cs#L125-L126

Added lines #L125 - L126 were not covered by tests
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -109,18 +109,15 @@
throw new BadRequestException("This organization cannot use groups.");
}

if (organization.FlexibleCollections)
if (group.AccessAll)
{
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.");
}
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.");

Check warning on line 120 in src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs

View check run for this annotation

Codecov / codecov/patch

src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs#L119-L120

Added lines #L119 - L120 were not covered by tests
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,5 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interface

public interface IUpdateOrganizationUserCommand
{
Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, IEnumerable<CollectionAccessSelection> collections, IEnumerable<Guid>? groups);
Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, List<CollectionAccessSelection> collections, IEnumerable<Guid>? groups);
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
}

public async Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId,
IEnumerable<CollectionAccessSelection> collections, IEnumerable<Guid>? groups)
List<CollectionAccessSelection>? collections, IEnumerable<Guid>? groups)
{
if (user.Id.Equals(default(Guid)))
{
Expand All @@ -59,22 +59,19 @@
throw new BadRequestException("Organization must have at least one confirmed owner.");
}

// If the organization is using Flexible Collections, prevent use of any deprecated permissions
var organization = await _organizationRepository.GetByIdAsync(user.OrganizationId);
if (organization.FlexibleCollections && user.AccessAll)
if (user.AccessAll)
{
throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the user to collections instead.");
}

if (organization.FlexibleCollections && collections?.Any() == true)
if (collections?.Count > 0)
{
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
// updating Stripe
Expand All @@ -83,17 +80,13 @@
var additionalSmSeatsRequired = await _countNewSmSeatsRequiredQuery.CountNewSmSeatsRequiredAsync(user.OrganizationId, 1);
if (additionalSmSeatsRequired > 0)
{
var organization = await _organizationRepository.GetByIdAsync(user.OrganizationId);

Check warning on line 83 in src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs

View check run for this annotation

Codecov / codecov/patch

src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs#L83

Added line #L83 was not covered by tests
var update = new SecretsManagerSubscriptionUpdate(organization, true)
.AdjustSeats(additionalSmSeatsRequired);
await _updateSecretsManagerSubscriptionCommand.UpdateSubscriptionAsync(update);
}
}

if (user.AccessAll)
{
// We don't need any collections if we're flagged to have all access.
collections = new List<CollectionAccessSelection>();
}
await _organizationUserRepository.ReplaceAsync(user, collections);

if (groups != null)
Expand Down
Loading
Loading