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

* Removed two unnecessary delete clauses when removing content types (they are looking for user group Ids, but we are deleting a content type).

* Renamed table name constant with obsoletion to better reflect name and contents of table.

* Added granular permission for property value records to delete clauses when deleting a document type.

* Delete property value permissions for removed property types.

* Added integration tests to verify behaviour.

Author: AndyButland

src/Umbraco.Cms.Persistence.Sqlite/Services/SqliteSyntaxProvider.cs

@@ -162,6 +162,8 @@ ilist.[unique] AS isUnique
 
     public override string ConvertDateToOrderableString => "{0}";
 
+    public override string ConvertUniqueIdentifierToString => "{0}";
+
     public override string RenameTable => "ALTER TABLE {0} RENAME TO {1}";
 
     /// <inheritdoc />

src/Umbraco.Core/Persistence/Constants-DatabaseSchema.cs

@@ -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.")]
             public const string ElementTypeTree = /*TableNamePrefix*/ "cms" + "ContentType2ContentType";
+
+            public const string ContentTypeTree = /*TableNamePrefix*/ "cms" + "ContentType2ContentType";
+
             public const string DataType = TableNamePrefix + "DataType";
             public const string Template = /*TableNamePrefix*/ "cms" + "Template";
 

src/Umbraco.Infrastructure/Persistence/Dtos/ContentType2ContentTypeDto.cs

@@ -4,7 +4,7 @@
 
 namespace Umbraco.Cms.Infrastructure.Persistence.Dtos;
 
-[TableName(Constants.DatabaseSchema.Tables.ElementTypeTree)]
+[TableName(Constants.DatabaseSchema.Tables.ContentTypeTree)]
 [ExplicitColumns]
 internal sealed class ContentType2ContentTypeDto
 {

src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs

@@ -438,7 +438,7 @@ protected void PersistUpdatedBaseContentType(IContentTypeComposition entity)
             IEnumerable<int> propertyTypeToDeleteIds = dbPropertyTypeIds.Except(entityPropertyTypes);
             foreach (var propertyTypeId in propertyTypeToDeleteIds)
             {
-                DeletePropertyType(entity.Id, propertyTypeId);
+                DeletePropertyType(entity, propertyTypeId);
             }
         }
 
@@ -647,7 +647,7 @@ protected void PersistUpdatedBaseContentType(IContentTypeComposition entity)
         {
             foreach (var id in orphanPropertyTypeIds)
             {
-                DeletePropertyType(entity.Id, id);
+                DeletePropertyType(entity, id);
             }
         }
 
@@ -1410,16 +1410,27 @@ private void RenormalizeDocumentEditedFlags(
         }
     }
 
-    private void DeletePropertyType(int contentTypeId, int propertyTypeId)
+    private void DeletePropertyType(IContentTypeComposition contentType, int propertyTypeId)
     {
-        // first clear dependencies
+        // First clear dependencies.
         Database.Delete<TagRelationshipDto>("WHERE propertyTypeId = @Id", new { Id = propertyTypeId });
         Database.Delete<PropertyDataDto>("WHERE propertyTypeId = @Id", new { Id = propertyTypeId });
 
-        // then delete the property type
+        // Clear the property value permissions, which aren't a hard dependency with a foreign key, but we want to ensure
+        // that any for removed property types are cleared.
+        var uniqueIdAsString = string.Format(SqlContext.SqlSyntax.ConvertUniqueIdentifierToString, "uniqueId");
+        var permissionSearchString = SqlContext.SqlSyntax.GetConcat(
+            "(SELECT " + uniqueIdAsString + " FROM " + Constants.DatabaseSchema.Tables.PropertyType + " WHERE id = @PropertyTypeId)",
+            "'|%'");
+
+        Database.Delete<UserGroup2GranularPermissionDto>(
+            "WHERE uniqueId = @ContentTypeKey AND permission LIKE " + permissionSearchString,
+            new { ContentTypeKey = contentType.Key, PropertyTypeId = propertyTypeId });
+
+        // Finally delete the property type.
         Database.Delete<PropertyTypeDto>(
             "WHERE contentTypeId = @Id AND id = @PropertyTypeId",
-            new { Id = contentTypeId, PropertyTypeId = propertyTypeId });
+            new { contentType.Id, PropertyTypeId = propertyTypeId });
     }
 
     protected void ValidateAlias(TEntity entity)
