diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/InviteUsers/SendOrganizationInvitesCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/InviteUsers/SendOrganizationInvitesCommand.cs index 496c6be275ac..52fb637a498e 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/InviteUsers/SendOrganizationInvitesCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/InviteUsers/SendOrganizationInvitesCommand.cs @@ -5,6 +5,7 @@ using Bit.Core.AdminConsole.Enums; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.InviteUsers.Models; using Bit.Core.AdminConsole.OrganizationFeatures.Policies; +using Bit.Core.AdminConsole.Utilities.DebuggingInstruments; using Bit.Core.Auth.Models.Business; using Bit.Core.Auth.Models.Business.Tokenables; using Bit.Core.Auth.Repositories; @@ -13,6 +14,7 @@ using Bit.Core.Repositories; using Bit.Core.Services; using Bit.Core.Tokens; +using Microsoft.Extensions.Logging; namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.InviteUsers; @@ -22,40 +24,77 @@ public class SendOrganizationInvitesCommand( IPolicyQuery policyQuery, IOrgUserInviteTokenableFactory orgUserInviteTokenableFactory, IDataProtectorTokenFactory dataProtectorTokenFactory, - IMailService mailService) : ISendOrganizationInvitesCommand + IMailService mailService, + ILogger logger) : ISendOrganizationInvitesCommand { public async Task SendInvitesAsync(SendInvitesRequest request) { + var (orgUsers, orgUserEmails) = ExcludeUsersWithoutEmail(request.Users); + if (orgUsers.Count == 0) + { + return; + } + var inviterEmail = await GetInviterEmailAsync(request.InvitingUserId); var orgInvitesInfo = await BuildOrganizationInvitesInfoAsync( - request.Users, request.Organization, request.InitOrganization, inviterEmail); + orgUsers, orgUserEmails, request.Organization, request.InitOrganization, inviterEmail); await mailService.SendUpdatedOrganizationInviteEmailsAsync(orgInvitesInfo); } - private async Task BuildOrganizationInvitesInfoAsync(IEnumerable orgUsers, - Organization organization, bool initOrganization = false, string inviterEmail = null) + /// + /// Removes Organizations Users that do not have an Email address. + /// + /// + /// An invited org user is expected to have an email address, but SSO JIT provisioning can leave it + /// null. We attempt to get the existing users, a NULL value throws an error when mapping to the Emails parameter. + /// + /// Attempting to use the existing User record by populated UserId would also result in a failure because when + /// validating the token, we would fail to look up the organization user with the Email from the User record. + /// Instead, we are opting to dropping the users from the invites to be sent, and we'll log the invalid state for + /// a more complete bug fix. + /// + private (List OrgUsers, List Emails) ExcludeUsersWithoutEmail( + OrganizationUser[] requestedOrgUsers) { - // Materialize the sequence into a list to avoid multiple enumeration warnings - var orgUsersList = orgUsers.ToList(); + var (orgUsers, emails) = requestedOrgUsers.Aggregate( + (OrgUsers: new List(), Emails: new List()), + (aggregate, orgUser) => + { + if (string.IsNullOrWhiteSpace(orgUser.Email)) + { + logger.LogUserInviteStateDiagnostics(orgUser); + return aggregate; + } + + aggregate.OrgUsers.Add(orgUser); + aggregate.Emails.Add(orgUser.Email); + return aggregate; + }); + return (orgUsers, emails); + } + + private async Task BuildOrganizationInvitesInfoAsync(List orgUsers, + List orgUserEmails, Organization organization, bool initOrganization, string inviterEmail) + { // Email links must include information about the org and user for us to make routing decisions client side // Given an org user, determine if existing BW user exists - var orgUserEmails = orgUsersList.Select(ou => ou.Email).ToList(); var existingUsers = await userRepository.GetManyByEmailsAsync(orgUserEmails); // hash existing users emails list for O(1) lookups - var existingUserEmailsHashSet = new HashSet(existingUsers.Select(u => u.Email)); + var existingUserEmailsHashSet = new HashSet(existingUsers.Select(u => u.Email), + StringComparer.OrdinalIgnoreCase); - // Create a dictionary of org user guids and bools for whether or not they have an existing BW user - var orgUserHasExistingUserDict = orgUsersList.ToDictionary( + // Create a dictionary of org user guids and bools for whether they have an existing BW user + var orgUserHasExistingUserDict = orgUsers.ToDictionary( ou => ou.Id, ou => existingUserEmailsHashSet.Contains(ou.Email) ); - // Determine if org has SSO enabled and if user is required to login with SSO + // Determine if org has SSO enabled and if user is required to log in with SSO // Note: we only want to call the DB after checking if the org can use SSO per plan and if they have any policies enabled. var orgSsoEnabled = organization.UseSso && (await ssoConfigurationRepository.GetByOrganizationIdAsync(organization.Id))?.Enabled == true; - // Even though the require SSO policy can be turned on regardless of SSO being enabled, for this logic, we only + // Even though the Require SSO policy can be turned on regardless of SSO being enabled, for this logic, we only // need to check the policy if the org has SSO enabled. var orgSsoLoginRequiredPolicyEnabled = orgSsoEnabled && organization.UsePolicies && @@ -70,7 +109,8 @@ private async Task BuildOrganizationInvitesInfoAsync(IE return (orgUser, new ExpiringToken(protectedToken, orgUserInviteTokenable.ExpirationDate)); } - var orgUsersWithExpTokens = orgUsers.Select(MakeOrgUserExpiringTokenPair); + // Materialized so that consumers enumerating more than once do not mint a new token each time + var orgUsersWithExpTokens = orgUsers.Select(MakeOrgUserExpiringTokenPair).ToList(); return new OrganizationInvitesInfo( organization, diff --git a/src/Core/AdminConsole/Utilities/DebuggingInstruments/UserInviteDebuggingLogger.cs b/src/Core/AdminConsole/Utilities/DebuggingInstruments/UserInviteDebuggingLogger.cs index 6a0e58152251..60a664e9423d 100644 --- a/src/Core/AdminConsole/Utilities/DebuggingInstruments/UserInviteDebuggingLogger.cs +++ b/src/Core/AdminConsole/Utilities/DebuggingInstruments/UserInviteDebuggingLogger.cs @@ -21,25 +21,26 @@ public static void LogUserInviteStateDiagnostics(this ILogger logger, IEnumerabl { try { - var invalidInviteState = allOrgUsers.Any(user => user.Status == OrganizationUserStatusType.Invited && user.Email.IsNullOrWhiteSpace()); + var orgUserList = allOrgUsers.ToList(); + + var invalidInviteState = orgUserList.Any(user => user.Status == OrganizationUserStatusType.Invited && user.Email.IsNullOrWhiteSpace()); if (invalidInviteState) { - var logData = MapObjectDataToLog(allOrgUsers); + var logData = MapObjectDataToLog(orgUserList); logger.LogWarning("Warning invalid invited state. {logData}", logData); } - var invalidConfirmedOrAcceptedState = allOrgUsers.Any(user => (user.Status == OrganizationUserStatusType.Confirmed || user.Status == OrganizationUserStatusType.Accepted) && !user.Email.IsNullOrWhiteSpace()); + var invalidConfirmedOrAcceptedState = orgUserList.Any(user => user.Status is OrganizationUserStatusType.Confirmed or OrganizationUserStatusType.Accepted && !user.Email.IsNullOrWhiteSpace()); if (invalidConfirmedOrAcceptedState) { - var logData = MapObjectDataToLog(allOrgUsers); - logger.LogWarning("Warning invalid confirmed or accepted state. {logData}", logData); + var logData = MapObjectDataToLog(orgUserList); + logger.LogWarning("Warning invalid confirmed or accepted state. {LogData}", logData); } } catch (Exception exception) { - // Ensure that this debugging instrument does not interfere with the current flow. logger.LogWarning(exception, "Unexpected exception from UserInviteDebuggingLogger"); } diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/InviteUsers/SendOrganizationInvitesCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/InviteUsers/SendOrganizationInvitesCommandTests.cs index 63aa39a74e65..b203583d98d9 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/InviteUsers/SendOrganizationInvitesCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/InviteUsers/SendOrganizationInvitesCommandTests.cs @@ -9,6 +9,7 @@ using Bit.Core.Auth.Repositories; using Bit.Core.Billing.Enums; using Bit.Core.Entities; +using Bit.Core.Enums; using Bit.Core.Models.Mail; using Bit.Core.Repositories; using Bit.Core.Services; @@ -18,6 +19,7 @@ using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; using Bit.Test.Common.Fakes; +using Microsoft.Extensions.Logging; using NSubstitute; using NSubstitute.ReturnsExtensions; using Xunit; @@ -264,9 +266,175 @@ await sutProvider.GetDependency().Received(1) info.InviterEmail == null)); } + [Theory] + [BitAutoData((string)null)] + [BitAutoData("")] + [BitAutoData(" ")] + public async Task SendInvitesAsync_WhenAnOrgUserHasNoEmail_DoesNotLookThatEmailUp( + string blankEmail, + Organization organization, + OrganizationUser invite, + OrganizationUser inviteWithoutEmail, + SutProvider sutProvider) + { + SetupSutProviderWithNoExistingUsers(sutProvider); + + // Arrange - an invited org user with no email, as left behind by SSO just-in-time provisioning + inviteWithoutEmail.Email = blankEmail; + + // Act + await sutProvider.Sut.SendInvitesAsync(new SendInvitesRequest([invite, inviteWithoutEmail], organization)); + + // Assert - a blank email in this lookup fails the whole send against SQL Server + await sutProvider.GetDependency().DidNotReceive() + .GetManyByEmailsAsync(Arg.Is>(emails => emails.Any(string.IsNullOrWhiteSpace))); + } + + [Theory] + [BitAutoData((string)null)] + [BitAutoData("")] + [BitAutoData(" ")] + public async Task SendInvitesAsync_WhenAnOrgUserHasNoEmail_StillSendsTheOtherInvites( + string blankEmail, + Organization organization, + OrganizationUser invite, + OrganizationUser inviteWithoutEmail, + User linkedUser, + SutProvider sutProvider) + { + SetupSutProviderWithNoExistingUsers(sutProvider); + + // Arrange - a linked UserId does not make the org user invitable, the invite token is validated + // against the stored email at accept time + inviteWithoutEmail.Email = blankEmail; + inviteWithoutEmail.UserId = linkedUser.Id; + + // Act + await sutProvider.Sut.SendInvitesAsync(new SendInvitesRequest([invite, inviteWithoutEmail], organization)); + + // Assert + await sutProvider.GetDependency().Received(1) + .SendUpdatedOrganizationInviteEmailsAsync(Arg.Is(info => + info.OrgUserTokenPairs.Count() == 1 && + info.OrgUserTokenPairs.Single().OrgUser.Id == invite.Id)); + } + + [Theory, BitAutoData] + public async Task SendInvitesAsync_WhenAnOrgUserHasNoEmail_MailInfoIsInternallyConsistent( + Organization organization, + OrganizationUser invite, + OrganizationUser inviteWithoutEmail, + SutProvider sutProvider) + { + SetupSutProviderWithNoExistingUsers(sutProvider); + + // Arrange + inviteWithoutEmail.Email = null; + + // Act + await sutProvider.Sut.SendInvitesAsync(new SendInvitesRequest([invite, inviteWithoutEmail], organization)); + + // Assert - every mailed org user needs a recipient and an entry in the existing user dictionary + await sutProvider.GetDependency().Received(1) + .SendUpdatedOrganizationInviteEmailsAsync(Arg.Is(info => + info.OrgUserTokenPairs.All(pair => + !string.IsNullOrWhiteSpace(pair.OrgUser.Email) && + info.OrgUserHasExistingUserDict.ContainsKey(pair.OrgUser.Id)))); + } + + [Theory, BitAutoData] + public async Task SendInvitesAsync_WhenNoOrgUserHasAnEmail_SendsNothing( + Organization organization, + OrganizationUser inviteWithoutEmail, + SutProvider sutProvider) + { + SetupSutProviderWithNoExistingUsers(sutProvider); + + // Arrange + inviteWithoutEmail.Email = null; + + // Act + await sutProvider.Sut.SendInvitesAsync(new SendInvitesRequest([inviteWithoutEmail], organization)); + + // Assert + await sutProvider.GetDependency().DidNotReceive() + .SendUpdatedOrganizationInviteEmailsAsync(Arg.Any()); + } + + [Theory, BitAutoData] + public async Task SendInvitesAsync_WhenAnOrgUserHasNoEmail_WarnsWithTheSkippedOrgUserId( + Organization organization, + OrganizationUser invite, + OrganizationUser inviteWithoutEmail, + SutProvider sutProvider) + { + SetupSutProviderWithNoExistingUsers(sutProvider); + + // Arrange + inviteWithoutEmail.Email = null; + inviteWithoutEmail.Status = OrganizationUserStatusType.Invited; + inviteWithoutEmail.OrganizationId = organization.Id; + + // Act + await sutProvider.Sut.SendInvitesAsync(new SendInvitesRequest([invite, inviteWithoutEmail], organization)); + + // Assert - the log is the only signal an operator gets, and it must not carry an email address + sutProvider.GetDependency>().Received(1).Log( + LogLevel.Warning, + Arg.Any(), + Arg.Is(state => + state.ToString().Contains(inviteWithoutEmail.Id.ToString()) && + state.ToString().Contains(organization.Id.ToString()) && + !state.ToString().Contains(invite.Email)), + null, + Arg.Any>()); + } + + [Theory, BitAutoData] + public async Task SendInvitesAsync_WhenAccountEmailDiffersInCase_TreatsUserAsExisting( + Organization organization, + OrganizationUser invite, + User existingUser, + SutProvider sutProvider) + { + SetupSutProviderWithNoExistingUsers(sutProvider); + + // Arrange - email lookups are case insensitive everywhere else in the invite flow + invite.Email = "member@example.com"; + existingUser.Email = "Member@Example.com"; + + sutProvider.GetDependency() + .GetManyByEmailsAsync(Arg.Any>()) + .Returns([existingUser]); + + // Act + await sutProvider.Sut.SendInvitesAsync(new SendInvitesRequest([invite], organization)); + + // Assert + await sutProvider.GetDependency().Received(1) + .SendUpdatedOrganizationInviteEmailsAsync(Arg.Is(info => + info.OrgUserHasExistingUserDict[invite.Id])); + } + private void SetupSutProvider(SutProvider sutProvider) { sutProvider.SetDependency(_orgUserInviteTokenDataFactory, "orgUserInviteTokenDataFactory"); sutProvider.Create(); } + + private void SetupSutProviderWithNoExistingUsers(SutProvider sutProvider) + { + SetupSutProvider(sutProvider); + + sutProvider.GetDependency() + .GetManyByEmailsAsync(Arg.Any>()) + .Returns([]); + + sutProvider.GetDependency() + .CreateToken(Arg.Any()) + .Returns(info => new OrgUserInviteTokenable(info.Arg()) + { + ExpirationDate = DateTime.UtcNow.Add(TimeSpan.FromDays(5)) + }); + } }