nhyatt/bitwarden
1
0
Fork 0
mirror of https://github.com/bitwarden/server.git synced 2025-05-20 11:04:31 -05:00
Code

[PM-16777] Fix exception when bulk restoring revoked users who never accepted invitations (#5224)

Browse Source 
* Fix null handling for UserId in Two Factor Authentication checks

* Add tests for restoring users with and without 2FA policies
This commit is contained in:
Rui Tomé 2025-01-20 14:59:10 +00:00 committed by GitHub
parent 5423e5d52f
commit 04e5626c57
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 107 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
src/Core/AdminConsole/Services/Implementations/OrganizationService.cs
View File

@ -2165,7 +2165,8 @@ public class OrganizationService : IOrganizationService
// Query Two Factor Authentication status for all users in the organization // Query Two Factor Authentication status for all users in the organization
// This is an optimization to avoid querying the Two Factor Authentication status for each user individually // This is an optimization to avoid querying the Two Factor Authentication status for each user individually
var organizationUsersTwoFactorEnabled = await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(filteredUsers.Select(ou => ou.UserId.Value)); var organizationUsersTwoFactorEnabled = await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(
filteredUsers.Where(ou => ou.UserId.HasValue).Select(ou => ou.UserId.Value));
var result = new List<Tuple<OrganizationUser, string>>(); var result = new List<Tuple<OrganizationUser, string>>();
@ -2188,7 +2189,8 @@ public class OrganizationService : IOrganizationService
throw new BadRequestException("Only owners can restore other owners."); throw new BadRequestException("Only owners can restore other owners.");
} }
var twoFactorIsEnabled = organizationUsersTwoFactorEnabled.FirstOrDefault(ou => ou.userId == organizationUser.UserId.Value).twoFactorIsEnabled; var twoFactorIsEnabled = organizationUser.UserId.HasValue
&& organizationUsersTwoFactorEnabled.FirstOrDefault(ou => ou.userId == organizationUser.UserId.Value).twoFactorIsEnabled;
await CheckPoliciesBeforeRestoreAsync(organizationUser, twoFactorIsEnabled); await CheckPoliciesBeforeRestoreAsync(organizationUser, twoFactorIsEnabled);
var status = GetPriorActiveOrganizationUserStatusType(organizationUser); var status = GetPriorActiveOrganizationUserStatusType(organizationUser);

103
test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs
View File

