From 4443f8b88a02fe61bd14faa57385872eebcf2b59 Mon Sep 17 00:00:00 2001 From: Kenn Jacobsen Date: Mon, 26 Feb 2024 15:52:02 +0100 Subject: [PATCH] Validate collisions in domain assignments (#15759) * Validate collisions in domain assignments * Update OpenApi.json --- .../Document/UpdateDomainsController.cs | 18 ++++-- .../DocumentBuilderExtensions.cs | 1 + .../Factories/DomainPresentationFactory.cs | 32 ++++++++++ .../Factories/IDomainPresentationFactory.cs | 9 +++ src/Umbraco.Cms.Api.Management/OpenApi.json | 61 ++++++++++++++++++- .../Document/DomainAssignmentModel.cs | 8 +++ .../ContentEditing/DomainUpdateResult.cs | 8 +++ src/Umbraco.Core/Services/DomainService.cs | 39 +++++++----- src/Umbraco.Core/Services/IDomainService.cs | 2 +- .../OperationStatus/DomainOperationStatus.cs | 3 +- .../UrlAndDomains/DomainAndUrlsTests.cs | 30 +++++---- 11 files changed, 178 insertions(+), 33 deletions(-) create mode 100644 src/Umbraco.Cms.Api.Management/Factories/DomainPresentationFactory.cs create mode 100644 src/Umbraco.Cms.Api.Management/Factories/IDomainPresentationFactory.cs create mode 100644 src/Umbraco.Cms.Api.Management/ViewModels/Document/DomainAssignmentModel.cs create mode 100644 src/Umbraco.Core/Models/ContentEditing/DomainUpdateResult.cs diff --git a/src/Umbraco.Cms.Api.Management/Controllers/Document/UpdateDomainsController.cs b/src/Umbraco.Cms.Api.Management/Controllers/Document/UpdateDomainsController.cs index e4e9d6e749..73de2cdf40 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/Document/UpdateDomainsController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/Document/UpdateDomainsController.cs @@ -1,14 +1,14 @@ using Asp.Versioning; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; -using Umbraco.Cms.Api.Common.Builders; +using Umbraco.Cms.Api.Management.Factories; using Umbraco.Cms.Api.Management.ViewModels.Document; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Mapping; -using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.ContentEditing; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Services.OperationStatus; +using Umbraco.Extensions; namespace Umbraco.Cms.Api.Management.Controllers.Document; @@ -17,20 +17,25 @@ public class UpdateDomainsController : DocumentControllerBase { private readonly IDomainService _domainService; private readonly IUmbracoMapper _umbracoMapper; + private readonly IDomainPresentationFactory _domainPresentationFactory; - public UpdateDomainsController(IDomainService domainService, IUmbracoMapper umbracoMapper) + public UpdateDomainsController(IDomainService domainService, IUmbracoMapper umbracoMapper, IDomainPresentationFactory domainPresentationFactory) { _domainService = domainService; _umbracoMapper = umbracoMapper; + _domainPresentationFactory = domainPresentationFactory; } [MapToApiVersion("1.0")] [HttpPut("{id:guid}/domains")] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status409Conflict)] public async Task Update(Guid id, UpdateDomainsRequestModel updateModel) { DomainsUpdateModel domainsUpdateModel = _umbracoMapper.Map(updateModel)!; - Attempt, DomainOperationStatus> result = await _domainService.UpdateDomainsAsync(id, domainsUpdateModel); + Attempt result = await _domainService.UpdateDomainsAsync(id, domainsUpdateModel); return result.Success ? Ok() @@ -52,6 +57,11 @@ public class UpdateDomainsController : DocumentControllerBase .WithTitle("Duplicate domain name detected") .WithDetail("One or more of the specified domain names were duplicates, possibly of assignments to other content items.") .Build()), + DomainOperationStatus.ConflictingDomainName => Conflict(problemDetailsBuilder + .WithTitle("Conflicting domain name detected") + .WithDetail("One or more of the specified domain names were conflicting with domain assignments to other content items.") + .WithExtension("conflictingDomainNames", _domainPresentationFactory.CreateDomainAssignmentModels(result.Result.ConflictingDomains.EmptyNull())) + .Build()), _ => StatusCode(StatusCodes.Status500InternalServerError, problemDetailsBuilder .WithTitle("Unknown domain update operation status.") .Build()), diff --git a/src/Umbraco.Cms.Api.Management/DependencyInjection/DocumentBuilderExtensions.cs b/src/Umbraco.Cms.Api.Management/DependencyInjection/DocumentBuilderExtensions.cs index 497b2841a2..e2721e5bf2 100644 --- a/src/Umbraco.Cms.Api.Management/DependencyInjection/DocumentBuilderExtensions.cs +++ b/src/Umbraco.Cms.Api.Management/DependencyInjection/DocumentBuilderExtensions.cs @@ -15,6 +15,7 @@ internal static class DocumentBuilderExtensions builder.Services.AddTransient(); builder.Services.AddTransient(); builder.Services.AddTransient(); + builder.Services.AddTransient(); builder.WithCollectionBuilder() .Add() diff --git a/src/Umbraco.Cms.Api.Management/Factories/DomainPresentationFactory.cs b/src/Umbraco.Cms.Api.Management/Factories/DomainPresentationFactory.cs new file mode 100644 index 0000000000..f2ea0df030 --- /dev/null +++ b/src/Umbraco.Cms.Api.Management/Factories/DomainPresentationFactory.cs @@ -0,0 +1,32 @@ +using Umbraco.Cms.Api.Management.ViewModels; +using Umbraco.Cms.Api.Management.ViewModels.Document; +using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Services; +using Umbraco.Extensions; + +namespace Umbraco.Cms.Api.Management.Factories; + +internal sealed class DomainPresentationFactory : IDomainPresentationFactory +{ + private readonly IEntityService _entityService; + + public DomainPresentationFactory(IEntityService entityService) + => _entityService = entityService; + + public IEnumerable CreateDomainAssignmentModels(IEnumerable domains) + => domains + .Where(domain => domain.RootContentId.HasValue) + .Select(domain => + { + Attempt keyResult = _entityService.GetKey(domain.RootContentId!.Value, UmbracoObjectTypes.Document); + return keyResult.Success + ? new DomainAssignmentModel + { + DomainName = domain.DomainName, Content = new ReferenceByIdModel(keyResult.Result) + } + : null; + }) + .WhereNotNull() + .ToArray(); +} diff --git a/src/Umbraco.Cms.Api.Management/Factories/IDomainPresentationFactory.cs b/src/Umbraco.Cms.Api.Management/Factories/IDomainPresentationFactory.cs new file mode 100644 index 0000000000..d07424fce3 --- /dev/null +++ b/src/Umbraco.Cms.Api.Management/Factories/IDomainPresentationFactory.cs @@ -0,0 +1,9 @@ +using Umbraco.Cms.Api.Management.ViewModels.Document; +using Umbraco.Cms.Core.Models; + +namespace Umbraco.Cms.Api.Management.Factories; + +public interface IDomainPresentationFactory +{ + IEnumerable CreateDomainAssignmentModels(IEnumerable domains); +} diff --git a/src/Umbraco.Cms.Api.Management/OpenApi.json b/src/Umbraco.Cms.Api.Management/OpenApi.json index 5e0f762867..253ade9adf 100644 --- a/src/Umbraco.Cms.Api.Management/OpenApi.json +++ b/src/Umbraco.Cms.Api.Management/OpenApi.json @@ -5024,8 +5024,65 @@ } }, "responses": { - "200": { - "description": "Success" + "400": { + "description": "Bad Request", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, + "404": { + "description": "Not Found", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, + "409": { + "description": "Conflict", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } }, "401": { "description": "The resource is protected and requires an authentication token" diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/Document/DomainAssignmentModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/Document/DomainAssignmentModel.cs new file mode 100644 index 0000000000..cb2dc23457 --- /dev/null +++ b/src/Umbraco.Cms.Api.Management/ViewModels/Document/DomainAssignmentModel.cs @@ -0,0 +1,8 @@ +namespace Umbraco.Cms.Api.Management.ViewModels.Document; + +public sealed class DomainAssignmentModel +{ + public required string DomainName { get; set; } + + public required ReferenceByIdModel Content { get; set; } +} diff --git a/src/Umbraco.Core/Models/ContentEditing/DomainUpdateResult.cs b/src/Umbraco.Core/Models/ContentEditing/DomainUpdateResult.cs new file mode 100644 index 0000000000..e00cabbe63 --- /dev/null +++ b/src/Umbraco.Core/Models/ContentEditing/DomainUpdateResult.cs @@ -0,0 +1,8 @@ +namespace Umbraco.Cms.Core.Models.ContentEditing; + +public sealed class DomainUpdateResult +{ + public IEnumerable Domains { get; init; } = Enumerable.Empty(); + + public IEnumerable? ConflictingDomains { get; init; } +} diff --git a/src/Umbraco.Core/Services/DomainService.cs b/src/Umbraco.Core/Services/DomainService.cs index fa09bfc987..b4e103f1a4 100644 --- a/src/Umbraco.Core/Services/DomainService.cs +++ b/src/Umbraco.Core/Services/DomainService.cs @@ -178,12 +178,12 @@ public class DomainService : RepositoryService, IDomainService } /// - public async Task, DomainOperationStatus>> UpdateDomainsAsync(Guid contentKey, DomainsUpdateModel updateModel) + public async Task> UpdateDomainsAsync(Guid contentKey, DomainsUpdateModel updateModel) { IContent? content = _contentService.GetById(contentKey); if (content == null) { - return Attempt.FailWithStatus(DomainOperationStatus.ContentNotFound, Enumerable.Empty()); + return Attempt.FailWithStatus(DomainOperationStatus.ContentNotFound, new DomainUpdateResult()); } using ICoreScope scope = ScopeProvider.CreateCoreScope(); @@ -194,7 +194,7 @@ public class DomainService : RepositoryService, IDomainService // validate language ISO codes if (HasInvalidIsoCode(updateModel, languageIdByIsoCode.Keys)) { - return Attempt.FailWithStatus(DomainOperationStatus.LanguageNotFound, Enumerable.Empty()); + return Attempt.FailWithStatus(DomainOperationStatus.LanguageNotFound, new DomainUpdateResult()); } // ensure all domain names in the update model are lowercased @@ -206,16 +206,19 @@ public class DomainService : RepositoryService, IDomainService // make sure we're not attempting to assign duplicate domains if (updateModel.Domains.GroupBy(domain => domain.DomainName).Any(group => group.Count() > 1)) { - return Attempt.FailWithStatus(DomainOperationStatus.DuplicateDomainName, Enumerable.Empty()); + return Attempt.FailWithStatus(DomainOperationStatus.DuplicateDomainName, new DomainUpdateResult()); } // grab all current domain assignments IDomain[] allDomains = (await GetAllAsync(true)).ToArray(); // validate the domain names in the update model - if (HasDomainNameConflicts(content.Id, updateModel, allDomains)) + IDomain[] conflicts = GetDomainNameConflicts(content.Id, updateModel, allDomains); + if (conflicts.Any()) { - return Attempt.FailWithStatus(DomainOperationStatus.DuplicateDomainName, Enumerable.Empty()); + return Attempt.FailWithStatus( + DomainOperationStatus.ConflictingDomainName, + new DomainUpdateResult { ConflictingDomains = conflicts }); } // find the domains currently assigned to the content item @@ -234,7 +237,7 @@ public class DomainService : RepositoryService, IDomainService scope.Complete(); // this is the only error scenario in DeleteAll - return Attempt.FailWithStatus(DomainOperationStatus.CancelledByNotification, newAssignedDomains.AsEnumerable()); + return Attempt.FailWithStatus(DomainOperationStatus.CancelledByNotification, new DomainUpdateResult()); } // update all domain assignments (also current ones, in case sort order or ISO code has changed) @@ -242,8 +245,13 @@ public class DomainService : RepositoryService, IDomainService scope.Complete(); return result - ? Attempt.SucceedWithStatus(DomainOperationStatus.Success, newAssignedDomains.AsEnumerable()) - : Attempt.FailWithStatus(DomainOperationStatus.CancelledByNotification, newAssignedDomains.AsEnumerable()); + ? Attempt.SucceedWithStatus( + DomainOperationStatus.Success, + new DomainUpdateResult + { + Domains = newAssignedDomains.AsEnumerable() + }) + : Attempt.FailWithStatus(DomainOperationStatus.CancelledByNotification, new DomainUpdateResult()); } /// @@ -257,16 +265,19 @@ public class DomainService : RepositoryService, IDomainService .Any(); /// - /// Tests if any of the domain names in the update model are assigned to other content items + /// Returns any current domain assignments in conflict with the updateModel domain names /// - private bool HasDomainNameConflicts(int contentId, DomainsUpdateModel updateModel, IEnumerable allDomains) + private IDomain[] GetDomainNameConflicts(int contentId, DomainsUpdateModel updateModel, IEnumerable allDomains) { - var domainNamesAssignedToOtherContent = allDomains + IDomain[] domainsAssignedToOtherContent = allDomains .Where(domain => domain.IsWildcard == false && domain.RootContentId != contentId) - .Select(domain => domain.DomainName.ToLowerInvariant()) .ToArray(); - return updateModel.Domains.Any(domainModel => domainNamesAssignedToOtherContent.InvariantContains(domainModel.DomainName)); + var updateModelDomainNames = updateModel.Domains.Select(domain => domain.DomainName).ToArray(); + + return domainsAssignedToOtherContent + .Where(domain => updateModelDomainNames.InvariantContains(domain.DomainName)) + .ToArray(); } /// diff --git a/src/Umbraco.Core/Services/IDomainService.cs b/src/Umbraco.Core/Services/IDomainService.cs index f5a5370a6c..2d03a8a966 100644 --- a/src/Umbraco.Core/Services/IDomainService.cs +++ b/src/Umbraco.Core/Services/IDomainService.cs @@ -47,5 +47,5 @@ public interface IDomainService : IService /// /// The key of the content item. /// The domain assignments to apply. - Task, DomainOperationStatus>> UpdateDomainsAsync(Guid contentKey, DomainsUpdateModel updateModel); + Task> UpdateDomainsAsync(Guid contentKey, DomainsUpdateModel updateModel); } diff --git a/src/Umbraco.Core/Services/OperationStatus/DomainOperationStatus.cs b/src/Umbraco.Core/Services/OperationStatus/DomainOperationStatus.cs index c54f356c04..a752684b2e 100644 --- a/src/Umbraco.Core/Services/OperationStatus/DomainOperationStatus.cs +++ b/src/Umbraco.Core/Services/OperationStatus/DomainOperationStatus.cs @@ -6,5 +6,6 @@ public enum DomainOperationStatus CancelledByNotification, ContentNotFound, LanguageNotFound, - DuplicateDomainName + DuplicateDomainName, + ConflictingDomainName } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Web.BackOffice/UrlAndDomains/DomainAndUrlsTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Web.BackOffice/UrlAndDomains/DomainAndUrlsTests.cs index 82a29d0b0a..abc4e3894c 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Web.BackOffice/UrlAndDomains/DomainAndUrlsTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Web.BackOffice/UrlAndDomains/DomainAndUrlsTests.cs @@ -101,7 +101,7 @@ public class DomainAndUrlsTests : UmbracoIntegrationTest } } - VerifyDomains(result.Result.ToArray()); + VerifyDomains(result.Result.Domains.ToArray()); // re-get and verify again var domains = await domainService.GetAssignedDomainsAsync(Root.Key, true); @@ -135,7 +135,7 @@ public class DomainAndUrlsTests : UmbracoIntegrationTest } } - VerifyDomains(result.Result.ToArray()); + VerifyDomains(result.Result.Domains.ToArray()); // re-get and verify again var domains = await domainService.GetAssignedDomainsAsync(Root.Key, true); @@ -157,14 +157,14 @@ public class DomainAndUrlsTests : UmbracoIntegrationTest var result = await domainService.UpdateDomainsAsync(Root.Key, updateModel); Assert.IsTrue(result.Success); Assert.AreEqual(DomainOperationStatus.Success, result.Status); - Assert.AreEqual(3, result.Result.Count()); + Assert.AreEqual(3, result.Result.Domains.Count()); updateModel.Domains = Enumerable.Empty(); result = await domainService.UpdateDomainsAsync(Root.Key, updateModel); Assert.IsTrue(result.Success); Assert.AreEqual(DomainOperationStatus.Success, result.Status); - Assert.AreEqual(0, result.Result.Count()); + Assert.AreEqual(0, result.Result.Domains.Count()); // re-get and verify again var domains = await domainService.GetAssignedDomainsAsync(Root.Key, true); @@ -186,16 +186,16 @@ public class DomainAndUrlsTests : UmbracoIntegrationTest var result = await domainService.UpdateDomainsAsync(Root.Key, updateModel); Assert.IsTrue(result.Success); Assert.AreEqual(DomainOperationStatus.Success, result.Status); - Assert.AreEqual(3, result.Result.Count()); + Assert.AreEqual(3, result.Result.Domains.Count()); updateModel.Domains = new[] { updateModel.Domains.First(), updateModel.Domains.Last() }; result = await domainService.UpdateDomainsAsync(Root.Key, updateModel); Assert.IsTrue(result.Success); Assert.AreEqual(DomainOperationStatus.Success, result.Status); - Assert.AreEqual(2, result.Result.Count()); - Assert.AreEqual(Cultures.First(), result.Result.First().LanguageIsoCode); - Assert.AreEqual(Cultures.Last(), result.Result.Last().LanguageIsoCode); + Assert.AreEqual(2, result.Result.Domains.Count()); + Assert.AreEqual(Cultures.First(), result.Result.Domains.First().LanguageIsoCode); + Assert.AreEqual(Cultures.Last(), result.Result.Domains.Last().LanguageIsoCode); } [Test] @@ -275,10 +275,10 @@ public class DomainAndUrlsTests : UmbracoIntegrationTest var result = await domainService.UpdateDomainsAsync(Root.Key, updateModel); Assert.IsTrue(result.Success); - Assert.AreEqual(1, result.Result.Count()); + Assert.AreEqual(1, result.Result.Domains.Count()); // default culture is represented as a wildcard domain - var domain = result.Result.First(); + var domain = result.Result.Domains.First(); Assert.IsTrue(domain.IsWildcard); Assert.AreEqual(culture, domain.LanguageIsoCode); Assert.AreEqual("*" + Root.Id, domain.DomainName); @@ -352,7 +352,15 @@ public class DomainAndUrlsTests : UmbracoIntegrationTest result = await domainService.UpdateDomainsAsync(copy.Key, updateModel); Assert.IsFalse(result.Success); - Assert.AreEqual(DomainOperationStatus.DuplicateDomainName, result.Status); + Assert.AreEqual(DomainOperationStatus.ConflictingDomainName, result.Status); + + Assert.IsNotNull(result.Result.ConflictingDomains); + Assert.IsNotEmpty(result.Result.ConflictingDomains); + Assert.AreEqual(updateModel.Domains.Count(), result.Result.ConflictingDomains.Count()); + foreach (var culture in Cultures) + { + Assert.IsNotNull(result.Result.ConflictingDomains.SingleOrDefault(c => c.RootContentId == Root.Id && c.DomainName == GetDomainUrlFromCultureCode(culture))); + } } private static string GetDomainUrlFromCultureCode(string culture) =>