[PM-863] Fix Organization Folders in EF Databases (#2856)

* Fix Setting Organization Folders

* Fix Formatting

* Added ReplaceAsync Test

* Fix SQL Server Test

* Update Replace Call Also

* Be Case Insensitive With Guids

* Fix Assignment to Cipher

src/Infrastructure.EntityFramework/Repositories/Queries/UserCipherDetailsQuery.cs

@@ -50,7 +50,7 @@ public class UserCipherDetailsQuery : IQuery<CipherDetails>
 
                     where ou.AccessAll || cu.CollectionId != null || g.AccessAll || cg.CollectionId != null
 
-                    select new { c, ou, o, cc, cu, gu, g, cg }.c;
+                    select c;
 
         var query2 = from c in dbContext.Ciphers
                      where c.UserId == _userId
@@ -79,14 +79,23 @@ public class UserCipherDetailsQuery : IQuery<CipherDetails>
 
     private static Guid? GetFolderId(Guid? userId, Cipher cipher)
     {
-        if (userId.HasValue && !string.IsNullOrWhiteSpace(cipher.Folders))
+        try
         {
-            var folders = JsonSerializer.Deserialize<Dictionary<Guid, Guid>>(cipher.Folders);
+            if (userId.HasValue && !string.IsNullOrWhiteSpace(cipher.Folders))
-            if (folders.TryGetValue(userId.Value, out var folder))
             {
-                return folder;
+                var folders = JsonSerializer.Deserialize<Dictionary<Guid, Guid>>(cipher.Folders);
+                if (folders.TryGetValue(userId.Value, out var folder))
+                {
+                    return folder;
+                }
             }
+
+            return null;
+        }
+        catch
+        {
+            // Some Folders might be in an invalid format like: '{ "", "<ValidGuid>" }'
+            return null;
         }
-        return null;
     }
 }

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

@@ -1,4 +1,6 @@
-using AutoMapper;
+using System.Text.Json;
+using System.Text.Json.Nodes;
+using AutoMapper;
 using Bit.Core.Enums;
 using Bit.Core.Utilities;
 using Bit.Core.Vault.Enums;
@@ -13,8 +15,8 @@ using Bit.Infrastructure.EntityFramework.Vault.Repositories.Queries;
 using LinqToDB.EntityFrameworkCore;
 using Microsoft.EntityFrameworkCore;
 using Microsoft.Extensions.DependencyInjection;
-using Newtonsoft.Json;
+using NS = Newtonsoft.Json;
-using Newtonsoft.Json.Linq;
+using NSL = Newtonsoft.Json.Linq;
 using User = Bit.Core.Entities.User;
 
 namespace Bit.Infrastructure.EntityFramework.Vault.Repositories;
