From aed260544dba5865b148758665f4d761ec2ac6b1 Mon Sep 17 00:00:00 2001 From: Matteo Piovanelli Date: Fri, 8 Apr 2022 08:45:56 +0200 Subject: [PATCH] Fixes #8550 (#8551) * Added permissions allowing management of users belonging to specific roles only * Added condition to manage superuser * Fixed the case where an user with no special ManageUserByRole Permission would end up being able to manage a user with no role, because of the behavior of Enumerable.All on empty collections. --- .../Orchard.Roles/Orchard.Roles.csproj | 2 + .../Security/ManageUserByRolePermissions.cs | 64 +++++++++ .../ManageUserByRoleSecurityEventHandler.cs | 128 ++++++++++++++++++ .../Controllers/AdminController.cs | 31 +++++ 4 files changed, 225 insertions(+) create mode 100644 src/Orchard.Web/Modules/Orchard.Roles/Security/ManageUserByRolePermissions.cs create mode 100644 src/Orchard.Web/Modules/Orchard.Roles/Security/ManageUserByRoleSecurityEventHandler.cs diff --git a/src/Orchard.Web/Modules/Orchard.Roles/Orchard.Roles.csproj b/src/Orchard.Web/Modules/Orchard.Roles/Orchard.Roles.csproj index 3a3a84185..f20e33f41 100644 --- a/src/Orchard.Web/Modules/Orchard.Roles/Orchard.Roles.csproj +++ b/src/Orchard.Web/Modules/Orchard.Roles/Orchard.Roles.csproj @@ -137,6 +137,8 @@ + + diff --git a/src/Orchard.Web/Modules/Orchard.Roles/Security/ManageUserByRolePermissions.cs b/src/Orchard.Web/Modules/Orchard.Roles/Security/ManageUserByRolePermissions.cs new file mode 100644 index 000000000..eff59ce9e --- /dev/null +++ b/src/Orchard.Web/Modules/Orchard.Roles/Security/ManageUserByRolePermissions.cs @@ -0,0 +1,64 @@ +using System.Collections.Generic; +using System.Linq; +using Orchard.Data; +using Orchard.Environment.Extensions.Models; +using Orchard.Roles.Constants; +using Orchard.Roles.Models; +using Orchard.Security.Permissions; +using UserPermissions = Orchard.Users.Permissions; + +namespace Orchard.Roles.Security { + public class ManageUserByRolePermissions : IPermissionProvider { + private readonly IRepository _roleRepository; + + public virtual Feature Feature { get; set; } + + private static readonly Permission ManageUsersInRoleTemplate = + new Permission { + Description = "Manage Users in Role - {0}", + Name = "ManageUsersInRole_{0}", + ImpliedBy = new[] { UserPermissions.ManageUsers } + }; + + public ManageUserByRolePermissions( + // A dependency on IRoleService to get the list of roles would lead to a + // circular dependency, because that service has methods to handle the + // permissions for each specific role. + IRepository roleRepository) { + + _roleRepository = roleRepository; + } + + public static Permission CreatePermissionForManageUsersInRole(string roleName) { + return new Permission { + Description = string.Format(ManageUsersInRoleTemplate.Description, roleName), + Name = string.Format(ManageUsersInRoleTemplate.Name, roleName), + ImpliedBy = ManageUsersInRoleTemplate.ImpliedBy + }; + } + + + private IEnumerable GetManageUsersInRolePermissions() { + var allRoleNames = _roleRepository.Table + .Select(r => r.Name) + .ToList() + // Never have to manage explicitly Anonymous or Authenticated roles + .Except(SystemRoles.GetSystemRoles()); + foreach (var roleName in allRoleNames) { + yield return CreatePermissionForManageUsersInRole(roleName); + } + } + + public IEnumerable GetPermissions() { + foreach (var permission in GetManageUsersInRolePermissions()) { + yield return permission; + } + yield break; + } + + public IEnumerable GetDefaultStereotypes() { + return Enumerable.Empty(); + } + + } +} \ No newline at end of file diff --git a/src/Orchard.Web/Modules/Orchard.Roles/Security/ManageUserByRoleSecurityEventHandler.cs b/src/Orchard.Web/Modules/Orchard.Roles/Security/ManageUserByRoleSecurityEventHandler.cs new file mode 100644 index 000000000..9c0adf054 --- /dev/null +++ b/src/Orchard.Web/Modules/Orchard.Roles/Security/ManageUserByRoleSecurityEventHandler.cs @@ -0,0 +1,128 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using Orchard.ContentManagement; +using Orchard.Roles.Constants; +using Orchard.Roles.Models; +using Orchard.Roles.Services; +using Orchard.Security; +using UserPermissions = Orchard.Users.Permissions; + +namespace Orchard.Roles.Security { + public class ManageUserByRoleSecurityEventHandler : IAuthorizationServiceEventHandler { + + private readonly IWorkContextAccessor _workContextAccessor; + private readonly IRoleService _roleService; + + private Lazy _authorizationService; + + private string _superUserName; + public ManageUserByRoleSecurityEventHandler( + IWorkContextAccessor workContextAccessor, + IRoleService roleService) { + + _workContextAccessor = workContextAccessor; + _roleService = roleService; + + _superUserName = _workContextAccessor.GetContext().CurrentSite.SuperUser; + _authorizationService = new Lazy(() => + _workContextAccessor.GetContext().Resolve()); + _allRoleNames = _roleService + .GetRoles() + .Select(r => r.Name) + .Except(SystemRoles.GetSystemRoles()); + } + + // memorize this to avoid fetching this information potentially several times per request + private IEnumerable _allRoleNames; + + public void Adjust(CheckAccessContext context) { + if (!context.Granted + && context.Permission == UserPermissions.ManageUsers) { + // check that the user that is being managed is in the roles that + // the current user is allowed to manage. + var manager = context.User; + var managed = context.Content.As(); + if (manager != null) { + + if (managed == null) { + // Not checking permission to manage a specific user + // Any "manage" permission is probably fine? + var rolesToCheck = _allRoleNames; + if (GrantPermission(rolesToCheck, manager, null)) { + context.Granted = true; + context.Adjusted = true; + } + } else { + // We prevent Manage permissions on specific roles to affect the SuperUser. Only users + // that actually have the full ManageUsers permissions will be able to manage them. + if(!IsSuperUser(managed)) { + // Checking permission to manage a specific user + // The user we are attempting to manage must belong to to a subset of + // all those roles. + var theirRoleNames = managed + .GetRuntimeUserRoles() + // Never have to manage explicitly Anonymous or Authenticated roles + .Except(SystemRoles.GetSystemRoles()); + + if (GrantPermission(theirRoleNames, manager, managed)) { + context.Granted = true; + context.Adjusted = true; + } + } + } + } + } + } + + private bool GrantPermission( + IEnumerable roleNamesToCheck, + IUser manager, IUser managed) { + + if (_authorizationService.Value != null) { + if (managed == null) { + // not checking on a specific user, so permission on any role is fine + return roleNamesToCheck.Any(rn => + _authorizationService.Value.TryCheckAccess( + ManageUserByRolePermissions.CreatePermissionForManageUsersInRole(rn), + manager, managed)); + } else { + // checking permissions on a specific user, so we need to have permissions + // to manage all their roles + if (roleNamesToCheck.Any()) { + return roleNamesToCheck.All(rn => + _authorizationService.Value.TryCheckAccess( + ManageUserByRolePermissions.CreatePermissionForManageUsersInRole(rn), + manager, managed)); + } else { + // if the specific user has no assigned role, they are just an "Authenticated" user. + // Enumerable.All applied to that would return true, which may not be correct. We + // only wish to return true if the user has any of the ManageUserByRole Permission. + // To verify that, we test across all possible roles. + return _allRoleNames + .Any(rn => + _authorizationService.Value.TryCheckAccess( + ManageUserByRolePermissions.CreatePermissionForManageUsersInRole(rn), + manager, managed)); + } + } + } + // if we can't test, fail the test + return false; + } + + private bool IsSuperUser(IUser user) { + + var isSuperUser = string.Equals(user.UserName, _superUserName); + // We could be testing the SiteOwner permission as well but: + // - user can only have that permission if they belong to a Role the Permission is assigned to. + // - if we can manage that role, then there's no reason why we should prevent it from being + // managed here. + return isSuperUser; + } + + public void Checking(CheckAccessContext context) { } + + public void Complete(CheckAccessContext context) { } + } +} \ No newline at end of file diff --git a/src/Orchard.Web/Modules/Orchard.Users/Controllers/AdminController.cs b/src/Orchard.Web/Modules/Orchard.Users/Controllers/AdminController.cs index ff795a8b5..9c2ab60cf 100644 --- a/src/Orchard.Web/Modules/Orchard.Users/Controllers/AdminController.cs +++ b/src/Orchard.Web/Modules/Orchard.Users/Controllers/AdminController.cs @@ -231,6 +231,7 @@ namespace Orchard.Users.Controllers { } public ActionResult Edit(int id) { + // check manage permission on any user if (!Services.Authorizer.Authorize(Permissions.ManageUsers, T("Not authorized to manage users"))) return new HttpUnauthorizedResult(); @@ -239,6 +240,11 @@ namespace Orchard.Users.Controllers { if (user == null) return HttpNotFound(); + // check manage permission on specific user + if (!Services.Authorizer.Authorize(Permissions.ManageUsers, + user, T("Not authorized to manage users"))) + return new HttpUnauthorizedResult(); + var editor = Shape.EditorTemplate(TemplateName: "Parts/User.Edit", Model: new UserEditViewModel {User = user}, Prefix: null); editor.Metadata.Position = "2"; var model = Services.ContentManager.BuildEditor(user); @@ -257,6 +263,11 @@ namespace Orchard.Users.Controllers { if (user == null) return HttpNotFound(); + // check manage permission on specific user + if (!Services.Authorizer.Authorize(Permissions.ManageUsers, + user, T("Not authorized to manage users"))) + return new HttpUnauthorizedResult(); + string previousName = user.UserName; var model = Services.ContentManager.UpdateEditor(user, this); @@ -306,6 +317,11 @@ namespace Orchard.Users.Controllers { if (user == null) return HttpNotFound(); + // check manage permission on specific user + if (!Services.Authorizer.Authorize(Permissions.ManageUsers, + user, T("Not authorized to manage users"))) + return new HttpUnauthorizedResult(); + if (string.Equals(Services.WorkContext.CurrentSite.SuperUser, user.UserName, StringComparison.Ordinal)) { Services.Notifier.Error(T("The Super user can't be removed. Please disable this account or specify another Super user account.")); } @@ -330,6 +346,11 @@ namespace Orchard.Users.Controllers { if (user == null) return HttpNotFound(); + // check manage permission on specific user + if (!Services.Authorizer.Authorize(Permissions.ManageUsers, + user, T("Not authorized to manage users"))) + return new HttpUnauthorizedResult(); + var siteUrl = Services.WorkContext.CurrentSite.BaseUrl; if (string.IsNullOrWhiteSpace(siteUrl)) { @@ -352,6 +373,11 @@ namespace Orchard.Users.Controllers { if (user == null) return HttpNotFound(); + // check manage permission on specific user + if (!Services.Authorizer.Authorize(Permissions.ManageUsers, + user, T("Not authorized to manage users"))) + return new HttpUnauthorizedResult(); + user.As().RegistrationStatus = UserStatus.Approved; Services.Notifier.Success(T("User {0} approved", user.UserName)); _userEventHandlers.Approved(user); @@ -369,6 +395,11 @@ namespace Orchard.Users.Controllers { if (user == null) return HttpNotFound(); + // check manage permission on specific user + if (!Services.Authorizer.Authorize(Permissions.ManageUsers, + user, T("Not authorized to manage users"))) + return new HttpUnauthorizedResult(); + if (string.Equals(Services.WorkContext.CurrentUser.UserName, user.UserName, StringComparison.Ordinal)) { Services.Notifier.Error(T("You can't disable your own account. Please log in with another account")); }