From a0aff9d10c52e45450b3d31b7e23cd3c1339e89d Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 7 Aug 2025 14:22:19 +0200 Subject: [PATCH] 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. --- .../Services/SqliteSyntaxProvider.cs | 2 + .../Persistence/Constants-DatabaseSchema.cs | 5 ++ .../Dtos/ContentType2ContentTypeDto.cs | 2 +- .../Implement/ContentTypeRepositoryBase.cs | 47 +++++----- .../SqlSyntax/ISqlSyntaxProvider.cs | 2 + .../SqlSyntax/SqlSyntaxProviderBase.cs | 2 + .../Repositories/ContentTypeRepositoryTest.cs | 88 ++++++++++++++++++- 7 files changed, 124 insertions(+), 24 deletions(-) diff --git a/src/Umbraco.Cms.Persistence.Sqlite/Services/SqliteSyntaxProvider.cs b/src/Umbraco.Cms.Persistence.Sqlite/Services/SqliteSyntaxProvider.cs index 2ed07cbf02..6e326fcaaa 100644 --- a/src/Umbraco.Cms.Persistence.Sqlite/Services/SqliteSyntaxProvider.cs +++ b/src/Umbraco.Cms.Persistence.Sqlite/Services/SqliteSyntaxProvider.cs @@ -162,6 +162,8 @@ public class SqliteSyntaxProvider : SqlSyntaxProviderBase public override string ConvertDateToOrderableString => "{0}"; + public override string ConvertUniqueIdentifierToString => "{0}"; + public override string RenameTable => "ALTER TABLE {0} RENAME TO {1}"; /// diff --git a/src/Umbraco.Core/Persistence/Constants-DatabaseSchema.cs b/src/Umbraco.Core/Persistence/Constants-DatabaseSchema.cs index 9dc3148b7d..cfa564ca43 100644 --- a/src/Umbraco.Core/Persistence/Constants-DatabaseSchema.cs +++ b/src/Umbraco.Core/Persistence/Constants-DatabaseSchema.cs @@ -20,7 +20,12 @@ public static partial class Constants 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"; diff --git a/src/Umbraco.Infrastructure/Persistence/Dtos/ContentType2ContentTypeDto.cs b/src/Umbraco.Infrastructure/Persistence/Dtos/ContentType2ContentTypeDto.cs index 19a3922161..e4c4bf77de 100644 --- a/src/Umbraco.Infrastructure/Persistence/Dtos/ContentType2ContentTypeDto.cs +++ b/src/Umbraco.Infrastructure/Persistence/Dtos/ContentType2ContentTypeDto.cs @@ -4,7 +4,7 @@ using Umbraco.Cms.Infrastructure.Persistence.DatabaseAnnotations; namespace Umbraco.Cms.Infrastructure.Persistence.Dtos; -[TableName(Constants.DatabaseSchema.Tables.ElementTypeTree)] +[TableName(Constants.DatabaseSchema.Tables.ContentTypeTree)] [ExplicitColumns] internal sealed class ContentType2ContentTypeDto { diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs index d9dc0434be..0b0a964298 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs @@ -438,7 +438,7 @@ AND umbracoNode.id <> @id", IEnumerable propertyTypeToDeleteIds = dbPropertyTypeIds.Except(entityPropertyTypes); foreach (var propertyTypeId in propertyTypeToDeleteIds) { - DeletePropertyType(entity.Id, propertyTypeId); + DeletePropertyType(entity, propertyTypeId); } } @@ -647,7 +647,7 @@ AND umbracoNode.id <> @id", { foreach (var id in orphanPropertyTypeIds) { - DeletePropertyType(entity.Id, id); + DeletePropertyType(entity, id); } } @@ -1410,16 +1410,27 @@ AND umbracoNode.id <> @id", } } - private void DeletePropertyType(int contentTypeId, int propertyTypeId) + private void DeletePropertyType(IContentTypeComposition contentType, int propertyTypeId) { - // first clear dependencies + // First clear dependencies. Database.Delete("WHERE propertyTypeId = @Id", new { Id = propertyTypeId }); Database.Delete("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( + "WHERE uniqueId = @ContentTypeKey AND permission LIKE " + permissionSearchString, + new { ContentTypeKey = contentType.Key, PropertyTypeId = propertyTypeId }); + + // Finally delete the property type. Database.Delete( "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 @@ WHERE {Constants.DatabaseSchema.Tables.Content}.nodeId IN (@ids) AND cmsContentT // is included here just to be 100% sure since it has a FK on cmsPropertyType. var list = new List { - "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; } diff --git a/src/Umbraco.Infrastructure/Persistence/SqlSyntax/ISqlSyntaxProvider.cs b/src/Umbraco.Infrastructure/Persistence/SqlSyntax/ISqlSyntaxProvider.cs index d1a2c1b0d2..1b01a74535 100644 --- a/src/Umbraco.Infrastructure/Persistence/SqlSyntax/ISqlSyntaxProvider.cs +++ b/src/Umbraco.Infrastructure/Persistence/SqlSyntax/ISqlSyntaxProvider.cs @@ -72,6 +72,8 @@ public interface ISqlSyntaxProvider string ConvertDecimalToOrderableString { get; } + string ConvertUniqueIdentifierToString => throw new NotImplementedException(); + /// /// Returns the default isolation level for the database /// diff --git a/src/Umbraco.Infrastructure/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs b/src/Umbraco.Infrastructure/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs index 089bc23814..c5a98b9062 100644 --- a/src/Umbraco.Infrastructure/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs +++ b/src/Umbraco.Infrastructure/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs @@ -469,6 +469,8 @@ public abstract class SqlSyntaxProviderBase : ISqlSyntaxProvider 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(); diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/ContentTypeRepositoryTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/ContentTypeRepositoryTest.cs index bba1bd864b..24a066d965 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/ContentTypeRepositoryTest.cs +++ b/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(); private IDocumentRepository DocumentRepository => GetRequiredService(); + private IContentService ContentService => GetRequiredService(); + private IUserGroupRepository UserGroupRepository => GetRequiredService(); + private ContentTypeRepository ContentTypeRepository => (ContentTypeRepository)GetRequiredService(); @@ -918,4 +921,83 @@ internal sealed class ContentTypeRepositoryTest : UmbracoIntegrationTest 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; + } }