From 6995c184e33efa3ff38e9355c18c6af6d5b99fb6 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 29 Aug 2017 01:00:08 +1000 Subject: [PATCH] Updates user editor, non-admins can see all user groups but cannot edit them, they can save user that contain groups that they don't have access to if they were already there but cannot add new ones they dont' have access to --- .../common/resources/usergroups.resource.js | 13 ++++- .../users/views/groups/groups.controller.js | 32 +++++++++---- .../src/views/users/views/groups/groups.html | 44 +++++++++++------ .../users/views/users/users.controller.js | 2 +- .../Editors/UserEditorAuthorizationHelper.cs | 48 +++++++++++++------ .../Editors/UserGroupsController.cs | 15 ++++-- .../Models/ContentEditing/UserDetail.cs | 8 +++- .../Models/Mapping/UserModelMapper.cs | 3 ++ 8 files changed, 119 insertions(+), 46 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/common/resources/usergroups.resource.js b/src/Umbraco.Web.UI.Client/src/common/resources/usergroups.resource.js index 90d9e58736..a0901ce0db 100644 --- a/src/Umbraco.Web.UI.Client/src/common/resources/usergroups.resource.js +++ b/src/Umbraco.Web.UI.Client/src/common/resources/usergroups.resource.js @@ -50,12 +50,21 @@ } - function getUserGroups() { + function getUserGroups(args) { + + if (!args) { + args = { onlyCurrentUserGroups: true }; + } + if (args.onlyCurrentUserGroups === undefined || args.onlyCurrentUserGroups === null) { + args.onlyCurrentUserGroups = true; + } + return umbRequestHelper.resourcePromise( $http.get( umbRequestHelper.getApiUrl( "userGroupsApiBaseUrl", - "GetUserGroups")), + "GetUserGroups", + args)), "Failed to retrieve user groups"); } diff --git a/src/Umbraco.Web.UI.Client/src/views/users/views/groups/groups.controller.js b/src/Umbraco.Web.UI.Client/src/views/users/views/groups/groups.controller.js index e7331de240..92d70765b0 100644 --- a/src/Umbraco.Web.UI.Client/src/views/users/views/groups/groups.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/users/views/groups/groups.controller.js @@ -1,7 +1,7 @@ (function () { "use strict"; - function UserGroupsController($scope, $timeout, $location, userGroupsResource, formHelper, localizationService) { + function UserGroupsController($scope, $timeout, $location, userService, userGroupsResource, formHelper, localizationService) { var vm = this; @@ -14,14 +14,21 @@ vm.selectUserGroup = selectUserGroup; vm.deleteUserGroups = deleteUserGroups; + var currentUser = null; + function onInit() { vm.loading = true; - // Get usergroups - userGroupsResource.getUserGroups().then(function (userGroups) { - vm.userGroups = userGroups; - vm.loading = false; + userService.getCurrentUser().then(function(user) { + currentUser = user; + // Get usergroups + userGroupsResource.getUserGroups({ onlyCurrentUserGroups: false }).then(function (userGroups) { + vm.userGroups = _.map(userGroups, function (ug) { + return { group: ug, isMember: user.userGroups.indexOf(ug.alias) !== -1} + }); + vm.loading = false; + }); }); } @@ -34,22 +41,31 @@ } function clickUserGroup(userGroup) { + + if (currentUser.userGroups.indexOf(userGroup.group.alias) === -1) { + return; + } + if (vm.selection.length > 0) { selectUserGroup(userGroup, vm.selection); } else { - goToUserGroup(userGroup.id); + goToUserGroup(userGroup.group.id); } } function selectUserGroup(userGroup, selection, event) { + if (currentUser.userGroups.indexOf(userGroup.group.alias) === -1) { + return; + } + if (userGroup.selected) { - var index = selection.indexOf(userGroup.id); + var index = selection.indexOf(userGroup.group.id); selection.splice(index, 1); userGroup.selected = false; } else { userGroup.selected = true; - vm.selection.push(userGroup.id); + vm.selection.push(userGroup.group.id); } if(event){ diff --git a/src/Umbraco.Web.UI.Client/src/views/users/views/groups/groups.html b/src/Umbraco.Web.UI.Client/src/views/users/views/groups/groups.html index 7a1cd100bb..4c54ce88b1 100644 --- a/src/Umbraco.Web.UI.Client/src/views/users/views/groups/groups.html +++ b/src/Umbraco.Web.UI.Client/src/views/users/views/groups/groups.html @@ -67,22 +67,38 @@ -
- -
-
- +
+
\ No newline at end of file diff --git a/src/Umbraco.Web.UI.Client/src/views/users/views/users/users.controller.js b/src/Umbraco.Web.UI.Client/src/views/users/views/users/users.controller.js index 4df493cfe6..3ea6c46580 100644 --- a/src/Umbraco.Web.UI.Client/src/views/users/views/users/users.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/users/views/users/users.controller.js @@ -106,7 +106,7 @@ getUsers(); // Get user groups - userGroupsResource.getUserGroups().then(function (userGroups) { + userGroupsResource.getUserGroups({ onlyCurrentUserGroups: false}).then(function (userGroups) { vm.userGroups = userGroups; }); diff --git a/src/Umbraco.Web/Editors/UserEditorAuthorizationHelper.cs b/src/Umbraco.Web/Editors/UserEditorAuthorizationHelper.cs index 745108c7b8..b81ec7b038 100644 --- a/src/Umbraco.Web/Editors/UserEditorAuthorizationHelper.cs +++ b/src/Umbraco.Web/Editors/UserEditorAuthorizationHelper.cs @@ -59,24 +59,42 @@ namespace Umbraco.Web.Editors if (userGroupAliases != null) { - var userGroups = _userService.GetUserGroupsByAlias(userGroupAliases.ToArray()).ToArray(); + var savingGroupAliases = userGroupAliases.ToArray(); - // d) A user cannot assign a group to another user that grants them access to a start node they don't have access to - foreach (var group in userGroups) - { - pathResult = AuthorizePath(currentUser, - group.StartContentId.HasValue ? new[] { group.StartContentId.Value } : null, - group.StartMediaId.HasValue ? new[] { group.StartMediaId.Value } : null); - if (pathResult == false) - return pathResult; - } + //only validate any groups that have changed. + //a non-admin user can remove groups and add groups that they have access to + //but they cannot add a group that they do not have access to or that grants them + //path or section access that they don't have access to. - // e) A user cannot set a section on another user that they don't have access to - var allGroupSections = userGroups.SelectMany(x => x.AllowedSections).Distinct(); - var missingSectionAccess = allGroupSections.Except(currentUser.AllowedSections).ToArray(); - if (missingSectionAccess.Length > 0) + var newGroups = savingUser == null + ? savingGroupAliases + : savingGroupAliases.Except(savingUser.Groups.Select(x => x.Alias)).ToArray(); + + var userGroupsChanged = savingUser != null && newGroups.Length > 0; + + if (userGroupsChanged) { - return Attempt.Fail("The current user does not have access to sections " + string.Join(",", missingSectionAccess)); + var userGroups = _userService.GetUserGroupsByAlias(newGroups).ToArray(); + + //TODO: pretty sure d + e can be done by just checking if the newGroups being added are groups that the current user is a member of + + // d) A user cannot assign a group to another user that grants them access to a start node they don't have access to + foreach (var group in userGroups) + { + pathResult = AuthorizePath(currentUser, + group.StartContentId.HasValue ? new[] { group.StartContentId.Value } : null, + group.StartMediaId.HasValue ? new[] { group.StartMediaId.Value } : null); + if (pathResult == false) + return pathResult; + } + + // e) A user cannot set a section on another user that they don't have access to + var allGroupSections = userGroups.SelectMany(x => x.AllowedSections).Distinct(); + var missingSectionAccess = allGroupSections.Except(currentUser.AllowedSections).ToArray(); + if (missingSectionAccess.Length > 0) + { + return Attempt.Fail("The current user does not have access to sections " + string.Join(",", missingSectionAccess)); + } } } diff --git a/src/Umbraco.Web/Editors/UserGroupsController.cs b/src/Umbraco.Web/Editors/UserGroupsController.cs index 9db73a82a0..33b1d9dc08 100644 --- a/src/Umbraco.Web/Editors/UserGroupsController.cs +++ b/src/Umbraco.Web/Editors/UserGroupsController.cs @@ -92,13 +92,20 @@ namespace Umbraco.Web.Editors /// Returns all user groups /// /// - public IEnumerable GetUserGroups() + public IEnumerable GetUserGroups(bool onlyCurrentUserGroups = true) { - var allGroups = Mapper.Map, IEnumerable>(Services.UserService.GetAllUserGroups()); + var allGroups = Mapper.Map, IEnumerable>(Services.UserService.GetAllUserGroups()) + .ToList(); - //if admin, return all groups - if (Security.CurrentUser.IsAdmin()) + var isAdmin = Security.CurrentUser.IsAdmin(); + if (isAdmin) return allGroups; + + if (onlyCurrentUserGroups == false) + { + //this user is not an admin so in that case we need to exlude all admin users + allGroups.RemoveAt(allGroups.IndexOf(allGroups.Find(basic => basic.Alias == Constants.Security.AdminGroupAlias))); return allGroups; + } //we cannot return user groups that this user does not have access to var currentUserGroups = Security.CurrentUser.Groups.Select(x => x.Alias).ToArray(); diff --git a/src/Umbraco.Web/Models/ContentEditing/UserDetail.cs b/src/Umbraco.Web/Models/ContentEditing/UserDetail.cs index f613065eee..75e1462ec3 100644 --- a/src/Umbraco.Web/Models/ContentEditing/UserDetail.cs +++ b/src/Umbraco.Web/Models/ContentEditing/UserDetail.cs @@ -27,10 +27,14 @@ namespace Umbraco.Web.Models.ContentEditing public string EmailHash { get; set; } [Obsolete("This should not be used it exists for legacy reasons only, use user groups instead, it will be removed in future versions")] - [EditorBrowsable(EditorBrowsableState.Never)] + [EditorBrowsable(EditorBrowsableState.Never)] [ReadOnly(true)] [DataMember(Name = "userType")] - public string UserType { get; set; } + public string UserType { get; set; } + + [ReadOnly(true)] + [DataMember(Name = "userGroups")] + public string[] UserGroups { get; set; } /// /// Gets/sets the number of seconds for the user's auth ticket to expire diff --git a/src/Umbraco.Web/Models/Mapping/UserModelMapper.cs b/src/Umbraco.Web/Models/Mapping/UserModelMapper.cs index bd25c5da07..7f4fb222ca 100644 --- a/src/Umbraco.Web/Models/Mapping/UserModelMapper.cs +++ b/src/Umbraco.Web/Models/Mapping/UserModelMapper.cs @@ -321,12 +321,15 @@ namespace Umbraco.Web.Models.Mapping detail => detail.EmailHash, opt => opt.MapFrom(user => user.Email.ToLowerInvariant().Trim().GenerateHash())) .ForMember(detail => detail.SecondsUntilTimeout, opt => opt.Ignore()) + .ForMember(detail => detail.UserGroups, opt => opt.Ignore()) .AfterMap((user, detail) => { //we need to map the legacy UserType //the best we can do here is to return the user's first user group as a IUserType object //but we should attempt to return any group that is the built in ones first var groups = user.Groups.ToArray(); + detail.UserGroups = user.Groups.Select(x => x.Alias).ToArray(); + if (groups.Length == 0) { //In backwards compatibility land, a user type cannot be null! so we need to return a fake one.