Skip to content

Commit

Permalink
Refactor IUserService methods GetOrganizationsManagingUserAsync and I…
Browse files Browse the repository at this point in the history
…sManagedByAnyOrganizationAsync to not return nullable objects. Update ProfileOrganizationResponseModel.UserIsManagedByOrganization to not be nullable
  • Loading branch information
r-tome committed Oct 11, 2024
1 parent d6a3a10 commit ee24a91
Show file tree
Hide file tree
Showing 11 changed files with 27 additions and 83 deletions.
4 changes: 1 addition & 3 deletions src/Api/AdminConsole/Controllers/OrganizationsController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,8 @@ public async Task<ListResponseModel<ProfileOrganizationResponseModel>> GetUser()
var organizations = await _organizationUserRepository.GetManyDetailsByUserAsync(userId,
OrganizationUserStatusType.Confirmed);

#nullable enable
var organizationManagingActiveUser = await _userService.GetOrganizationsManagingUserAsync(userId);
var organizationIdsManagingActiveUser = organizationManagingActiveUser?.Select(o => o.Id);
#nullable disable
var organizationIdsManagingActiveUser = organizationManagingActiveUser.Select(o => o.Id);

Check warning on line 125 in src/Api/AdminConsole/Controllers/OrganizationsController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/AdminConsole/Controllers/OrganizationsController.cs#L124-L125

Added lines #L124 - L125 were not covered by tests

var responses = organizations.Select(o => new ProfileOrganizationResponseModel(o, organizationIdsManagingActiveUser));

Check warning on line 127 in src/Api/AdminConsole/Controllers/OrganizationsController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/AdminConsole/Controllers/OrganizationsController.cs#L127

Added line #L127 was not covered by tests
return new ListResponseModel<ProfileOrganizationResponseModel>(responses);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,7 @@ public ProfileOrganizationResponseModel(string str) : base(str) { }

