From 49f3c9e4b2e890a64ca6055d3f46a1ec5c41545b Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 9 Aug 2013 16:39:09 +1000 Subject: [PATCH] Adds more code and tests for permissions checks --- src/Umbraco.Core/Models/UserExtensions.cs | 55 ++++ src/Umbraco.Core/Umbraco.Core.csproj | 2 + .../ContentControllerHostedTests.cs | 80 ------ ...eUserPermissionForContentAttributeTests.cs | 234 ++++++++++++++++++ .../Models/UserExtensionsTests.cs | 27 ++ src/Umbraco.Tests/Umbraco.Tests.csproj | 3 +- src/Umbraco.Web/Editors/ContentController.cs | 2 +- ...EnsureUserPermissionForContentAttribute.cs | 92 +++++-- .../FilterAllowedOutgoingContentAttribute.cs | 2 - 9 files changed, 390 insertions(+), 107 deletions(-) create mode 100644 src/Umbraco.Core/Models/UserExtensions.cs delete mode 100644 src/Umbraco.Tests/Controllers/WebApiEditors/ContentControllerHostedTests.cs create mode 100644 src/Umbraco.Tests/Controllers/WebApiEditors/EnsureUserPermissionForContentAttributeTests.cs create mode 100644 src/Umbraco.Tests/Models/UserExtensionsTests.cs diff --git a/src/Umbraco.Core/Models/UserExtensions.cs b/src/Umbraco.Core/Models/UserExtensions.cs new file mode 100644 index 0000000000..d76c9f4b54 --- /dev/null +++ b/src/Umbraco.Core/Models/UserExtensions.cs @@ -0,0 +1,55 @@ +using System; +using System.Globalization; +using Umbraco.Core.Models.Membership; + +namespace Umbraco.Core.Models +{ + internal static class UserExtensions + { + /// + /// Checks if the user has access to the content item based on their start noe + /// + /// + /// + /// + internal static bool HasPathAccess(this IUser user, IContent content) + { + if (user == null) throw new ArgumentNullException("user"); + if (content == null) throw new ArgumentNullException("content"); + var formattedPath = "," + content.Path + ","; + var formattedStartNodeId = "," + user.StartContentId.ToString(CultureInfo.InvariantCulture) + ","; + var formattedRecycleBinId = "," + Constants.System.RecycleBinContent + ","; + + //only users with root access have access to the recycle bin + if (formattedPath.Contains(formattedRecycleBinId)) + { + return user.StartContentId == Constants.System.Root; + } + + return formattedPath.Contains(formattedStartNodeId); + } + + /// + /// Checks if the user has access to the media item based on their start noe + /// + /// + /// + /// + internal static bool HasPathAccess(this IUser user, IMedia media) + { + if (user == null) throw new ArgumentNullException("user"); + if (media == null) throw new ArgumentNullException("media"); + var formattedPath = "," + media.Path + ","; + var formattedStartNodeId = "," + user.StartContentId.ToString(CultureInfo.InvariantCulture) + ","; + var formattedRecycleBinId = "," + Constants.System.RecycleBinMedia + ","; + + //only users with root access have access to the recycle bin + if (formattedPath.Contains(formattedRecycleBinId) && user.StartContentId == Constants.System.Root) + { + return true; + } + + return formattedPath.Contains(formattedStartNodeId); + } + } +} \ No newline at end of file diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 4159988a39..ae5ccb1e60 100644 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -223,6 +223,7 @@ + @@ -261,6 +262,7 @@ + diff --git a/src/Umbraco.Tests/Controllers/WebApiEditors/ContentControllerHostedTests.cs b/src/Umbraco.Tests/Controllers/WebApiEditors/ContentControllerHostedTests.cs deleted file mode 100644 index cedeeae0aa..0000000000 --- a/src/Umbraco.Tests/Controllers/WebApiEditors/ContentControllerHostedTests.cs +++ /dev/null @@ -1,80 +0,0 @@ -using System; -using System.Collections.Generic; -using System.Linq; -using System.Net.Http; -using System.Text; -using System.Threading.Tasks; -using System.Web.Http; -using System.Web.Http.SelfHost; -using NUnit.Framework; -using Umbraco.Tests.TestHelpers; -using Umbraco.Web; -using Umbraco.Web.WebApi; -using Umbraco.Web.WebApi.Filters; -using umbraco.presentation.channels.businesslogic; - -namespace Umbraco.Tests.Controllers.WebApiEditors -{ - //we REALLY need a way to nicely mock the service context, etc... so we don't have to do integration tests... coming soon. - - //NOTE: The below self hosted stuff does work so need to get some tests written. Some are not possible atm because - // of the legacy SQL calls like checking permissions. - - //[TestFixture] - //public class ContentControllerHostedTests : BaseRoutingTest - //{ - - // protected override DatabaseBehavior DatabaseTestBehavior - // { - // get { return DatabaseBehavior.NoDatabasePerFixture; } - // } - - // public override void TearDown() - // { - // base.TearDown(); - // UmbracoAuthorizeAttribute.Enable = true; - // UmbracoApplicationAuthorizeAttribute.Enable = true; - // } - - // /// - // /// Tests to ensure that the response filter works so that any items the user - // /// doesn't have access to are removed - // /// - // [Test] - // public async void Get_By_Ids_Response_Filtered() - // { - // UmbracoAuthorizeAttribute.Enable = false; - // UmbracoApplicationAuthorizeAttribute.Enable = false; - - // var baseUrl = string.Format("http://{0}:9876", Environment.MachineName); - // var url = baseUrl + "/api/Content/GetByIds?ids=1&ids=2"; - - // var routingCtx = GetRoutingContext(url, 1234, null, true); - - // var config = new HttpSelfHostConfiguration(baseUrl); - // using (var server = new HttpSelfHostServer(config)) - // { - // var route = config.Routes.MapHttpRoute("test", "api/Content/GetByIds", - // new - // { - // controller = "Content", - // action = "GetByIds", - // id = RouteParameter.Optional - // }); - // route.DataTokens["Namespaces"] = new string[] { "Umbraco.Web.Editors" }; - - // var client = new HttpClient(server); - - // var request = new HttpRequestMessage - // { - // RequestUri = new Uri(url), - // Method = HttpMethod.Get - // }; - - // var result = await client.SendAsync(request); - // } - - // } - - //} -} diff --git a/src/Umbraco.Tests/Controllers/WebApiEditors/EnsureUserPermissionForContentAttributeTests.cs b/src/Umbraco.Tests/Controllers/WebApiEditors/EnsureUserPermissionForContentAttributeTests.cs new file mode 100644 index 0000000000..72d0cf3641 --- /dev/null +++ b/src/Umbraco.Tests/Controllers/WebApiEditors/EnsureUserPermissionForContentAttributeTests.cs @@ -0,0 +1,234 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Net.Http; +using System.Text; +using System.Threading.Tasks; +using System.Web.Http; +using System.Web.Http.Controllers; +using System.Web.Http.SelfHost; +using NUnit.Framework; +using Rhino.Mocks; +using Umbraco.Core.Models; +using Umbraco.Core.Models.Membership; +using Umbraco.Core.Services; +using Umbraco.Tests.TestHelpers; +using Umbraco.Web; +using Umbraco.Web.WebApi; +using Umbraco.Web.WebApi.Filters; +using umbraco.presentation.channels.businesslogic; + +namespace Umbraco.Tests.Controllers.WebApiEditors +{ + [TestFixture] + public class EnsureUserPermissionForContentAttributeTests + { + [Test] + public void Does_Not_Throw_Exception_When_Access_Allowed_By_Path() + { + //arrange + var user = MockRepository.GenerateStub(); + user.Id = 9; + user.StartContentId = -1; + var content = MockRepository.GenerateStub(); + content.Path = "-1,1234,5678"; + var contentService = MockRepository.GenerateStub(); + contentService.Stub(x => x.GetById(1234)).Return(content); + var userService = MockRepository.GenerateStub(); + var permissions = new List(); + userService.Stub(x => x.GetPermissions(user, 1234)).Return(permissions); + var ctx = new HttpActionContext(); + ctx.ActionArguments.Add("id", 1234); + var attribute = new EnsureUserPermissionForContentAttribute(user, userService, contentService, 1234, 'F'); + + //act + attribute.OnActionExecuting(ctx); + + //assert + Assert.Pass(); + } + + [Test] + public void Throws_Exception_When_No_Content_Found() + { + //arrange + var user = MockRepository.GenerateStub(); + user.Id = 9; + user.StartContentId = -1; + var content = MockRepository.GenerateStub(); + content.Path = "-1,1234,5678"; + var contentService = MockRepository.GenerateStub(); + contentService.Stub(x => x.GetById(0)).Return(content); + var userService = MockRepository.GenerateStub(); + var permissions = new List(); + userService.Stub(x => x.GetPermissions(user, 1234)).Return(permissions); + var ctx = new HttpActionContext(); + var attribute = new EnsureUserPermissionForContentAttribute(user, userService, contentService, 1234, 'F'); + + //act/assert + Assert.Throws(() => attribute.OnActionExecuting(ctx)); + } + + [Test] + public void Throws_Exception_When_No_Access_By_Path() + { + //arrange + var user = MockRepository.GenerateStub(); + user.Id = 9; + user.StartContentId = 9876; + var content = MockRepository.GenerateStub(); + content.Path = "-1,1234,5678"; + var contentService = MockRepository.GenerateStub(); + contentService.Stub(x => x.GetById(1234)).Return(content); + var userService = MockRepository.GenerateStub(); + var permissions = new List(); + userService.Stub(x => x.GetPermissions(user, 1234)).Return(permissions); + var ctx = new HttpActionContext(); + var attribute = new EnsureUserPermissionForContentAttribute(user, userService, contentService, 1234, 'F'); + + //act/assert + Assert.Throws(() => attribute.OnActionExecuting(ctx)); + } + + [Test] + public void Throws_Exception_When_No_Access_By_Permission() + { + //arrange + var user = MockRepository.GenerateStub(); + user.Id = 9; + user.StartContentId = -1; + var content = MockRepository.GenerateStub(); + content.Path = "-1,1234,5678"; + var contentService = MockRepository.GenerateStub(); + contentService.Stub(x => x.GetById(1234)).Return(content); + var userService = MockRepository.GenerateStub(); + var permissions = new List + { + new EntityPermission(9, 1234, new string[]{ "A", "B", "C" }) + }; + userService.Stub(x => x.GetPermissions(user, 1234)).Return(permissions); + var ctx = new HttpActionContext(); + var attribute = new EnsureUserPermissionForContentAttribute(user, userService, contentService, 1234, 'F'); + + //act/assert + Assert.Throws(() => attribute.OnActionExecuting(ctx)); + } + + [Test] + public void Does_Not_Throw_Exception_When_Access_Allowed_By_Permission() + { + //arrange + var user = MockRepository.GenerateStub(); + user.Id = 9; + user.StartContentId = -1; + var content = MockRepository.GenerateStub(); + content.Path = "-1,1234,5678"; + var contentService = MockRepository.GenerateStub(); + contentService.Stub(x => x.GetById(1234)).Return(content); + var userService = MockRepository.GenerateStub(); + var permissions = new List + { + new EntityPermission(9, 1234, new string[]{ "A", "F", "C" }) + }; + userService.Stub(x => x.GetPermissions(user, 1234)).Return(permissions); + var ctx = new HttpActionContext(); + ctx.ActionArguments.Add("id", 1234); + var attribute = new EnsureUserPermissionForContentAttribute(user, userService, contentService, 1234, 'F'); + + //act + attribute.OnActionExecuting(ctx); + + //assert + Assert.Pass(); + } + + [Test] + public void Does_Not_Throw_Exception_When_No_Permissions_Assigned() + { + //arrange + var user = MockRepository.GenerateStub(); + user.Id = 9; + user.StartContentId = -1; + var content = MockRepository.GenerateStub(); + content.Path = "-1,1234,5678"; + var contentService = MockRepository.GenerateStub(); + contentService.Stub(x => x.GetById(1234)).Return(content); + var userService = MockRepository.GenerateStub(); + var permissions = new List(); + userService.Stub(x => x.GetPermissions(user, 1234)).Return(permissions); + var ctx = new HttpActionContext(); + ctx.ActionArguments.Add("id", 1234); + var attribute = new EnsureUserPermissionForContentAttribute(user, userService, contentService, 1234, 'F'); + + //act + attribute.OnActionExecuting(ctx); + + //assert + Assert.Pass(); + } + + } + + //we REALLY need a way to nicely mock the service context, etc... so we don't have to do integration tests... coming soon. + + //NOTE: The below self hosted stuff does work so need to get some tests written. Some are not possible atm because + // of the legacy SQL calls like checking permissions. + + //[TestFixture] + //public class ContentControllerHostedTests : BaseRoutingTest + //{ + + // protected override DatabaseBehavior DatabaseTestBehavior + // { + // get { return DatabaseBehavior.NoDatabasePerFixture; } + // } + + // public override void TearDown() + // { + // base.TearDown(); + // UmbracoAuthorizeAttribute.Enable = true; + // UmbracoApplicationAuthorizeAttribute.Enable = true; + // } + + // /// + // /// Tests to ensure that the response filter works so that any items the user + // /// doesn't have access to are removed + // /// + // [Test] + // public async void Get_By_Ids_Response_Filtered() + // { + // UmbracoAuthorizeAttribute.Enable = false; + // UmbracoApplicationAuthorizeAttribute.Enable = false; + + // var baseUrl = string.Format("http://{0}:9876", Environment.MachineName); + // var url = baseUrl + "/api/Content/GetByIds?ids=1&ids=2"; + + // var routingCtx = GetRoutingContext(url, 1234, null, true); + + // var config = new HttpSelfHostConfiguration(baseUrl); + // using (var server = new HttpSelfHostServer(config)) + // { + // var route = config.Routes.MapHttpRoute("test", "api/Content/GetByIds", + // new + // { + // controller = "Content", + // action = "GetByIds", + // id = RouteParameter.Optional + // }); + // route.DataTokens["Namespaces"] = new string[] { "Umbraco.Web.Editors" }; + + // var client = new HttpClient(server); + + // var request = new HttpRequestMessage + // { + // RequestUri = new Uri(url), + // Method = HttpMethod.Get + // }; + + // var result = await client.SendAsync(request); + // } + + // } + + //} +} diff --git a/src/Umbraco.Tests/Models/UserExtensionsTests.cs b/src/Umbraco.Tests/Models/UserExtensionsTests.cs new file mode 100644 index 0000000000..a1ced63aba --- /dev/null +++ b/src/Umbraco.Tests/Models/UserExtensionsTests.cs @@ -0,0 +1,27 @@ +using NUnit.Framework; +using Rhino.Mocks; +using Umbraco.Core.Models; +using Umbraco.Core.Models.Membership; + +namespace Umbraco.Tests.Models +{ + [TestFixture] + public class UserExtensionsTests + { + [TestCase(2, "-1,1,2,3,4,5", true)] + [TestCase(6, "-1,1,2,3,4,5", false)] + [TestCase(-1, "-1,1,2,3,4,5", true)] + [TestCase(5, "-1,1,2,3,4,5", true)] + [TestCase(-1, "-1,-20,1,2,3,4,5", true)] + [TestCase(1, "-1,-20,1,2,3,4,5", false)] + public void Determines_Path_Based_Access_To_Content(int userId, string contentPath, bool outcome) + { + var user = MockRepository.GenerateStub(); + user.StartContentId = userId; + var content = MockRepository.GenerateStub(); + content.Path = contentPath; + + Assert.AreEqual(outcome, user.HasPathAccess(content)); + } + } +} \ No newline at end of file diff --git a/src/Umbraco.Tests/Umbraco.Tests.csproj b/src/Umbraco.Tests/Umbraco.Tests.csproj index 1689c452be..bf90e63a2c 100644 --- a/src/Umbraco.Tests/Umbraco.Tests.csproj +++ b/src/Umbraco.Tests/Umbraco.Tests.csproj @@ -203,11 +203,12 @@ - + + diff --git a/src/Umbraco.Web/Editors/ContentController.cs b/src/Umbraco.Web/Editors/ContentController.cs index ea7443fe2f..2780547bae 100644 --- a/src/Umbraco.Web/Editors/ContentController.cs +++ b/src/Umbraco.Web/Editors/ContentController.cs @@ -296,7 +296,7 @@ namespace Umbraco.Web.Editors /// attributed with EnsureUserPermissionForContent to verify the user has access to the recycle bin /// [HttpDelete] - [EnsureUserPermissionForContent(true)] + [EnsureUserPermissionForContent(Constants.System.RecycleBinContent)] public HttpResponseMessage EmptyRecycleBin() { //TODO: We need to check if the user is allowed access to the recycle bin! diff --git a/src/Umbraco.Web/WebApi/Filters/EnsureUserPermissionForContentAttribute.cs b/src/Umbraco.Web/WebApi/Filters/EnsureUserPermissionForContentAttribute.cs index 1e833a5c7d..797e10cd84 100644 --- a/src/Umbraco.Web/WebApi/Filters/EnsureUserPermissionForContentAttribute.cs +++ b/src/Umbraco.Web/WebApi/Filters/EnsureUserPermissionForContentAttribute.cs @@ -1,11 +1,14 @@ using System; using System.Collections; +using System.Globalization; using System.Linq; using System.Web.Http; using System.Web.Http.Controllers; using System.Web.Http.Filters; using Umbraco.Core; using Umbraco.Core.Models; +using Umbraco.Core.Models.Membership; +using Umbraco.Core.Services; using umbraco.BusinessLogic.Actions; namespace Umbraco.Web.WebApi.Filters @@ -23,14 +26,51 @@ namespace Umbraco.Web.WebApi.Filters /// internal sealed class EnsureUserPermissionForContentAttribute : ActionFilterAttribute { - private readonly bool _onlyCheckStartNode; + private int? _nodeId; + private readonly IUser _user; + private readonly IUserService _userService; + private readonly IContentService _contentService; + private IContentService ContentService + { + get { return _contentService ?? ApplicationContext.Current.Services.ContentService; } + } + private IUserService UserService + { + get { return _userService ?? ApplicationContext.Current.Services.UserService; } + } + private IUser User + { + get { return _user ?? UmbracoContext.Current.Security.CurrentUser; } + } + private readonly string _paramName; private readonly char _permissionToCheck; - public EnsureUserPermissionForContentAttribute(bool onlyCheckStartNode) + /// + /// used for unit testing + /// + /// + /// + /// + /// + /// + internal EnsureUserPermissionForContentAttribute(IUser user, IUserService userService, IContentService contentService, int nodeId, char permissionToCheck) { - _onlyCheckStartNode = onlyCheckStartNode; + _user = user; + _userService = userService; + _contentService = contentService; + _nodeId = nodeId; + _permissionToCheck = permissionToCheck; } + + /// + /// This constructor will only be able to test the start node access + /// + public EnsureUserPermissionForContentAttribute(int nodeId) + { + _nodeId = nodeId; + } + public EnsureUserPermissionForContentAttribute(string paramName) { _paramName = paramName; @@ -49,30 +89,36 @@ namespace Umbraco.Web.WebApi.Filters public override void OnActionExecuting(HttpActionContext actionContext) { - if (UmbracoContext.Current.UmbracoUser == null) + if (User == null) + { + throw new HttpResponseException(System.Net.HttpStatusCode.Unauthorized); + } + + if (_nodeId.HasValue == false) + { + if (actionContext.ActionArguments[_paramName] == null) + { + throw new InvalidOperationException("No argument found for the current action with the name: " + _paramName); + } + + _nodeId = (int)actionContext.ActionArguments[_paramName]; + } + + var contentItem = ContentService.GetById(_nodeId.Value); + if (contentItem == null) + { + throw new HttpResponseException(System.Net.HttpStatusCode.NotFound); + } + + var hasPathAccess = User.HasPathAccess(contentItem); + + if (hasPathAccess == false) { throw new HttpResponseException(System.Net.HttpStatusCode.Unauthorized); } - if (actionContext.ActionArguments[_paramName] == null) - { - throw new InvalidOperationException("No argument found for the current action with the name: " + _paramName); - } - - var nodeId = (int)actionContext.ActionArguments[_paramName]; - - //var contentItem = ApplicationContext.Current.Services.ContentService.GetById(nodeId); - - //var hasPathAccess = (Path.Contains("-20") || ("," + Path + ",").Contains("," + getUser().StartNodeId.ToString() + ",")) - - if (_onlyCheckStartNode) - { - //TODO: implement this as well! - } - - //TODO: Change these calls to a service layer call and make sure we can mock it! - var permissions = UmbracoContext.Current.UmbracoUser.GetPermissions(nodeId); - if (permissions.ToCharArray().Contains(_permissionToCheck)) + var permission = UserService.GetPermissions(User, _nodeId.Value).FirstOrDefault(); + if (permission == null || permission.AssignedPermissions.Contains(_permissionToCheck.ToString(CultureInfo.InvariantCulture))) { base.OnActionExecuting(actionContext); } diff --git a/src/Umbraco.Web/WebApi/Filters/FilterAllowedOutgoingContentAttribute.cs b/src/Umbraco.Web/WebApi/Filters/FilterAllowedOutgoingContentAttribute.cs index 1d8efc257a..d0146beb5e 100644 --- a/src/Umbraco.Web/WebApi/Filters/FilterAllowedOutgoingContentAttribute.cs +++ b/src/Umbraco.Web/WebApi/Filters/FilterAllowedOutgoingContentAttribute.cs @@ -95,8 +95,6 @@ namespace Umbraco.Web.WebApi.Filters private void SetValueForResponse(ObjectContent objectContent, dynamic newVal) { - var t = objectContent.Value.GetType(); - if (objectContent.Value is IEnumerable) { //objectContent.Value = DynamicCast(newVal, t);