[PM-22405] - Remove invalid users from invites

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<OrgUserInviteTokenable> dataProtectorTokenFactory,
-    IMailService mailService) : ISendOrganizationInvitesCommand
+    IMailService mailService,
+    ILogger<SendOrganizationInvitesCommand> 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<OrganizationInvitesInfo> BuildOrganizationInvitesInfoAsync(IEnumerable<OrganizationUser> orgUsers,
-        Organization organization, bool initOrganization = false, string inviterEmail = null)
+    /// <summary>
+    /// Removes Organizations Users that do not have an Email address.
+    /// </summary>
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    private (List<OrganizationUser> OrgUsers, List<string> 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<OrganizationUser>(), Emails: new List<string>()),
+            (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<OrganizationInvitesInfo> BuildOrganizationInvitesInfoAsync(List<OrganizationUser> orgUsers,
+        List<string> 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<string>(existingUsers.Select(u => u.Email));
+        var existingUserEmailsHashSet = new HashSet<string>(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<OrganizationInvitesInfo> 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,

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");
         }

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<IMailService>().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<SendOrganizationInvitesCommand> 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<IUserRepository>().DidNotReceive()
+            .GetManyByEmailsAsync(Arg.Is<IEnumerable<string>>(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<SendOrganizationInvitesCommand> 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<IMailService>().Received(1)
+            .SendUpdatedOrganizationInviteEmailsAsync(Arg.Is<OrganizationInvitesInfo>(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<SendOrganizationInvitesCommand> 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<IMailService>().Received(1)
+            .SendUpdatedOrganizationInviteEmailsAsync(Arg.Is<OrganizationInvitesInfo>(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<SendOrganizationInvitesCommand> sutProvider)
+    {
+        SetupSutProviderWithNoExistingUsers(sutProvider);
+
+        // Arrange
+        inviteWithoutEmail.Email = null;
+
+        // Act
+        await sutProvider.Sut.SendInvitesAsync(new SendInvitesRequest([inviteWithoutEmail], organization));
+
+        // Assert
+        await sutProvider.GetDependency<IMailService>().DidNotReceive()
+            .SendUpdatedOrganizationInviteEmailsAsync(Arg.Any<OrganizationInvitesInfo>());
+    }
+
+    [Theory, BitAutoData]
+    public async Task SendInvitesAsync_WhenAnOrgUserHasNoEmail_WarnsWithTheSkippedOrgUserId(
+        Organization organization,
+        OrganizationUser invite,
+        OrganizationUser inviteWithoutEmail,
+        SutProvider<SendOrganizationInvitesCommand> 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<ILogger<SendOrganizationInvitesCommand>>().Received(1).Log(
+            LogLevel.Warning,
+            Arg.Any<EventId>(),
+            Arg.Is<object>(state =>
+                state.ToString().Contains(inviteWithoutEmail.Id.ToString()) &&
+                state.ToString().Contains(organization.Id.ToString()) &&
+                !state.ToString().Contains(invite.Email)),
+            null,
+            Arg.Any<Func<object, Exception, string>>());
+    }
+
+    [Theory, BitAutoData]
+    public async Task SendInvitesAsync_WhenAccountEmailDiffersInCase_TreatsUserAsExisting(
+        Organization organization,
+        OrganizationUser invite,
+        User existingUser,
+        SutProvider<SendOrganizationInvitesCommand> 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<IUserRepository>()
+            .GetManyByEmailsAsync(Arg.Any<IEnumerable<string>>())
+            .Returns([existingUser]);
+
+        // Act
+        await sutProvider.Sut.SendInvitesAsync(new SendInvitesRequest([invite], organization));
+
+        // Assert
+        await sutProvider.GetDependency<IMailService>().Received(1)
+            .SendUpdatedOrganizationInviteEmailsAsync(Arg.Is<OrganizationInvitesInfo>(info =>
+                info.OrgUserHasExistingUserDict[invite.Id]));
+    }
+
     private void SetupSutProvider(SutProvider<SendOrganizationInvitesCommand> sutProvider)
     {
         sutProvider.SetDependency(_orgUserInviteTokenDataFactory, "orgUserInviteTokenDataFactory");
         sutProvider.Create();
     }
+
+    private void SetupSutProviderWithNoExistingUsers(SutProvider<SendOrganizationInvitesCommand> sutProvider)
+    {
+        SetupSutProvider(sutProvider);
+
+        sutProvider.GetDependency<IUserRepository>()
+            .GetManyByEmailsAsync(Arg.Any<IEnumerable<string>>())
+            .Returns([]);
+
+        sutProvider.GetDependency<IOrgUserInviteTokenableFactory>()
+            .CreateToken(Arg.Any<OrganizationUser>())
+            .Returns(info => new OrgUserInviteTokenable(info.Arg<OrganizationUser>())
+            {
+                ExpirationDate = DateTime.UtcNow.Add(TimeSpan.FromDays(5))
+            });
+    }
 }