@ -2180,4 +2180,107 @@ OrganizationUserInvite invite, SutProvider<OrganizationService> sutProvider)
} }
); );
} }
[Theory, BitAutoData]
public async Task RestoreUsers_Success(Organization organization,
[OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.Owner)] OrganizationUser owner,
[OrganizationUser(OrganizationUserStatusType.Revoked)] OrganizationUser orgUser1,
[OrganizationUser(OrganizationUserStatusType.Revoked)] OrganizationUser orgUser2,
SutProvider<OrganizationService> sutProvider)
{
// Arrange
RestoreRevokeUser_Setup(organization, owner, orgUser1, sutProvider);
var organizationUserRepository = sutProvider.GetDependency<IOrganizationUserRepository>();
var eventService = sutProvider.GetDependency<IEventService>();
var twoFactorIsEnabledQuery = sutProvider.GetDependency<ITwoFactorIsEnabledQuery>();
var userService = Substitute.For<IUserService>();
orgUser1.Email = orgUser2.Email = null; // Mock that users were previously confirmed
orgUser1.OrganizationId = orgUser2.OrganizationId = organization.Id;
organizationUserRepository
.GetManyAsync(Arg.Is<IEnumerable<Guid>>(ids => ids.Contains(orgUser1.Id) && ids.Contains(orgUser2.Id)))
.Returns(new[] { orgUser1, orgUser2 });
twoFactorIsEnabledQuery
.TwoFactorIsEnabledAsync(Arg.Is<IEnumerable<Guid>>(ids => ids.Contains(orgUser1.UserId!.Value) && ids.Contains(orgUser2.UserId!.Value)))
.Returns(new List<(Guid userId, bool twoFactorIsEnabled)>
{
(orgUser1.UserId!.Value, true),
(orgUser2.UserId!.Value, false)
});
// Act
var result = await sutProvider.Sut.RestoreUsersAsync(organization.Id, new[] { orgUser1.Id, orgUser2.Id }, owner.Id, userService);
// Assert
Assert.Equal(2, result.Count);
Assert.All(result, r => Assert.Empty(r.Item2)); // No error messages
await organizationUserRepository
.Received(1)
.RestoreAsync(orgUser1.Id, OrganizationUserStatusType.Confirmed);
await organizationUserRepository
.Received(1)
.RestoreAsync(orgUser2.Id, OrganizationUserStatusType.Confirmed);
await eventService.Received(1)
.LogOrganizationUserEventAsync(orgUser1, EventType.OrganizationUser_Restored);
await eventService.Received(1)
.LogOrganizationUserEventAsync(orgUser2, EventType.OrganizationUser_Restored);
}
[Theory, BitAutoData]
public async Task RestoreUsers_With2FAPolicy_BlocksNonCompliantUser(Organization organization,
[OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.Owner)] OrganizationUser owner,
[OrganizationUser(OrganizationUserStatusType.Revoked)] OrganizationUser orgUser1,
[OrganizationUser(OrganizationUserStatusType.Revoked)] OrganizationUser orgUser2,
[OrganizationUser(OrganizationUserStatusType.Revoked)] OrganizationUser orgUser3,
SutProvider<OrganizationService> sutProvider)
{
// Arrange
RestoreRevokeUser_Setup(organization, owner, orgUser1, sutProvider);
var organizationUserRepository = sutProvider.GetDependency<IOrganizationUserRepository>();
var userRepository = sutProvider.GetDependency<IUserRepository>();
var policyService = sutProvider.GetDependency<IPolicyService>();
var userService = Substitute.For<IUserService>();
orgUser1.Email = orgUser2.Email = null;
orgUser3.UserId = null;
orgUser3.Key = null;
orgUser1.OrganizationId = orgUser2.OrganizationId = orgUser3.OrganizationId = organization.Id;
organizationUserRepository
.GetManyAsync(Arg.Is<IEnumerable<Guid>>(ids => ids.Contains(orgUser1.Id) && ids.Contains(orgUser2.Id) && ids.Contains(orgUser3.Id)))
.Returns(new[] { orgUser1, orgUser2, orgUser3 });
userRepository.GetByIdAsync(orgUser2.UserId!.Value).Returns(new User { Email = "test@example.com" });
// Setup 2FA policy
policyService.GetPoliciesApplicableToUserAsync(Arg.Any<Guid>(), PolicyType.TwoFactorAuthentication, Arg.Any<OrganizationUserStatusType>())
.Returns(new[] { new OrganizationUserPolicyDetails { OrganizationId = organization.Id, PolicyType = PolicyType.TwoFactorAuthentication } });
// User1 has 2FA, User2 doesn't
sutProvider.GetDependency<ITwoFactorIsEnabledQuery>()
.TwoFactorIsEnabledAsync(Arg.Is<IEnumerable<Guid>>(ids => ids.Contains(orgUser1.UserId!.Value) && ids.Contains(orgUser2.UserId!.Value)))
.Returns(new List<(Guid userId, bool twoFactorIsEnabled)>
{
(orgUser1.UserId!.Value, true),
(orgUser2.UserId!.Value, false)
});
// Act
var result = await sutProvider.Sut.RestoreUsersAsync(organization.Id, new[] { orgUser1.Id, orgUser2.Id, orgUser3.Id }, owner.Id, userService);
// Assert
Assert.Equal(3, result.Count);
Assert.Empty(result[0].Item2); // First user should succeed
Assert.Contains("two-step login", result[1].Item2); // Second user should fail
Assert.Empty(result[2].Item2); // Third user should succeed
await organizationUserRepository
.Received(1)
.RestoreAsync(orgUser1.Id, OrganizationUserStatusType.Confirmed);
await organizationUserRepository
.DidNotReceive()
.RestoreAsync(orgUser2.Id, Arg.Any<OrganizationUserStatusType>());
await organizationUserRepository
.Received(1)
.RestoreAsync(orgUser3.Id, OrganizationUserStatusType.Invited);
}
} }