[PM-21612] [Unified] Fix unhandled error when editing an invited member

src/Core/Entities/User.cs

@@ -36,6 +36,11 @@ public class User : ITableObject<Guid>, IStorableSubscriber, IRevisable, ITwoFac
     public string? TwoFactorRecoveryCode { get; set; }
     public string? EquivalentDomains { get; set; }
     public string? ExcludedGlobalEquivalentDomains { get; set; }
+    /// <summary>
+    /// The Account Revision Date is used to check if new sync needs to occur. It should be updated
+    /// whenever a change is made that affects a client's sync data; for example, updating their vault or
+    /// organization membership.
+    /// </summary>
     public DateTime AccountRevisionDate { get; set; } = DateTime.UtcNow;
     public string? Key { get; set; }
     public string? PublicKey { get; set; }

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

@@ -1,4 +1,5 @@
-using AutoMapper;
+using System.Diagnostics;
+using AutoMapper;
 using Bit.Core.AdminConsole.Enums;
 using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.InviteUsers.Models;
 using Bit.Core.Enums;
@@ -7,11 +8,12 @@
 using Bit.Core.Models.Data.Organizations.OrganizationUsers;
 using Bit.Core.Repositories;
 using Bit.Infrastructure.EntityFramework.Models;
+using Bit.Infrastructure.EntityFramework.Repositories;
 using Bit.Infrastructure.EntityFramework.Repositories.Queries;
 using Microsoft.EntityFrameworkCore;
 using Microsoft.Extensions.DependencyInjection;
 
-namespace Bit.Infrastructure.EntityFramework.Repositories;
+namespace Bit.Infrastructure.EntityFramework.AdminConsole.Repositories;
 
 public class OrganizationUserRepository : Repository<Core.Entities.OrganizationUser, OrganizationUser, Guid>, IOrganizationUserRepository
 {
@@ -440,15 +442,23 @@ on ou.UserId equals u.Id
         }
     }
 
-    public async override Task ReplaceAsync(Core.Entities.OrganizationUser organizationUser)
+    public override async Task ReplaceAsync(Core.Entities.OrganizationUser organizationUser)
     {
         await base.ReplaceAsync(organizationUser);
-        using (var scope = ServiceScopeFactory.CreateScope())
+
+        // Only bump account revision dates for confirmed OrgUsers,
+        // as this is the only status that receives sync data from the organization
+        if (organizationUser.Status is not OrganizationUserStatusType.Confirmed)
         {
-            var dbContext = GetDatabaseContext(scope);
-            await dbContext.UserBumpAccountRevisionDateAsync(organizationUser.UserId.GetValueOrDefault());
-            await dbContext.SaveChangesAsync();
+            return;
         }
+
+        Debug.Assert(organizationUser.UserId is not null, "OrganizationUser is confirmed but does not have a UserId.");
+
+        using var scope = ServiceScopeFactory.CreateScope();
+        var dbContext = GetDatabaseContext(scope);
+        await dbContext.UserBumpAccountRevisionDateAsync(organizationUser.UserId.Value);
+        await dbContext.SaveChangesAsync();
     }
 
     public async Task ReplaceAsync(Core.Entities.OrganizationUser obj, IEnumerable<CollectionAccessSelection> requestedCollections)

src/Infrastructure.EntityFramework/Repositories/DatabaseContextExtensions.cs

@@ -1,4 +1,6 @@
-using System.Diagnostics;
+#nullable enable
+
+using System.Diagnostics;
 using Bit.Core.AdminConsole.Enums.Provider;
 using Bit.Core.Auth.Enums;
 using Bit.Core.Enums;