@@ -198,9 +200,9 @@ public class CipherRepository : Repository<Core.Vault.Entities.Cipher, Cipher, G
         {
             var dbContext = GetDatabaseContext(scope);
             var cipher = await dbContext.Ciphers.FindAsync(cipherId);
-            var attachmentsJson = JObject.Parse(cipher.Attachments);
+            var attachmentsJson = NSL.JObject.Parse(cipher.Attachments);
             attachmentsJson.Remove(attachmentId);
-            cipher.Attachments = JsonConvert.SerializeObject(attachmentsJson);
+            cipher.Attachments = NS.JsonConvert.SerializeObject(attachmentsJson);
             await dbContext.SaveChangesAsync();
 
             if (cipher.OrganizationId.HasValue)
@@ -396,8 +398,8 @@ public class CipherRepository : Repository<Core.Vault.Entities.Cipher, Cipher, G
             await idsToMove.ForEachAsync(cipher =>
             {
                 var foldersJson = string.IsNullOrWhiteSpace(cipher.Folders) ?
-                    new JObject() :
+                    new NSL.JObject() :
-                    JObject.Parse(cipher.Folders);
+                    NSL.JObject.Parse(cipher.Folders);
 
                 if (folderId.HasValue)
                 {
@@ -409,7 +411,7 @@ public class CipherRepository : Repository<Core.Vault.Entities.Cipher, Cipher, G
                     foldersJson.Remove(userId.ToString());
                 }
                 dbContext.Attach(cipher);
-                cipher.Folders = JsonConvert.SerializeObject(foldersJson);
+                cipher.Folders = NS.JsonConvert.SerializeObject(foldersJson);
             });
             await dbContext.UserBumpAccountRevisionDateAsync(userId);
             await dbContext.SaveChangesAsync();
@@ -418,27 +420,27 @@ public class CipherRepository : Repository<Core.Vault.Entities.Cipher, Cipher, G
 
     public async Task ReplaceAsync(CipherDetails cipher)
     {
-        cipher.UserId = cipher.OrganizationId.HasValue ?
-            null :
-            cipher.UserId;
         using (var scope = ServiceScopeFactory.CreateScope())
         {
             var dbContext = GetDatabaseContext(scope);
             var entity = await dbContext.Ciphers.FindAsync(cipher.Id);
             if (entity != null)
             {
-                var userIdKey = $"\"{cipher.UserId}\"";
                 if (cipher.Favorite)
                 {
                     if (cipher.Favorites == null)
                     {
-                        cipher.Favorites = $"{{{userIdKey}:true}}";
+                        var jsonObject = new JsonObject(new[]
+                        {
+                            new KeyValuePair<string, JsonNode>(cipher.UserId.Value.ToString(), true),
+                        });
+                        cipher.Favorites = JsonSerializer.Serialize(jsonObject);
                     }
                     else
                     {
                         var favorites = CoreHelpers.LoadClassFromJsonData<Dictionary<Guid, bool>>(cipher.Favorites);
                         favorites.Add(cipher.UserId.Value, true);
-                        cipher.Favorites = JsonConvert.SerializeObject(favorites);
+                        cipher.Favorites = JsonSerializer.Serialize(favorites);
                     }
                 }
                 else
@@ -447,32 +449,45 @@ public class CipherRepository : Repository<Core.Vault.Entities.Cipher, Cipher, G
                     {
                         var favorites = CoreHelpers.LoadClassFromJsonData<Dictionary<Guid, bool>>(cipher.Favorites);
                         favorites.Remove(cipher.UserId.Value);
-                        cipher.Favorites = JsonConvert.SerializeObject(favorites);
+                        cipher.Favorites = JsonSerializer.Serialize(favorites);
                     }
                 }
                 if (cipher.FolderId.HasValue)
                 {
                     if (cipher.Folders == null)
                     {
-                        cipher.Folders = $"{{{userIdKey}:\"{cipher.FolderId}\"}}";
+                        var jsonObject = new JsonObject(new[]
+                        {
+                            new KeyValuePair<string, JsonNode>(cipher.UserId.Value.ToString(), cipher.FolderId),
+                        });
+                        cipher.Folders = JsonSerializer.Serialize(jsonObject);
                     }
                     else
                     {
                         var folders = CoreHelpers.LoadClassFromJsonData<Dictionary<Guid, Guid>>(cipher.Folders);
                         folders.Add(cipher.UserId.Value, cipher.FolderId.Value);
-                        cipher.Folders = JsonConvert.SerializeObject(folders);
+                        cipher.Folders = JsonSerializer.Serialize(folders);
                     }
                 }
                 else
                 {
                     if (cipher.Folders != null && cipher.Folders.Contains(cipher.UserId.Value.ToString()))
                     {
-                        var folders = CoreHelpers.LoadClassFromJsonData<Dictionary<Guid, bool>>(cipher.Favorites);
+                        var folders = CoreHelpers.LoadClassFromJsonData<Dictionary<Guid, Guid>>(cipher.Folders);
                         folders.Remove(cipher.UserId.Value);
-                        cipher.Favorites = JsonConvert.SerializeObject(folders);
+                        cipher.Folders = JsonSerializer.Serialize(folders);
                     }
                 }
-                var mappedEntity = Mapper.Map<Cipher>((Core.Vault.Entities.Cipher)cipher);
+
+                // Check if this cipher is a part of an organization, and if so do
+                // not save the UserId into the database. This must be done after we
+                // set the user specific data like Folders and Favorites because
+                // the UserId key is used for that
+                cipher.UserId = cipher.OrganizationId.HasValue ?
+                    null :
+                    cipher.UserId;
+
+                var mappedEntity = Mapper.Map<Cipher>(cipher);
                 dbContext.Entry(entity).CurrentValues.SetValues(mappedEntity);
 
                 if (cipher.OrganizationId.HasValue)
@@ -701,10 +716,10 @@ public class CipherRepository : Repository<Core.Vault.Entities.Cipher, Cipher, G
             var cipher = await dbContext.Ciphers.FindAsync(attachment.Id);
             var attachments = string.IsNullOrWhiteSpace(cipher.Attachments) ?
                 new Dictionary<string, CipherAttachment.MetaData>() :
-                JsonConvert.DeserializeObject<Dictionary<string, CipherAttachment.MetaData>>(cipher.Attachments);
+                NS.JsonConvert.DeserializeObject<Dictionary<string, CipherAttachment.MetaData>>(cipher.Attachments);
-            var metaData = JsonConvert.DeserializeObject<CipherAttachment.MetaData>(attachment.AttachmentData);
+            var metaData = NS.JsonConvert.DeserializeObject<CipherAttachment.MetaData>(attachment.AttachmentData);
             attachments[attachment.AttachmentId] = metaData;
-            cipher.Attachments = JsonConvert.SerializeObject(attachments);
+            cipher.Attachments = NS.JsonConvert.SerializeObject(attachments);
             await dbContext.SaveChangesAsync();
 
             if (attachment.OrganizationId.HasValue)
@@ -744,7 +759,7 @@ public class CipherRepository : Repository<Core.Vault.Entities.Cipher, Cipher, G
             var dbContext = GetDatabaseContext(scope);
             var cipher = await dbContext.Ciphers.FindAsync(id);
 
-            var foldersJson = JObject.Parse(cipher.Folders);
+            var foldersJson = NSL.JObject.Parse(cipher.Folders);
             if (foldersJson == null && folderId.HasValue)
             {
                 foldersJson.Add(userId.ToString(), folderId.Value);
@@ -758,7 +773,7 @@ public class CipherRepository : Repository<Core.Vault.Entities.Cipher, Cipher, G
                 foldersJson.Remove(userId.ToString());
             }
 
-            var favoritesJson = JObject.Parse(cipher.Favorites);
+            var favoritesJson = NSL.JObject.Parse(cipher.Favorites);
             if (favorite)
             {
                 favoritesJson.Add(userId.ToString(), favorite);

test/Infrastructure.IntegrationTest/Vault/Repositories/CipherRepositoryTests.cs

@@ -1,4 +1,5 @@
-using Bit.Core.Entities;
+using System.Text.Json;
+using Bit.Core.Entities;
 using Bit.Core.Enums;
 using Bit.Core.Models.Data;
 using Bit.Core.Repositories;
@@ -121,4 +122,84 @@ public class CipherRepositoryTests
         var collectionCiphers = await collectionCipherRepository.GetManyByOrganizationIdAsync(organization.Id);
         Assert.NotEmpty(collectionCiphers);
     }
+
+    [DatabaseTheory, DatabaseData]
+    public async Task ReplaceAsync_SuccessfullyMovesCipherToOrganization(IUserRepository userRepository,
+        ICipherRepository cipherRepository,
+        IOrganizationRepository organizationRepository,
+        IOrganizationUserRepository organizationUserRepository,
+        IFolderRepository folderRepository,
+        ITestDatabaseHelper helper)
+    {
+        // This tests what happens when a cipher is moved into an organizations
+        var user = await userRepository.CreateAsync(new User
+        {
+            Name = "Test User",
+            Email = $"test+{Guid.NewGuid()}@email.com",
+            ApiKey = "TEST",
+            SecurityStamp = "stamp",
+        });
+
+
+        user = await userRepository.GetByIdAsync(user.Id);
+
+        // Create cipher in personal vault
+        var createdCipher = await cipherRepository.CreateAsync(new Cipher
+        {
+            UserId = user.Id,
+            Data = "", // TODO: EF does not enforce this as NOT NULL
+        });
+
+        var organization = await organizationRepository.CreateAsync(new Organization
+        {
+            Name = "Test Organization",
+            BillingEmail = user.Email,
+            Plan = "Test" // TODO: EF does not enforce this as NOT NULL
+        });
+
+        _ = await organizationUserRepository.CreateAsync(new OrganizationUser
+        {
+            UserId = user.Id,
+            OrganizationId = organization.Id,
+            Status = OrganizationUserStatusType.Confirmed,
+            Type = OrganizationUserType.Owner,
+        });
+
+        var folder = await folderRepository.CreateAsync(new Folder
+        {
+            Name = "FolderName",
+            UserId = user.Id,
+        });
+
+        helper.ClearTracker();
+
+        // Move cipher to organization vault
+        await cipherRepository.ReplaceAsync(new CipherDetails
+        {
+            Id = createdCipher.Id,
+            UserId = user.Id,
+            OrganizationId = organization.Id,
+            FolderId = folder.Id,
+            Data = "", // TODO: EF does not enforce this as NOT NULL
+        });
+
+        var updatedCipher = await cipherRepository.GetByIdAsync(createdCipher.Id);
+
+        Assert.Null(updatedCipher.UserId);
+        Assert.Equal(organization.Id, updatedCipher.OrganizationId);
+        Assert.NotNull(updatedCipher.Folders);
+
+        using var foldersJsonDocument = JsonDocument.Parse(updatedCipher.Folders);
+        var foldersJsonElement = foldersJsonDocument.RootElement;
+        Assert.Equal(JsonValueKind.Object, foldersJsonElement.ValueKind);
+
+        // TODO: Should we force similar casing for guids across DB's
+        // I'd rather we only interact with them as the actual Guid type
+        var userProperty = foldersJsonElement
+            .EnumerateObject()
+            .FirstOrDefault(jp => string.Equals(jp.Name, user.Id.ToString(), StringComparison.OrdinalIgnoreCase));
+
+        Assert.NotEqual(default, userProperty);
+        Assert.Equal(folder.Id, userProperty.Value.GetGuid());
+    }
 }