@@ -1555,20 +1566,16 @@ protected override IEnumerable<string> GetDeleteClauses()
         // is included here just to be 100% sure since it has a FK on cmsPropertyType.
         var list = new List<string>
         {
-            "DELETE FROM umbracoUser2NodeNotify WHERE nodeId = @id",
-            "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)",
-            "DELETE FROM cmsTagRelationship WHERE nodeId = @id",
-            "DELETE FROM cmsContentTypeAllowedContentType WHERE Id = @id",
-            "DELETE FROM cmsContentTypeAllowedContentType WHERE AllowedId = @id",
-            "DELETE FROM cmsContentType2ContentType WHERE parentContentTypeId = @id",
-            "DELETE FROM cmsContentType2ContentType WHERE childContentTypeId = @id",
-            "DELETE FROM " + Constants.DatabaseSchema.Tables.PropertyData +
-            " WHERE propertyTypeId IN (SELECT id FROM cmsPropertyType WHERE contentTypeId = @id)",
-            "DELETE FROM " + Constants.DatabaseSchema.Tables.PropertyType +
-            " WHERE contentTypeId = @id",
-            "DELETE FROM " + Constants.DatabaseSchema.Tables.PropertyTypeGroup +
-            " WHERE contenttypeNodeId = @id",
+            "DELETE FROM " + Constants.DatabaseSchema.Tables.User2NodeNotify + " WHERE nodeId = @id",
+            "DELETE FROM " + Constants.DatabaseSchema.Tables.UserGroup2GranularPermission + " WHERE uniqueId IN (SELECT uniqueId FROM umbracoNode WHERE id = @id)",
+            "DELETE FROM " + Constants.DatabaseSchema.Tables.TagRelationship + " WHERE nodeId = @id",
+            "DELETE FROM " + Constants.DatabaseSchema.Tables.ContentChildType + " WHERE Id = @id",
+            "DELETE FROM " + Constants.DatabaseSchema.Tables.ContentChildType + " WHERE AllowedId = @id",
+            "DELETE FROM " + Constants.DatabaseSchema.Tables.ContentTypeTree + " WHERE parentContentTypeId = @id",
+            "DELETE FROM " + Constants.DatabaseSchema.Tables.ContentTypeTree + " WHERE childContentTypeId = @id",
+            "DELETE FROM " + Constants.DatabaseSchema.Tables.PropertyData + " WHERE propertyTypeId IN (SELECT id FROM cmsPropertyType WHERE contentTypeId = @id)",
+            "DELETE FROM " + Constants.DatabaseSchema.Tables.PropertyType + " WHERE contentTypeId = @id",
+            "DELETE FROM " + Constants.DatabaseSchema.Tables.PropertyTypeGroup + " WHERE contenttypeNodeId = @id",
         };
         return list;
     }

src/Umbraco.Infrastructure/Persistence/SqlSyntax/ISqlSyntaxProvider.cs

@@ -72,6 +72,8 @@ public interface ISqlSyntaxProvider
 
     string ConvertDecimalToOrderableString { get; }
 
+    string ConvertUniqueIdentifierToString => throw new NotImplementedException();
+
     /// <summary>
     ///     Returns the default isolation level for the database
     /// </summary>

src/Umbraco.Infrastructure/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs

@@ -469,6 +469,8 @@ public abstract void HandleCreateTable(
 
     public virtual string ConvertDecimalToOrderableString => "REPLACE(STR({0}, 20, 9), SPACE(1), '0')";
 
+    public virtual string ConvertUniqueIdentifierToString => "CONVERT(nvarchar(36), {0})";
+
     private DbTypes InitColumnTypeMap()
     {
         var dbTypeMap = new DbTypesFactory();

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

@@ -1,19 +1,19 @@
 // Copyright (c) Umbraco.
 // See LICENSE for more details.
 
-using System.Collections.Generic;
-using System.Linq;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using Moq;
 using NUnit.Framework;
+using Umbraco.Cms.Api.Management.Mapping.Permissions;
 using Umbraco.Cms.Core;
 using Umbraco.Cms.Core.Cache;
 using Umbraco.Cms.Core.Configuration.Models;
 using Umbraco.Cms.Core.IO;
 using Umbraco.Cms.Core.Mapping;
 using Umbraco.Cms.Core.Models;
-using Umbraco.Cms.Core.Models.ContentEditing;
+using Umbraco.Cms.Core.Models.Membership;
+using Umbraco.Cms.Core.Models.Membership.Permissions;
 using Umbraco.Cms.Core.Persistence;
 using Umbraco.Cms.Core.Persistence.Repositories;
 using Umbraco.Cms.Core.Services;
@@ -52,8 +52,11 @@ internal sealed class ContentTypeRepositoryTest : UmbracoIntegrationTest
     private IMediaTypeRepository MediaTypeRepository => GetRequiredService<IMediaTypeRepository>();
 
     private IDocumentRepository DocumentRepository => GetRequiredService<IDocumentRepository>();
+
     private IContentService ContentService => GetRequiredService<IContentService>();
 
+    private IUserGroupRepository UserGroupRepository => GetRequiredService<IUserGroupRepository>();
+
     private ContentTypeRepository ContentTypeRepository =>
         (ContentTypeRepository)GetRequiredService<IContentTypeRepository>();
 
@@ -918,4 +921,83 @@ public void Can_Update_Variation_Of_Element_Type_Property()
             Assert.That(hasCulture, Is.True);
         }
     }
