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

[PM-15637] Notify Custom Users with “Manage Account Recovery” permission for Device Approval Requests #5359

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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 @@ -60,4 +60,12 @@ UpdateEncryptedDataForKeyRotation UpdateForKeyRotation(Guid userId,
Task<ICollection<OrganizationUser>> GetManyByOrganizationWithClaimedDomainsAsync(Guid organizationId);

Task RevokeManyByIdAsync(IEnumerable<Guid> organizationUserIds);

/// <summary>
/// Returns a list of OrganizationUsersUserDetails with the specified role.
/// </summary>
/// <param name="organizationId">The organization to search within</param>
/// <param name="role">The role to search for</param>
/// <returns>A list of OrganizationUsersUserDetails with the specified role</returns>
Task<IEnumerable<OrganizationUserUserDetails>> GetManyDetailsByRoleAsync(Guid organizationId, OrganizationUserType role);
}
30 changes: 27 additions & 3 deletions src/Core/Auth/Services/Implementations/AuthRequestService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -297,10 +297,34 @@ private async Task NotifyAdminsOfDeviceApprovalRequestAsync(OrganizationUser org
return;
}

var admins = await _organizationUserRepository.GetManyByMinimumRoleAsync(
var adminEmails = await GetAdminAndAccountRecoveryEmailsAsync(organizationUser.OrganizationId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None-blocking question: I guess it's safe to say that adminEmails will never be empty because an organization will have at least one owner?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly! We always check if an organization has at least one confirmed owner when updating/removing a user.


await _mailService.SendDeviceApprovalRequestedNotificationEmailAsync(
adminEmails,
organizationUser.OrganizationId,
user.Email,
user.Name);
}

/// <summary>
/// Returns a list of emails for admins and custom users with the ManageResetPassword permission.
/// </summary>
/// <param name="organizationId">The organization to search within</param>
private async Task<List<string>> GetAdminAndAccountRecoveryEmailsAsync(Guid organizationId)
{
var admins = await _organizationUserRepository.GetManyByMinimumRoleAsync(
organizationId,
OrganizationUserType.Admin);
var adminEmails = admins.Select(a => a.Email).Distinct().ToList();
await _mailService.SendDeviceApprovalRequestedNotificationEmailAsync(adminEmails, organizationUser.OrganizationId, user.Email, user.Name);

var customUsers = await _organizationUserRepository.GetManyDetailsByRoleAsync(
organizationId,
OrganizationUserType.Custom);

return admins.Select(a => a.Email)
.Concat(customUsers
.Where(a => a.GetPermissions().ManageResetPassword)
.Select(a => a.Email))
.Distinct()
.ToList();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -567,4 +567,17 @@
new { OrganizationUserIds = JsonSerializer.Serialize(organizationUserIds), Status = OrganizationUserStatusType.Revoked },
commandType: CommandType.StoredProcedure);
}

public async Task<IEnumerable<OrganizationUserUserDetails>> GetManyDetailsByRoleAsync(Guid organizationId, OrganizationUserType role)
{
using (var connection = new SqlConnection(ConnectionString))
{
var results = await connection.QueryAsync<OrganizationUserUserDetails>(
"[dbo].[OrganizationUser_ReadManyDetailsByRole]",
new { OrganizationId = organizationId, Role = role },
commandType: CommandType.StoredProcedure);

Check warning on line 578 in src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs

View check run for this annotation

Codecov / codecov/patch

src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs#L572-L578

Added lines #L572 - L578 were not covered by tests

return results.ToList();

Check warning on line 580 in src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs

View check run for this annotation

Codecov / codecov/patch

src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs#L580

Added line #L580 was not covered by tests
}
}

Check warning on line 582 in src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs

View check run for this annotation

Codecov / codecov/patch

src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs#L582

Added line #L582 was not covered by tests
}
Original file line number Diff line number Diff line change
Expand Up @@ -733,4 +733,25 @@

await dbContext.UserBumpAccountRevisionDateByOrganizationUserIdsAsync(organizationUserIds);
}

public async Task<IEnumerable<OrganizationUserUserDetails>> GetManyDetailsByRoleAsync(Guid organizationId, OrganizationUserType role)
{
using (var scope = ServiceScopeFactory.CreateScope())
{
var dbContext = GetDatabaseContext(scope);
var query = from ou in dbContext.OrganizationUsers
join u in dbContext.Users
on ou.UserId equals u.Id
where ou.OrganizationId == organizationId &&
ou.Type == role &&
ou.Status == OrganizationUserStatusType.Confirmed
select new OrganizationUserUserDetails
{
Id = ou.Id,
Email = ou.Email ?? u.Email,
Permissions = ou.Permissions
};
return await query.ToListAsync();

Check warning on line 754 in src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs

View check run for this annotation

Codecov / codecov/patch

src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs#L738-L754

Added lines #L738 - L754 were not covered by tests
}
}