public ProfileOrganizationResponseModel(
OrganizationUserOrganizationDetails organization,
#nullable enable
IEnumerable<Guid>? organizationIdsManagingUser
#nullable disable
)
IEnumerable<Guid> organizationIdsManagingUser)
: this("profileOrganization")
{
Id = organization.OrganizationId;
Expand Down Expand Up @@ -70,7 +67,7 @@ public ProfileOrganizationResponseModel(
AccessSecretsManager = organization.AccessSecretsManager;
LimitCollectionCreationDeletion = organization.LimitCollectionCreationDeletion;
AllowAdminAccessToAllCollectionItems = organization.AllowAdminAccessToAllCollectionItems;
UserIsManagedByOrganization = organizationIdsManagingUser?.Contains(organization.OrganizationId);
UserIsManagedByOrganization = organizationIdsManagingUser.Contains(organization.OrganizationId);

if (organization.SsoConfig != null)
{
Expand Down Expand Up @@ -135,7 +132,9 @@ public ProfileOrganizationResponseModel(
/// <remarks>
/// An organization manages a user if the user's email domain is verified by the organization and the user is a member of it.
/// The organization must be enabled and able to have verified domains.
/// This property is null if the Account Deprovisioning feature flag is disabled.
/// </remarks>
public bool? UserIsManagedByOrganization { get; set; }
/// <returns>
/// False if the Account Deprovisioning feature flag is disabled.
/// </returns>
public bool UserIsManagedByOrganization { get; set; }
}
21 changes: 2 additions & 19 deletions src/Api/Auth/Controllers/AccountsController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -443,10 +443,7 @@ await _providerUserRepository.GetManyOrganizationDetailsByUserAsync(user.Id,

var twoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user);
var hasPremiumFromOrg = await _userService.HasPremiumFromOrganization(user);

#nullable enable
var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user.Id);
#nullable disable

var response = new ProfileResponseModel(user, organizationUserDetails, providerUserDetails,
providerUserOrganizationDetails, twoFactorEnabled,
Expand All @@ -460,10 +457,7 @@ public async Task<ListResponseModel<ProfileOrganizationResponseModel>> GetOrgani
var userId = _userService.GetProperUserId(User);
var organizationUserDetails = await _organizationUserRepository.GetManyDetailsByUserAsync(userId.Value,
OrganizationUserStatusType.Confirmed);

#nullable enable
var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(userId.Value);

Check warning on line 460 in src/Api/Auth/Controllers/AccountsController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/Auth/Controllers/AccountsController.cs#L460

Added line #L460 was not covered by tests
#nullable disable

var responseData = organizationUserDetails.Select(o => new ProfileOrganizationResponseModel(o, organizationIdsManagingActiveUser));

Check warning on line 462 in src/Api/Auth/Controllers/AccountsController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/Auth/Controllers/AccountsController.cs#L462

Added line #L462 was not covered by tests
return new ListResponseModel<ProfileOrganizationResponseModel>(responseData);
Expand All @@ -483,10 +477,7 @@ public async Task<ProfileResponseModel> PutProfile([FromBody] UpdateProfileReque

var twoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user);
var hasPremiumFromOrg = await _userService.HasPremiumFromOrganization(user);

#nullable enable
var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user.Id);

Check warning on line 480 in src/Api/Auth/Controllers/AccountsController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/Auth/Controllers/AccountsController.cs#L480

Added line #L480 was not covered by tests
#nullable disable

var response = new ProfileResponseModel(user, null, null, null, twoFactorEnabled, hasPremiumFromOrg, organizationIdsManagingActiveUser);

Check warning on line 482 in src/Api/Auth/Controllers/AccountsController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/Auth/Controllers/AccountsController.cs#L482

Added line #L482 was not covered by tests
return response;
Expand All @@ -505,10 +496,7 @@ public async Task<ProfileResponseModel> PutAvatar([FromBody] UpdateAvatarRequest

var userTwoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user);
var userHasPremiumFromOrganization = await _userService.HasPremiumFromOrganization(user);

#nullable enable
var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user.Id);

Check warning on line 499 in src/Api/Auth/Controllers/AccountsController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/Auth/Controllers/AccountsController.cs#L499

Added line #L499 was not covered by tests
#nullable disable

var response = new ProfileResponseModel(user, null, null, null, userTwoFactorEnabled, userHasPremiumFromOrganization, organizationIdsManagingActiveUser);

Check warning on line 501 in src/Api/Auth/Controllers/AccountsController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/Auth/Controllers/AccountsController.cs#L501

Added line #L501 was not covered by tests
return response;
Expand Down Expand Up @@ -661,10 +649,7 @@ public async Task<PaymentResponseModel> PostPremium(PremiumRequestModel model)

var userTwoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user);
var userHasPremiumFromOrganization = await _userService.HasPremiumFromOrganization(user);

#nullable enable
var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user.Id);

Check warning on line 652 in src/Api/Auth/Controllers/AccountsController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/Auth/Controllers/AccountsController.cs#L652

Added line #L652 was not covered by tests
#nullable disable

var profile = new ProfileResponseModel(user, null, null, null, userTwoFactorEnabled, userHasPremiumFromOrganization, organizationIdsManagingActiveUser);

Check warning on line 654 in src/Api/Auth/Controllers/AccountsController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/Auth/Controllers/AccountsController.cs#L654

Added line #L654 was not covered by tests
return new PaymentResponseModel
Expand Down Expand Up @@ -954,11 +939,9 @@ public async Task VerifyOTP([FromBody] VerifyOTPRequestModel model)
}
}