@@ -11,8 +13,18 @@
 
 public static class DatabaseContextExtensions
 {
+    /// <summary>
+    /// Bump the account revision date for the user.
+    /// The caller is responsible for providing a valid UserId (not a null or default Guid) for a user that exists
+    /// in the database.
+    /// </summary>
     public static async Task UserBumpAccountRevisionDateAsync(this DatabaseContext context, Guid userId)
     {
+        if (userId == Guid.Empty)
+        {
+            throw new ArgumentException("Invalid UserId.");
+        }
+
         var user = await context.Users.FindAsync(userId);
         Debug.Assert(user is not null, "The user id is expected to be validated as a true-in database user before making this call.");
         user.AccountRevisionDate = DateTime.UtcNow;

src/Infrastructure.EntityFramework/Vault/Repositories/CipherRepository.cs

@@ -144,6 +144,7 @@
 
     public async Task CreateAsync(IEnumerable<Core.Vault.Entities.Cipher> ciphers, IEnumerable<Core.Vault.Entities.Folder> folders)
     {
+        ciphers = ciphers.ToList();
         if (!ciphers.Any())
         {
             return;
@@ -156,7 +157,14 @@
             await dbContext.BulkCopyAsync(base.DefaultBulkCopyOptions, folderEntities);
             var cipherEntities = Mapper.Map<List<Cipher>>(ciphers);
             await dbContext.BulkCopyAsync(base.DefaultBulkCopyOptions, cipherEntities);
-            await dbContext.UserBumpAccountRevisionDateAsync(ciphers.First().UserId.GetValueOrDefault());
+
+            // Assumption: all ciphers belong to the same user
+            var userId = ciphers.FirstOrDefault(c => c.UserId.HasValue)?.UserId;
+            if (userId != null)
+            {
+                await dbContext.UserBumpAccountRevisionDateAsync(userId.Value);
+            }
+
             await dbContext.SaveChangesAsync();
         }
     }
@@ -902,7 +910,7 @@
             var foldersJson = NSL.JObject.Parse(cipher.Folders);
             if (foldersJson == null && folderId.HasValue)
             {
                 foldersJson.Add(userId.ToString(), folderId.Value);
             }
             else if (foldersJson != null && folderId.HasValue)
             {
@@ -910,7 +918,7 @@
             }
             else
             {
                 foldersJson.Remove(userId.ToString());
             }
 
             var favoritesJson = NSL.JObject.Parse(cipher.Favorites);

test/Infrastructure.EFIntegration.Test/AdminConsole/Repositories/OrganizationRepositoryTests.cs

@@ -7,6 +7,7 @@
 using Bit.Infrastructure.EFIntegration.Test.AutoFixture;
 using Bit.Infrastructure.EFIntegration.Test.Repositories.EqualityComparers;
 using Bit.Infrastructure.EntityFramework.AdminConsole.Models;
+using Bit.Infrastructure.EntityFramework.AdminConsole.Repositories;
 using Xunit;
 using EfRepo = Bit.Infrastructure.EntityFramework.Repositories;
 using Organization = Bit.Core.AdminConsole.Entities.Organization;
@@ -161,7 +162,7 @@ public async Task GetManyAbilitiesAsync_Works(SqlRepo.OrganizationRepository sql
 
     [CiSkippedTheory, EfOrganizationUserAutoData]
     public async Task SearchUnassignedAsync_Works(OrganizationUser orgUser, User user, Organization org,
-        List<EfRepo.OrganizationUserRepository> efOrgUserRepos, List<EfRepo.OrganizationRepository> efOrgRepos, List<EfRepo.UserRepository> efUserRepos,
+        List<OrganizationUserRepository> efOrgUserRepos, List<EfRepo.OrganizationRepository> efOrgRepos, List<EfRepo.UserRepository> efUserRepos,
         SqlRepo.OrganizationUserRepository sqlOrgUserRepo, SqlRepo.OrganizationRepository sqlOrgRepo, SqlRepo.UserRepository sqlUserRepo)
     {
         orgUser.Type = OrganizationUserType.Owner;

test/Infrastructure.EFIntegration.Test/AdminConsole/Repositories/OrganizationUserRepositoryTests.cs

@@ -24,7 +24,7 @@ public class OrganizationUserRepositoryTests
 {
     [CiSkippedTheory, EfOrganizationUserAutoData]
     public async Task CreateAsync_Works_DataMatches(OrganizationUser orgUser, User user, Organization org,
-        OrganizationUserCompare equalityComparer, List<EfRepo.OrganizationUserRepository> suts,
+        OrganizationUserCompare equalityComparer, List<EfAdminConsoleRepo.OrganizationUserRepository> suts,
         List<EfRepo.OrganizationRepository> efOrgRepos, List<EfRepo.UserRepository> efUserRepos,
         SqlRepo.OrganizationUserRepository sqlOrgUserRepo, SqlRepo.UserRepository sqlUserRepo,
         SqlRepo.OrganizationRepository sqlOrgRepo)
@@ -67,7 +67,7 @@ public async Task ReplaceAsync_Works_DataMatches(
         User user,
         Organization org,
         OrganizationUserCompare equalityComparer,
-        List<EfRepo.OrganizationUserRepository> suts,
+        List<EfAdminConsoleRepo.OrganizationUserRepository> suts,
         List<EfRepo.UserRepository> efUserRepos,
         List<EfRepo.OrganizationRepository> efOrgRepos,
         SqlRepo.OrganizationUserRepository sqlOrgUserRepo,
@@ -113,7 +113,7 @@ SqlRepo.OrganizationRepository sqlOrgRepo
     }
 
     [CiSkippedTheory, EfOrganizationUserAutoData]
-    public async Task DeleteAsync_Works_DataMatches(OrganizationUser orgUser, User user, Organization org, List<EfRepo.OrganizationUserRepository> suts,
+    public async Task DeleteAsync_Works_DataMatches(OrganizationUser orgUser, User user, Organization org, List<EfAdminConsoleRepo.OrganizationUserRepository> suts,
         List<EfRepo.UserRepository> efUserRepos, List<EfRepo.OrganizationRepository> efOrgRepos,
         SqlRepo.OrganizationUserRepository sqlOrgUserRepo, SqlRepo.UserRepository sqlUserRepo,
         SqlRepo.OrganizationRepository sqlOrgRepo)
@@ -188,7 +188,7 @@ public async Task GetByUserIdWithPolicyDetailsAsync_Works_DataMatches(
         List<EfAdminConsoleRepo.PolicyRepository> efPolicyRepository,
         List<EfRepo.UserRepository> efUserRepository,
         List<EfRepo.OrganizationRepository> efOrganizationRepository,
-        List<EfRepo.OrganizationUserRepository> suts,
+        List<EfAdminConsoleRepo.OrganizationUserRepository> suts,
         List<EfAdminConsoleRepo.ProviderRepository> efProviderRepository,
         List<EfAdminConsoleRepo.ProviderOrganizationRepository> efProviderOrganizationRepository,
         List<EfAdminConsoleRepo.ProviderUserRepository> efProviderUserRepository,

test/Infrastructure.EFIntegration.Test/AutoFixture/OrganizationUserFixtures.cs

@@ -7,6 +7,7 @@
 using Bit.Core.Models.Data;
 using Bit.Core.Test.AutoFixture.OrganizationUserFixtures;
 using Bit.Core.Test.AutoFixture.UserFixtures;
+using Bit.Infrastructure.EntityFramework.AdminConsole.Repositories;
 using Bit.Infrastructure.EntityFramework.Repositories;
 using Bit.Test.Common.AutoFixture;
 using Bit.Test.Common.AutoFixture.Attributes;

test/Infrastructure.EFIntegration.Test/Vault/AutoFixture/CipherFixtures.cs

@@ -5,6 +5,7 @@
 using Bit.Core.Vault.Entities;
 using Bit.Core.Vault.Models.Data;
 using Bit.Infrastructure.EFIntegration.Test.AutoFixture.Relays;
+using Bit.Infrastructure.EntityFramework.AdminConsole.Repositories;
 using Bit.Infrastructure.EntityFramework.Repositories;
 using Bit.Infrastructure.EntityFramework.Vault.Repositories;
 using Bit.Test.Common.AutoFixture;

test/Infrastructure.EFIntegration.Test/Vault/Repositories/CipherRepositoryTests.cs

@@ -9,6 +9,7 @@
 using Bit.Test.Common.AutoFixture.Attributes;
 using LinqToDB;
 using Xunit;
+using EfAdminConsoleRepo = Bit.Infrastructure.EntityFramework.AdminConsole.Repositories;
 using EfRepo = Bit.Infrastructure.EntityFramework.Repositories;
 using EfVaultRepo = Bit.Infrastructure.EntityFramework.Vault.Repositories;
 using SqlRepo = Bit.Infrastructure.Dapper.Repositories;
@@ -112,7 +113,7 @@ public async Task CreateAsync_BumpsUserAccountRevisionDate(Cipher cipher, User u
     [CiSkippedTheory, EfOrganizationCipherCustomize, BitAutoData]
     public async Task CreateAsync_BumpsOrgUserAccountRevisionDates(Cipher cipher, List<User> users,
         List<OrganizationUser> orgUsers, Collection collection, Organization org, List<EfVaultRepo.CipherRepository> suts, List<EfRepo.UserRepository> efUserRepos, List<EfRepo.OrganizationRepository> efOrgRepos,
-        List<EfRepo.OrganizationUserRepository> efOrgUserRepos, List<EfRepo.CollectionRepository> efCollectionRepos)
+        List<EfAdminConsoleRepo.OrganizationUserRepository> efOrgUserRepos, List<EfRepo.CollectionRepository> efCollectionRepos)
     {
         var savedCiphers = new List<Cipher>();
         foreach (var sut in suts)

test/Infrastructure.IntegrationTest/AdminConsole/OrganizationTestHelpers.cs

@@ -47,11 +47,32 @@ public static Task<OrganizationUser> CreateTestOrganizationUserAsync(
             Type = OrganizationUserType.Owner
         });
 
+    public static Task<OrganizationUser> CreateTestOrganizationUserInviteAsync(
+        this IOrganizationUserRepository organizationUserRepository,
+        Organization organization)
+        => organizationUserRepository.CreateAsync(new OrganizationUser
+        {
+            OrganizationId = organization.Id,
+            UserId = null, // Invites are not linked to a UserId
+            Status = OrganizationUserStatusType.Invited,
+            Type = OrganizationUserType.Owner
+        });
+
     public static Task<Group> CreateTestGroupAsync(
         this IGroupRepository groupRepository,
         Organization organization,
         string identifier = "test")
         => groupRepository.CreateAsync(
             new Group { OrganizationId = organization.Id, Name = $"{identifier} {Guid.NewGuid()}" }
         );
+
+    public static Task<Collection> CreateTestCollectionAsync(
+        this ICollectionRepository collectionRepository,
+        Organization organization,
+        string identifier = "test")
+    => collectionRepository.CreateAsync(new Collection
+    {
+        OrganizationId = organization.Id,
+        Name = $"{identifier} {Guid.NewGuid()}"
+    });
 }

test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepository/OrganizationUserReplaceTests.cs

@@ -0,0 +1,88 @@
+using Bit.Core.Enums;
+using Bit.Core.Models.Data;
+using Bit.Core.Repositories;
+using Xunit;
+
+namespace Bit.Infrastructure.IntegrationTest.AdminConsole.Repositories.OrganizationUserRepository;
+
+public class OrganizationUserReplaceTests
+{
+    /// <summary>
+    /// Specifically tests OrganizationUsers in the invited state, which is unique because
+    /// they're not linked to a UserId.
+    /// </summary>
+    [DatabaseTheory, DatabaseData]
+    public async Task ReplaceAsync_WithCollectionAccess_WhenUserIsInvited_Success(
+        IOrganizationRepository organizationRepository,
+        IOrganizationUserRepository organizationUserRepository,
+        ICollectionRepository collectionRepository)
+    {
+        var organization = await organizationRepository.CreateTestOrganizationAsync();
+
+        var orgUser = await organizationUserRepository.CreateTestOrganizationUserInviteAsync(organization);
+
+        // Act: update the user, including collection access so we test this overloaded method
+        orgUser.Type = OrganizationUserType.Admin;
+        orgUser.AccessSecretsManager = true;
+        var collection = await collectionRepository.CreateTestCollectionAsync(organization);
+
+        await organizationUserRepository.ReplaceAsync(orgUser, [
+            new CollectionAccessSelection { Id = collection.Id, Manage = true }
+        ]);
+
+        // Assert
+        var (actualOrgUser, actualCollections) = await organizationUserRepository.GetByIdWithCollectionsAsync(orgUser.Id);
+        Assert.NotNull(actualOrgUser);
+        Assert.Equal(OrganizationUserType.Admin, actualOrgUser.Type);
+        Assert.True(actualOrgUser.AccessSecretsManager);
+
+        var collectionAccess = Assert.Single(actualCollections);
+        Assert.Equal(collection.Id, collectionAccess.Id);
+        Assert.True(collectionAccess.Manage);
+    }
+
+    /// <summary>
+    /// Tests OrganizationUsers in the Confirmed status, which is a stand-in for all other
+    /// non-Invited statuses (which are all linked to a UserId).
+    /// </summary>
+    /// <param name="organizationRepository"></param>
+    /// <param name="organizationUserRepository"></param>
+    /// <param name="collectionRepository"></param>
+    [DatabaseTheory, DatabaseData]
+    public async Task ReplaceAsync_WithCollectionAccess_WhenUserIsConfirmed_Success(
+        IUserRepository userRepository,
+        IOrganizationRepository organizationRepository,
+        IOrganizationUserRepository organizationUserRepository,
+        ICollectionRepository collectionRepository)
+    {
+        var organization = await organizationRepository.CreateTestOrganizationAsync();
+
+        var user = await userRepository.CreateTestUserAsync();
+        // OrganizationUser is linked with the User in the Confirmed status
+        var orgUser = await organizationUserRepository.CreateTestOrganizationUserAsync(organization, user);
+
+        // Act: update the user, including collection access so we test this overloaded method
+        orgUser.Type = OrganizationUserType.Admin;
+        orgUser.AccessSecretsManager = true;
+        var collection = await collectionRepository.CreateTestCollectionAsync(organization);
+
+        await organizationUserRepository.ReplaceAsync(orgUser, [
+            new CollectionAccessSelection { Id = collection.Id, Manage = true }
+        ]);
+
+        // Assert
+        var (actualOrgUser, actualCollections) = await organizationUserRepository.GetByIdWithCollectionsAsync(orgUser.Id);
+        Assert.NotNull(actualOrgUser);
+        Assert.Equal(OrganizationUserType.Admin, actualOrgUser.Type);
+        Assert.True(actualOrgUser.AccessSecretsManager);
+
+        var collectionAccess = Assert.Single(actualCollections);
+        Assert.Equal(collection.Id, collectionAccess.Id);
+        Assert.True(collectionAccess.Manage);
+
+        // Account revision date should be updated to a later date
+        var actualUser = await userRepository.GetByIdAsync(user.Id);
+        Assert.NotNull(actualUser);
+        Assert.True(actualUser.AccountRevisionDate.CompareTo(user.AccountRevisionDate) > 0);
+    }
+}

test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepositoryTests.cs → test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepository/OrganizationUserRepositoryTests.cs

@@ -8,7 +8,7 @@
 using Bit.Core.Utilities;
 using Xunit;
 
-namespace Bit.Infrastructure.IntegrationTest.AdminConsole.Repositories;
+namespace Bit.Infrastructure.IntegrationTest.AdminConsole.Repositories.OrganizationUserRepository;
 
 public class OrganizationUserRepositoryTests
 {