From f313a4f583c1c9f5fedac09c018aec51eb8f7fa4 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 24 Nov 2020 11:56:53 +1100 Subject: [PATCH] some cleanup of handlers that check multiple query strings --- .../Authorization/AdminUsersHandler.cs | 62 +++++++++-------- .../ContentPermissionsQueryStringHandler.cs | 66 ++++++++----------- ...ontentPermissionsQueryStringRequirement.cs | 6 +- .../MediaPermissionsQueryStringHandler.cs | 35 ++++------ .../MediaPermissionsQueryStringRequirement.cs | 6 +- .../Controllers/AuthenticationController.cs | 1 - .../BackOfficeServiceCollectionExtensions.cs | 34 +++++----- 7 files changed, 98 insertions(+), 112 deletions(-) diff --git a/src/Umbraco.Web.BackOffice/Authorization/AdminUsersHandler.cs b/src/Umbraco.Web.BackOffice/Authorization/AdminUsersHandler.cs index 05c18b4bb6..14f76c48dd 100644 --- a/src/Umbraco.Web.BackOffice/Authorization/AdminUsersHandler.cs +++ b/src/Umbraco.Web.BackOffice/Authorization/AdminUsersHandler.cs @@ -1,5 +1,6 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Http; +using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; using Umbraco.Core; @@ -33,8 +34,40 @@ namespace Umbraco.Web.BackOffice.Authorization protected override Task HandleRequirementAsync(AuthorizationHandlerContext context, AdminUsersRequirement requirement) { - var isAuth = IsAuthorized(requirement); - if (!isAuth.HasValue || isAuth.Value) + int[] userIds; + + var queryString = _httpContextAcessor.HttpContext?.Request.Query[requirement.QueryStringName]; + if (!queryString.HasValue) + { + // don't set status since we cannot determine ourselves + return Task.CompletedTask; + } + + if (int.TryParse(queryString, out var userId)) + { + userIds = new[] { userId }; + } + else + { + var ids = _httpContextAcessor.HttpContext.Request.Query.Where(x => x.Key == requirement.QueryStringName).ToList(); + if (ids.Count == 0) + { + // don't set status since we cannot determine ourselves + return Task.CompletedTask; + } + userIds = ids.Select(x => x.Value.TryConvertTo()).Where(x => x.Success).Select(x => x.Result).ToArray(); + } + + if (userIds.Length == 0) + { + // don't set status since we cannot determine ourselves + return Task.CompletedTask; + } + + var users = _userService.GetUsersById(userIds); + var isAuth = users.All(user => _userEditorAuthorizationHelper.IsAuthorized(_backofficeSecurityAccessor.BackOfficeSecurity.CurrentUser, user, null, null, null) != false); + + if (isAuth) { context.Succeed(requirement); } @@ -45,30 +78,5 @@ namespace Umbraco.Web.BackOffice.Authorization return Task.CompletedTask; } - - private bool? IsAuthorized(AdminUsersRequirement requirement) - { - int[] userIds; - - var queryString = _httpContextAcessor.HttpContext?.Request.Query[requirement.QueryStringName]; - if (!queryString.HasValue) return null; - - if (int.TryParse(queryString, out var userId)) - { - userIds = new[] { userId }; - } - else - { - var ids = _httpContextAcessor.HttpContext.Request.Query.Where(x => x.Key == requirement.QueryStringName).ToArray(); - if (ids.Length == 0) - return null; - userIds = ids.Select(x => x.Value.TryConvertTo()).Where(x => x.Success).Select(x => x.Result).ToArray(); - } - - if (userIds.Length == 0) return null; - - var users = _userService.GetUsersById(userIds); - return users.All(user => _userEditorAuthorizationHelper.IsAuthorized(_backofficeSecurityAccessor.BackOfficeSecurity.CurrentUser, user, null, null, null) != false); - } } } diff --git a/src/Umbraco.Web.BackOffice/Authorization/ContentPermissionsQueryStringHandler.cs b/src/Umbraco.Web.BackOffice/Authorization/ContentPermissionsQueryStringHandler.cs index 0534b92fc9..4d9e5baf1d 100644 --- a/src/Umbraco.Web.BackOffice/Authorization/ContentPermissionsQueryStringHandler.cs +++ b/src/Umbraco.Web.BackOffice/Authorization/ContentPermissionsQueryStringHandler.cs @@ -39,36 +39,30 @@ namespace Umbraco.Web.BackOffice.Authorization int nodeId; if (requirement.NodeId.HasValue == false) { - StringValues routeVal; - foreach(var qs in requirement.QueryStringNames) + if (!_httpContextAccessor.HttpContext.Request.Query.TryGetValue(requirement.QueryStringName, out var routeVal)) { - if (_httpContextAccessor.HttpContext.Request.Query.TryGetValue(qs, out routeVal)) - { - break; - } - } - - if (routeVal.Count == 0) - { - throw new InvalidOperationException("No argument found for the current action with by names " + string.Join(", ", requirement.QueryStringNames)); - } - - var argument = routeVal.ToString(); - // if the argument is an int, it will parse and can be assigned to nodeId - // if might be a udi, so check that next - // otherwise treat it as a guid - unlikely we ever get here - if (int.TryParse(argument, out int parsedId)) - { - nodeId = parsedId; - } - else if (UdiParser.TryParse(argument, true, out var udi)) - { - nodeId = _entityService.GetId(udi).Result; + // don't set status since we cannot determine ourselves + return Task.CompletedTask; } else { - Guid.TryParse(argument, out Guid key); - nodeId = _entityService.GetId(key, UmbracoObjectTypes.Document).Result; + var argument = routeVal.ToString(); + // if the argument is an int, it will parse and can be assigned to nodeId + // if might be a udi, so check that next + // otherwise treat it as a guid - unlikely we ever get here + if (int.TryParse(argument, out int parsedId)) + { + nodeId = parsedId; + } + else if (UdiParser.TryParse(argument, true, out var udi)) + { + nodeId = _entityService.GetId(udi).Result; + } + else + { + Guid.TryParse(argument, out Guid key); + nodeId = _entityService.GetId(key, UmbracoObjectTypes.Document).Result; + } } } else @@ -81,21 +75,17 @@ namespace Umbraco.Web.BackOffice.Authorization out IContent contentItem, new[] { requirement.PermissionToCheck }); - if (permissionResult == ContentPermissions.ContentAccess.NotFound) + switch (permissionResult) { - return null; + case ContentPermissions.ContentAccess.Denied: + context.Fail(); + break; + case ContentPermissions.ContentAccess.NotFound: + default: + context.Succeed(requirement); + break; } - if (permissionResult == ContentPermissions.ContentAccess.Denied) - { - context.Fail(); - } - else - { - context.Succeed(requirement); - } - - if (contentItem != null) { //store the content item in request cache so it can be resolved in the controller without re-looking it up diff --git a/src/Umbraco.Web.BackOffice/Authorization/ContentPermissionsQueryStringRequirement.cs b/src/Umbraco.Web.BackOffice/Authorization/ContentPermissionsQueryStringRequirement.cs index 9153427614..2d558c569c 100644 --- a/src/Umbraco.Web.BackOffice/Authorization/ContentPermissionsQueryStringRequirement.cs +++ b/src/Umbraco.Web.BackOffice/Authorization/ContentPermissionsQueryStringRequirement.cs @@ -25,14 +25,14 @@ namespace Umbraco.Web.BackOffice.Authorization /// /// /// - public ContentPermissionsQueryStringRequirement(char permissionToCheck, string[] paramNames) + public ContentPermissionsQueryStringRequirement(char permissionToCheck, string paramName = "id") { - QueryStringNames = paramNames; + QueryStringName = paramName; PermissionToCheck = permissionToCheck; } public int? NodeId { get; } - public string[] QueryStringNames { get; } + public string QueryStringName { get; } public char PermissionToCheck { get; } } } diff --git a/src/Umbraco.Web.BackOffice/Authorization/MediaPermissionsQueryStringHandler.cs b/src/Umbraco.Web.BackOffice/Authorization/MediaPermissionsQueryStringHandler.cs index 09983064ad..a4f8d32ee6 100644 --- a/src/Umbraco.Web.BackOffice/Authorization/MediaPermissionsQueryStringHandler.cs +++ b/src/Umbraco.Web.BackOffice/Authorization/MediaPermissionsQueryStringHandler.cs @@ -20,27 +20,21 @@ namespace Umbraco.Web.BackOffice.Authorization public MediaPermissionsQueryStringHandler( IBackOfficeSecurityAccessor backofficeSecurityAccessor, IHttpContextAccessor httpContextAccessor, + IEntityService entityService, MediaPermissions mediaPermissions) { _backofficeSecurityAccessor = backofficeSecurityAccessor; _httpContextAccessor = httpContextAccessor; + _entityService = entityService; _mediaPermissions = mediaPermissions; } protected override Task HandleRequirementAsync(AuthorizationHandlerContext context, MediaPermissionsQueryStringRequirement requirement) { - StringValues routeVal; - foreach (var qs in requirement.QueryStringNames) + if (!_httpContextAccessor.HttpContext.Request.Query.TryGetValue(requirement.QueryStringName, out var routeVal)) { - if (_httpContextAccessor.HttpContext.Request.Query.TryGetValue(qs, out routeVal)) - { - break; - } - } - - if (routeVal.Count == 0) - { - throw new InvalidOperationException("No argument found for the current action with by names " + string.Join(", ", requirement.QueryStringNames)); + // don't set status since we cannot determine ourselves + return Task.CompletedTask; } int nodeId; @@ -68,18 +62,15 @@ namespace Umbraco.Web.BackOffice.Authorization nodeId, out var mediaItem); - if (permissionResult == MediaPermissions.MediaAccess.NotFound) + switch (permissionResult) { - return null; - } - - if (permissionResult == MediaPermissions.MediaAccess.Denied) - { - context.Fail(); - } - else - { - context.Succeed(requirement); + case MediaPermissions.MediaAccess.Denied: + context.Fail(); + break; + case MediaPermissions.MediaAccess.NotFound: + default: + context.Succeed(requirement); + break; } if (mediaItem != null) diff --git a/src/Umbraco.Web.BackOffice/Authorization/MediaPermissionsQueryStringRequirement.cs b/src/Umbraco.Web.BackOffice/Authorization/MediaPermissionsQueryStringRequirement.cs index 75bb78cea1..c7b62a570f 100644 --- a/src/Umbraco.Web.BackOffice/Authorization/MediaPermissionsQueryStringRequirement.cs +++ b/src/Umbraco.Web.BackOffice/Authorization/MediaPermissionsQueryStringRequirement.cs @@ -4,11 +4,11 @@ namespace Umbraco.Web.BackOffice.Authorization { public class MediaPermissionsQueryStringRequirement : IAuthorizationRequirement { - public MediaPermissionsQueryStringRequirement(string[] paramNames) + public MediaPermissionsQueryStringRequirement(string paramName) { - QueryStringNames = paramNames; + QueryStringName = paramName; } - public string[] QueryStringNames { get; } + public string QueryStringName { get; } } } diff --git a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs index 1a6f9d1c21..e78395321d 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs @@ -27,7 +27,6 @@ using Umbraco.Web.Common.Controllers; using Umbraco.Web.Common.Exceptions; using Umbraco.Web.Common.Filters; using Umbraco.Web.Common.Security; -using Umbraco.Web.Editors.Filters; using Umbraco.Web.Models; using Umbraco.Web.Models.ContentEditing; using Constants = Umbraco.Core.Constants; diff --git a/src/Umbraco.Web.BackOffice/Extensions/BackOfficeServiceCollectionExtensions.cs b/src/Umbraco.Web.BackOffice/Extensions/BackOfficeServiceCollectionExtensions.cs index 2c3cb13f6f..6daf5849b1 100644 --- a/src/Umbraco.Web.BackOffice/Extensions/BackOfficeServiceCollectionExtensions.cs +++ b/src/Umbraco.Web.BackOffice/Extensions/BackOfficeServiceCollectionExtensions.cs @@ -135,18 +135,12 @@ namespace Umbraco.Extensions services.AddAuthorization(options => { - // these are the query strings we will check for media ids when permission checking - var mediaPermissionQueryStrings = new[] { "id" }; - options.AddPolicy(AuthorizationPolicies.MediaPermissionPathById, policy => { policy.AuthenticationSchemes.Add(Constants.Security.BackOfficeAuthenticationType); - policy.Requirements.Add(new MediaPermissionsQueryStringRequirement(mediaPermissionQueryStrings)); + policy.Requirements.Add(new MediaPermissionsQueryStringRequirement("id")); }); - // these are the query strings we will check for content ids when permission checking - var contentPermissionQueryStrings = new[] { "id", "contentId" }; - options.AddPolicy(AuthorizationPolicies.ContentPermissionEmptyRecycleBin, policy => { policy.AuthenticationSchemes.Add(Constants.Security.BackOfficeAuthenticationType); @@ -156,37 +150,41 @@ namespace Umbraco.Extensions options.AddPolicy(AuthorizationPolicies.ContentPermissionAdministrationById, policy => { policy.AuthenticationSchemes.Add(Constants.Security.BackOfficeAuthenticationType); - policy.Requirements.Add(new ContentPermissionsQueryStringRequirement(ActionRights.ActionLetter, contentPermissionQueryStrings)); + policy.Requirements.Add(new ContentPermissionsQueryStringRequirement(ActionRights.ActionLetter)); + policy.Requirements.Add(new ContentPermissionsQueryStringRequirement(ActionRights.ActionLetter, "contentId")); }); options.AddPolicy(AuthorizationPolicies.ContentPermissionProtectById, policy => { policy.AuthenticationSchemes.Add(Constants.Security.BackOfficeAuthenticationType); - policy.Requirements.Add(new ContentPermissionsQueryStringRequirement(ActionProtect.ActionLetter, contentPermissionQueryStrings)); + policy.Requirements.Add(new ContentPermissionsQueryStringRequirement(ActionProtect.ActionLetter)); + policy.Requirements.Add(new ContentPermissionsQueryStringRequirement(ActionProtect.ActionLetter, "contentId")); }); options.AddPolicy(AuthorizationPolicies.ContentPermissionRollbackById, policy => { policy.AuthenticationSchemes.Add(Constants.Security.BackOfficeAuthenticationType); - policy.Requirements.Add(new ContentPermissionsQueryStringRequirement(ActionRollback.ActionLetter, contentPermissionQueryStrings)); + policy.Requirements.Add(new ContentPermissionsQueryStringRequirement(ActionRollback.ActionLetter)); + policy.Requirements.Add(new ContentPermissionsQueryStringRequirement(ActionRollback.ActionLetter, "contentId")); + }); + + options.AddPolicy(AuthorizationPolicies.ContentPermissionPublishById, policy => + { + policy.AuthenticationSchemes.Add(Constants.Security.BackOfficeAuthenticationType); + policy.Requirements.Add(new ContentPermissionsQueryStringRequirement(ActionPublish.ActionLetter)); }); options.AddPolicy(AuthorizationPolicies.ContentPermissionBrowseById, policy => { policy.AuthenticationSchemes.Add(Constants.Security.BackOfficeAuthenticationType); - policy.Requirements.Add(new ContentPermissionsQueryStringRequirement(ActionPublish.ActionLetter, contentPermissionQueryStrings)); - }); - - options.AddPolicy(AuthorizationPolicies.ContentPermissionBrowseById, policy => - { - policy.AuthenticationSchemes.Add(Constants.Security.BackOfficeAuthenticationType); - policy.Requirements.Add(new ContentPermissionsQueryStringRequirement(ActionBrowse.ActionLetter, contentPermissionQueryStrings)); + policy.Requirements.Add(new ContentPermissionsQueryStringRequirement(ActionBrowse.ActionLetter)); + policy.Requirements.Add(new ContentPermissionsQueryStringRequirement(ActionBrowse.ActionLetter, "contentId")); }); options.AddPolicy(AuthorizationPolicies.ContentPermissionDeleteById, policy => { policy.AuthenticationSchemes.Add(Constants.Security.BackOfficeAuthenticationType); - policy.Requirements.Add(new ContentPermissionsQueryStringRequirement(ActionDelete.ActionLetter, contentPermissionQueryStrings)); + policy.Requirements.Add(new ContentPermissionsQueryStringRequirement(ActionDelete.ActionLetter)); }); options.AddPolicy(AuthorizationPolicies.BackOfficeAccess, policy =>