#nullable enable
private async Task<IEnumerable<Guid>?> GetOrganizationIdsManagingUserAsync(Guid userId)
private async Task<IEnumerable<Guid>> GetOrganizationIdsManagingUserAsync(Guid userId)
{
var organizationManagingUser = await _userService.GetOrganizationsManagingUserAsync(userId);
return organizationManagingUser?.Select(o => o.Id);
return organizationManagingUser.Select(o => o.Id);
}
#nullable disable
}
4 changes: 1 addition & 3 deletions src/Api/Billing/Controllers/OrganizationsController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -201,10 +201,8 @@ await addSecretsManagerSubscriptionCommand.SignUpAsync(organization, model.Addit
var organizationDetails = await organizationUserRepository.GetDetailsByUserAsync(userId, organization.Id,
OrganizationUserStatusType.Confirmed);

#nullable enable
var organizationManagingActiveUser = await userService.GetOrganizationsManagingUserAsync(userId);
var organizationIdsManagingActiveUser = organizationManagingActiveUser?.Select(o => o.Id);
#nullable disable
var organizationIdsManagingActiveUser = organizationManagingActiveUser.Select(o => o.Id);

return new ProfileOrganizationResponseModel(organizationDetails, organizationIdsManagingActiveUser);
}
Expand Down
5 changes: 1 addition & 4 deletions src/Api/Models/Response/ProfileResponseModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,7 @@ public ProfileResponseModel(User user,
IEnumerable<ProviderUserOrganizationDetails> providerUserOrganizationDetails,
bool twoFactorEnabled,
bool premiumFromOrganization,
#nullable enable
IEnumerable<Guid>? organizationIdsManagingUser
#nullable disable
) : base("profile")
IEnumerable<Guid> organizationIdsManagingUser) : base("profile")
{
if (user == null)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Api/Vault/Controllers/CiphersController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -912,7 +912,7 @@ public async Task PostPurge([FromBody] SecretVerificationRequestModel model, str

// If Account Deprovisioning is enabled, we need to check if the user is managed by any organization.
if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)
&& await _userService.IsManagedByAnyOrganizationAsync(user.Id) == true)
&& await _userService.IsManagedByAnyOrganizationAsync(user.Id))
{
throw new BadRequestException("Accounts managed by an organization cannot be purged.");

Check warning on line 917 in src/Api/Vault/Controllers/CiphersController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/Vault/Controllers/CiphersController.cs#L915-L917

Added lines #L915 - L917 were not covered by tests
}
Expand Down
31 changes: 2 additions & 29 deletions src/Api/Vault/Controllers/SyncController.cs
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
using Bit.Api.Vault.Models.Response;
using Bit.Core;
using Bit.Core.AdminConsole.Entities;
using Bit.Core.AdminConsole.Enums.Provider;
using Bit.Core.AdminConsole.Repositories;
using Bit.Core.Entities;
using Bit.Core.Enums;
using Bit.Core.Exceptions;
using Bit.Core.Models.Data;
using Bit.Core.Models.Data.Organizations.OrganizationUsers;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Core.Settings;
Expand Down Expand Up @@ -95,37 +93,12 @@ await _providerUserRepository.GetManyOrganizationDetailsByUserAsync(user.Id,

var userTwoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user);
var userHasPremiumFromOrganization = await _userService.HasPremiumFromOrganization(user);

#nullable enable
var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user, organizationUserDetails);
#nullable disable
var organizationManagingActiveUser = await _userService.GetOrganizationsManagingUserAsync(user.Id);
var organizationIdsManagingActiveUser = organizationManagingActiveUser.Select(o => o.Id);

var response = new SyncResponseModel(_globalSettings, user, userTwoFactorEnabled, userHasPremiumFromOrganization,
organizationIdsManagingActiveUser, organizationUserDetails, providerUserDetails, providerUserOrganizationDetails,
folders, collections, ciphers, collectionCiphersGroupDict, excludeDomains, policies, sends);
return response;
}

#nullable enable
/// <summary>
/// Gets the IDs of the organizations that manage a user.
/// </summary>
/// <remarks>
/// Organizations are considered to manage a user if the user's email domain is verified by the organization and the user is a member of it.
/// The organization must be enabled and able to have verified domains.
/// </remarks>
private async Task<IEnumerable<Guid>?> GetOrganizationIdsManagingUserAsync(User user, IEnumerable<OrganizationUserOrganizationDetails> organizationUserDetails)
{
// Account deprovisioning must be enabled and organizations must be enabled and able to have verified domains.
// TODO: Replace "UseSso" with a new organization ability like "UseOrganizationDomains" (PM-11622).
if (!_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)
|| !organizationUserDetails.Any(o => o is { Enabled: true, UseSso: true }))
{
return null;
}

