From 457e293fdc5a56569c5a4ac74ef786563553e0ce Mon Sep 17 00:00:00 2001 From: Jordan Aasen <166539328+jaasen-livefront@users.noreply.github.com> Date: Fri, 19 Dec 2025 11:35:01 -0800 Subject: [PATCH] [PM-29017] - improve logic for cipher SaveDetailsAsync validation (#6731) * improve logic for cipher SaveDetailsAsync validation. fix tests * revert change * fix test * remove duplicate semicolon --- .../Services/Implementations/CipherService.cs | 7 +- .../Vault/Services/CipherServiceTests.cs | 73 ++++++++++++++----- 2 files changed, 55 insertions(+), 25 deletions(-) diff --git a/src/Core/Vault/Services/Implementations/CipherService.cs b/src/Core/Vault/Services/Implementations/CipherService.cs index 2085345b16..bb752b471f 100644 --- a/src/Core/Vault/Services/Implementations/CipherService.cs +++ b/src/Core/Vault/Services/Implementations/CipherService.cs @@ -1029,11 +1029,8 @@ public class CipherService : ICipherService var existingCipherData = DeserializeCipherData(existingCipher); var newCipherData = DeserializeCipherData(cipher); - // "hidden password" users may not add cipher key encryption - if (existingCipher.Key == null && cipher.Key != null) - { - throw new BadRequestException("You do not have permission to add cipher key encryption."); - } + // For hidden-password users, never allow Key to change at all. + cipher.Key = existingCipher.Key; // Keep only non-hidden fileds from the new cipher var nonHiddenFields = newCipherData.Fields?.Where(f => f.Type != FieldType.Hidden) ?? []; // Get hidden fields from the existing cipher diff --git a/test/Core.Test/Vault/Services/CipherServiceTests.cs b/test/Core.Test/Vault/Services/CipherServiceTests.cs index c5eecb8f34..fc84651951 100644 --- a/test/Core.Test/Vault/Services/CipherServiceTests.cs +++ b/test/Core.Test/Vault/Services/CipherServiceTests.cs @@ -1215,12 +1215,12 @@ public class CipherServiceTests private static SaveDetailsAsyncDependencies GetSaveDetailsAsyncDependencies( SutProvider sutProvider, string newPassword, - bool viewPassword, - bool editPermission, + bool permission, string? key = null, string? totp = null, CipherLoginFido2CredentialData[]? passkeys = null, - CipherFieldData[]? fields = null + CipherFieldData[]? fields = null, + string? existingKey = "OriginalKey" ) { var cipherDetails = new CipherDetails @@ -1233,13 +1233,22 @@ public class CipherServiceTests Key = key, }; - var newLoginData = new CipherLoginData { Username = "user", Password = newPassword, Totp = totp, Fido2Credentials = passkeys, Fields = fields }; + var newLoginData = new CipherLoginData + { + Username = "user", + Password = newPassword, + Totp = totp, + Fido2Credentials = passkeys, + Fields = fields + }; + cipherDetails.Data = JsonSerializer.Serialize(newLoginData); var existingCipher = new Cipher { Id = cipherDetails.Id, Type = CipherType.Login, + Key = existingKey, Data = JsonSerializer.Serialize( new CipherLoginData { @@ -1261,7 +1270,14 @@ public class CipherServiceTests var permissions = new Dictionary { - { cipherDetails.Id, new OrganizationCipherPermission { ViewPassword = viewPassword, Edit = editPermission } } + { + cipherDetails.Id, + new OrganizationCipherPermission + { + ViewPassword = permission, + Edit = permission + } + } }; sutProvider.GetDependency() @@ -1278,7 +1294,7 @@ public class CipherServiceTests [Theory, BitAutoData] public async Task SaveDetailsAsync_PasswordNotChangedWithoutViewPasswordPermission(string _, SutProvider sutProvider) { - var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", viewPassword: false, editPermission: true); + var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", permission: false); await deps.SutProvider.Sut.SaveDetailsAsync( deps.CipherDetails, @@ -1294,7 +1310,7 @@ public class CipherServiceTests [Theory, BitAutoData] public async Task SaveDetailsAsync_PasswordNotChangedWithoutEditPermission(string _, SutProvider sutProvider) { - var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", viewPassword: true, editPermission: false); + var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", permission: false); await deps.SutProvider.Sut.SaveDetailsAsync( deps.CipherDetails, @@ -1310,7 +1326,7 @@ public class CipherServiceTests [Theory, BitAutoData] public async Task SaveDetailsAsync_PasswordChangedWithPermission(string _, SutProvider sutProvider) { - var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", viewPassword: true, editPermission: true); + var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", permission: true); await deps.SutProvider.Sut.SaveDetailsAsync( deps.CipherDetails, @@ -1326,7 +1342,11 @@ public class CipherServiceTests [Theory, BitAutoData] public async Task SaveDetailsAsync_CipherKeyChangedWithPermission(string _, SutProvider sutProvider) { - var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", viewPassword: true, editPermission: true, "NewKey"); + var deps = GetSaveDetailsAsyncDependencies( + sutProvider, + newPassword: "NewPassword", + permission: true, + key: "NewKey"); await deps.SutProvider.Sut.SaveDetailsAsync( deps.CipherDetails, @@ -1336,27 +1356,40 @@ public class CipherServiceTests true); Assert.Equal("NewKey", deps.CipherDetails.Key); + + await sutProvider.GetDependency() + .Received() + .ReplaceAsync(Arg.Is(c => c.Id == deps.CipherDetails.Id && c.Key == "NewKey")); } [Theory, BitAutoData] - public async Task SaveDetailsAsync_CipherKeyChangedWithoutPermission(string _, SutProvider sutProvider) + public async Task SaveDetailsAsync_CipherKeyNotChangedWithoutPermission(string _, SutProvider sutProvider) { - var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", viewPassword: true, editPermission: false, "NewKey"); + var deps = GetSaveDetailsAsyncDependencies( + sutProvider, + newPassword: "NewPassword", + permission: false, + key: "NewKey" + ); - var exception = await Assert.ThrowsAsync(() => deps.SutProvider.Sut.SaveDetailsAsync( + await deps.SutProvider.Sut.SaveDetailsAsync( deps.CipherDetails, deps.CipherDetails.UserId.Value, deps.CipherDetails.RevisionDate, null, - true)); + true); - Assert.Contains("do not have permission", exception.Message); + Assert.Equal("OriginalKey", deps.CipherDetails.Key); + + await sutProvider.GetDependency() + .Received() + .ReplaceAsync(Arg.Is(c => c.Id == deps.CipherDetails.Id && c.Key == "OriginalKey")); } [Theory, BitAutoData] public async Task SaveDetailsAsync_TotpChangedWithoutPermission(string _, SutProvider sutProvider) { - var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", viewPassword: true, editPermission: false, totp: "NewTotp"); + var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", permission: false, totp: "NewTotp"); await deps.SutProvider.Sut.SaveDetailsAsync( deps.CipherDetails, @@ -1372,7 +1405,7 @@ public class CipherServiceTests [Theory, BitAutoData] public async Task SaveDetailsAsync_TotpChangedWithPermission(string _, SutProvider sutProvider) { - var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", viewPassword: true, editPermission: true, totp: "NewTotp"); + var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", permission: true, totp: "NewTotp"); await deps.SutProvider.Sut.SaveDetailsAsync( deps.CipherDetails, @@ -1397,7 +1430,7 @@ public class CipherServiceTests } }; - var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", viewPassword: true, editPermission: false, passkeys: passkeys); + var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", permission: false, passkeys: passkeys); await deps.SutProvider.Sut.SaveDetailsAsync( deps.CipherDetails, @@ -1422,7 +1455,7 @@ public class CipherServiceTests } }; - var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", viewPassword: true, editPermission: true, passkeys: passkeys); + var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", permission: true, passkeys: passkeys); await deps.SutProvider.Sut.SaveDetailsAsync( deps.CipherDetails, @@ -1439,7 +1472,7 @@ public class CipherServiceTests [BitAutoData] public async Task SaveDetailsAsync_HiddenFieldsChangedWithoutPermission(string _, SutProvider sutProvider) { - var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", viewPassword: false, editPermission: false, fields: + var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", permission: false, fields: [ new CipherFieldData { @@ -1464,7 +1497,7 @@ public class CipherServiceTests [BitAutoData] public async Task SaveDetailsAsync_HiddenFieldsChangedWithPermission(string _, SutProvider sutProvider) { - var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", viewPassword: true, editPermission: true, fields: + var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", permission: true, fields: [ new CipherFieldData {