Skip to content

Only apply validation on content update to variant cultures where the editor has permission for the culture #18778

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
using Asp.Versioning;
using Asp.Versioning;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.DependencyInjection;
using Umbraco.Cms.Api.Management.Factories;
using Umbraco.Cms.Api.Management.ViewModels.Document;
using Umbraco.Cms.Core;
using Umbraco.Cms.Core.DependencyInjection;
using Umbraco.Cms.Core.Models.ContentEditing;
using Umbraco.Cms.Core.Security;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Services.OperationStatus;

Expand All @@ -16,15 +19,32 @@ public class ValidateCreateDocumentController : CreateDocumentControllerBase
{
private readonly IDocumentEditingPresentationFactory _documentEditingPresentationFactory;
private readonly IContentEditingService _contentEditingService;
private readonly IBackOfficeSecurityAccessor _backOfficeSecurityAccessor;

[Obsolete("Please use the constructor taking all parameters. Scheduled for removal in Umbraco 17.")]
public ValidateCreateDocumentController(
IAuthorizationService authorizationService,
IDocumentEditingPresentationFactory documentEditingPresentationFactory,
IContentEditingService contentEditingService)
: this(
authorizationService,
documentEditingPresentationFactory,
contentEditingService,
StaticServiceProvider.Instance.GetRequiredService<IBackOfficeSecurityAccessor>())
{
}

[ActivatorUtilitiesConstructor]
public ValidateCreateDocumentController(
IAuthorizationService authorizationService,
IDocumentEditingPresentationFactory documentEditingPresentationFactory,
IContentEditingService contentEditingService,
IBackOfficeSecurityAccessor backOfficeSecurityAccessor)
: base(authorizationService)
{
_documentEditingPresentationFactory = documentEditingPresentationFactory;
_contentEditingService = contentEditingService;
_backOfficeSecurityAccessor = backOfficeSecurityAccessor;
}

[HttpPost("validate")]
Expand All @@ -36,7 +56,10 @@ public async Task<IActionResult> Validate(CancellationToken cancellationToken, C
=> await HandleRequest(requestModel, async () =>
{
ContentCreateModel model = _documentEditingPresentationFactory.MapCreateModel(requestModel);
Attempt<ContentValidationResult, ContentEditingOperationStatus> result = await _contentEditingService.ValidateCreateAsync(model);
Attempt<ContentValidationResult, ContentEditingOperationStatus> result =
await _contentEditingService.ValidateCreateAsync(
model,
CurrentUserKey(_backOfficeSecurityAccessor));

return result.Success
? Ok()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
using Asp.Versioning;
using Asp.Versioning;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.DependencyInjection;
using Umbraco.Cms.Api.Management.Factories;
using Umbraco.Cms.Api.Management.ViewModels.Document;
using Umbraco.Cms.Core;
using Umbraco.Cms.Core.DependencyInjection;
using Umbraco.Cms.Core.Models.ContentEditing;
using Umbraco.Cms.Core.Security;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Services.OperationStatus;

Expand All @@ -17,15 +20,32 @@ public class ValidateUpdateDocumentController : UpdateDocumentControllerBase
{
private readonly IContentEditingService _contentEditingService;
private readonly IDocumentEditingPresentationFactory _documentEditingPresentationFactory;
private readonly IBackOfficeSecurityAccessor _backOfficeSecurityAccessor;

[Obsolete("Please use the constructor taking all parameters. Scheduled for removal in Umbraco 17.")]
public ValidateUpdateDocumentController(
IAuthorizationService authorizationService,
IContentEditingService contentEditingService,
IDocumentEditingPresentationFactory documentEditingPresentationFactory)
: this(
authorizationService,
contentEditingService,
documentEditingPresentationFactory,
StaticServiceProvider.Instance.GetRequiredService<IBackOfficeSecurityAccessor>())
{
}

[ActivatorUtilitiesConstructor]
public ValidateUpdateDocumentController(
IAuthorizationService authorizationService,
IContentEditingService contentEditingService,
IDocumentEditingPresentationFactory documentEditingPresentationFactory,
IBackOfficeSecurityAccessor backOfficeSecurityAccessor)
: base(authorizationService)
{
_contentEditingService = contentEditingService;
_documentEditingPresentationFactory = documentEditingPresentationFactory;
_backOfficeSecurityAccessor = backOfficeSecurityAccessor;
}

[HttpPut("{id:guid}/validate")]
Expand Down Expand Up @@ -62,7 +82,11 @@ public async Task<IActionResult> ValidateV1_1(CancellationToken cancellationToke
=> await HandleRequest(id, requestModel, async () =>
{
ValidateContentUpdateModel model = _documentEditingPresentationFactory.MapValidateUpdateModel(requestModel);
Attempt<ContentValidationResult, ContentEditingOperationStatus> result = await _contentEditingService.ValidateUpdateAsync(id, model);
Attempt<ContentValidationResult, ContentEditingOperationStatus> result =
await _contentEditingService.ValidateUpdateAsync(
id,
model,
CurrentUserKey(_backOfficeSecurityAccessor));

return result.Success
? Ok()
Expand Down
63 changes: 49 additions & 14 deletions src/Umbraco.Core/Services/ContentEditingService.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Umbraco.Cms.Core.Configuration.Models;
using Umbraco.Cms.Core.Models;
Expand Down Expand Up @@ -64,7 +64,7 @@
return await Task.FromResult(content);
}

[Obsolete("Please use the validate update method that is not obsoleted. Will be removed in V16.")]
[Obsolete("Please use the validate update method that is not obsoleted. Scheduled for removal in V16.")]
public async Task<Attempt<ContentValidationResult, ContentEditingOperationStatus>> ValidateUpdateAsync(Guid key, ContentUpdateModel updateModel)
{
IContent? content = ContentService.GetById(key);
Expand All @@ -73,16 +73,50 @@
: Attempt.FailWithStatus(ContentEditingOperationStatus.NotFound, new ContentValidationResult());
}

[Obsolete("Please use the validate update method that is not obsoleted. Scheduled for removal in V17.")]
public async Task<Attempt<ContentValidationResult, ContentEditingOperationStatus>> ValidateUpdateAsync(Guid key, ValidateContentUpdateModel updateModel)
=> await ValidateUpdateAsync(key, updateModel, Guid.Empty);

public async Task<Attempt<ContentValidationResult, ContentEditingOperationStatus>> ValidateUpdateAsync(Guid key, ValidateContentUpdateModel updateModel, Guid userKey)
{
IContent? content = ContentService.GetById(key);
return content is not null
? await ValidateCulturesAndPropertiesAsync(updateModel, content.ContentType.Key, updateModel.Cultures)
? await ValidateCulturesAndPropertiesAsync(updateModel, content.ContentType.Key, await GetCulturesToValidate(updateModel.Cultures, userKey))
: Attempt.FailWithStatus(ContentEditingOperationStatus.NotFound, new ContentValidationResult());
}

[Obsolete("Please use the validate create method that is not obsoleted. Scheduled for removal in V17.")]
public async Task<Attempt<ContentValidationResult, ContentEditingOperationStatus>> ValidateCreateAsync(ContentCreateModel createModel)
=> await ValidateCulturesAndPropertiesAsync(createModel, createModel.ContentTypeKey, createModel.Variants.Select(variant => variant.Culture));
=> await ValidateCreateAsync(createModel, Guid.Empty);

public async Task<Attempt<ContentValidationResult, ContentEditingOperationStatus>> ValidateCreateAsync(ContentCreateModel createModel, Guid userKey)
=> await ValidateCulturesAndPropertiesAsync(createModel, createModel.ContentTypeKey, await GetCulturesToValidate(createModel.Variants.Select(variant => variant.Culture), userKey));

private async Task<IEnumerable<string?>?> GetCulturesToValidate(IEnumerable<string?>? cultures, Guid userKey)
{
// Cultures to validate can be provided by the calling code, but if the editor is restricted to only have
// access to certain languages, we don't want to validate by any they aren't allowed to edit.

// TODO: Remove this check once the obsolete overloads to ValidateCreateAsync and ValidateUpdateAsync that don't provide a user key are removed.
// We only have this to ensure backwards compatibility with the obsolete overloads.
if (userKey == Guid.Empty)
{
return cultures;
}

HashSet<string>? allowedCultures = await GetAllowedCulturesForEditingUser(userKey);

if (cultures == null)
{
// If no cultures are provided, we are asking to validate all cultures. But if the user doesn't have access to all, we
// should only validate the ones they do.
var allCultures = (await _languageService.GetAllAsync()).Select(x => x.IsoCode).ToList();
return allowedCultures.Count == allCultures.Count ? null : allowedCultures;
}

// If explicit cultures are provided, we should only validate the ones the user has access to.
return cultures.Where(x => !string.IsNullOrEmpty(x) && allowedCultures.Contains(x)).ToList();
}

public async Task<Attempt<ContentCreateResult, ContentEditingOperationStatus>> CreateAsync(ContentCreateModel createModel, Guid userKey)
{
Expand Down Expand Up @@ -127,16 +161,7 @@

IContent? existingContent = await GetAsync(contentWithPotentialUnallowedChanges.Key);

IUser? user = await _userService.GetAsync(userKey);

if (user is null)
{
return contentWithPotentialUnallowedChanges;
}

var allowedLanguageIds = user.CalculateAllowedLanguageIds(_localizationService)!;

var allowedCultures = (await _languageService.GetIsoCodesByIdsAsync(allowedLanguageIds)).ToHashSet();
HashSet<string>? allowedCultures = await GetAllowedCulturesForEditingUser(userKey);

Check notice on line 164 in src/Umbraco.Core/Services/ContentEditingService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v15/dev)

✅ Getting better: Complex Method

EnsureOnlyAllowedFieldsAreUpdated decreases in cyclomatic complexity from 17 to 16, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.

ILanguage? defaultLanguage = await _languageService.GetDefaultLanguageAsync();

Expand Down Expand Up @@ -211,6 +236,16 @@
return contentWithPotentialUnallowedChanges;
}

private async Task<HashSet<string>> GetAllowedCulturesForEditingUser(Guid userKey)
{
IUser? user = await _userService.GetAsync(userKey)
?? throw new InvalidOperationException($"Could not find user by key {userKey} when editing or validating content.");

var allowedLanguageIds = user.CalculateAllowedLanguageIds(_localizationService)!;

return (await _languageService.GetIsoCodesByIdsAsync(allowedLanguageIds)).ToHashSet();
}

public async Task<Attempt<ContentUpdateResult, ContentEditingOperationStatus>> UpdateAsync(Guid key, ContentUpdateModel updateModel, Guid userKey)
{
IContent? content = ContentService.GetById(key);
Expand Down
16 changes: 14 additions & 2 deletions src/Umbraco.Core/Services/IContentEditingService.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Models.ContentEditing;
using Umbraco.Cms.Core.Services.OperationStatus;

Expand All @@ -8,13 +8,25 @@ public interface IContentEditingService
{
Task<IContent?> GetAsync(Guid key);

[Obsolete("Please use the validate create method that is not obsoleted. Scheduled for removal in Umbraco 17.")]
Task<Attempt<ContentValidationResult, ContentEditingOperationStatus>> ValidateCreateAsync(ContentCreateModel createModel);

[Obsolete("Please use the validate update method that is not obsoleted. Will be removed in V16.")]
Task<Attempt<ContentValidationResult, ContentEditingOperationStatus>> ValidateCreateAsync(ContentCreateModel createModel, Guid userKey)
#pragma warning disable CS0618 // Type or member is obsolete
=> ValidateCreateAsync(createModel);
#pragma warning restore CS0618 // Type or member is obsolete

[Obsolete("Please use the validate update method that is not obsoleted. Scheduled for removal in Umbraco 16.")]
Task<Attempt<ContentValidationResult, ContentEditingOperationStatus>> ValidateUpdateAsync(Guid key, ContentUpdateModel updateModel);

[Obsolete("Please use the validate update method that is not obsoleted. Scheduled for removal in Umbraco 17.")]
Task<Attempt<ContentValidationResult, ContentEditingOperationStatus>> ValidateUpdateAsync(Guid key, ValidateContentUpdateModel updateModel);

Task<Attempt<ContentValidationResult, ContentEditingOperationStatus>> ValidateUpdateAsync(Guid key, ValidateContentUpdateModel updateModel, Guid userKey)
#pragma warning disable CS0618 // Type or member is obsolete
=> ValidateUpdateAsync(key, updateModel);
#pragma warning restore CS0618 // Type or member is obsolete

Task<Attempt<ContentCreateResult, ContentEditingOperationStatus>> CreateAsync(ContentCreateModel createModel, Guid userKey);

Task<Attempt<ContentUpdateResult, ContentEditingOperationStatus>> UpdateAsync(Guid key, ContentUpdateModel updateModel, Guid userKey);
Expand Down
45 changes: 41 additions & 4 deletions tests/Umbraco.Tests.Common/Builders/UserGroupBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using Moq;
using Umbraco.Cms.Core.Models.Membership;
using Umbraco.Cms.Core.Models.Membership.Permissions;
using Umbraco.Cms.Core.Strings;
using Umbraco.Cms.Tests.Common.Builders.Extensions;
using Umbraco.Cms.Tests.Common.Builders.Interfaces;
Expand All @@ -27,6 +28,8 @@
{
private string _alias;
private IEnumerable<string> _allowedSections = Enumerable.Empty<string>();
private IEnumerable<int> _allowedLanguages = Enumerable.Empty<int>();
private IEnumerable<IGranularPermission> _granularPermissions = Enumerable.Empty<IGranularPermission>();
private string _icon;
private int? _id;
private Guid? _key;
Expand Down Expand Up @@ -95,13 +98,24 @@
return this;
}


public UserGroupBuilder<TParent> WithAllowedSections(IList<string> allowedSections)
{
_allowedSections = allowedSections;
return this;
}

public UserGroupBuilder<TParent> WithAllowedLanguages(IList<int> allowedLanguages)
{
_allowedLanguages = allowedLanguages;
return this;
}

public UserGroupBuilder<TParent> WithGranularPermissions(IList<IGranularPermission> granularPermissions)
{
_granularPermissions = granularPermissions;
return this;
}

public UserGroupBuilder<TParent> WithStartContentId(int startContentId)
{
_startContentId = startContentId;
Expand Down Expand Up @@ -144,17 +158,40 @@
Id = id,
Key = key,
StartContentId = startContentId,
StartMediaId = startMediaId
StartMediaId = startMediaId,
Permissions = _permissions
};

userGroup.Permissions = _permissions;
BuildAllowedSections(userGroup);
BuildAllowedLanguages(userGroup);
BuildGranularPermissions(userGroup);

return userGroup;
}

Check notice on line 170 in tests/Umbraco.Tests.Common/Builders/UserGroupBuilder.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v15/dev)

✅ Getting better: Complex Method

Build decreases in cyclomatic complexity from 10 to 9, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.


private void BuildAllowedSections(UserGroup userGroup)
{
foreach (var section in _allowedSections)
{
userGroup.AddAllowedSection(section);
}
}

return userGroup;
private void BuildAllowedLanguages(UserGroup userGroup)
{
foreach (var language in _allowedLanguages)
{
userGroup.AddAllowedLanguage(language);
}
}

private void BuildGranularPermissions(UserGroup userGroup)
{
foreach (var permission in _granularPermissions)
{
userGroup.GranularPermissions.Add(permission);
}
}

public static UserGroup CreateUserGroup(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
using Bogus.DataSets;
using NUnit.Framework;
using Umbraco.Cms.Core;
using Umbraco.Cms.Core.Models;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using NUnit.Framework;
using NUnit.Framework;
using Umbraco.Cms.Core;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Models.ContentEditing;
Expand Down Expand Up @@ -346,6 +346,7 @@ public async Task Cannot_Update_Invariant_Readonly_Property_Value()
InvariantName = "Updated Name",
InvariantProperties = new[]
{
new PropertyValueModel { Alias = "title", Value = "The initial title" },
new PropertyValueModel { Alias = "label", Value = "The updated label value" }
}
};
Expand Down Expand Up @@ -390,6 +391,7 @@ public async Task Cannot_Update_Variant_Readonly_Property_Value()
Name = "Updated English Name",
Properties = new []
{
new PropertyValueModel { Alias = "variantTitle", Value = "The initial English title" },
new PropertyValueModel { Alias = "variantLabel", Value = "The updated English label value" }
}
},
Expand All @@ -399,6 +401,7 @@ public async Task Cannot_Update_Variant_Readonly_Property_Value()
Name = "Updated Danish Name",
Properties = new []
{
new PropertyValueModel { Alias = "variantTitle", Value = "The initial Danish title" },
new PropertyValueModel { Alias = "variantLabel", Value = "The updated Danish label value" }
}
}
Expand Down
Loading