var organizationsManagingUser = await _userService.GetOrganizationsManagingUserAsync(user.Id);
return organizationsManagingUser?.Select(o => o.Id);
}
#nullable disable
}
4 changes: 1 addition & 3 deletions src/Api/Vault/Models/Response/SyncResponseModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,7 @@ public SyncResponseModel(
User user,
bool userTwoFactorEnabled,
bool userHasPremiumFromOrganization,
#nullable enable
IEnumerable<Guid>? organizationIdsManagingUser,
#nullable disable
IEnumerable<Guid> organizationIdsManagingUser,
IEnumerable<OrganizationUserOrganizationDetails> organizationUserDetails,
IEnumerable<ProviderUserProviderDetails> providerUserDetails,
IEnumerable<ProviderUserOrganizationDetails> providerUserOrganizationDetails,
Expand Down
11 changes: 6 additions & 5 deletions src/Core/Services/IUserService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ Task<IdentityResult> UpdatePasswordHash(User user, string newPassword,
/// </summary>
Task<bool> IsLegacyUser(string userId);

#nullable enable
/// <summary>
/// Indicates if the user is managed by any organization.
/// </summary>
Expand All @@ -97,14 +96,16 @@ Task<IdentityResult> UpdatePasswordHash(User user, string newPassword,
/// The organization must be enabled and able to have verified domains.
/// </remarks>
/// <returns>
/// This returns a nullable object because if the Account Deprovisioning feature flag is disabled, we should return null.
/// False if the Account Deprovisioning feature flag is disabled.
/// </returns>
Task<bool?> IsManagedByAnyOrganizationAsync(Guid userId);
Task<bool> IsManagedByAnyOrganizationAsync(Guid userId);

/// <summary>
/// Gets the organizations that manage the user.
/// </summary>
/// <returns>
/// An empty collection if the Account Deprovisioning feature flag is disabled.
/// </returns>
/// <inheritdoc cref="IsManagedByAnyOrganizationAsync(Guid)"/>
Task<IEnumerable<Organization>?> GetOrganizationsManagingUserAsync(Guid userId);
#nullable disable
Task<IEnumerable<Organization>> GetOrganizationsManagingUserAsync(Guid userId);
}
10 changes: 4 additions & 6 deletions src/Core/Services/Implementations/UserService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1263,18 +1263,17 @@ public async Task<bool> IsLegacyUser(string userId)
return IsLegacyUser(user);
}

#nullable enable
public async Task<bool?> IsManagedByAnyOrganizationAsync(Guid userId)
public async Task<bool> IsManagedByAnyOrganizationAsync(Guid userId)
{
var managingOrganizations = await GetOrganizationsManagingUserAsync(userId);
return managingOrganizations?.Any();
return managingOrganizations.Any();
}

public async Task<IEnumerable<Organization>?> GetOrganizationsManagingUserAsync(Guid userId)
public async Task<IEnumerable<Organization>> GetOrganizationsManagingUserAsync(Guid userId)
{
if (!_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning))
{
return null;
return Enumerable.Empty<Organization>();
}

// Get all organizations that have verified the user's email domain.
Expand All @@ -1285,7 +1284,6 @@ public async Task<bool> IsLegacyUser(string userId)
// Verified domains were tied to SSO, so we currently check the "UseSso" organization ability.
return organizationsWithVerifiedUserEmailDomain.Where(organization => organization is { Enabled: true, UseSso: true });
}
#nullable disable

/// <inheritdoc cref="IsLegacyUser(string)"/>
public static bool IsLegacyUser(User user)
Expand Down
5 changes: 2 additions & 3 deletions test/Core.Test/Services/UserServiceTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -279,18 +279,17 @@ await tokenProvider
}

[Theory, BitAutoData]
public async Task IsManagedByAnyOrganizationAsync_WithAccountDeprovisioningDisabled_ReturnsNull(
public async Task IsManagedByAnyOrganizationAsync_WithAccountDeprovisioningDisabled_ReturnsFalse(
SutProvider<UserService> sutProvider, Guid userId)
{
sutProvider.GetDependency<IFeatureService>()
.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)
.Returns(false);

var result = await sutProvider.Sut.IsManagedByAnyOrganizationAsync(userId);
Assert.Null(result);
Assert.False(result);
}


[Theory, BitAutoData]
public async Task IsManagedByAnyOrganizationAsync_WithAccountDeprovisioningEnabled_WithManagingEnabledOrganization_ReturnsTrue(
SutProvider<UserService> sutProvider, Guid userId, Organization organization)
Expand Down

0 comments on commit ee24a91

Please sign in to comment.