Skip to content

Remove property value permissions when related content and/or property types are removed #19778

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.

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
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

AndyButland
Copy link
Contributor

@AndyButland AndyButland commented Jul 23, 2025

Prerequisites

  • I have added steps to test this contribution in the description below

Description

Granular permissions based on document property values permissions have been introduced as options on user groups. They store a reference to a content type key, a property type key and the permission allowed. However currently when content types are deleted or property types removed from a document type, they aren't deleted.

This PR adds that functionality such that:

  • When a content type is deleted, any permission records associated with that key are removed.
    • This just needed an addition to the delete statements that are called when deleting a content type.
    • Note that I fixed on bug in passing here, in the existing delete clauses including some that would never include a content type key. They won't do any harm as they work on node IDs so there's no chance of a clash, but in any case we don't need to make these SQL calls.
  • When a property type is removed from a content type, any permission records associated with that property type key are removed.
    • This was was a bit more involved as the property type key is embedded in the permission string. But I've found a method that works across SQLServer and SQLite.

Testing

Integration tests have been added to verify the behaviour for both scenarios.

For manual testing:

  • Create a document type with one or more properties.
  • Add permissions to one or more user groups using these properties.
  • Remove one or more properties, save the document type and verify the user group permissions associated with the property type are removed.
  • Delete the document type, the user group permissions associated with the document type are removed.

@Copilot Copilot AI review requested due to automatic review settings July 23, 2025 08:15
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request implements cleanup functionality for property value permissions when content types or property types are removed. The changes ensure referential integrity by automatically removing orphaned permission records that would otherwise remain in the database after their associated content types or property types are deleted.

Key changes include:

  • Enhanced content type deletion to remove associated property value permissions
  • Added property type removal logic to clean up permissions containing property type references
  • Added database syntax support for converting GUIDs to strings across SQL Server and SQLite
  • Comprehensive integration tests to verify the cleanup behavior

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
ContentTypeRepositoryBase.cs Core logic for cleaning up property value permissions during content type and property type operations
SqlSyntaxProviderBase.cs Added base implementation for GUID to string conversion
ISqlSyntaxProvider.cs Interface method for GUID to string conversion
SqliteSyntaxProvider.cs SQLite-specific implementation of GUID to string conversion
ContentTypeRepositoryTest.cs Integration tests verifying permission cleanup behavior
Constants-DatabaseSchema.cs Added proper constant naming for content type tree table
ContentType2ContentTypeDto.cs Updated to use new constant name
appsettings.Tests.json Added comment for database type configuration
Comments suppressed due to low confidence (1)

tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/ContentTypeRepositoryTest.cs:14

  • The comment suggests testing with both SQLite and LocalDb, but the test only runs with SQLite. Consider adding parameterized tests to verify the behavior works correctly across both database types, especially given the cross-database SQL syntax requirements.
using Umbraco.Cms.Core.Models;

@@ -20,7 +20,12 @@ public static class Tables
public const string ContentType = /*TableNamePrefix*/ "cms" + "ContentType";
public const string ContentChildType = /*TableNamePrefix*/ "cms" + "ContentTypeAllowedContentType";
public const string DocumentType = /*TableNamePrefix*/ "cms" + "DocumentType";

[Obsolete("Please use ContentTypeTree instead. Scheduled for removal in Umbraco 18.")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just some clean-up. I don't know why this constant was named ElementTypeTree but I've updated it to something that better matches the name and content of the table.

Comment on lines -1559 to -1560
"DELETE FROM umbracoUserGroup2Permission WHERE userGroupKey IN (SELECT [umbracoUserGroup].[Key] FROM umbracoUserGroup WHERE Id = @id)",
"DELETE FROM umbracoUserGroup2GranularPermission WHERE userGroupKey IN (SELECT [umbracoUserGroup].[Key] FROM umbracoUserGroup WHERE Id = @id)",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I've removed these two. We are deleting a content type here but the query is on the user group table.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant