From ebdd30f5d469643d54addb3b5b816a35124f9a5f Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Wed, 3 Aug 2022 07:09:22 +1000 Subject: [PATCH] [EC-388] Enforce organization policies when restoring user (#2152) --- .../Scim/Controllers/v2/UsersController.cs | 4 +- .../OrganizationUsersController.cs | 4 +- src/Core/Services/IOrganizationService.cs | 4 +- .../Implementations/OrganizationService.cs | 55 ++++++++++- .../dbo/Functions/PolicyApplicableToUser.sql | 4 +- .../Policy_CountByTypeApplicableToUser.sql | 2 +- .../Policy_ReadByTypeApplicableToUser.sql | 2 +- .../2022-07-28_00_CheckPoliciesOnRestore.sql | 95 +++++++++++++++++++ 8 files changed, 158 insertions(+), 12 deletions(-) create mode 100644 util/Migrator/DbScripts/2022-07-28_00_CheckPoliciesOnRestore.sql diff --git a/bitwarden_license/src/Scim/Controllers/v2/UsersController.cs b/bitwarden_license/src/Scim/Controllers/v2/UsersController.cs index ca01c8ee1c..3d3a91ca0c 100644 --- a/bitwarden_license/src/Scim/Controllers/v2/UsersController.cs +++ b/bitwarden_license/src/Scim/Controllers/v2/UsersController.cs @@ -193,7 +193,7 @@ namespace Bit.Scim.Controllers.v2 if (model.Active && orgUser.Status == OrganizationUserStatusType.Revoked) { - await _organizationService.RestoreUserAsync(orgUser, null); + await _organizationService.RestoreUserAsync(orgUser, null, _userService); } else if (!model.Active && orgUser.Status != OrganizationUserStatusType.Revoked) { @@ -229,7 +229,7 @@ namespace Bit.Scim.Controllers.v2 var active = activeProperty.GetBoolean(); if (active && orgUser.Status == OrganizationUserStatusType.Revoked) { - await _organizationService.RestoreUserAsync(orgUser, null); + await _organizationService.RestoreUserAsync(orgUser, null, _userService); operationHandled = true; } else if (!active && orgUser.Status != OrganizationUserStatusType.Revoked) diff --git a/src/Api/Controllers/OrganizationUsersController.cs b/src/Api/Controllers/OrganizationUsersController.cs index 1df8aaef42..b1e5451ebc 100644 --- a/src/Api/Controllers/OrganizationUsersController.cs +++ b/src/Api/Controllers/OrganizationUsersController.cs @@ -423,14 +423,14 @@ namespace Bit.Api.Controllers [HttpPut("{id}/restore")] public async Task RestoreAsync(Guid orgId, Guid id) { - await RestoreOrRevokeUserAsync(orgId, id, _organizationService.RestoreUserAsync); + await RestoreOrRevokeUserAsync(orgId, id, (orgUser, userId) => _organizationService.RestoreUserAsync(orgUser, userId, _userService)); } [HttpPatch("restore")] [HttpPut("restore")] public async Task> BulkRestoreAsync(Guid orgId, [FromBody] OrganizationUserBulkRequestModel model) { - return await RestoreOrRevokeUsersAsync(orgId, model, _organizationService.RestoreUsersAsync); + return await RestoreOrRevokeUsersAsync(orgId, model, (orgId, orgUserIds, restoringUserId) => _organizationService.RestoreUsersAsync(orgId, orgUserIds, restoringUserId, _userService)); } private async Task RestoreOrRevokeUserAsync( diff --git a/src/Core/Services/IOrganizationService.cs b/src/Core/Services/IOrganizationService.cs index a0dbe1e633..076cd3eb8b 100644 --- a/src/Core/Services/IOrganizationService.cs +++ b/src/Core/Services/IOrganizationService.cs @@ -61,8 +61,8 @@ namespace Bit.Core.Services Task RevokeUserAsync(OrganizationUser organizationUser, Guid? revokingUserId); Task>> RevokeUsersAsync(Guid organizationId, IEnumerable organizationUserIds, Guid? revokingUserId); - Task RestoreUserAsync(OrganizationUser organizationUser, Guid? restoringUserId); + Task RestoreUserAsync(OrganizationUser organizationUser, Guid? restoringUserId, IUserService userService); Task>> RestoreUsersAsync(Guid organizationId, - IEnumerable organizationUserIds, Guid? restoringUserId); + IEnumerable organizationUserIds, Guid? restoringUserId, IUserService userService); } } diff --git a/src/Core/Services/Implementations/OrganizationService.cs b/src/Core/Services/Implementations/OrganizationService.cs index da26f1fc25..6ab9f1e66f 100644 --- a/src/Core/Services/Implementations/OrganizationService.cs +++ b/src/Core/Services/Implementations/OrganizationService.cs @@ -2300,7 +2300,7 @@ namespace Bit.Core.Services return result; } - public async Task RestoreUserAsync(OrganizationUser organizationUser, Guid? restoringUserId) + public async Task RestoreUserAsync(OrganizationUser organizationUser, Guid? restoringUserId, IUserService userService) { if (organizationUser.Status != OrganizationUserStatusType.Revoked) { @@ -2318,6 +2318,8 @@ namespace Bit.Core.Services throw new BadRequestException("Only owners can restore other owners."); } + await CheckPoliciesBeforeRestoreAsync(organizationUser, userService); + var status = GetPriorActiveOrganizationUserStatusType(organizationUser); await _organizationUserRepository.RestoreAsync(organizationUser.Id, status); @@ -2326,7 +2328,7 @@ namespace Bit.Core.Services } public async Task>> RestoreUsersAsync(Guid organizationId, - IEnumerable organizationUserIds, Guid? restoringUserId) + IEnumerable organizationUserIds, Guid? restoringUserId, IUserService userService) { var orgUsers = await _organizationUserRepository.GetManyAsync(organizationUserIds); var filteredUsers = orgUsers.Where(u => u.OrganizationId == organizationId) @@ -2364,6 +2366,8 @@ namespace Bit.Core.Services throw new BadRequestException("Only owners can restore other owners."); } + await CheckPoliciesBeforeRestoreAsync(organizationUser, userService); + var status = GetPriorActiveOrganizationUserStatusType(organizationUser); await _organizationUserRepository.RestoreAsync(organizationUser.Id, status); @@ -2381,6 +2385,53 @@ namespace Bit.Core.Services return result; } + private async Task CheckPoliciesBeforeRestoreAsync(OrganizationUser orgUser, IUserService userService) + { + // An invited OrganizationUser isn't linked with a user account yet, so these checks are irrelevant + // The user will be subject to the same checks when they try to accept the invite + if (GetPriorActiveOrganizationUserStatusType(orgUser) == OrganizationUserStatusType.Invited) + { + return; + } + + var userId = orgUser.UserId.Value; + + // Enforce Single Organization Policy of organization user is being restored to + var allOrgUsers = await _organizationUserRepository.GetManyByUserAsync(userId); + var hasOtherOrgs = allOrgUsers.Any(ou => ou.OrganizationId != orgUser.OrganizationId); + var singleOrgPoliciesApplyingToRevokedUsers = await _policyRepository.GetManyByTypeApplicableToUserIdAsync(userId, + PolicyType.SingleOrg, OrganizationUserStatusType.Revoked); + var singleOrgPolicyApplies = singleOrgPoliciesApplyingToRevokedUsers.Any(p => p.OrganizationId == orgUser.OrganizationId); + + if (hasOtherOrgs && singleOrgPolicyApplies) + { + throw new BadRequestException("You cannot restore this user until " + + "they leave or remove all other organizations."); + } + + // Enforce Single Organization Policy of other organizations user is a member of + var singleOrgPolicyCount = await _policyRepository.GetCountByTypeApplicableToUserIdAsync(userId, + PolicyType.SingleOrg); + if (singleOrgPolicyCount > 0) + { + throw new BadRequestException("You cannot restore this user because they are a member of " + + "another organization which forbids it"); + } + + // Enforce Two Factor Authentication Policy of organization user is trying to join + var user = await _userRepository.GetByIdAsync(userId); + if (!await userService.TwoFactorIsEnabledAsync(user)) + { + var invitedTwoFactorPolicies = await _policyRepository.GetManyByTypeApplicableToUserIdAsync(userId, + PolicyType.TwoFactorAuthentication, OrganizationUserStatusType.Invited); + if (invitedTwoFactorPolicies.Any(p => p.OrganizationId == orgUser.OrganizationId)) + { + throw new BadRequestException("You cannot restore this user until they enable " + + "two-step login on their user account."); + } + } + } + static OrganizationUserStatusType GetPriorActiveOrganizationUserStatusType(OrganizationUser organizationUser) { // Determine status to revert back to diff --git a/src/Sql/dbo/Functions/PolicyApplicableToUser.sql b/src/Sql/dbo/Functions/PolicyApplicableToUser.sql index aab2b1952d..9118360d71 100644 --- a/src/Sql/dbo/Functions/PolicyApplicableToUser.sql +++ b/src/Sql/dbo/Functions/PolicyApplicableToUser.sql @@ -25,11 +25,11 @@ LEFT JOIN WHERE ( ( - OU.[Status] > 0 + OU.[Status] != 0 -- OrgUsers who have accepted their invite and are linked to a UserId AND OU.[UserId] = @UserId ) OR ( - OU.[Status] = 0 -- 'Invited' OrgUsers are not linked to a UserId yet, so we have to look up their email + OU.[Status] = 0 -- 'Invited' OrgUsers are not linked to a UserId yet, so we have to look up their email AND OU.[Email] IN (SELECT U.Email FROM [dbo].[UserView] U WHERE U.Id = @UserId) ) ) diff --git a/src/Sql/dbo/Stored Procedures/Policy_CountByTypeApplicableToUser.sql b/src/Sql/dbo/Stored Procedures/Policy_CountByTypeApplicableToUser.sql index 0f75909804..3577c0e3de 100644 --- a/src/Sql/dbo/Stored Procedures/Policy_CountByTypeApplicableToUser.sql +++ b/src/Sql/dbo/Stored Procedures/Policy_CountByTypeApplicableToUser.sql @@ -1,7 +1,7 @@ CREATE PROCEDURE [dbo].[Policy_CountByTypeApplicableToUser] @UserId UNIQUEIDENTIFIER, @PolicyType TINYINT, - @MinimumStatus TINYINT + @MinimumStatus SMALLINT AS BEGIN SET NOCOUNT ON diff --git a/src/Sql/dbo/Stored Procedures/Policy_ReadByTypeApplicableToUser.sql b/src/Sql/dbo/Stored Procedures/Policy_ReadByTypeApplicableToUser.sql index db0b4040d1..5f215f99d4 100644 --- a/src/Sql/dbo/Stored Procedures/Policy_ReadByTypeApplicableToUser.sql +++ b/src/Sql/dbo/Stored Procedures/Policy_ReadByTypeApplicableToUser.sql @@ -1,7 +1,7 @@ CREATE PROCEDURE [dbo].[Policy_ReadByTypeApplicableToUser] @UserId UNIQUEIDENTIFIER, @PolicyType TINYINT, - @MinimumStatus TINYINT + @MinimumStatus SMALLINT AS BEGIN SET NOCOUNT ON diff --git a/util/Migrator/DbScripts/2022-07-28_00_CheckPoliciesOnRestore.sql b/util/Migrator/DbScripts/2022-07-28_00_CheckPoliciesOnRestore.sql new file mode 100644 index 0000000000..7f3a2b23d8 --- /dev/null +++ b/util/Migrator/DbScripts/2022-07-28_00_CheckPoliciesOnRestore.sql @@ -0,0 +1,95 @@ +-- 2022-06-08_00_DeactivatedUserStatus changed UserStatus from TINYINT to SMALLINT but these sprocs were missed + +-- Policy_ReadByTypeApplicableToUser +IF OBJECT_ID('[dbo].[Policy_ReadByTypeApplicableToUser]') IS NOT NULL +BEGIN + DROP PROCEDURE [dbo].[Policy_ReadByTypeApplicableToUser] +END +GO + +CREATE PROCEDURE [dbo].[Policy_ReadByTypeApplicableToUser] + @UserId UNIQUEIDENTIFIER, + @PolicyType TINYINT, + @MinimumStatus SMALLINT +AS +BEGIN + SET NOCOUNT ON + +SELECT * +FROM [dbo].[PolicyApplicableToUser](@UserId, @PolicyType, @MinimumStatus) +END +GO + +-- Policy_CountByTypeApplicableToUser +IF OBJECT_ID('[dbo].[Policy_CountByTypeApplicableToUser]') IS NOT NULL +BEGIN + DROP PROCEDURE [dbo].[Policy_CountByTypeApplicableToUser] +END +GO + +CREATE PROCEDURE [dbo].[Policy_CountByTypeApplicableToUser] + @UserId UNIQUEIDENTIFIER, + @PolicyType TINYINT, + @MinimumStatus SMALLINT +AS +BEGIN + SET NOCOUNT ON + +SELECT COUNT(1) +FROM [dbo].[PolicyApplicableToUser](@UserId, @PolicyType, @MinimumStatus) +END +GO + +-- We need to update this function because we now have OrganizationUserStatusTypes that are less than zero, +-- and Deactivated/Revoked users may or may not be associated with a UserId +IF OBJECT_ID('[dbo].[PolicyApplicableToUser]') IS NOT NULL +BEGIN + DROP FUNCTION [dbo].[PolicyApplicableToUser] +END +GO + +CREATE FUNCTION [dbo].[PolicyApplicableToUser] +( + @UserId UNIQUEIDENTIFIER, + @PolicyType TINYINT, + @MinimumStatus SMALLINT +) +RETURNS TABLE +AS RETURN +SELECT + P.* +FROM + [dbo].[PolicyView] P +INNER JOIN + [dbo].[OrganizationUserView] OU ON P.[OrganizationId] = OU.[OrganizationId] +LEFT JOIN + (SELECT + PU.UserId, + PO.OrganizationId + FROM + [dbo].[ProviderUserView] PU + INNER JOIN + [ProviderOrganizationView] PO ON PO.[ProviderId] = PU.[ProviderId]) PUPO + ON PUPO.UserId = OU.UserId + AND PUPO.OrganizationId = P.OrganizationId +WHERE + ( + ( + OU.[Status] != 0 -- OrgUsers who have accepted their invite and are linked to a UserId + AND OU.[UserId] = @UserId + ) + OR ( + OU.[Status] = 0 -- 'Invited' OrgUsers are not linked to a UserId yet, so we have to look up their email + AND OU.[Email] IN (SELECT U.Email FROM [dbo].[UserView] U WHERE U.Id = @UserId) + ) + ) + AND P.[Type] = @PolicyType + AND P.[Enabled] = 1 + AND OU.[Status] >= @MinimumStatus + AND OU.[Type] >= 2 -- Not an owner (0) or admin (1) + AND ( -- Can't manage policies + OU.[Permissions] IS NULL + OR COALESCE(JSON_VALUE(OU.[Permissions], '$.managePolicies'), 'false') = 'false' + ) + AND PUPO.[UserId] IS NULL -- Not a provider +GO \ No newline at end of file