From 92af8ac881638e8a29f7dfc64ef0799a950a3842 Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska <21998037+elit0451@users.noreply.github.com> Date: Wed, 1 Nov 2023 11:30:32 +0100 Subject: [PATCH] Check content permissions before performing action (#15043) * Setting actionContext.Result when authz wasn't successful * Taking into account permissions when it is a new node * Cleanup * Passing nodeId as path when new item --- .../Security/ContentPermissions.cs | 59 ++++++++----------- .../Controllers/ContentController.cs | 23 ++++---- .../Controllers/ContentControllerBase.cs | 1 - .../Filters/ContentSaveValidationAttribute.cs | 1 + 4 files changed, 36 insertions(+), 48 deletions(-) diff --git a/src/Umbraco.Core/Security/ContentPermissions.cs b/src/Umbraco.Core/Security/ContentPermissions.cs index db27d100c6..0ad85971cf 100644 --- a/src/Umbraco.Core/Security/ContentPermissions.cs +++ b/src/Umbraco.Core/Security/ContentPermissions.cs @@ -167,12 +167,7 @@ public class ContentPermissions throw new ArgumentNullException(nameof(user)); } - if (permissionsToCheck == null) - { - permissionsToCheck = Array.Empty(); - } - - bool? hasPathAccess = null; + bool hasPathAccess; entity = null; if (nodeId == Constants.System.Root) @@ -183,19 +178,17 @@ public class ContentPermissions { hasPathAccess = user.HasContentBinAccess(_entityService, _appCaches); } - - if (hasPathAccess.HasValue) + else { - return hasPathAccess.Value ? ContentAccess.Granted : ContentAccess.Denied; - } + entity = _entityService.Get(nodeId, UmbracoObjectTypes.Document); - entity = _entityService.Get(nodeId, UmbracoObjectTypes.Document); - if (entity == null) - { - return ContentAccess.NotFound; - } + if (entity == null) + { + return ContentAccess.NotFound; + } - hasPathAccess = user.HasContentPathAccess(entity, _entityService, _appCaches); + hasPathAccess = user.HasContentPathAccess(entity, _entityService, _appCaches); + } if (hasPathAccess == false) { @@ -208,7 +201,8 @@ public class ContentPermissions } // get the implicit/inherited permissions for the user for this path - return CheckPermissionsPath(entity.Path, user, permissionsToCheck) + // if there is no entity for this id, than just use the id as the path (i.e. -1 or -20) + return CheckPermissionsPath(entity?.Path ?? nodeId.ToString(), user, permissionsToCheck) ? ContentAccess.Granted : ContentAccess.Denied; } @@ -235,12 +229,7 @@ public class ContentPermissions throw new ArgumentNullException(nameof(user)); } - if (permissionsToCheck == null) - { - permissionsToCheck = Array.Empty(); - } - - bool? hasPathAccess = null; + bool hasPathAccess; contentItem = null; if (nodeId == Constants.System.Root) @@ -251,19 +240,17 @@ public class ContentPermissions { hasPathAccess = user.HasContentBinAccess(_entityService, _appCaches); } - - if (hasPathAccess.HasValue) + else { - return hasPathAccess.Value ? ContentAccess.Granted : ContentAccess.Denied; - } + contentItem = _contentService.GetById(nodeId); - contentItem = _contentService.GetById(nodeId); - if (contentItem == null) - { - return ContentAccess.NotFound; - } + if (contentItem == null) + { + return ContentAccess.NotFound; + } - hasPathAccess = user.HasPathAccess(contentItem, _entityService, _appCaches); + hasPathAccess = user.HasPathAccess(contentItem, _entityService, _appCaches); + } if (hasPathAccess == false) { @@ -276,7 +263,8 @@ public class ContentPermissions } // get the implicit/inherited permissions for the user for this path - return CheckPermissionsPath(contentItem.Path, user, permissionsToCheck) + // if there is no content item for this id, than just use the id as the path (i.e. -1 or -20) + return CheckPermissionsPath(contentItem?.Path ?? nodeId.ToString(), user, permissionsToCheck) ? ContentAccess.Granted : ContentAccess.Denied; } @@ -288,8 +276,7 @@ public class ContentPermissions permissionsToCheck = Array.Empty(); } - // get the implicit/inherited permissions for the user for this path, - // if there is no content item for this id, than just use the id as the path (i.e. -1 or -20) + // get the implicit/inherited permissions for the user for this path EntityPermissionSet permission = _userService.GetPermissionsForPath(user, path); var allowed = true; diff --git a/src/Umbraco.Web.BackOffice/Controllers/ContentController.cs b/src/Umbraco.Web.BackOffice/Controllers/ContentController.cs index 944068114a..9ec67319d9 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/ContentController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/ContentController.cs @@ -759,6 +759,7 @@ public class ContentController : ContentControllerBase return pagedResult; } + /// /// Creates a blueprint from a content item /// @@ -1057,7 +1058,7 @@ public class ContentController : ContentControllerBase AddDomainWarnings(publishStatus.Content, successfulCultures, globalNotifications); AddPublishStatusNotifications(new[] { publishStatus }, globalNotifications, notifications, successfulCultures); } - break; + break; case ContentSaveAction.PublishWithDescendants: case ContentSaveAction.PublishWithDescendantsNew: { @@ -1074,7 +1075,7 @@ public class ContentController : ContentControllerBase AddDomainWarnings(publishStatus, successfulCultures, globalNotifications); AddPublishStatusNotifications(publishStatus, globalNotifications, notifications, successfulCultures); } - break; + break; case ContentSaveAction.PublishWithDescendantsForce: case ContentSaveAction.PublishWithDescendantsForceNew: { @@ -1090,7 +1091,7 @@ public class ContentController : ContentControllerBase var publishStatus = PublishBranchInternal(contentItem, true, cultureForInvariantErrors, out wasCancelled, out var successfulCultures).ToList(); AddPublishStatusNotifications(publishStatus, globalNotifications, notifications, successfulCultures); } - break; + break; default: throw new ArgumentOutOfRangeException(); } @@ -2745,7 +2746,7 @@ public class ContentController : ContentControllerBase } } } - break; + break; case PublishResultType.SuccessPublish: { // TODO: Here we should have messaging for when there are release dates specified like https://github.com/umbraco/Umbraco-CMS/pull/3507 @@ -2773,7 +2774,7 @@ public class ContentController : ContentControllerBase } } } - break; + break; case PublishResultType.FailedPublishPathNotPublished: { //TODO: This doesn't take into account variations with the successfulCultures param @@ -2782,14 +2783,14 @@ public class ContentController : ContentControllerBase _localizedTextService.Localize(null, "publish"), _localizedTextService.Localize("publish", "contentPublishedFailedByParent", new[] { names }).Trim()); } - break; + break; case PublishResultType.FailedPublishCancelledByEvent: { //TODO: This doesn't take into account variations with the successfulCultures param var names = string.Join(", ", status.Select(x => $"'{x.Content?.Name}'")); AddCancelMessage(display, "publish", "contentPublishedFailedByEvent", new[] { names }); } - break; + break; case PublishResultType.FailedPublishAwaitingRelease: { //TODO: This doesn't take into account variations with the successfulCultures param @@ -2798,7 +2799,7 @@ public class ContentController : ContentControllerBase _localizedTextService.Localize(null, "publish"), _localizedTextService.Localize("publish", "contentPublishedFailedAwaitingRelease", new[] { names }).Trim()); } - break; + break; case PublishResultType.FailedPublishHasExpired: { //TODO: This doesn't take into account variations with the successfulCultures param @@ -2807,7 +2808,7 @@ public class ContentController : ContentControllerBase _localizedTextService.Localize(null, "publish"), _localizedTextService.Localize("publish", "contentPublishedFailedExpired", new[] { names }).Trim()); } - break; + break; case PublishResultType.FailedPublishIsTrashed: { //TODO: This doesn't take into account variations with the successfulCultures param @@ -2816,7 +2817,7 @@ public class ContentController : ContentControllerBase _localizedTextService.Localize(null, "publish"), _localizedTextService.Localize("publish", "contentPublishedFailedIsTrashed", new[] { names }).Trim()); } - break; + break; case PublishResultType.FailedPublishContentInvalid: { if (successfulCultures == null) @@ -2840,7 +2841,7 @@ public class ContentController : ContentControllerBase } } } - break; + break; case PublishResultType.FailedPublishMandatoryCultureMissing: display.AddWarningNotification( _localizedTextService.Localize(null, "publish"), diff --git a/src/Umbraco.Web.BackOffice/Controllers/ContentControllerBase.cs b/src/Umbraco.Web.BackOffice/Controllers/ContentControllerBase.cs index 36a60843fb..3a20f7a02d 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/ContentControllerBase.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/ContentControllerBase.cs @@ -79,7 +79,6 @@ public abstract class ContentControllerBase : BackOfficeNotificationsController ModelState.AddModelError("id", $"content with id: {id} was not found"); NotFoundObjectResult errorResponse = NotFound(ModelState); - return errorResponse; } diff --git a/src/Umbraco.Web.BackOffice/Filters/ContentSaveValidationAttribute.cs b/src/Umbraco.Web.BackOffice/Filters/ContentSaveValidationAttribute.cs index fb8629c461..7cf6ba3cf5 100644 --- a/src/Umbraco.Web.BackOffice/Filters/ContentSaveValidationAttribute.cs +++ b/src/Umbraco.Web.BackOffice/Filters/ContentSaveValidationAttribute.cs @@ -278,6 +278,7 @@ internal sealed class ContentSaveValidationAttribute : TypeFilterAttribute if (!authorizationResult.Succeeded) { + actionContext.Result = new ForbidResult(); return false; }