Check warning on line 756 in src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs

View check run for this annotation

Codecov / codecov/patch

src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs#L756

Added line #L756 was not covered by tests
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
CREATE PROCEDURE [dbo].[OrganizationUser_ReadManyDetailsByRole]
@OrganizationId UNIQUEIDENTIFIER,
@Role TINYINT
AS
BEGIN
SET NOCOUNT ON

SELECT
*
FROM
[dbo].[OrganizationUserUserDetailsView]
WHERE
OrganizationId = @OrganizationId
AND Status = 2 -- 2 = Confirmed
AND [Type] = @Role
END
32 changes: 30 additions & 2 deletions test/Core.Test/Auth/Services/AuthRequestServiceTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@
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.Platform.Push;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Core.Settings;
using Bit.Core.Utilities;
using Bit.Test.Common.AutoFixture;
using Bit.Test.Common.AutoFixture.Attributes;
using Bit.Test.Common.Helpers;
Expand Down Expand Up @@ -347,14 +349,24 @@ public async Task CreateAuthRequestAsync_AdminApproval_WithAdminNotifications_Cr
User user,
OrganizationUser organizationUser1,
OrganizationUserUserDetails admin1,
OrganizationUserUserDetails customUser1,
OrganizationUser organizationUser2,
OrganizationUserUserDetails admin2,
OrganizationUserUserDetails admin3)
OrganizationUserUserDetails admin3,
OrganizationUserUserDetails customUser2)
{
createModel.Type = AuthRequestType.AdminApproval;
user.Email = createModel.Email;
organizationUser1.UserId = user.Id;
organizationUser2.UserId = user.Id;
customUser1.Permissions = CoreHelpers.ClassToJsonData(new Permissions
{
ManageResetPassword = false,
});
customUser2.Permissions = CoreHelpers.ClassToJsonData(new Permissions
{
ManageResetPassword = true,
});

sutProvider.GetDependency<IFeatureService>()
.IsEnabled(FeatureFlagKeys.DeviceApprovalRequestAdminNotifications)
Expand Down Expand Up @@ -392,6 +404,13 @@ public async Task CreateAuthRequestAsync_AdminApproval_WithAdminNotifications_Cr
admin1,
]);

sutProvider.GetDependency<IOrganizationUserRepository>()
.GetManyDetailsByRoleAsync(organizationUser1.OrganizationId, OrganizationUserType.Custom)
.Returns(
[
customUser1,
]);

sutProvider.GetDependency<IOrganizationUserRepository>()
.GetManyByMinimumRoleAsync(organizationUser2.OrganizationId, OrganizationUserType.Admin)
.Returns(
Expand All @@ -400,6 +419,13 @@ public async Task CreateAuthRequestAsync_AdminApproval_WithAdminNotifications_Cr
admin3,
]);

sutProvider.GetDependency<IOrganizationUserRepository>()
.GetManyDetailsByRoleAsync(organizationUser2.OrganizationId, OrganizationUserType.Custom)
.Returns(
[
customUser2,
]);

sutProvider.GetDependency<IAuthRequestRepository>()
.CreateAsync(Arg.Any<AuthRequest>())
.Returns(c => c.ArgAt<AuthRequest>(0));
Expand Down Expand Up @@ -435,7 +461,9 @@ await sutProvider.GetDependency<IMailService>()
await sutProvider.GetDependency<IMailService>()
.Received(1)
.SendDeviceApprovalRequestedNotificationEmailAsync(
Arg.Is<IEnumerable<string>>(emails => emails.Count() == 2 && emails.Contains(admin2.Email) && emails.Contains(admin3.Email)),
Arg.Is<IEnumerable<string>>(emails => emails.Count() == 3 &&
emails.Contains(admin2.Email) && emails.Contains(admin3.Email) &&
emails.Contains(customUser2.Email)),
organizationUser2.OrganizationId,
user.Email,
user.Name);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
CREATE OR ALTER PROCEDURE [dbo].[OrganizationUser_ReadManyDetailsByRole]
@OrganizationId UNIQUEIDENTIFIER,
@Role TINYINT
AS
BEGIN
SET NOCOUNT ON

SELECT
*
FROM
[dbo].[OrganizationUserUserDetailsView]
WHERE
OrganizationId = @OrganizationId
AND Status = 2 -- 2 = Confirmed
AND [Type] = @Role
END
Loading