Skip to content

[PM-25675] - fix NormalCipherPermissions.CanDelete #6666

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jaasen-livefront wants to merge 3 commits into
base: main
Choose a base branch
from
+6 −6
Open

[PM-25675] - fix NormalCipherPermissions.CanDelete #6666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d20354f
fix NormalCipherPermissions.CanDelete
jaasen-livefront Dec 2, 2025
7877358
fix test
jaasen-livefront Dec 2, 2025
6351178
fix tests
jaasen-livefront Dec 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Core/Vault/Authorization/Permissions/NormalCipherPermissions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public static bool CanDelete(User user, CipherDetails cipherDetails, Organizatio
throw new Exception("Cipher needs to belong to a user or an organization.");
}

if (user.Id == cipherDetails.UserId)
if (cipherDetails.OrganizationId == null && user.Id == cipherDetails.UserId)
Copy link
Contributor

@claude claude bot Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Excellent fix! Adding the cipherDetails.OrganizationId == null check correctly distinguishes between personal and organization ciphers, even when the SQL view injects the user's ID into UserId for org ciphers.

This properly enforces the ownership boundary: personal ciphers can be deleted by their owner (UserId matches and no OrganizationId), while org ciphers must go through the organization permission checks below.

{
return true;
}
Expand Down
4 changes: 2 additions & 2 deletions test/Api.Test/Vault/Controllers/CiphersControllerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public async Task PutCollections_vNextShouldSaveUpdatedCipher(Guid id, CipherCol
sutProvider.GetDependency<ICipherRepository>().GetByIdAsync(id, userId).ReturnsForAnyArgs(cipherDetails);

sutProvider.GetDependency<ICollectionCipherRepository>().GetManyByUserIdCipherIdAsync(userId, id).Returns((ICollection<CollectionCipher>)new List<CollectionCipher>());
sutProvider.GetDependency<IApplicationCacheService>().GetOrganizationAbilitiesAsync().Returns(new Dictionary<Guid, OrganizationAbility> { { cipherDetails.OrganizationId.Value, new OrganizationAbility() } });
sutProvider.GetDependency<IApplicationCacheService>().GetOrganizationAbilitiesAsync().Returns(new Dictionary<Guid, OrganizationAbility> { { cipherDetails.OrganizationId.Value, new OrganizationAbility { Id = cipherDetails.OrganizationId.Value } } });
var cipherService = sutProvider.GetDependency<ICipherService>();

await sutProvider.Sut.PutCollections_vNext(id, model);
Expand All @@ -95,7 +95,7 @@ public async Task PutCollections_vNextReturnOptionalDetailsCipherUnavailableFals
sutProvider.GetDependency<ICipherRepository>().GetByIdAsync(id, userId).ReturnsForAnyArgs(cipherDetails);

sutProvider.GetDependency<ICollectionCipherRepository>().GetManyByUserIdCipherIdAsync(userId, id).Returns((ICollection<CollectionCipher>)new List<CollectionCipher>());
sutProvider.GetDependency<IApplicationCacheService>().GetOrganizationAbilitiesAsync().Returns(new Dictionary<Guid, OrganizationAbility> { { cipherDetails.OrganizationId.Value, new OrganizationAbility() } });
sutProvider.GetDependency<IApplicationCacheService>().GetOrganizationAbilitiesAsync().Returns(new Dictionary<Guid, OrganizationAbility> { { cipherDetails.OrganizationId.Value, new OrganizationAbility { Id = cipherDetails.OrganizationId.Value } } });

var result = await sutProvider.Sut.PutCollections_vNext(id, model);

Expand Down
6 changes: 3 additions & 3 deletions test/Core.Test/Vault/Authorization/Permissions/NormalCipherPermissionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public void CanRestore_WhenCipherDoesNotBelongToInputOrganization_ShouldThrowExc
var cipherDetails = new CipherDetails { UserId = null, OrganizationId = Guid.NewGuid() };

// Act
var exception = Assert.Throws<Exception>(() => NormalCipherPermissions.CanDelete(user, cipherDetails, organizationAbility));
var exception = Assert.Throws<Exception>(() => NormalCipherPermissions.CanRestore(user, cipherDetails, organizationAbility));

// Assert
Assert.Equal("Cipher does not belong to the input organization.", exception.Message);
Expand All @@ -92,11 +92,11 @@ public void CanDelete_WhenCipherIsOwnedByOrganization(
// Arrange
var user = new User { Id = Guid.Empty };
var organizationId = Guid.NewGuid();
var cipherDetails = new CipherDetails { Manage = manage, Edit = edit, UserId = null, OrganizationId = organizationId };
var cipherDetails = new CipherDetails { Manage = manage, Edit = edit, UserId = user.Id, OrganizationId = organizationId };
Copy link
Contributor

@claude claude bot Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 This test now sets UserId = user.Id for organization ciphers, which matches what the SQL view (UserCipherDetails) returns to the application layer. However, this differs from the actual database state where org ciphers have UserId = null.

Question: Is this intentional to match the SQL view behavior? Or should the test maintain UserId = null to better represent the underlying data model?

For comparison, the CanRestore_WhenCipherIsOwnedByOrganization test (line 23) still uses UserId = null. Should that be updated for consistency, or is there a reason for the difference?

var organizationAbility = new OrganizationAbility { Id = organizationId, LimitItemDeletion = limitItemDeletion };

// Act
var result = NormalCipherPermissions.CanRestore(user, cipherDetails, organizationAbility);
var result = NormalCipherPermissions.CanDelete(user, cipherDetails, organizationAbility);

// Assert
Assert.Equal(result, expectedResult);
Expand Down
Loading