diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Authorization/ContentPermissionsQueryStringHandlerTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Authorization/ContentPermissionsQueryStringHandlerTests.cs index e5a9a32b7d..10549a6d7d 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Authorization/ContentPermissionsQueryStringHandlerTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Authorization/ContentPermissionsQueryStringHandlerTests.cs @@ -182,7 +182,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Web.BackOffice.Authorization { var mockBackOfficeSecurityAccessor = CreateMockBackOfficeSecurityAccessor(); var mockEntityService = CreateMockEntityService(); - var contentPermissions = CreateContentPermissions(nodeId, permissionsForPath); + var contentPermissions = CreateContentPermissions(mockEntityService.Object, nodeId, permissionsForPath); return new ContentPermissionsQueryStringHandler(mockBackOfficeSecurityAccessor.Object, httpContextAccessor, mockEntityService.Object, contentPermissions); } @@ -214,7 +214,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Web.BackOffice.Authorization .Build(); } - private static ContentPermissions CreateContentPermissions(int nodeId, string[] permissionsForPath) + private static ContentPermissions CreateContentPermissions(IEntityService entityService, int nodeId, string[] permissionsForPath) { var mockUserService = new Mock(); @@ -227,8 +227,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Web.BackOffice.Authorization .Setup(x => x.GetById(It.Is(y => y == nodeId))) .Returns(CreateContent(nodeId)); - var mockEntityService = new Mock(); - return new ContentPermissions(mockUserService.Object, mockContentService.Object, mockEntityService.Object); + return new ContentPermissions(mockUserService.Object, mockContentService.Object, entityService); } private static IContent CreateContent(int nodeId) diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Authorization/MediaPermissionsQueryStringHandlerTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Authorization/MediaPermissionsQueryStringHandlerTests.cs new file mode 100644 index 0000000000..80e2cddb75 --- /dev/null +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Authorization/MediaPermissionsQueryStringHandlerTests.cs @@ -0,0 +1,211 @@ +using System; +using System.Collections.Generic; +using System.Security.Claims; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Primitives; +using Moq; +using NUnit.Framework; +using Umbraco.Core; +using Umbraco.Core.Models; +using Umbraco.Core.Models.Membership; +using Umbraco.Core.Security; +using Umbraco.Core.Services; +using Umbraco.Tests.Common.Builders; +using Umbraco.Web.BackOffice.Authorization; + +namespace Umbraco.Tests.UnitTests.Umbraco.Web.BackOffice.Authorization +{ + public class MediaPermissionsQueryStringHandlerTests + { + private const string QueryStringName = "id"; + private const int NodeId = 1000; + private static readonly Guid NodeGuid = Guid.NewGuid(); + private static readonly Udi NodeUdi = UdiParser.Parse($"umb://document/{NodeGuid.ToString().ToLowerInvariant().Replace("-", string.Empty)}"); + + [Test] + public async Task Node_Id_Missing_From_QueryString_Is_Authorized() + { + var authHandlerContext = CreateAuthorizationHandlerContext(); + var mockHttpContextAccessor = CreateMockHttpContextAccessor(queryStringName: "xxx"); + var sut = CreateHandler(mockHttpContextAccessor.Object, NodeId); + + await sut.HandleAsync(authHandlerContext); + + Assert.IsTrue(authHandlerContext.HasSucceeded); + } + + [Test] + public async Task Node_Integer_Id_From_QueryString_With_Permission_Is_Authorized() + { + var authHandlerContext = CreateAuthorizationHandlerContext(); + var mockHttpContextAccessor = CreateMockHttpContextAccessor(queryStringValue: NodeId.ToString()); + var sut = CreateHandler(mockHttpContextAccessor.Object, NodeId); + + await sut.HandleAsync(authHandlerContext); + + Assert.IsTrue(authHandlerContext.HasSucceeded); + AssertMediaCached(mockHttpContextAccessor); + } + + [Test] + public async Task Node_Integer_Id_From_QueryString_Without_Permission_Is_Not_Authorized() + { + var authHandlerContext = CreateAuthorizationHandlerContext(); + var mockHttpContextAccessor = CreateMockHttpContextAccessor(queryStringValue: NodeId.ToString()); + var sut = CreateHandler(mockHttpContextAccessor.Object, NodeId, startMediaId: 1001); + + await sut.HandleAsync(authHandlerContext); + + Assert.IsFalse(authHandlerContext.HasSucceeded); + AssertMediaCached(mockHttpContextAccessor); + } + + [Test] + public async Task Node_Udi_Id_From_QueryString_With_Permission_Is_Authorized() + { + var authHandlerContext = CreateAuthorizationHandlerContext(); + var mockHttpContextAccessor = CreateMockHttpContextAccessor(queryStringValue: NodeUdi.ToString()); + var sut = CreateHandler(mockHttpContextAccessor.Object, NodeId); + + await sut.HandleAsync(authHandlerContext); + + Assert.IsTrue(authHandlerContext.HasSucceeded); + AssertMediaCached(mockHttpContextAccessor); + } + + [Test] + public async Task Node_Udi_Id_From_QueryString_Without_Permission_Is_Not_Authorized() + { + var authHandlerContext = CreateAuthorizationHandlerContext(); + var mockHttpContextAccessor = CreateMockHttpContextAccessor(queryStringValue: NodeUdi.ToString()); + var sut = CreateHandler(mockHttpContextAccessor.Object, NodeId, startMediaId: 1001); + + await sut.HandleAsync(authHandlerContext); + + Assert.IsFalse(authHandlerContext.HasSucceeded); + AssertMediaCached(mockHttpContextAccessor); + } + + [Test] + public async Task Node_Guid_Id_From_QueryString_With_Permission_Is_Authorized() + { + var authHandlerContext = CreateAuthorizationHandlerContext(); + var mockHttpContextAccessor = CreateMockHttpContextAccessor(queryStringValue: NodeGuid.ToString()); + var sut = CreateHandler(mockHttpContextAccessor.Object, NodeId); + + await sut.HandleAsync(authHandlerContext); + + Assert.IsTrue(authHandlerContext.HasSucceeded); + AssertMediaCached(mockHttpContextAccessor); + } + + [Test] + public async Task Node_Guid_Id_From_QueryString_Without_Permission_Is_Not_Authorized() + { + var authHandlerContext = CreateAuthorizationHandlerContext(); + var mockHttpContextAccessor = CreateMockHttpContextAccessor(queryStringValue: NodeGuid.ToString()); + var sut = CreateHandler(mockHttpContextAccessor.Object, NodeId, startMediaId: 1001); + + await sut.HandleAsync(authHandlerContext); + + Assert.IsFalse(authHandlerContext.HasSucceeded); + AssertMediaCached(mockHttpContextAccessor); + } + + [Test] + public async Task Node_Invalid_Id_From_QueryString_Is_Authorized() + { + var authHandlerContext = CreateAuthorizationHandlerContext(); + var mockHttpContextAccessor = CreateMockHttpContextAccessor(queryStringValue: "invalid"); + var sut = CreateHandler(mockHttpContextAccessor.Object, NodeId); + + await sut.HandleAsync(authHandlerContext); + + Assert.IsTrue(authHandlerContext.HasSucceeded); + } + + private static AuthorizationHandlerContext CreateAuthorizationHandlerContext() + { + var requirement = new MediaPermissionsQueryStringRequirement(QueryStringName); + var user = new ClaimsPrincipal(new ClaimsIdentity(new List())); + var resource = new object(); + return new AuthorizationHandlerContext(new List { requirement }, user, resource); + } + + private static Mock CreateMockHttpContextAccessor(string queryStringName = QueryStringName, string queryStringValue = "") + { + var mockHttpContextAccessor = new Mock(); + var mockHttpContext = new Mock(); + var mockHttpRequest = new Mock(); + var queryParams = new Dictionary + { + { queryStringName, queryStringValue }, + }; + mockHttpRequest.SetupGet(x => x.Query).Returns(new QueryCollection(queryParams)); + mockHttpContext.SetupGet(x => x.Request).Returns(mockHttpRequest.Object); + mockHttpContext.SetupGet(x => x.Items).Returns(new Dictionary()); + mockHttpContextAccessor.SetupGet(x => x.HttpContext).Returns(mockHttpContext.Object); + return mockHttpContextAccessor; + } + + private MediaPermissionsQueryStringHandler CreateHandler(IHttpContextAccessor httpContextAccessor, int nodeId, int startMediaId = -1) + { + var mockBackOfficeSecurityAccessor = CreateMockBackOfficeSecurityAccessor(startMediaId); + var mockEntityService = CreateMockEntityService(); + var mediaPermissions = CreateMediaPermissions(mockEntityService.Object, nodeId); + return new MediaPermissionsQueryStringHandler(mockBackOfficeSecurityAccessor.Object, httpContextAccessor, mockEntityService.Object, mediaPermissions); + } + + private static Mock CreateMockEntityService() + { + var mockEntityService = new Mock(); + mockEntityService + .Setup(x => x.GetId(It.Is(y => y == NodeUdi))) + .Returns(Attempt.Succeed(NodeId)); + mockEntityService + .Setup(x => x.GetId(It.Is(y => y == NodeGuid), It.Is(y => y == UmbracoObjectTypes.Document))) + .Returns(Attempt.Succeed(NodeId)); + return mockEntityService; + } + + private static Mock CreateMockBackOfficeSecurityAccessor(int startMediaId) + { + var user = CreateUser(startMediaId); + var mockBackOfficeSecurity = new Mock(); + mockBackOfficeSecurity.SetupGet(x => x.CurrentUser).Returns(user); + var mockBackOfficeSecurityAccessor = new Mock(); + mockBackOfficeSecurityAccessor.Setup(x => x.BackOfficeSecurity).Returns(mockBackOfficeSecurity.Object); + return mockBackOfficeSecurityAccessor; + } + + private static User CreateUser(int startMediaId) + { + return new UserBuilder() + .WithStartMediaId(startMediaId) + .Build(); + } + + private static MediaPermissions CreateMediaPermissions(IEntityService entityService, int nodeId) + { + var mockMediaService = new Mock(); + mockMediaService + .Setup(x => x.GetById(It.Is(y => y == nodeId))) + .Returns(CreateMedia(nodeId)); + + return new MediaPermissions(mockMediaService.Object, entityService); + } + + private static IMedia CreateMedia(int nodeId) + { + var mediaType = MediaTypeBuilder.CreateSimpleMediaType("image", "Image"); + return MediaBuilder.CreateSimpleMedia(mediaType, "Test image", -1, nodeId); + } + + private static void AssertMediaCached(Mock mockHttpContextAccessor) + { + Assert.AreEqual(NodeId, ((IMedia)mockHttpContextAccessor.Object.HttpContext.Items[typeof(IMedia).ToString()]).Id); + } + } +} diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Authorization/MediaPermissionsResourceHandlerTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Authorization/MediaPermissionsResourceHandlerTests.cs index 8db98dc081..19e2a6f06c 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Authorization/MediaPermissionsResourceHandlerTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Authorization/MediaPermissionsResourceHandlerTests.cs @@ -75,7 +75,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Web.BackOffice.Authorization private static IMedia CreateMedia(int nodeId) { var mediaType = MediaTypeBuilder.CreateSimpleMediaType("image", "Image"); - return MediaBuilder.CreateSimpleMedia(mediaType, "Test image", -1); + return MediaBuilder.CreateSimpleMedia(mediaType, "Test image", -1, nodeId); } private MediaPermissionsResourceHandler CreateHandler(int nodeId, int startMediaId = -1) diff --git a/src/Umbraco.Web.BackOffice/Authorization/ContentPermissionsQueryStringHandler.cs b/src/Umbraco.Web.BackOffice/Authorization/ContentPermissionsQueryStringHandler.cs index 57eea7b8b2..863cbdfc3c 100644 --- a/src/Umbraco.Web.BackOffice/Authorization/ContentPermissionsQueryStringHandler.cs +++ b/src/Umbraco.Web.BackOffice/Authorization/ContentPermissionsQueryStringHandler.cs @@ -12,11 +12,8 @@ namespace Umbraco.Web.BackOffice.Authorization /// /// Used to authorize if the user has the correct permission access to the content for the content id specified in a query string /// - public class ContentPermissionsQueryStringHandler : MustSatisfyRequirementAuthorizationHandler + public class ContentPermissionsQueryStringHandler : PermissionsQueryStringHandler { - private readonly IBackOfficeSecurityAccessor _backofficeSecurityAccessor; - private readonly IHttpContextAccessor _httpContextAccessor; - private readonly IEntityService _entityService; private readonly ContentPermissions _contentPermissions; public ContentPermissionsQueryStringHandler( @@ -24,10 +21,8 @@ namespace Umbraco.Web.BackOffice.Authorization IHttpContextAccessor httpContextAccessor, IEntityService entityService, ContentPermissions contentPermissions) + : base(backofficeSecurityAccessor, httpContextAccessor, entityService) { - _backofficeSecurityAccessor = backofficeSecurityAccessor; - _httpContextAccessor = httpContextAccessor; - _entityService = entityService; _contentPermissions = contentPermissions; } @@ -36,7 +31,7 @@ namespace Umbraco.Web.BackOffice.Authorization int nodeId; if (requirement.NodeId.HasValue == false) { - if (!_httpContextAccessor.HttpContext.Request.Query.TryGetValue(requirement.QueryStringName, out var routeVal)) + if (!HttpContextAccessor.HttpContext.Request.Query.TryGetValue(requirement.QueryStringName, out var routeVal)) { // Must succeed this requirement since we cannot process it return Task.FromResult(true); @@ -45,24 +40,9 @@ namespace Umbraco.Web.BackOffice.Authorization { var argument = routeVal.ToString(); - // If the argument is an int, it will parse and can be assigned to nodeId. - // It might be a udi, so check that next. - // Otherwise treat it as a guid - unlikely we ever get here. - // Failing that, we can't parse it so must succeed this requirement since we cannot process it. - if (int.TryParse(argument, out int parsedId)) - { - nodeId = parsedId; - } - else if (UdiParser.TryParse(argument, true, out var udi)) - { - nodeId = _entityService.GetId(udi).Result; - } - else if (Guid.TryParse(argument, out var key)) - { - nodeId = _entityService.GetId(key, UmbracoObjectTypes.Document).Result; - } - else + if (!TryParseNodeId(argument, out nodeId)) { + // Must succeed this requirement since we cannot process it. return Task.FromResult(true); } } @@ -73,14 +53,14 @@ namespace Umbraco.Web.BackOffice.Authorization } var permissionResult = _contentPermissions.CheckPermissions(nodeId, - _backofficeSecurityAccessor.BackOfficeSecurity.CurrentUser, + BackofficeSecurityAccessor.BackOfficeSecurity.CurrentUser, out IContent contentItem, new[] { requirement.PermissionToCheck }); if (contentItem != null) { // Store the content item in request cache so it can be resolved in the controller without re-looking it up. - _httpContextAccessor.HttpContext.Items[typeof(IContent).ToString()] = contentItem; + HttpContextAccessor.HttpContext.Items[typeof(IContent).ToString()] = contentItem; } return permissionResult switch diff --git a/src/Umbraco.Web.BackOffice/Authorization/MediaPermissionsQueryStringHandler.cs b/src/Umbraco.Web.BackOffice/Authorization/MediaPermissionsQueryStringHandler.cs index a8d3b72918..d6ef44cef1 100644 --- a/src/Umbraco.Web.BackOffice/Authorization/MediaPermissionsQueryStringHandler.cs +++ b/src/Umbraco.Web.BackOffice/Authorization/MediaPermissionsQueryStringHandler.cs @@ -1,71 +1,51 @@ -using Microsoft.AspNetCore.Authorization; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Http; -using Microsoft.Extensions.Primitives; -using System; -using System.Threading.Tasks; -using Umbraco.Core; using Umbraco.Core.Models; using Umbraco.Core.Security; using Umbraco.Core.Services; namespace Umbraco.Web.BackOffice.Authorization { - public class MediaPermissionsQueryStringHandler : MustSatisfyRequirementAuthorizationHandler + public class MediaPermissionsQueryStringHandler : PermissionsQueryStringHandler { - private readonly IBackOfficeSecurityAccessor _backofficeSecurityAccessor; - private readonly IHttpContextAccessor _httpContextAccessor; private readonly MediaPermissions _mediaPermissions; - private readonly IEntityService _entityService; public MediaPermissionsQueryStringHandler( IBackOfficeSecurityAccessor backofficeSecurityAccessor, IHttpContextAccessor httpContextAccessor, IEntityService entityService, MediaPermissions mediaPermissions) + : base(backofficeSecurityAccessor, httpContextAccessor, entityService) { - _backofficeSecurityAccessor = backofficeSecurityAccessor; - _httpContextAccessor = httpContextAccessor; - _entityService = entityService; _mediaPermissions = mediaPermissions; } protected override Task IsAuthorized(AuthorizationHandlerContext context, MediaPermissionsQueryStringRequirement requirement) { - if (!_httpContextAccessor.HttpContext.Request.Query.TryGetValue(requirement.QueryStringName, out var routeVal)) + if (!HttpContextAccessor.HttpContext.Request.Query.TryGetValue(requirement.QueryStringName, out var routeVal)) { - // must succeed this requirement since we cannot process it + // Must succeed this requirement since we cannot process it. return Task.FromResult(true); } - int nodeId; - 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)) + + if (!TryParseNodeId(argument, out int nodeId)) { - 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; + // Must succeed this requirement since we cannot process it. + return Task.FromResult(true); } var permissionResult = _mediaPermissions.CheckPermissions( - _backofficeSecurityAccessor.BackOfficeSecurity.CurrentUser, + BackofficeSecurityAccessor.BackOfficeSecurity.CurrentUser, nodeId, out var mediaItem); if (mediaItem != null) { - //store the content item in request cache so it can be resolved in the controller without re-looking it up - _httpContextAccessor.HttpContext.Items[typeof(IMedia).ToString()] = mediaItem; + // Store the media item in request cache so it can be resolved in the controller without re-looking it up. + HttpContextAccessor.HttpContext.Items[typeof(IMedia).ToString()] = mediaItem; } return permissionResult switch @@ -73,6 +53,6 @@ namespace Umbraco.Web.BackOffice.Authorization MediaPermissions.MediaAccess.Denied => Task.FromResult(false), _ => Task.FromResult(true), }; - } + } } } diff --git a/src/Umbraco.Web.BackOffice/Authorization/PermissionsQueryStringHandler.cs b/src/Umbraco.Web.BackOffice/Authorization/PermissionsQueryStringHandler.cs new file mode 100644 index 0000000000..49f62dc2f9 --- /dev/null +++ b/src/Umbraco.Web.BackOffice/Authorization/PermissionsQueryStringHandler.cs @@ -0,0 +1,58 @@ +using System; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Http; +using Umbraco.Core; +using Umbraco.Core.Models; +using Umbraco.Core.Security; +using Umbraco.Core.Services; + +namespace Umbraco.Web.BackOffice.Authorization +{ + public abstract class PermissionsQueryStringHandler : MustSatisfyRequirementAuthorizationHandler + where T : IAuthorizationRequirement + { + public PermissionsQueryStringHandler( + IBackOfficeSecurityAccessor backofficeSecurityAccessor, + IHttpContextAccessor httpContextAccessor, + IEntityService entityService) + { + BackofficeSecurityAccessor = backofficeSecurityAccessor; + HttpContextAccessor = httpContextAccessor; + EntityService = entityService; + } + + protected IBackOfficeSecurityAccessor BackofficeSecurityAccessor { get; set; } + + protected IHttpContextAccessor HttpContextAccessor { get; set; } + + protected IEntityService EntityService { get; set; } + + protected bool TryParseNodeId(string argument, out int nodeId) + { + // If the argument is an int, it will parse and can be assigned to nodeId. + // It might be a udi, so check that next. + // Otherwise treat it as a guid - unlikely we ever get here. + // Failing that, we can't parse it. + if (int.TryParse(argument, out int parsedId)) + { + nodeId = parsedId; + return true; + } + else if (UdiParser.TryParse(argument, true, out var udi)) + { + nodeId = EntityService.GetId(udi).Result; + return true; + } + else if (Guid.TryParse(argument, out var key)) + { + nodeId = EntityService.GetId(key, UmbracoObjectTypes.Document).Result; + return true; + } + else + { + nodeId = 0; + return false; + } + } + } +}