From 311d322129d6db5ed5c3f1be98ee58e4e35ed2d4 Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Mon, 21 Aug 2023 13:08:26 +0200 Subject: [PATCH 01/15] Add code infrastructure to validate file content (#14657) * Implemented modular architecture for filestream security sanitization with an svg-html example * 31440: Refactoring, applied to more entry points and removed test analyzer * 31440 Added Unittests for FileStreamSecurityValidator * PR fixes and better unittest mock names --------- Co-authored-by: Sven Geusens --- .../DependencyInjection/UmbracoBuilder.cs | 3 + .../EmbeddedResources/Lang/en.xml | 1 + .../EmbeddedResources/Lang/en_us.xml | 1 + .../EmbeddedResources/Lang/nl.xml | 1 + .../Security/FileStreamSecurityValidator.cs | 38 ++++++ .../Security/IFileStreamSecurityAnalyzer.cs | 20 +++ .../Security/IFileStreamSecurityValidator.cs | 11 ++ .../FileUploadPropertyValueEditor.cs | 11 +- .../ImageCropperPropertyValueEditor.cs | 11 +- .../Controllers/CurrentUserController.cs | 35 +++++ .../Controllers/MediaController.cs | 68 +++++++++- .../Controllers/UsersController.cs | 68 +++++++++- .../FileStreamSecurityValidatorTests.cs | 123 ++++++++++++++++++ 13 files changed, 382 insertions(+), 9 deletions(-) create mode 100644 src/Umbraco.Core/Security/FileStreamSecurityValidator.cs create mode 100644 src/Umbraco.Core/Security/IFileStreamSecurityAnalyzer.cs create mode 100644 src/Umbraco.Core/Security/IFileStreamSecurityValidator.cs create mode 100644 tests/Umbraco.Tests.UnitTests/Umbraco.Core/Security/FileStreamSecurityValidatorTests.cs diff --git a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs index a850a8f371..dcf1ee5183 100644 --- a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs +++ b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs @@ -327,6 +327,9 @@ namespace Umbraco.Cms.Core.DependencyInjection Services.AddUnique(provider => new CultureImpactFactory(provider.GetRequiredService>())); Services.AddUnique(); Services.AddUnique(); + + // Register filestream security analyzers + Services.AddUnique(); } } } diff --git a/src/Umbraco.Core/EmbeddedResources/Lang/en.xml b/src/Umbraco.Core/EmbeddedResources/Lang/en.xml index 3b7bf261bd..a2ab932701 100644 --- a/src/Umbraco.Core/EmbeddedResources/Lang/en.xml +++ b/src/Umbraco.Core/EmbeddedResources/Lang/en.xml @@ -341,6 +341,7 @@ Failed to create a folder under parent id %0% Failed to rename the folder with id %0% Drag and drop your file(s) into the area + One or more file security validations have failed Create a new member diff --git a/src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml b/src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml index 44d07b2511..699bf69ebd 100644 --- a/src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml +++ b/src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml @@ -352,6 +352,7 @@ Failed to rename the folder with id %0% Drag and drop your file(s) into the area Upload is not allowed in this location. + One or more file security validations have failed Create a new member diff --git a/src/Umbraco.Core/EmbeddedResources/Lang/nl.xml b/src/Umbraco.Core/EmbeddedResources/Lang/nl.xml index eab1f856d9..847f6807d4 100644 --- a/src/Umbraco.Core/EmbeddedResources/Lang/nl.xml +++ b/src/Umbraco.Core/EmbeddedResources/Lang/nl.xml @@ -340,6 +340,7 @@ Kan de map met id %0% niet hernoemen Sleep en zet je bestand(en) neer in dit gebied Upload is niet toegelaten in deze locatie. + Een of meerdere veiligheid validaties zijn gefaald voor het bestand Maak nieuw lid aan diff --git a/src/Umbraco.Core/Security/FileStreamSecurityValidator.cs b/src/Umbraco.Core/Security/FileStreamSecurityValidator.cs new file mode 100644 index 0000000000..764ea37d3d --- /dev/null +++ b/src/Umbraco.Core/Security/FileStreamSecurityValidator.cs @@ -0,0 +1,38 @@ +namespace Umbraco.Cms.Core.Security; + +public class FileStreamSecurityValidator : IFileStreamSecurityValidator +{ + private readonly IEnumerable _fileAnalyzers; + + public FileStreamSecurityValidator(IEnumerable fileAnalyzers) + { + _fileAnalyzers = fileAnalyzers; + } + + /// + /// Analyzes whether the file content is considered safe with registered IFileStreamSecurityAnalyzers + /// + /// Needs to be a Read seekable stream + /// Whether the file is considered safe after running the necessary analyzers + public bool IsConsideredSafe(Stream fileStream) + { + foreach (var fileAnalyzer in _fileAnalyzers) + { + fileStream.Seek(0, SeekOrigin.Begin); + if (!fileAnalyzer.ShouldHandle(fileStream)) + { + continue; + } + + fileStream.Seek(0, SeekOrigin.Begin); + if (fileAnalyzer.IsConsideredSafe(fileStream) == false) + { + return false; + } + } + fileStream.Seek(0, SeekOrigin.Begin); + // If no analyzer we consider the file to be safe as the implementer has the possibility to add additional analyzers + // Or all analyzers deem te file to be safe + return true; + } +} diff --git a/src/Umbraco.Core/Security/IFileStreamSecurityAnalyzer.cs b/src/Umbraco.Core/Security/IFileStreamSecurityAnalyzer.cs new file mode 100644 index 0000000000..408f161f21 --- /dev/null +++ b/src/Umbraco.Core/Security/IFileStreamSecurityAnalyzer.cs @@ -0,0 +1,20 @@ +namespace Umbraco.Cms.Core.Security; + +public interface IFileStreamSecurityAnalyzer +{ + + /// + /// Indicates whether the analyzer should process the file + /// The implementation should be considerably faster than IsConsideredSafe + /// + /// + /// + bool ShouldHandle(Stream fileStream); + + /// + /// Analyzes whether the file content is considered safe + /// + /// Needs to be a Read/Write seekable stream + /// Whether the file is considered safe + bool IsConsideredSafe(Stream fileStream); +} diff --git a/src/Umbraco.Core/Security/IFileStreamSecurityValidator.cs b/src/Umbraco.Core/Security/IFileStreamSecurityValidator.cs new file mode 100644 index 0000000000..9dc60507ac --- /dev/null +++ b/src/Umbraco.Core/Security/IFileStreamSecurityValidator.cs @@ -0,0 +1,11 @@ +namespace Umbraco.Cms.Core.Security; + +public interface IFileStreamSecurityValidator +{ + /// + /// Analyzes wether the file content is considered safe with registered IFileStreamSecurityAnalyzers + /// + /// Needs to be a Read seekable stream + /// Whether the file is considered safe after running the necessary analyzers + bool IsConsideredSafe(Stream fileStream); +} diff --git a/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs b/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs index 5a14a1afc1..34a4d33fd9 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs @@ -5,6 +5,7 @@ using Microsoft.Extensions.Options; using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.IO; using Umbraco.Cms.Core.Models.Editors; +using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Serialization; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Strings; @@ -18,6 +19,7 @@ namespace Umbraco.Cms.Core.PropertyEditors; internal class FileUploadPropertyValueEditor : DataValueEditor { private readonly MediaFileManager _mediaFileManager; + private readonly IFileStreamSecurityValidator _fileStreamSecurityValidator; private ContentSettings _contentSettings; public FileUploadPropertyValueEditor( @@ -27,10 +29,12 @@ internal class FileUploadPropertyValueEditor : DataValueEditor IShortStringHelper shortStringHelper, IOptionsMonitor contentSettings, IJsonSerializer jsonSerializer, - IIOHelper ioHelper) + IIOHelper ioHelper, + IFileStreamSecurityValidator fileStreamSecurityValidator) : base(localizedTextService, shortStringHelper, jsonSerializer, ioHelper, attribute) { _mediaFileManager = mediaFileManager ?? throw new ArgumentNullException(nameof(mediaFileManager)); + _fileStreamSecurityValidator = fileStreamSecurityValidator; _contentSettings = contentSettings.CurrentValue ?? throw new ArgumentNullException(nameof(contentSettings)); contentSettings.OnChange(x => _contentSettings = x); } @@ -147,6 +151,11 @@ internal class FileUploadPropertyValueEditor : DataValueEditor using (FileStream filestream = File.OpenRead(file.TempFilePath)) { + if (_fileStreamSecurityValidator.IsConsideredSafe(filestream) == false) + { + return null; + } + // TODO: Here it would make sense to do the auto-fill properties stuff but the API doesn't allow us to do that right // since we'd need to be able to return values for other properties from these methods _mediaFileManager.FileSystem.AddFile(filepath, filestream, true); // must overwrite! diff --git a/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs b/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs index 223e62d5a3..2c44ef57e3 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs @@ -10,6 +10,7 @@ using Umbraco.Cms.Core.IO; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.Editors; using Umbraco.Cms.Core.PropertyEditors.ValueConverters; +using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Serialization; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Strings; @@ -24,6 +25,7 @@ namespace Umbraco.Cms.Core.PropertyEditors; internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core vs web? { private readonly IDataTypeService _dataTypeService; + private readonly IFileStreamSecurityValidator _fileStreamSecurityValidator; private readonly ILogger _logger; private readonly MediaFileManager _mediaFileManager; private ContentSettings _contentSettings; @@ -37,13 +39,15 @@ internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core v IOptionsMonitor contentSettings, IJsonSerializer jsonSerializer, IIOHelper ioHelper, - IDataTypeService dataTypeService) + IDataTypeService dataTypeService, + IFileStreamSecurityValidator fileStreamSecurityValidator) : base(localizedTextService, shortStringHelper, jsonSerializer, ioHelper, attribute) { _logger = logger ?? throw new ArgumentNullException(nameof(logger)); _mediaFileManager = mediaFileSystem ?? throw new ArgumentNullException(nameof(mediaFileSystem)); _contentSettings = contentSettings.CurrentValue; _dataTypeService = dataTypeService; + _fileStreamSecurityValidator = fileStreamSecurityValidator; contentSettings.OnChange(x => _contentSettings = x); } @@ -236,6 +240,11 @@ internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core v using (FileStream filestream = File.OpenRead(file.TempFilePath)) { + if (_fileStreamSecurityValidator.IsConsideredSafe(filestream) == false) + { + return null; + } + // TODO: Here it would make sense to do the auto-fill properties stuff but the API doesn't allow us to do that right // since we'd need to be able to return values for other properties from these methods _mediaFileManager.FileSystem.AddFile(filepath, filestream, true); // must overwrite! diff --git a/src/Umbraco.Web.BackOffice/Controllers/CurrentUserController.cs b/src/Umbraco.Web.BackOffice/Controllers/CurrentUserController.cs index f867ccc5a1..4992c4921e 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/CurrentUserController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/CurrentUserController.cs @@ -48,9 +48,43 @@ public class CurrentUserController : UmbracoAuthorizedJsonController private readonly IShortStringHelper _shortStringHelper; private readonly IUmbracoMapper _umbracoMapper; private readonly IUserDataService _userDataService; + private readonly IFileStreamSecurityValidator? _fileStreamSecurityValidator; // make non nullable in v14 private readonly IUserService _userService; [ActivatorUtilitiesConstructor] + public CurrentUserController( + MediaFileManager mediaFileManager, + IOptionsSnapshot contentSettings, + IHostingEnvironment hostingEnvironment, + IImageUrlGenerator imageUrlGenerator, + IBackOfficeSecurityAccessor backofficeSecurityAccessor, + IUserService userService, + IUmbracoMapper umbracoMapper, + IBackOfficeUserManager backOfficeUserManager, + ILocalizedTextService localizedTextService, + AppCaches appCaches, + IShortStringHelper shortStringHelper, + IPasswordChanger passwordChanger, + IUserDataService userDataService, + IFileStreamSecurityValidator fileStreamSecurityValidator) + { + _mediaFileManager = mediaFileManager; + _contentSettings = contentSettings.Value; + _hostingEnvironment = hostingEnvironment; + _imageUrlGenerator = imageUrlGenerator; + _backofficeSecurityAccessor = backofficeSecurityAccessor; + _userService = userService; + _umbracoMapper = umbracoMapper; + _backOfficeUserManager = backOfficeUserManager; + _localizedTextService = localizedTextService; + _appCaches = appCaches; + _shortStringHelper = shortStringHelper; + _passwordChanger = passwordChanger; + _userDataService = userDataService; + _fileStreamSecurityValidator = fileStreamSecurityValidator; + } + + [Obsolete("Use constructor overload that has fileStreamSecurityValidator, scheduled for removal in v14")] public CurrentUserController( MediaFileManager mediaFileManager, IOptionsSnapshot contentSettings, @@ -285,6 +319,7 @@ public class CurrentUserController : UmbracoAuthorizedJsonController _contentSettings, _hostingEnvironment, _imageUrlGenerator, + _fileStreamSecurityValidator, _backofficeSecurityAccessor.BackOfficeSecurity?.GetUserId().Result ?? 0); } diff --git a/src/Umbraco.Web.BackOffice/Controllers/MediaController.cs b/src/Umbraco.Web.BackOffice/Controllers/MediaController.cs index a26934b3c1..807061e5aa 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MediaController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MediaController.cs @@ -5,6 +5,7 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Infrastructure; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Microsoft.Extensions.Primitives; @@ -50,6 +51,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers; public class MediaController : ContentControllerBase { private readonly AppCaches _appCaches; + private readonly IFileStreamSecurityValidator? _fileStreamSecurityValidator; // make non nullable in v14 private readonly IAuthorizationService _authorizationService; private readonly IBackOfficeSecurityAccessor _backofficeSecurityAccessor; private readonly ContentSettings _contentSettings; @@ -66,6 +68,58 @@ public class MediaController : ContentControllerBase private readonly ISqlContext _sqlContext; private readonly IUmbracoMapper _umbracoMapper; + [ActivatorUtilitiesConstructor] + public MediaController( + ICultureDictionary cultureDictionary, + ILoggerFactory loggerFactory, + IShortStringHelper shortStringHelper, + IEventMessagesFactory eventMessages, + ILocalizedTextService localizedTextService, + IOptionsSnapshot contentSettings, + IMediaTypeService mediaTypeService, + IMediaService mediaService, + IEntityService entityService, + IBackOfficeSecurityAccessor backofficeSecurityAccessor, + IUmbracoMapper umbracoMapper, + IDataTypeService dataTypeService, + ISqlContext sqlContext, + IContentTypeBaseServiceProvider contentTypeBaseServiceProvider, + IRelationService relationService, + PropertyEditorCollection propertyEditors, + MediaFileManager mediaFileManager, + MediaUrlGeneratorCollection mediaUrlGenerators, + IHostingEnvironment hostingEnvironment, + IImageUrlGenerator imageUrlGenerator, + IJsonSerializer serializer, + IAuthorizationService authorizationService, + AppCaches appCaches, + IFileStreamSecurityValidator streamSecurityValidator) + : base(cultureDictionary, loggerFactory, shortStringHelper, eventMessages, localizedTextService, serializer) + { + _shortStringHelper = shortStringHelper; + _contentSettings = contentSettings.Value; + _mediaTypeService = mediaTypeService; + _mediaService = mediaService; + _entityService = entityService; + _backofficeSecurityAccessor = backofficeSecurityAccessor; + _umbracoMapper = umbracoMapper; + _dataTypeService = dataTypeService; + _localizedTextService = localizedTextService; + _sqlContext = sqlContext; + _contentTypeBaseServiceProvider = contentTypeBaseServiceProvider; + _relationService = relationService; + _propertyEditors = propertyEditors; + _mediaFileManager = mediaFileManager; + _mediaUrlGenerators = mediaUrlGenerators; + _hostingEnvironment = hostingEnvironment; + _logger = loggerFactory.CreateLogger(); + _imageUrlGenerator = imageUrlGenerator; + _authorizationService = authorizationService; + _appCaches = appCaches; + _fileStreamSecurityValidator = streamSecurityValidator; + } + + [Obsolete("Use constructor overload that has fileStreamSecurityValidator, scheduled for removal in v14")] public MediaController( ICultureDictionary cultureDictionary, ILoggerFactory loggerFactory, @@ -724,6 +778,17 @@ public class MediaController : ContentControllerBase continue; } + using var stream = new MemoryStream(); + await formFile.CopyToAsync(stream); + if (_fileStreamSecurityValidator != null && _fileStreamSecurityValidator.IsConsideredSafe(stream) == false) + { + tempFiles.Notifications.Add(new BackOfficeNotification( + _localizedTextService.Localize("speechBubbles", "operationFailedHeader"), + _localizedTextService.Localize("media", "fileSecurityValidationFailure"), + NotificationStyle.Warning)); + continue; + } + if (string.IsNullOrEmpty(mediaTypeAlias)) { mediaTypeAlias = Constants.Conventions.MediaTypes.File; @@ -801,11 +866,8 @@ public class MediaController : ContentControllerBase IMedia createdMediaItem = _mediaService.CreateMedia(mediaItemName, parentId.Value, mediaTypeAlias, _backofficeSecurityAccessor.BackOfficeSecurity?.CurrentUser?.Id ?? -1); - await using (Stream stream = formFile.OpenReadStream()) - { createdMediaItem.SetValue(_mediaFileManager, _mediaUrlGenerators, _shortStringHelper, _contentTypeBaseServiceProvider, Constants.Conventions.Media.File, fileName, stream); - } Attempt saveResult = _mediaService.Save(createdMediaItem, _backofficeSecurityAccessor.BackOfficeSecurity?.CurrentUser?.Id ?? -1); diff --git a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs index 1a8be10a5c..d8e6aa8924 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs @@ -55,6 +55,7 @@ public class UsersController : BackOfficeNotificationsController private readonly GlobalSettings _globalSettings; private readonly IHostingEnvironment _hostingEnvironment; private readonly IHttpContextAccessor _httpContextAccessor; + private readonly IFileStreamSecurityValidator? _fileStreamSecurityValidator; // make non nullable in v14 private readonly IImageUrlGenerator _imageUrlGenerator; private readonly LinkGenerator _linkGenerator; private readonly ILocalizedTextService _localizedTextService; @@ -72,6 +73,58 @@ public class UsersController : BackOfficeNotificationsController private readonly WebRoutingSettings _webRoutingSettings; [ActivatorUtilitiesConstructor] + public UsersController( + MediaFileManager mediaFileManager, + IOptionsSnapshot contentSettings, + IHostingEnvironment hostingEnvironment, + ISqlContext sqlContext, + IImageUrlGenerator imageUrlGenerator, + IOptionsSnapshot securitySettings, + IEmailSender emailSender, + IBackOfficeSecurityAccessor backofficeSecurityAccessor, + AppCaches appCaches, + IShortStringHelper shortStringHelper, + IUserService userService, + ILocalizedTextService localizedTextService, + IUmbracoMapper umbracoMapper, + IOptionsSnapshot globalSettings, + IBackOfficeUserManager backOfficeUserManager, + ILoggerFactory loggerFactory, + LinkGenerator linkGenerator, + IBackOfficeExternalLoginProviders externalLogins, + UserEditorAuthorizationHelper userEditorAuthorizationHelper, + IPasswordChanger passwordChanger, + IHttpContextAccessor httpContextAccessor, + IOptions webRoutingSettings, + IFileStreamSecurityValidator fileStreamSecurityValidator) + { + _mediaFileManager = mediaFileManager; + _contentSettings = contentSettings.Value; + _hostingEnvironment = hostingEnvironment; + _sqlContext = sqlContext; + _imageUrlGenerator = imageUrlGenerator; + _securitySettings = securitySettings.Value; + _emailSender = emailSender; + _backofficeSecurityAccessor = backofficeSecurityAccessor; + _appCaches = appCaches; + _shortStringHelper = shortStringHelper; + _userService = userService; + _localizedTextService = localizedTextService; + _umbracoMapper = umbracoMapper; + _globalSettings = globalSettings.Value; + _userManager = backOfficeUserManager; + _loggerFactory = loggerFactory; + _linkGenerator = linkGenerator; + _externalLogins = externalLogins; + _userEditorAuthorizationHelper = userEditorAuthorizationHelper; + _passwordChanger = passwordChanger; + _logger = _loggerFactory.CreateLogger(); + _httpContextAccessor = httpContextAccessor; + _fileStreamSecurityValidator = fileStreamSecurityValidator; + _webRoutingSettings = webRoutingSettings.Value; + } + + [Obsolete("Use constructor overload that has fileStreamSecurityValidator, scheduled for removal in v14")] public UsersController( MediaFileManager mediaFileManager, IOptionsSnapshot contentSettings, @@ -139,13 +192,15 @@ public class UsersController : BackOfficeNotificationsController [AppendUserModifiedHeader("id")] [Authorize(Policy = AuthorizationPolicies.AdminUserEditsRequireAdmin)] - public IActionResult PostSetAvatar(int id, IList file) => PostSetAvatarInternal(file, _userService, + public IActionResult PostSetAvatar(int id, IList file) + => PostSetAvatarInternal(file, _userService, _appCaches.RuntimeCache, _mediaFileManager, _shortStringHelper, _contentSettings, _hostingEnvironment, - _imageUrlGenerator, id); + _imageUrlGenerator,_fileStreamSecurityValidator, id); internal static IActionResult PostSetAvatarInternal(IList files, IUserService userService, IAppCache cache, MediaFileManager mediaFileManager, IShortStringHelper shortStringHelper, ContentSettings contentSettings, IHostingEnvironment hostingEnvironment, IImageUrlGenerator imageUrlGenerator, + IFileStreamSecurityValidator? fileStreamSecurityValidator, int id) { if (files is null) @@ -187,9 +242,14 @@ public class UsersController : BackOfficeNotificationsController //generate a path of known data, we don't want this path to be guessable user.Avatar = "UserAvatars/" + (user.Id + safeFileName).GenerateHash() + "." + ext; - using (Stream fs = file.OpenReadStream()) + //todo implement Filestreamsecurity + using (var ms = new MemoryStream()) { - mediaFileManager.FileSystem.AddFile(user.Avatar, fs, true); + file.CopyTo(ms); + if(fileStreamSecurityValidator != null && fileStreamSecurityValidator.IsConsideredSafe(ms) == false) + return new ValidationErrorResult("One or more file security analyzers deemed the contents of the file to be unsafe"); + + mediaFileManager.FileSystem.AddFile(user.Avatar, ms, true); } userService.Save(user); diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Security/FileStreamSecurityValidatorTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Security/FileStreamSecurityValidatorTests.cs new file mode 100644 index 0000000000..7cdee69daa --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Security/FileStreamSecurityValidatorTests.cs @@ -0,0 +1,123 @@ +using Moq; +using NUnit.Framework; +using Umbraco.Cms.Core.Security; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Security; + +public class FileStreamSecurityValidatorTests +{ + [Test] + public void IsConsideredSafe_True_NoAnalyzersPresent() + { + // Arrange + var sut = new FileStreamSecurityValidator(Enumerable.Empty()); + + using var memoryStream = new MemoryStream(); + using var streamWriter = new StreamWriter(memoryStream); + streamWriter.Write("TestContent"); + streamWriter.Flush(); + memoryStream.Seek(0, SeekOrigin.Begin); + + // Act + var validationResult = sut.IsConsideredSafe(memoryStream); + + // Assert + Assert.IsTrue(validationResult); + } + + [Test] + public void IsConsideredSafe_True_NoAnalyzerMatchesType() + { + // Arrange + var analyzerOne = new Mock(); + analyzerOne.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(false); + var analyzerTwo = new Mock(); + analyzerTwo.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(false); + + var sut = new FileStreamSecurityValidator(new List{analyzerOne.Object,analyzerTwo.Object}); + + using var memoryStream = new MemoryStream(); + using var streamWriter = new StreamWriter(memoryStream); + streamWriter.Write("TestContent"); + streamWriter.Flush(); + memoryStream.Seek(0, SeekOrigin.Begin); + + // Act + var validationResult = sut.IsConsideredSafe(memoryStream); + + // Assert + Assert.IsTrue(validationResult); + } + + [Test] + public void IsConsideredSafe_True_AllMatchingAnalyzersReturnTrue() + { + // Arrange + var matchingAnalyzerOne = new Mock(); + matchingAnalyzerOne.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(true); + matchingAnalyzerOne.Setup(analyzer => analyzer.IsConsideredSafe(It.IsAny())) + .Returns(true); + + var matchingAnalyzerTwo = new Mock(); + matchingAnalyzerTwo.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(true); + matchingAnalyzerTwo.Setup(analyzer => analyzer.IsConsideredSafe(It.IsAny())) + .Returns(true); + + var unmatchedAnalyzer = new Mock(); + unmatchedAnalyzer.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(false); + + var sut = new FileStreamSecurityValidator(new List{matchingAnalyzerOne.Object,matchingAnalyzerTwo.Object}); + + using var memoryStream = new MemoryStream(); + using var streamWriter = new StreamWriter(memoryStream); + streamWriter.Write("TestContent"); + streamWriter.Flush(); + memoryStream.Seek(0, SeekOrigin.Begin); + + // Act + var validationResult = sut.IsConsideredSafe(memoryStream); + + // Assert + Assert.IsTrue(validationResult); + } + + [Test] + public void IsConsideredSafe_False_AnyMatchingAnalyzersReturnFalse() + { + // Arrange + var saveMatchingAnalyzer = new Mock(); + saveMatchingAnalyzer.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(true); + saveMatchingAnalyzer.Setup(analyzer => analyzer.IsConsideredSafe(It.IsAny())) + .Returns(true); + + var unsafeMatchingAnalyzer = new Mock(); + unsafeMatchingAnalyzer.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(true); + unsafeMatchingAnalyzer.Setup(analyzer => analyzer.IsConsideredSafe(It.IsAny())) + .Returns(false); + + var unmatchedAnalyzer = new Mock(); + unmatchedAnalyzer.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(false); + + var sut = new FileStreamSecurityValidator(new List{saveMatchingAnalyzer.Object,unsafeMatchingAnalyzer.Object}); + + using var memoryStream = new MemoryStream(); + using var streamWriter = new StreamWriter(memoryStream); + streamWriter.Write("TestContent"); + streamWriter.Flush(); + memoryStream.Seek(0, SeekOrigin.Begin); + + // Act + var validationResult = sut.IsConsideredSafe(memoryStream); + + // Assert + Assert.IsFalse(validationResult); + } +} From 0dc18982ee9f2e078334b5a28de0908e79195176 Mon Sep 17 00:00:00 2001 From: Kenn Jacobsen Date: Mon, 21 Aug 2023 13:17:15 +0200 Subject: [PATCH 02/15] Do not allow content type property aliases that conflict with IPublishedElement (#14683) --- src/Umbraco.Core/Extensions/TypeExtensions.cs | 159 +++++++++--------- .../ContentTypeModelValidatorBase.cs | 6 +- 2 files changed, 82 insertions(+), 83 deletions(-) diff --git a/src/Umbraco.Core/Extensions/TypeExtensions.cs b/src/Umbraco.Core/Extensions/TypeExtensions.cs index e3da8d9ee1..1416888e73 100644 --- a/src/Umbraco.Core/Extensions/TypeExtensions.cs +++ b/src/Umbraco.Core/Extensions/TypeExtensions.cs @@ -219,96 +219,52 @@ public static class TypeExtensions /// public static PropertyInfo[] GetAllProperties(this Type type) { - if (type.IsInterface) - { - var propertyInfos = new List(); - - var considered = new List(); - var queue = new Queue(); - considered.Add(type); - queue.Enqueue(type); - while (queue.Count > 0) - { - Type subType = queue.Dequeue(); - foreach (Type subInterface in subType.GetInterfaces()) - { - if (considered.Contains(subInterface)) - { - continue; - } - - considered.Add(subInterface); - queue.Enqueue(subInterface); - } - - PropertyInfo[] typeProperties = subType.GetProperties( - BindingFlags.FlattenHierarchy - | BindingFlags.Public - | BindingFlags.NonPublic - | BindingFlags.Instance); - - IEnumerable newPropertyInfos = typeProperties - .Where(x => !propertyInfos.Contains(x)); - - propertyInfos.InsertRange(0, newPropertyInfos); - } - - return propertyInfos.ToArray(); - } - - return type.GetProperties(BindingFlags.FlattenHierarchy - | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance); + const BindingFlags bindingFlags = BindingFlags.FlattenHierarchy + | BindingFlags.Public + | BindingFlags.NonPublic + | BindingFlags.Instance; + return type.GetAllMemberInfos(t => t.GetProperties(bindingFlags)); } /// - /// Returns all public properties including inherited properties even for interfaces + /// Returns public properties including inherited properties even for interfaces /// /// /// - /// - /// taken from - /// http://stackoverflow.com/questions/358835/getproperties-to-return-all-properties-for-an-interface-inheritance-hierarchy - /// public static PropertyInfo[] GetPublicProperties(this Type type) { - if (type.IsInterface) - { - var propertyInfos = new List(); + const BindingFlags bindingFlags = BindingFlags.FlattenHierarchy + | BindingFlags.Public + | BindingFlags.Instance; + return type.GetAllMemberInfos(t => t.GetProperties(bindingFlags)); + } - var considered = new List(); - var queue = new Queue(); - considered.Add(type); - queue.Enqueue(type); - while (queue.Count > 0) - { - Type subType = queue.Dequeue(); - foreach (Type subInterface in subType.GetInterfaces()) - { - if (considered.Contains(subInterface)) - { - continue; - } + /// + /// Returns public methods including inherited methods even for interfaces + /// + /// + /// + public static MethodInfo[] GetPublicMethods(this Type type) + { + const BindingFlags bindingFlags = BindingFlags.FlattenHierarchy + | BindingFlags.Public + | BindingFlags.Instance; + return type.GetAllMemberInfos(t => t.GetMethods(bindingFlags)); + } - considered.Add(subInterface); - queue.Enqueue(subInterface); - } - - PropertyInfo[] typeProperties = subType.GetProperties( - BindingFlags.FlattenHierarchy - | BindingFlags.Public - | BindingFlags.Instance); - - IEnumerable newPropertyInfos = typeProperties - .Where(x => !propertyInfos.Contains(x)); - - propertyInfos.InsertRange(0, newPropertyInfos); - } - - return propertyInfos.ToArray(); - } - - return type.GetProperties(BindingFlags.FlattenHierarchy - | BindingFlags.Public | BindingFlags.Instance); + /// + /// Returns all methods including inherited methods even for interfaces + /// + /// Includes both Public and Non-Public methods + /// + /// + public static MethodInfo[] GetAllMethods(this Type type) + { + const BindingFlags bindingFlags = BindingFlags.FlattenHierarchy + | BindingFlags.Public + | BindingFlags.NonPublic + | BindingFlags.Instance; + return type.GetAllMemberInfos(t => t.GetMethods(bindingFlags)); } /// @@ -512,4 +468,47 @@ public static class TypeExtensions return attempt; } + + /// + /// taken from + /// http://stackoverflow.com/questions/358835/getproperties-to-return-all-properties-for-an-interface-inheritance-hierarchy + /// + private static T[] GetAllMemberInfos(this Type type, Func getMemberInfos) + where T : MemberInfo + { + if (type.IsInterface is false) + { + return getMemberInfos(type); + } + + var memberInfos = new List(); + + var considered = new List(); + var queue = new Queue(); + considered.Add(type); + queue.Enqueue(type); + while (queue.Count > 0) + { + Type subType = queue.Dequeue(); + foreach (Type subInterface in subType.GetInterfaces()) + { + if (considered.Contains(subInterface)) + { + continue; + } + + considered.Add(subInterface); + queue.Enqueue(subInterface); + } + + T[] typeMethodInfos = getMemberInfos(subType); + + IEnumerable newMethodInfos = typeMethodInfos + .Where(x => !memberInfos.Contains(x)); + + memberInfos.InsertRange(0, newMethodInfos); + } + + return memberInfos.ToArray(); + } } diff --git a/src/Umbraco.Web.BackOffice/ModelsBuilder/ContentTypeModelValidatorBase.cs b/src/Umbraco.Web.BackOffice/ModelsBuilder/ContentTypeModelValidatorBase.cs index ef59126c1c..47ad9e4e80 100644 --- a/src/Umbraco.Web.BackOffice/ModelsBuilder/ContentTypeModelValidatorBase.cs +++ b/src/Umbraco.Web.BackOffice/ModelsBuilder/ContentTypeModelValidatorBase.cs @@ -68,10 +68,10 @@ public abstract class ContentTypeModelValidatorBase : EditorV private ValidationResult? ValidateProperty(PropertyTypeBasic property, int groupIndex, int propertyIndex) { - // don't let them match any properties or methods in IPublishedContent + // don't let them match any properties or methods in IPublishedContent (including those defined in any base interfaces like IPublishedElement) // TODO: There are probably more! - var reservedProperties = typeof(IPublishedContent).GetProperties().Select(x => x.Name).ToArray(); - var reservedMethods = typeof(IPublishedContent).GetMethods().Select(x => x.Name).ToArray(); + var reservedProperties = typeof(IPublishedContent).GetPublicProperties().Select(x => x.Name).ToArray(); + var reservedMethods = typeof(IPublishedContent).GetPublicMethods().Select(x => x.Name).ToArray(); var alias = property.Alias; From 380af0057be1bfb1e1158afad9e037a6081ef641 Mon Sep 17 00:00:00 2001 From: Nikolaj Date: Tue, 22 Aug 2023 10:25:55 +0200 Subject: [PATCH 03/15] Bump version --- version.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version.json b/version.json index fe28088e74..6761d6acc5 100644 --- a/version.json +++ b/version.json @@ -1,6 +1,6 @@ { "$schema": "https://raw.githubusercontent.com/dotnet/Nerdbank.GitVersioning/master/src/NerdBank.GitVersioning/version.schema.json", - "version": "11.4.3", + "version": "11.5.0-rc", "assemblyVersion": { "precision": "build" }, From 38910a8d5c206934c02d266f88adec726ee83bd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Tue, 22 Aug 2023 11:27:16 +0200 Subject: [PATCH 04/15] directly render labels without angularJS template code (#14700) --- .../src/common/services/blockeditormodelobject.service.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Umbraco.Web.UI.Client/src/common/services/blockeditormodelobject.service.js b/src/Umbraco.Web.UI.Client/src/common/services/blockeditormodelobject.service.js index 20661d5d1c..606d16ad2d 100644 --- a/src/Umbraco.Web.UI.Client/src/common/services/blockeditormodelobject.service.js +++ b/src/Umbraco.Web.UI.Client/src/common/services/blockeditormodelobject.service.js @@ -644,6 +644,12 @@ blockObject.index = 0; if (blockObject.config.label && blockObject.config.label !== "" && blockObject.config.unsupported !== true) { + + // If the label does not contain any AngularJS template, then the MutationObserver wont give us any updates. To ensure labels without angular JS template code, we will just set the label directly for ones without '{{': + if(blockObject.config.label.indexOf("{{") === -1) { + blockObject.label = blockObject.config.label; + } + var labelElement = $('
', { text: blockObject.config.label}); var observer = new MutationObserver(function(mutations) { @@ -673,6 +679,7 @@ this.__labelScope = Object.assign(this.__labelScope, labelVars); $compile(labelElement.contents())(this.__labelScope); + }.bind(blockObject) } else { blockObject.__renderLabel = function() {}; From e1b4aebb0faa0fe479983d5940605f9e5c00296a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Tue, 22 Aug 2023 11:27:16 +0200 Subject: [PATCH 05/15] directly render labels without angularJS template code (#14700) --- .../src/common/services/blockeditormodelobject.service.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Umbraco.Web.UI.Client/src/common/services/blockeditormodelobject.service.js b/src/Umbraco.Web.UI.Client/src/common/services/blockeditormodelobject.service.js index 20661d5d1c..606d16ad2d 100644 --- a/src/Umbraco.Web.UI.Client/src/common/services/blockeditormodelobject.service.js +++ b/src/Umbraco.Web.UI.Client/src/common/services/blockeditormodelobject.service.js @@ -644,6 +644,12 @@ blockObject.index = 0; if (blockObject.config.label && blockObject.config.label !== "" && blockObject.config.unsupported !== true) { + + // If the label does not contain any AngularJS template, then the MutationObserver wont give us any updates. To ensure labels without angular JS template code, we will just set the label directly for ones without '{{': + if(blockObject.config.label.indexOf("{{") === -1) { + blockObject.label = blockObject.config.label; + } + var labelElement = $('
', { text: blockObject.config.label}); var observer = new MutationObserver(function(mutations) { @@ -673,6 +679,7 @@ this.__labelScope = Object.assign(this.__labelScope, labelVars); $compile(labelElement.contents())(this.__labelScope); + }.bind(blockObject) } else { blockObject.__renderLabel = function() {}; From 19ee8e925411d83a02d55b4872b76f53133d0b93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Tue, 22 Aug 2023 11:27:16 +0200 Subject: [PATCH 06/15] directly render labels without angularJS template code (#14700) --- .../src/common/services/blockeditormodelobject.service.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Umbraco.Web.UI.Client/src/common/services/blockeditormodelobject.service.js b/src/Umbraco.Web.UI.Client/src/common/services/blockeditormodelobject.service.js index 20661d5d1c..606d16ad2d 100644 --- a/src/Umbraco.Web.UI.Client/src/common/services/blockeditormodelobject.service.js +++ b/src/Umbraco.Web.UI.Client/src/common/services/blockeditormodelobject.service.js @@ -644,6 +644,12 @@ blockObject.index = 0; if (blockObject.config.label && blockObject.config.label !== "" && blockObject.config.unsupported !== true) { + + // If the label does not contain any AngularJS template, then the MutationObserver wont give us any updates. To ensure labels without angular JS template code, we will just set the label directly for ones without '{{': + if(blockObject.config.label.indexOf("{{") === -1) { + blockObject.label = blockObject.config.label; + } + var labelElement = $('
', { text: blockObject.config.label}); var observer = new MutationObserver(function(mutations) { @@ -673,6 +679,7 @@ this.__labelScope = Object.assign(this.__labelScope, labelVars); $compile(labelElement.contents())(this.__labelScope); + }.bind(blockObject) } else { blockObject.__renderLabel = function() {}; From 80ea5174c6d1e52f798cd892acf243ed8e6ed27d Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Tue, 22 Aug 2023 12:28:01 +0100 Subject: [PATCH 07/15] Updated JSON schema reference for Umbraco Forms. (#14701) --- src/JsonSchema/JsonSchema.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/JsonSchema/JsonSchema.csproj b/src/JsonSchema/JsonSchema.csproj index edf62516a5..07a29835de 100644 --- a/src/JsonSchema/JsonSchema.csproj +++ b/src/JsonSchema/JsonSchema.csproj @@ -13,6 +13,6 @@ - + From 9fd0b1986c39344f8f7331b5c8b69cca76d228d4 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Tue, 22 Aug 2023 12:28:01 +0100 Subject: [PATCH 08/15] Updated JSON schema reference for Umbraco Forms. (#14701) --- src/JsonSchema/JsonSchema.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/JsonSchema/JsonSchema.csproj b/src/JsonSchema/JsonSchema.csproj index edf62516a5..07a29835de 100644 --- a/src/JsonSchema/JsonSchema.csproj +++ b/src/JsonSchema/JsonSchema.csproj @@ -13,6 +13,6 @@ - + From e5a8d01004c151cbcf91dd0a50344b0b16864dab Mon Sep 17 00:00:00 2001 From: Ronald Barendse Date: Wed, 23 Aug 2023 10:09:43 +0200 Subject: [PATCH 09/15] Fix exceptions in Slider and Tags property value converters (#13782) * Fix IndexOutOfRangeException when converting single value to range in SliderValueConverter * Fix NullReferenceException while deserializing empty value in TagsValueConverter * Use invariant decimal parsing * Handle converting from slider to single value * Fix parsing range as single value * Make Handle methods autonomous --------- Co-authored-by: nikolajlauridsen --- .../Cache/DataTypeCacheRefresher.cs | 4 - .../ValueConverters/SliderValueConverter.cs | 127 ++++++++++++------ .../ValueConverters/TagsValueConverter.cs | 78 +++++------ 3 files changed, 125 insertions(+), 84 deletions(-) diff --git a/src/Umbraco.Core/Cache/DataTypeCacheRefresher.cs b/src/Umbraco.Core/Cache/DataTypeCacheRefresher.cs index ea661c5498..394630fa64 100644 --- a/src/Umbraco.Core/Cache/DataTypeCacheRefresher.cs +++ b/src/Umbraco.Core/Cache/DataTypeCacheRefresher.cs @@ -88,10 +88,6 @@ public sealed class DataTypeCacheRefresher : PayloadCacheRefresherBase _publishedSnapshotService.Notify(payloads)); diff --git a/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs b/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs index 76f5b62265..14e80952f4 100644 --- a/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs +++ b/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs @@ -1,4 +1,4 @@ -using System.Collections.Concurrent; +using System.Globalization; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.PublishedContent; using Umbraco.Cms.Core.Services; @@ -6,74 +6,123 @@ using Umbraco.Extensions; namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters; +/// +/// The slider property value converter. +/// +/// [DefaultPropertyValueConverter] public class SliderValueConverter : PropertyValueConverterBase { - private static readonly ConcurrentDictionary Storages = new(); - private readonly IDataTypeService _dataTypeService; + /// + /// Initializes a new instance of the class. + /// + public SliderValueConverter() + { } - public SliderValueConverter(IDataTypeService dataTypeService) => _dataTypeService = - dataTypeService ?? throw new ArgumentNullException(nameof(dataTypeService)); + /// + /// Initializes a new instance of the class. + /// + /// The data type service. + [Obsolete("The IDataTypeService is not used anymore. This constructor will be removed in a future version.")] + public SliderValueConverter(IDataTypeService dataTypeService) + { } - public static void ClearCaches() => Storages.Clear(); + /// + /// Clears the data type configuration caches. + /// + [Obsolete("Caching of data type configuration is not done anymore. This method will be removed in a future version.")] + public static void ClearCaches() + { } + /// public override bool IsConverter(IPublishedPropertyType propertyType) => propertyType.EditorAlias.InvariantEquals(Constants.PropertyEditors.Aliases.Slider); + /// public override Type GetPropertyValueType(IPublishedPropertyType propertyType) - => IsRangeDataType(propertyType.DataType.Id) ? typeof(Range) : typeof(decimal); + => IsRange(propertyType) ? typeof(Range) : typeof(decimal); + /// public override PropertyCacheLevel GetPropertyCacheLevel(IPublishedPropertyType propertyType) => PropertyCacheLevel.Element; + /// public override object? ConvertIntermediateToObject(IPublishedElement owner, IPublishedPropertyType propertyType, PropertyCacheLevel cacheLevel, object? source, bool preview) { - if (source == null) + bool isRange = IsRange(propertyType); + + var sourceString = source?.ToString(); + + return isRange + ? HandleRange(sourceString) + : HandleDecimal(sourceString); + } + + private static Range HandleRange(string? sourceString) + { + if (sourceString is null) { - return null; + return new Range(); } - if (IsRangeDataType(propertyType.DataType.Id)) - { - var rangeRawValues = source.ToString()!.Split(Constants.CharArrays.Comma); - Attempt minimumAttempt = rangeRawValues[0].TryConvertTo(); - Attempt maximumAttempt = rangeRawValues[1].TryConvertTo(); + string[] rangeRawValues = sourceString.Split(Constants.CharArrays.Comma); - if (minimumAttempt.Success && maximumAttempt.Success) + if (TryParseDecimal(rangeRawValues[0], out var minimum)) + { + if (rangeRawValues.Length == 1) { - return new Range { Maximum = maximumAttempt.Result, Minimum = minimumAttempt.Result }; + // Configuration is probably changed from single to range, return range with same min/max + return new Range + { + Minimum = minimum, + Maximum = minimum + }; + } + + if (rangeRawValues.Length == 2 && TryParseDecimal(rangeRawValues[1], out var maximum)) + { + return new Range + { + Minimum = minimum, + Maximum = maximum + }; } } - Attempt valueAttempt = source.ToString().TryConvertTo(); - if (valueAttempt.Success) + return new Range(); + } + + private static decimal HandleDecimal(string? sourceString) + { + if (string.IsNullOrEmpty(sourceString)) { - return valueAttempt.Result; + return default; } - // Something failed in the conversion of the strings to decimals - return null; + // This used to be a range slider, so we'll assign the minimum value as the new value + if (sourceString.Contains(',')) + { + var minimumValueRepresentation = sourceString.Split(Constants.CharArrays.Comma)[0]; + + if (TryParseDecimal(minimumValueRepresentation, out var minimum)) + { + return minimum; + } + } + else if (TryParseDecimal(sourceString, out var value)) + { + return value; + } + + return default; } /// - /// Discovers if the slider is set to range mode. + /// Helper method for parsing a double consistently /// - /// - /// The data type id. - /// - /// - /// The . - /// - private bool IsRangeDataType(int dataTypeId) => + private static bool TryParseDecimal(string? representation, out decimal value) + => decimal.TryParse(representation, NumberStyles.Number, CultureInfo.InvariantCulture, out value); - // GetPreValuesCollectionByDataTypeId is cached at repository level; - // still, the collection is deep-cloned so this is kinda expensive, - // better to cache here + trigger refresh in DataTypeCacheRefresher - // TODO: this is cheap now, remove the caching - Storages.GetOrAdd(dataTypeId, id => - { - IDataType? dataType = _dataTypeService.GetDataType(id); - SliderConfiguration? configuration = dataType?.ConfigurationAs(); - return configuration?.EnableRange ?? false; - }); + private static bool IsRange(IPublishedPropertyType propertyType) + => propertyType.DataType.ConfigurationAs()?.EnableRange == true; } diff --git a/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs b/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs index 3afc5a6596..2dd1c1d56e 100644 --- a/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs +++ b/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs @@ -1,4 +1,3 @@ -using System.Collections.Concurrent; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.PublishedContent; using Umbraco.Cms.Core.Serialization; @@ -7,69 +6,66 @@ using Umbraco.Extensions; namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters; +/// +/// The tags property value converter. +/// +/// [DefaultPropertyValueConverter] public class TagsValueConverter : PropertyValueConverterBase { - private static readonly ConcurrentDictionary Storages = new(); - private readonly IDataTypeService _dataTypeService; private readonly IJsonSerializer _jsonSerializer; + /// + /// Initializes a new instance of the class. + /// + /// The JSON serializer. + /// jsonSerializer + public TagsValueConverter(IJsonSerializer jsonSerializer) + => _jsonSerializer = jsonSerializer ?? throw new ArgumentNullException(nameof(jsonSerializer)); + + /// + /// Initializes a new instance of the class. + /// + /// The data type service. + /// The JSON serializer. + [Obsolete("The IDataTypeService is not used anymore. This constructor will be removed in a future version.")] public TagsValueConverter(IDataTypeService dataTypeService, IJsonSerializer jsonSerializer) - { - _dataTypeService = dataTypeService ?? throw new ArgumentNullException(nameof(dataTypeService)); - _jsonSerializer = jsonSerializer ?? throw new ArgumentNullException(nameof(jsonSerializer)); - } + : this(jsonSerializer) + { } - public static void ClearCaches() => Storages.Clear(); + /// + /// Clears the data type configuration caches. + /// + [Obsolete("Caching of data type configuration is not done anymore. This method will be removed in a future version.")] + public static void ClearCaches() + { } + /// public override bool IsConverter(IPublishedPropertyType propertyType) => propertyType.EditorAlias.InvariantEquals(Constants.PropertyEditors.Aliases.Tags); + /// public override Type GetPropertyValueType(IPublishedPropertyType propertyType) => typeof(IEnumerable); + /// public override PropertyCacheLevel GetPropertyCacheLevel(IPublishedPropertyType propertyType) => PropertyCacheLevel.Element; + /// public override object? ConvertSourceToIntermediate(IPublishedElement owner, IPublishedPropertyType propertyType, object? source, bool preview) { - if (source == null) + string? sourceString = source?.ToString(); + if (string.IsNullOrEmpty(sourceString)) { return Array.Empty(); } - // if Json storage type deserialize and return as string array - if (JsonStorageType(propertyType.DataType.Id)) - { - var array = source.ToString() is not null - ? _jsonSerializer.Deserialize(source.ToString()!) - : null; - return array ?? Array.Empty(); - } - - // Otherwise assume CSV storage type and return as string array - return source.ToString()?.Split(Constants.CharArrays.Comma, StringSplitOptions.RemoveEmptyEntries); + return IsJson(propertyType) + ? _jsonSerializer.Deserialize(sourceString) ?? Array.Empty() + : sourceString.Split(Constants.CharArrays.Comma, StringSplitOptions.RemoveEmptyEntries); } - public override object? ConvertIntermediateToObject(IPublishedElement owner, IPublishedPropertyType propertyType, PropertyCacheLevel cacheLevel, object? source, bool preview) => (string[]?)source; - - /// - /// Discovers if the tags data type is storing its data in a Json format - /// - /// - /// The data type id. - /// - /// - /// The . - /// - private bool JsonStorageType(int dataTypeId) => - - // GetDataType(id) is cached at repository level; still, there is some - // deep-cloning involved (expensive) - better cache here + trigger - // refresh in DataTypeCacheRefresher - Storages.GetOrAdd(dataTypeId, id => - { - TagConfiguration? configuration = _dataTypeService.GetDataType(id)?.ConfigurationAs(); - return configuration?.StorageType == TagsStorageType.Json; - }); + private static bool IsJson(IPublishedPropertyType propertyType) + => propertyType.DataType.ConfigurationAs()?.StorageType == TagsStorageType.Json; } From 3c37653012d1d4e43c74c605cdd5b786c51a5026 Mon Sep 17 00:00:00 2001 From: Ronald Barendse Date: Wed, 23 Aug 2023 10:09:43 +0200 Subject: [PATCH 10/15] Fix exceptions in Slider and Tags property value converters (#13782) * Fix IndexOutOfRangeException when converting single value to range in SliderValueConverter * Fix NullReferenceException while deserializing empty value in TagsValueConverter * Use invariant decimal parsing * Handle converting from slider to single value * Fix parsing range as single value * Make Handle methods autonomous --------- Co-authored-by: nikolajlauridsen --- .../Cache/DataTypeCacheRefresher.cs | 4 - .../ValueConverters/SliderValueConverter.cs | 127 ++++++++++++------ .../ValueConverters/TagsValueConverter.cs | 78 +++++------ 3 files changed, 125 insertions(+), 84 deletions(-) diff --git a/src/Umbraco.Core/Cache/DataTypeCacheRefresher.cs b/src/Umbraco.Core/Cache/DataTypeCacheRefresher.cs index ea661c5498..394630fa64 100644 --- a/src/Umbraco.Core/Cache/DataTypeCacheRefresher.cs +++ b/src/Umbraco.Core/Cache/DataTypeCacheRefresher.cs @@ -88,10 +88,6 @@ public sealed class DataTypeCacheRefresher : PayloadCacheRefresherBase _publishedSnapshotService.Notify(payloads)); diff --git a/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs b/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs index 76f5b62265..14e80952f4 100644 --- a/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs +++ b/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs @@ -1,4 +1,4 @@ -using System.Collections.Concurrent; +using System.Globalization; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.PublishedContent; using Umbraco.Cms.Core.Services; @@ -6,74 +6,123 @@ using Umbraco.Extensions; namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters; +/// +/// The slider property value converter. +/// +/// [DefaultPropertyValueConverter] public class SliderValueConverter : PropertyValueConverterBase { - private static readonly ConcurrentDictionary Storages = new(); - private readonly IDataTypeService _dataTypeService; + /// + /// Initializes a new instance of the class. + /// + public SliderValueConverter() + { } - public SliderValueConverter(IDataTypeService dataTypeService) => _dataTypeService = - dataTypeService ?? throw new ArgumentNullException(nameof(dataTypeService)); + /// + /// Initializes a new instance of the class. + /// + /// The data type service. + [Obsolete("The IDataTypeService is not used anymore. This constructor will be removed in a future version.")] + public SliderValueConverter(IDataTypeService dataTypeService) + { } - public static void ClearCaches() => Storages.Clear(); + /// + /// Clears the data type configuration caches. + /// + [Obsolete("Caching of data type configuration is not done anymore. This method will be removed in a future version.")] + public static void ClearCaches() + { } + /// public override bool IsConverter(IPublishedPropertyType propertyType) => propertyType.EditorAlias.InvariantEquals(Constants.PropertyEditors.Aliases.Slider); + /// public override Type GetPropertyValueType(IPublishedPropertyType propertyType) - => IsRangeDataType(propertyType.DataType.Id) ? typeof(Range) : typeof(decimal); + => IsRange(propertyType) ? typeof(Range) : typeof(decimal); + /// public override PropertyCacheLevel GetPropertyCacheLevel(IPublishedPropertyType propertyType) => PropertyCacheLevel.Element; + /// public override object? ConvertIntermediateToObject(IPublishedElement owner, IPublishedPropertyType propertyType, PropertyCacheLevel cacheLevel, object? source, bool preview) { - if (source == null) + bool isRange = IsRange(propertyType); + + var sourceString = source?.ToString(); + + return isRange + ? HandleRange(sourceString) + : HandleDecimal(sourceString); + } + + private static Range HandleRange(string? sourceString) + { + if (sourceString is null) { - return null; + return new Range(); } - if (IsRangeDataType(propertyType.DataType.Id)) - { - var rangeRawValues = source.ToString()!.Split(Constants.CharArrays.Comma); - Attempt minimumAttempt = rangeRawValues[0].TryConvertTo(); - Attempt maximumAttempt = rangeRawValues[1].TryConvertTo(); + string[] rangeRawValues = sourceString.Split(Constants.CharArrays.Comma); - if (minimumAttempt.Success && maximumAttempt.Success) + if (TryParseDecimal(rangeRawValues[0], out var minimum)) + { + if (rangeRawValues.Length == 1) { - return new Range { Maximum = maximumAttempt.Result, Minimum = minimumAttempt.Result }; + // Configuration is probably changed from single to range, return range with same min/max + return new Range + { + Minimum = minimum, + Maximum = minimum + }; + } + + if (rangeRawValues.Length == 2 && TryParseDecimal(rangeRawValues[1], out var maximum)) + { + return new Range + { + Minimum = minimum, + Maximum = maximum + }; } } - Attempt valueAttempt = source.ToString().TryConvertTo(); - if (valueAttempt.Success) + return new Range(); + } + + private static decimal HandleDecimal(string? sourceString) + { + if (string.IsNullOrEmpty(sourceString)) { - return valueAttempt.Result; + return default; } - // Something failed in the conversion of the strings to decimals - return null; + // This used to be a range slider, so we'll assign the minimum value as the new value + if (sourceString.Contains(',')) + { + var minimumValueRepresentation = sourceString.Split(Constants.CharArrays.Comma)[0]; + + if (TryParseDecimal(minimumValueRepresentation, out var minimum)) + { + return minimum; + } + } + else if (TryParseDecimal(sourceString, out var value)) + { + return value; + } + + return default; } /// - /// Discovers if the slider is set to range mode. + /// Helper method for parsing a double consistently /// - /// - /// The data type id. - /// - /// - /// The . - /// - private bool IsRangeDataType(int dataTypeId) => + private static bool TryParseDecimal(string? representation, out decimal value) + => decimal.TryParse(representation, NumberStyles.Number, CultureInfo.InvariantCulture, out value); - // GetPreValuesCollectionByDataTypeId is cached at repository level; - // still, the collection is deep-cloned so this is kinda expensive, - // better to cache here + trigger refresh in DataTypeCacheRefresher - // TODO: this is cheap now, remove the caching - Storages.GetOrAdd(dataTypeId, id => - { - IDataType? dataType = _dataTypeService.GetDataType(id); - SliderConfiguration? configuration = dataType?.ConfigurationAs(); - return configuration?.EnableRange ?? false; - }); + private static bool IsRange(IPublishedPropertyType propertyType) + => propertyType.DataType.ConfigurationAs()?.EnableRange == true; } diff --git a/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs b/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs index 3afc5a6596..2dd1c1d56e 100644 --- a/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs +++ b/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs @@ -1,4 +1,3 @@ -using System.Collections.Concurrent; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.PublishedContent; using Umbraco.Cms.Core.Serialization; @@ -7,69 +6,66 @@ using Umbraco.Extensions; namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters; +/// +/// The tags property value converter. +/// +/// [DefaultPropertyValueConverter] public class TagsValueConverter : PropertyValueConverterBase { - private static readonly ConcurrentDictionary Storages = new(); - private readonly IDataTypeService _dataTypeService; private readonly IJsonSerializer _jsonSerializer; + /// + /// Initializes a new instance of the class. + /// + /// The JSON serializer. + /// jsonSerializer + public TagsValueConverter(IJsonSerializer jsonSerializer) + => _jsonSerializer = jsonSerializer ?? throw new ArgumentNullException(nameof(jsonSerializer)); + + /// + /// Initializes a new instance of the class. + /// + /// The data type service. + /// The JSON serializer. + [Obsolete("The IDataTypeService is not used anymore. This constructor will be removed in a future version.")] public TagsValueConverter(IDataTypeService dataTypeService, IJsonSerializer jsonSerializer) - { - _dataTypeService = dataTypeService ?? throw new ArgumentNullException(nameof(dataTypeService)); - _jsonSerializer = jsonSerializer ?? throw new ArgumentNullException(nameof(jsonSerializer)); - } + : this(jsonSerializer) + { } - public static void ClearCaches() => Storages.Clear(); + /// + /// Clears the data type configuration caches. + /// + [Obsolete("Caching of data type configuration is not done anymore. This method will be removed in a future version.")] + public static void ClearCaches() + { } + /// public override bool IsConverter(IPublishedPropertyType propertyType) => propertyType.EditorAlias.InvariantEquals(Constants.PropertyEditors.Aliases.Tags); + /// public override Type GetPropertyValueType(IPublishedPropertyType propertyType) => typeof(IEnumerable); + /// public override PropertyCacheLevel GetPropertyCacheLevel(IPublishedPropertyType propertyType) => PropertyCacheLevel.Element; + /// public override object? ConvertSourceToIntermediate(IPublishedElement owner, IPublishedPropertyType propertyType, object? source, bool preview) { - if (source == null) + string? sourceString = source?.ToString(); + if (string.IsNullOrEmpty(sourceString)) { return Array.Empty(); } - // if Json storage type deserialize and return as string array - if (JsonStorageType(propertyType.DataType.Id)) - { - var array = source.ToString() is not null - ? _jsonSerializer.Deserialize(source.ToString()!) - : null; - return array ?? Array.Empty(); - } - - // Otherwise assume CSV storage type and return as string array - return source.ToString()?.Split(Constants.CharArrays.Comma, StringSplitOptions.RemoveEmptyEntries); + return IsJson(propertyType) + ? _jsonSerializer.Deserialize(sourceString) ?? Array.Empty() + : sourceString.Split(Constants.CharArrays.Comma, StringSplitOptions.RemoveEmptyEntries); } - public override object? ConvertIntermediateToObject(IPublishedElement owner, IPublishedPropertyType propertyType, PropertyCacheLevel cacheLevel, object? source, bool preview) => (string[]?)source; - - /// - /// Discovers if the tags data type is storing its data in a Json format - /// - /// - /// The data type id. - /// - /// - /// The . - /// - private bool JsonStorageType(int dataTypeId) => - - // GetDataType(id) is cached at repository level; still, there is some - // deep-cloning involved (expensive) - better cache here + trigger - // refresh in DataTypeCacheRefresher - Storages.GetOrAdd(dataTypeId, id => - { - TagConfiguration? configuration = _dataTypeService.GetDataType(id)?.ConfigurationAs(); - return configuration?.StorageType == TagsStorageType.Json; - }); + private static bool IsJson(IPublishedPropertyType propertyType) + => propertyType.DataType.ConfigurationAs()?.StorageType == TagsStorageType.Json; } From f97e9a9f34a2350aac4f5777ef240c9b63038f8e Mon Sep 17 00:00:00 2001 From: Ronald Barendse Date: Wed, 23 Aug 2023 10:09:43 +0200 Subject: [PATCH 11/15] Fix exceptions in Slider and Tags property value converters (#13782) * Fix IndexOutOfRangeException when converting single value to range in SliderValueConverter * Fix NullReferenceException while deserializing empty value in TagsValueConverter * Use invariant decimal parsing * Handle converting from slider to single value * Fix parsing range as single value * Make Handle methods autonomous --------- Co-authored-by: nikolajlauridsen --- .../Cache/DataTypeCacheRefresher.cs | 4 - .../ValueConverters/SliderValueConverter.cs | 127 ++++++++++++------ .../ValueConverters/TagsValueConverter.cs | 78 +++++------ 3 files changed, 125 insertions(+), 84 deletions(-) diff --git a/src/Umbraco.Core/Cache/DataTypeCacheRefresher.cs b/src/Umbraco.Core/Cache/DataTypeCacheRefresher.cs index ea661c5498..394630fa64 100644 --- a/src/Umbraco.Core/Cache/DataTypeCacheRefresher.cs +++ b/src/Umbraco.Core/Cache/DataTypeCacheRefresher.cs @@ -88,10 +88,6 @@ public sealed class DataTypeCacheRefresher : PayloadCacheRefresherBase _publishedSnapshotService.Notify(payloads)); diff --git a/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs b/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs index 76f5b62265..14e80952f4 100644 --- a/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs +++ b/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs @@ -1,4 +1,4 @@ -using System.Collections.Concurrent; +using System.Globalization; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.PublishedContent; using Umbraco.Cms.Core.Services; @@ -6,74 +6,123 @@ using Umbraco.Extensions; namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters; +/// +/// The slider property value converter. +/// +/// [DefaultPropertyValueConverter] public class SliderValueConverter : PropertyValueConverterBase { - private static readonly ConcurrentDictionary Storages = new(); - private readonly IDataTypeService _dataTypeService; + /// + /// Initializes a new instance of the class. + /// + public SliderValueConverter() + { } - public SliderValueConverter(IDataTypeService dataTypeService) => _dataTypeService = - dataTypeService ?? throw new ArgumentNullException(nameof(dataTypeService)); + /// + /// Initializes a new instance of the class. + /// + /// The data type service. + [Obsolete("The IDataTypeService is not used anymore. This constructor will be removed in a future version.")] + public SliderValueConverter(IDataTypeService dataTypeService) + { } - public static void ClearCaches() => Storages.Clear(); + /// + /// Clears the data type configuration caches. + /// + [Obsolete("Caching of data type configuration is not done anymore. This method will be removed in a future version.")] + public static void ClearCaches() + { } + /// public override bool IsConverter(IPublishedPropertyType propertyType) => propertyType.EditorAlias.InvariantEquals(Constants.PropertyEditors.Aliases.Slider); + /// public override Type GetPropertyValueType(IPublishedPropertyType propertyType) - => IsRangeDataType(propertyType.DataType.Id) ? typeof(Range) : typeof(decimal); + => IsRange(propertyType) ? typeof(Range) : typeof(decimal); + /// public override PropertyCacheLevel GetPropertyCacheLevel(IPublishedPropertyType propertyType) => PropertyCacheLevel.Element; + /// public override object? ConvertIntermediateToObject(IPublishedElement owner, IPublishedPropertyType propertyType, PropertyCacheLevel cacheLevel, object? source, bool preview) { - if (source == null) + bool isRange = IsRange(propertyType); + + var sourceString = source?.ToString(); + + return isRange + ? HandleRange(sourceString) + : HandleDecimal(sourceString); + } + + private static Range HandleRange(string? sourceString) + { + if (sourceString is null) { - return null; + return new Range(); } - if (IsRangeDataType(propertyType.DataType.Id)) - { - var rangeRawValues = source.ToString()!.Split(Constants.CharArrays.Comma); - Attempt minimumAttempt = rangeRawValues[0].TryConvertTo(); - Attempt maximumAttempt = rangeRawValues[1].TryConvertTo(); + string[] rangeRawValues = sourceString.Split(Constants.CharArrays.Comma); - if (minimumAttempt.Success && maximumAttempt.Success) + if (TryParseDecimal(rangeRawValues[0], out var minimum)) + { + if (rangeRawValues.Length == 1) { - return new Range { Maximum = maximumAttempt.Result, Minimum = minimumAttempt.Result }; + // Configuration is probably changed from single to range, return range with same min/max + return new Range + { + Minimum = minimum, + Maximum = minimum + }; + } + + if (rangeRawValues.Length == 2 && TryParseDecimal(rangeRawValues[1], out var maximum)) + { + return new Range + { + Minimum = minimum, + Maximum = maximum + }; } } - Attempt valueAttempt = source.ToString().TryConvertTo(); - if (valueAttempt.Success) + return new Range(); + } + + private static decimal HandleDecimal(string? sourceString) + { + if (string.IsNullOrEmpty(sourceString)) { - return valueAttempt.Result; + return default; } - // Something failed in the conversion of the strings to decimals - return null; + // This used to be a range slider, so we'll assign the minimum value as the new value + if (sourceString.Contains(',')) + { + var minimumValueRepresentation = sourceString.Split(Constants.CharArrays.Comma)[0]; + + if (TryParseDecimal(minimumValueRepresentation, out var minimum)) + { + return minimum; + } + } + else if (TryParseDecimal(sourceString, out var value)) + { + return value; + } + + return default; } /// - /// Discovers if the slider is set to range mode. + /// Helper method for parsing a double consistently /// - /// - /// The data type id. - /// - /// - /// The . - /// - private bool IsRangeDataType(int dataTypeId) => + private static bool TryParseDecimal(string? representation, out decimal value) + => decimal.TryParse(representation, NumberStyles.Number, CultureInfo.InvariantCulture, out value); - // GetPreValuesCollectionByDataTypeId is cached at repository level; - // still, the collection is deep-cloned so this is kinda expensive, - // better to cache here + trigger refresh in DataTypeCacheRefresher - // TODO: this is cheap now, remove the caching - Storages.GetOrAdd(dataTypeId, id => - { - IDataType? dataType = _dataTypeService.GetDataType(id); - SliderConfiguration? configuration = dataType?.ConfigurationAs(); - return configuration?.EnableRange ?? false; - }); + private static bool IsRange(IPublishedPropertyType propertyType) + => propertyType.DataType.ConfigurationAs()?.EnableRange == true; } diff --git a/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs b/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs index 3afc5a6596..2dd1c1d56e 100644 --- a/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs +++ b/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs @@ -1,4 +1,3 @@ -using System.Collections.Concurrent; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.PublishedContent; using Umbraco.Cms.Core.Serialization; @@ -7,69 +6,66 @@ using Umbraco.Extensions; namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters; +/// +/// The tags property value converter. +/// +/// [DefaultPropertyValueConverter] public class TagsValueConverter : PropertyValueConverterBase { - private static readonly ConcurrentDictionary Storages = new(); - private readonly IDataTypeService _dataTypeService; private readonly IJsonSerializer _jsonSerializer; + /// + /// Initializes a new instance of the class. + /// + /// The JSON serializer. + /// jsonSerializer + public TagsValueConverter(IJsonSerializer jsonSerializer) + => _jsonSerializer = jsonSerializer ?? throw new ArgumentNullException(nameof(jsonSerializer)); + + /// + /// Initializes a new instance of the class. + /// + /// The data type service. + /// The JSON serializer. + [Obsolete("The IDataTypeService is not used anymore. This constructor will be removed in a future version.")] public TagsValueConverter(IDataTypeService dataTypeService, IJsonSerializer jsonSerializer) - { - _dataTypeService = dataTypeService ?? throw new ArgumentNullException(nameof(dataTypeService)); - _jsonSerializer = jsonSerializer ?? throw new ArgumentNullException(nameof(jsonSerializer)); - } + : this(jsonSerializer) + { } - public static void ClearCaches() => Storages.Clear(); + /// + /// Clears the data type configuration caches. + /// + [Obsolete("Caching of data type configuration is not done anymore. This method will be removed in a future version.")] + public static void ClearCaches() + { } + /// public override bool IsConverter(IPublishedPropertyType propertyType) => propertyType.EditorAlias.InvariantEquals(Constants.PropertyEditors.Aliases.Tags); + /// public override Type GetPropertyValueType(IPublishedPropertyType propertyType) => typeof(IEnumerable); + /// public override PropertyCacheLevel GetPropertyCacheLevel(IPublishedPropertyType propertyType) => PropertyCacheLevel.Element; + /// public override object? ConvertSourceToIntermediate(IPublishedElement owner, IPublishedPropertyType propertyType, object? source, bool preview) { - if (source == null) + string? sourceString = source?.ToString(); + if (string.IsNullOrEmpty(sourceString)) { return Array.Empty(); } - // if Json storage type deserialize and return as string array - if (JsonStorageType(propertyType.DataType.Id)) - { - var array = source.ToString() is not null - ? _jsonSerializer.Deserialize(source.ToString()!) - : null; - return array ?? Array.Empty(); - } - - // Otherwise assume CSV storage type and return as string array - return source.ToString()?.Split(Constants.CharArrays.Comma, StringSplitOptions.RemoveEmptyEntries); + return IsJson(propertyType) + ? _jsonSerializer.Deserialize(sourceString) ?? Array.Empty() + : sourceString.Split(Constants.CharArrays.Comma, StringSplitOptions.RemoveEmptyEntries); } - public override object? ConvertIntermediateToObject(IPublishedElement owner, IPublishedPropertyType propertyType, PropertyCacheLevel cacheLevel, object? source, bool preview) => (string[]?)source; - - /// - /// Discovers if the tags data type is storing its data in a Json format - /// - /// - /// The data type id. - /// - /// - /// The . - /// - private bool JsonStorageType(int dataTypeId) => - - // GetDataType(id) is cached at repository level; still, there is some - // deep-cloning involved (expensive) - better cache here + trigger - // refresh in DataTypeCacheRefresher - Storages.GetOrAdd(dataTypeId, id => - { - TagConfiguration? configuration = _dataTypeService.GetDataType(id)?.ConfigurationAs(); - return configuration?.StorageType == TagsStorageType.Json; - }); + private static bool IsJson(IPublishedPropertyType propertyType) + => propertyType.DataType.ConfigurationAs()?.StorageType == TagsStorageType.Json; } From f2b0c0e8eb9c43d3ad7b0c6dc443c21604685504 Mon Sep 17 00:00:00 2001 From: Nikolaj Date: Thu, 24 Aug 2023 10:44:01 +0200 Subject: [PATCH 12/15] Bump version --- version.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version.json b/version.json index 6761d6acc5..840819c76b 100644 --- a/version.json +++ b/version.json @@ -1,6 +1,6 @@ { "$schema": "https://raw.githubusercontent.com/dotnet/Nerdbank.GitVersioning/master/src/NerdBank.GitVersioning/version.schema.json", - "version": "11.5.0-rc", + "version": "11.6.0-rc", "assemblyVersion": { "precision": "build" }, From 869b480dae03345f110a26fc0a5fa08e7f5b2487 Mon Sep 17 00:00:00 2001 From: Nikolaj Date: Thu, 24 Aug 2023 10:49:50 +0200 Subject: [PATCH 13/15] Bump version --- version.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version.json b/version.json index f4c3bd0a8a..3c0aeb9872 100644 --- a/version.json +++ b/version.json @@ -1,6 +1,6 @@ { "$schema": "https://raw.githubusercontent.com/dotnet/Nerdbank.GitVersioning/master/src/NerdBank.GitVersioning/version.schema.json", - "version": "10.7.0-rc", + "version": "10.8.0-rc", "assemblyVersion": { "precision": "build" }, From 1198c76d67e3aa9ef5cebeb54d66708186125165 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 25 Aug 2023 13:07:42 +0200 Subject: [PATCH 14/15] Remove parsing of short into integer (#14721) --- .../Persistence/Factories/UserGroupFactory.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Infrastructure/Persistence/Factories/UserGroupFactory.cs b/src/Umbraco.Infrastructure/Persistence/Factories/UserGroupFactory.cs index 3c4546da04..44d9cc6790 100644 --- a/src/Umbraco.Infrastructure/Persistence/Factories/UserGroupFactory.cs +++ b/src/Umbraco.Infrastructure/Persistence/Factories/UserGroupFactory.cs @@ -78,7 +78,7 @@ internal static class UserGroupFactory if (entity.HasIdentity) { - dto.Id = short.Parse(entity.Id.ToString()); + dto.Id = entity.Id; } return dto; From 3a53f51ef389202b07d76b0eafe4a6174fddabf9 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 25 Aug 2023 13:07:42 +0200 Subject: [PATCH 15/15] Remove parsing of short into integer (#14721) --- .../Persistence/Factories/UserGroupFactory.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Infrastructure/Persistence/Factories/UserGroupFactory.cs b/src/Umbraco.Infrastructure/Persistence/Factories/UserGroupFactory.cs index 3c4546da04..44d9cc6790 100644 --- a/src/Umbraco.Infrastructure/Persistence/Factories/UserGroupFactory.cs +++ b/src/Umbraco.Infrastructure/Persistence/Factories/UserGroupFactory.cs @@ -78,7 +78,7 @@ internal static class UserGroupFactory if (entity.HasIdentity) { - dto.Id = short.Parse(entity.Id.ToString()); + dto.Id = entity.Id; } return dto;