+
+    [Test]
+    public void Can_Remove_Property_Value_Permissions_On_Removal_Of_Property_Types()
+    {
+        var provider = ScopeProvider;
+        using (var scope = provider.CreateScope())
+        {
+            // Create, save and re-retrieve a content type and user group.
+            IContentType contentType = ContentTypeBuilder.CreateSimpleContentType(defaultTemplateId: 0);
+            ContentTypeRepository.Save(contentType);
+            contentType = ContentTypeRepository.Get(contentType.Id);
+
+            var userGroup = CreateUserGroupWithGranularPermissions(contentType);
+
+            // Remove property types and verify that the permission is removed from the user group.
+            contentType.RemovePropertyType("author");
+            ContentTypeRepository.Save(contentType);
+            userGroup = UserGroupRepository.Get(userGroup.Id);
+            Assert.AreEqual(3, userGroup.GranularPermissions.Count);
+
+            contentType.RemovePropertyType("bodyText");
+            ContentTypeRepository.Save(contentType);
+            userGroup = UserGroupRepository.Get(userGroup.Id);
+            Assert.AreEqual(2, userGroup.GranularPermissions.Count);
+
+            contentType.RemovePropertyType("title");
+            ContentTypeRepository.Save(contentType);
+            userGroup = UserGroupRepository.Get(userGroup.Id);
+            Assert.AreEqual(0, userGroup.GranularPermissions.Count);
+        }
+    }
+
+    [Test]
+    public void Can_Remove_Property_Value_Permissions_On_Removal_Of_Content_Type()
+    {
+        var provider = ScopeProvider;
+        using (var scope = provider.CreateScope())
+        {
+            // Create, save and re-retrieve a content type and user group.
+            IContentType contentType = ContentTypeBuilder.CreateSimpleContentType(defaultTemplateId: 0);
+            ContentTypeRepository.Save(contentType);
+            contentType = ContentTypeRepository.Get(contentType.Id);
+
+            var userGroup = CreateUserGroupWithGranularPermissions(contentType);
+
+            // Remove the content type and verify all permissions are removed from the user group.
+            ContentTypeRepository.Delete(contentType);
+            userGroup = UserGroupRepository.Get(userGroup.Id);
+            Assert.AreEqual(0, userGroup.GranularPermissions.Count);
+        }
+    }
+
+    private IUserGroup CreateUserGroupWithGranularPermissions(IContentType contentType)
+    {
+        DocumentPropertyValueGranularPermission CreatePermission(IPropertyType propertyType, string permission = "")
+            => new()
+            {
+                Key = contentType.Key,
+                Permission = propertyType.Key.ToString().ToLowerInvariant() + "|" + permission,
+            };
+
+        var titlePropertyType = contentType.PropertyTypes.Single(x => x.Alias == "title");
+        var bodyTextPropertyType = contentType.PropertyTypes.Single(x => x.Alias == "bodyText");
+        var authorPropertyType = contentType.PropertyTypes.Single(x => x.Alias == "author");
+
+        var userGroup = new UserGroupBuilder()
+            .WithGranularPermissions([
+                CreatePermission(titlePropertyType, "Umb.Document.PropertyValue.Read"),
+                CreatePermission(titlePropertyType, "Umb.Document.PropertyValue.Write"),
+                CreatePermission(bodyTextPropertyType, "Umb.Document.PropertyValue.Read"),
+                CreatePermission(authorPropertyType)
+            ])
+            .Build();
+        UserGroupRepository.Save(userGroup);
+        userGroup = UserGroupRepository.Get(userGroup.Id);
+
+        Assert.AreEqual(4, userGroup.GranularPermissions.Count);
+        return userGroup;
+    }
 }