From ef4e99b3a75f015d451f30d281053eee5619d93c Mon Sep 17 00:00:00 2001 From: Shaun Walker Date: Fri, 10 Mar 2023 08:28:37 -0500 Subject: [PATCH] further optimization of permissions - removed reference to Role to reduce API payload and minimize information disclosure --- .../Modules/Controls/PermissionGrid.razor | 12 +++--- .../Controls/Container/ModuleActionsBase.cs | 12 +++--- .../Themes/Controls/Theme/ControlPanel.razor | 4 +- .../Repository/ModuleDefinitionRepository.cs | 2 +- .../Repository/PermissionRepository.cs | 20 +++++---- Oqtane.Shared/Models/Permission.cs | 41 ++++++++++--------- Oqtane.Shared/Security/UserSecurity.cs | 18 ++++---- 7 files changed, 58 insertions(+), 51 deletions(-) diff --git a/Oqtane.Client/Modules/Controls/PermissionGrid.razor b/Oqtane.Client/Modules/Controls/PermissionGrid.razor index e545658a6..87e2e7f6e 100644 --- a/Oqtane.Client/Modules/Controls/PermissionGrid.razor +++ b/Oqtane.Client/Modules/Controls/PermissionGrid.razor @@ -167,7 +167,7 @@ _permissions.Add(new Permission(ModuleState.SiteId, segments[0], segments[1], role, null, true)); } // ensure admin access - if (!_permissions.Any(item => item.EntityName == segments[0] && item.PermissionName == segments[1] && item.Role.Name == RoleNames.Admin)) + if (!_permissions.Any(item => item.EntityName == segments[0] && item.PermissionName == segments[1] && item.RoleName == RoleNames.Admin)) { _permissions.Add(new Permission(ModuleState.SiteId, segments[0], segments[1], RoleNames.Admin, null, true)); } @@ -203,7 +203,7 @@ bool? isauthorized = null; if (roleName != "") { - var permission = _permissions.FirstOrDefault(item => item.EntityName == GetEntityName(permissionName) && item.PermissionName == GetPermissionName(permissionName) && item.Role.Name == roleName); + var permission = _permissions.FirstOrDefault(item => item.EntityName == GetEntityName(permissionName) && item.PermissionName == GetPermissionName(permissionName) && item.RoleName == roleName); if (permission != null) { isauthorized = permission.IsAuthorized; @@ -243,7 +243,7 @@ { if (roleName != "") { - var permission = _permissions.FirstOrDefault(item => item.EntityName == GetEntityName(permissionName) && item.PermissionName == GetPermissionName(permissionName) && item.Role.Name == roleName); + var permission = _permissions.FirstOrDefault(item => item.EntityName == GetEntityName(permissionName) && item.PermissionName == GetPermissionName(permissionName) && item.RoleName == roleName); if (permission != null) { _permissions.Remove(permission); @@ -307,7 +307,7 @@ { // remove deny all users, unauthenticated, and registered users var permissions = _permissions.Where(item => !item.IsAuthorized && - (item.Role.Name == RoleNames.Everyone || item.Role.Name == RoleNames.Unauthenticated || item.Role.Name == RoleNames.Registered)).ToList(); + (item.RoleName == RoleNames.Everyone || item.RoleName == RoleNames.Unauthenticated || item.RoleName == RoleNames.Registered)).ToList(); foreach (var permission in permissions) { _permissions.Remove(permission); @@ -316,7 +316,7 @@ { // remove deny administrators and host users permissions = _permissions.Where(item => !item.IsAuthorized && - (item.Role.Name == RoleNames.Admin || item.Role.Name == RoleNames.Host)).ToList(); + (item.RoleName == RoleNames.Admin || item.RoleName == RoleNames.Host)).ToList(); foreach (var permission in permissions) { _permissions.Remove(permission); @@ -325,7 +325,7 @@ { // add administrators role if neither host or administrator is assigned if (!_permissions.Any(item => item.EntityName == GetEntityName(permissionname) && item.PermissionName == GetPermissionName(permissionname) && - (item.Role.Name == RoleNames.Admin || item.Role.Name == RoleNames.Host))) + (item.RoleName == RoleNames.Admin || item.RoleName == RoleNames.Host))) { _permissions.Add(new Permission(ModuleState.SiteId, GetEntityName(permissionname), GetPermissionName(permissionname), RoleNames.Admin, null, true)); } diff --git a/Oqtane.Client/Themes/Controls/Container/ModuleActionsBase.cs b/Oqtane.Client/Themes/Controls/Container/ModuleActionsBase.cs index ae9223629..eb452b6bc 100644 --- a/Oqtane.Client/Themes/Controls/Container/ModuleActionsBase.cs +++ b/Oqtane.Client/Themes/Controls/Container/ModuleActionsBase.cs @@ -137,11 +137,11 @@ private async Task Settings(string url, PageModule pagemodule) private async Task Publish(string url, PageModule pagemodule) { var permissions = pagemodule.Module.PermissionList; - if (!permissions.Any(item => item.PermissionName == PermissionNames.View && item.Role.Name == RoleNames.Everyone)) + if (!permissions.Any(item => item.PermissionName == PermissionNames.View && item.RoleName == RoleNames.Everyone)) { permissions.Add(new Permission(ModuleState.SiteId, EntityNames.Page, pagemodule.PageId, PermissionNames.View, RoleNames.Everyone, null, true)); } - if (!permissions.Any(item => item.PermissionName == PermissionNames.View && item.Role.Name == RoleNames.Registered)) + if (!permissions.Any(item => item.PermissionName == PermissionNames.View && item.RoleName == RoleNames.Registered)) { permissions.Add(new Permission(ModuleState.SiteId, EntityNames.Page, pagemodule.PageId, PermissionNames.View, RoleNames.Registered, null, true)); } @@ -153,13 +153,13 @@ private async Task Publish(string url, PageModule pagemodule) private async Task Unpublish(string url, PageModule pagemodule) { var permissions = pagemodule.Module.PermissionList; - if (permissions.Any(item => item.PermissionName == PermissionNames.View && item.Role.Name == RoleNames.Everyone)) + if (permissions.Any(item => item.PermissionName == PermissionNames.View && item.RoleName == RoleNames.Everyone)) { - permissions.Remove(permissions.First(item => item.PermissionName == PermissionNames.View && item.Role.Name == RoleNames.Everyone)); + permissions.Remove(permissions.First(item => item.PermissionName == PermissionNames.View && item.RoleName == RoleNames.Everyone)); } - if (permissions.Any(item => item.PermissionName == PermissionNames.View && item.Role.Name == RoleNames.Registered)) + if (permissions.Any(item => item.PermissionName == PermissionNames.View && item.RoleName == RoleNames.Registered)) { - permissions.Remove(permissions.First(item => item.PermissionName == PermissionNames.View && item.Role.Name == RoleNames.Registered)); + permissions.Remove(permissions.First(item => item.PermissionName == PermissionNames.View && item.RoleName == RoleNames.Registered)); } pagemodule.Module.PermissionList = permissions; await ModuleService.UpdateModuleAsync(pagemodule.Module); diff --git a/Oqtane.Client/Themes/Controls/Theme/ControlPanel.razor b/Oqtane.Client/Themes/Controls/Theme/ControlPanel.razor index e2e269da0..c7601682e 100644 --- a/Oqtane.Client/Themes/Controls/Theme/ControlPanel.razor +++ b/Oqtane.Client/Themes/Controls/Theme/ControlPanel.razor @@ -537,11 +537,11 @@ if (UserSecurity.IsAuthorized(PageState.User, PermissionNames.Edit, PageState.Page.PermissionList)) { var permissions = PageState.Page.PermissionList; - if (!permissions.Any(item => item.PermissionName == PermissionNames.View && item.Role.Name == RoleNames.Everyone)) + if (!permissions.Any(item => item.PermissionName == PermissionNames.View && item.RoleName == RoleNames.Everyone)) { permissions.Add(new Permission(PageState.Site.SiteId, EntityNames.Page, PageState.Page.PageId, PermissionNames.View, RoleNames.Everyone, null, true)); } - if (!permissions.Any(item => item.PermissionName == PermissionNames.View && item.Role.Name == RoleNames.Registered)) + if (!permissions.Any(item => item.PermissionName == PermissionNames.View && item.RoleName == RoleNames.Registered)) { permissions.Add(new Permission(PageState.Site.SiteId, EntityNames.Page, PageState.Page.PageId, PermissionNames.View, RoleNames.Registered, null, true)); } diff --git a/Oqtane.Server/Repository/ModuleDefinitionRepository.cs b/Oqtane.Server/Repository/ModuleDefinitionRepository.cs index 55a93e2f3..f003cc106 100644 --- a/Oqtane.Server/Repository/ModuleDefinitionRepository.cs +++ b/Oqtane.Server/Repository/ModuleDefinitionRepository.cs @@ -300,7 +300,7 @@ private List ClonePermissions(int siteId, List permissio permission.EntityId = p.EntityId; permission.PermissionName = p.PermissionName; permission.RoleId = p.RoleId; - permission.Role = new Role { Name = p.Role.Name }; + permission.RoleName = p.RoleName; permission.UserId = p.UserId; permission.IsAuthorized = p.IsAuthorized; permissions.Add(permission); diff --git a/Oqtane.Server/Repository/PermissionRepository.cs b/Oqtane.Server/Repository/PermissionRepository.cs index cfc82ed44..8ec070f25 100644 --- a/Oqtane.Server/Repository/PermissionRepository.cs +++ b/Oqtane.Server/Repository/PermissionRepository.cs @@ -30,10 +30,17 @@ public IEnumerable GetPermissions(int siteId, string entityName) { return _cache.GetOrCreate($"permissions:{alias.TenantId}:{siteId}:{entityName}", entry => { + var roles = _roles.GetRoles(siteId, true).ToList(); + var permissions = _db.Permission.Where(item => item.SiteId == siteId).Where(item => item.EntityName == entityName).ToList(); + foreach (var permission in permissions) + { + if (permission.RoleId != null) + { + permission.RoleName = roles.Find(item => item.RoleId == permission.RoleId).Name; + } + } entry.SlidingExpiration = TimeSpan.FromMinutes(30); - return _db.Permission.Where(item => item.SiteId == siteId) - .Where(item => item.EntityName == entityName) - .Include(item => item.Role).ToList(); // eager load roles + return permissions; }); } return null; @@ -84,15 +91,14 @@ public void UpdatePermissions(int siteId, string entityName, int entityId, List< permission.SiteId = siteId; permission.EntityName = (string.IsNullOrEmpty(permission.EntityName)) ? entityName : permission.EntityName; permission.EntityId = (permission.EntityName == entityName) ? entityId : -1; - if (permission.RoleId == null && permission.Role != null && !string.IsNullOrEmpty(permission.Role.Name)) + if (permission.UserId == null && permission.RoleId == null && !string.IsNullOrEmpty(permission.RoleName)) { - var role = roles.FirstOrDefault(item => item.Name == permission.Role.Name); + var role = roles.FirstOrDefault(item => item.Name == permission.RoleName); if (role != null) { permission.RoleId = role.RoleId; } } - permission.Role = null; } // add or update permissions bool modified = false; @@ -112,7 +118,6 @@ public void UpdatePermissions(int siteId, string entityName, int entityId, List< if (current.IsAuthorized != permission.IsAuthorized) { current.IsAuthorized = permission.IsAuthorized; - current.Role = null; // remove linked reference to Role which can cause errors in EF Core change tracking _db.Entry(current).State = EntityState.Modified; modified = true; } @@ -129,7 +134,6 @@ public void UpdatePermissions(int siteId, string entityName, int entityId, List< if (!permissions.Any(item => item.EntityName == permission.EntityName && item.PermissionName == permission.PermissionName && item.EntityId == permission.EntityId && item.RoleId == permission.RoleId && item.UserId == permission.UserId)) { - permission.Role = null; // remove linked reference to Role which can cause errors in EF Core change tracking _db.Permission.Remove(permission); modified = true; } diff --git a/Oqtane.Shared/Models/Permission.cs b/Oqtane.Shared/Models/Permission.cs index bdaf49a09..8d4112521 100644 --- a/Oqtane.Shared/Models/Permission.cs +++ b/Oqtane.Shared/Models/Permission.cs @@ -1,3 +1,7 @@ +using System.ComponentModel.DataAnnotations.Schema; +using System.Text.Json.Serialization; +using System; + namespace Oqtane.Models { /// @@ -17,46 +21,40 @@ public class Permission : ModelBase public int SiteId { get; set; } /// - /// Name of the Entity these permissions apply to. + /// Name of the Entity these permissions apply to (ie. Module ) /// public string EntityName { get; set; } /// - /// ID of the Entity these permissions apply to. + /// ID of the Entity these permissions apply to (ie. a ModuleId). A value of -1 indicates the permission applies to all EntityNames regardless of ID (ie. API permissions) /// public int EntityId { get; set; } /// - /// What this permission is called. - /// TODO: todoc - must clarify what exactly this means, I assume any module can give it's own names for Permissions + /// Name of the permission (ie. View) /// public string PermissionName { get; set; } /// - /// this permission applies to. So if all users in the Role _Customers_ have this permission, then it would reference that Role. - /// If null, then the permission doesn't target a role but probably a (see ). + /// this permission applies to. If null then this is a permission. /// public int? RoleId { get; set; } - /// - /// this permission applies to. - /// If null, then the permission doesn't target a User but probably a (see ). + /// The role name associated to the RoleId. /// - public int? UserId { get; set; } + [NotMapped] + public string RoleName { get; set; } /// - /// Determines if Authorization is sufficient to receive this permission. + /// this permission applies to. If null then this is a permission. /// - public bool IsAuthorized { get; set; } + public int? UserId { get; set; } /// - /// Reference to the based on the - can be nullable. + /// The type of permission (ie. grant = true, deny = false) /// - /// - /// It's not certain if this will always be populated. TODO: todoc/verify - /// - public Role Role { get; set; } + public bool IsAuthorized { get; set; } public Permission() { @@ -90,17 +88,22 @@ private void Initialize(int siteId, string entityName, int entityId, string perm PermissionName = permissionName; if (!string.IsNullOrEmpty(roleName)) { - Role = new Role { Name = roleName }; RoleId = null; + RoleName = roleName; UserId = null; } else { - Role = null; RoleId = null; + RoleName = null; UserId = userId; } IsAuthorized = isAuthorized; } + + [Obsolete("The Role property is deprecated", false)] + [NotMapped] + [JsonIgnore] // exclude from API payload + public Role Role { get; set; } } } diff --git a/Oqtane.Shared/Security/UserSecurity.cs b/Oqtane.Shared/Security/UserSecurity.cs index 06e88c620..427503d78 100644 --- a/Oqtane.Shared/Security/UserSecurity.cs +++ b/Oqtane.Shared/Security/UserSecurity.cs @@ -51,20 +51,20 @@ private static bool IsAuthorized(int userId, string roles, List perm { // check if denied first isAuthorized = !permissionList.Where(item => !item.IsAuthorized && ( - (item.Role != null && ( - (item.Role.Name == RoleNames.Everyone) || - (item.Role.Name == RoleNames.Unauthenticated && userId == -1) || - roles.Split(';', StringSplitOptions.RemoveEmptyEntries).Contains(item.Role.Name))) || + (item.UserId == null && ( + (item.RoleName == RoleNames.Everyone) || + (item.RoleName == RoleNames.Unauthenticated && userId == -1) || + roles.Split(';', StringSplitOptions.RemoveEmptyEntries).Contains(item.RoleName))) || (item.UserId != null && item.UserId.Value == userId))).Any(); if (isAuthorized) { // then check if authorized isAuthorized = permissionList.Where(item => item.IsAuthorized && ( - (item.Role != null && ( - (item.Role.Name == RoleNames.Everyone) || - (item.Role.Name == RoleNames.Unauthenticated && userId == -1) || - roles.Split(';', StringSplitOptions.RemoveEmptyEntries).Contains(item.Role.Name))) || + (item.UserId == null && ( + (item.RoleName == RoleNames.Everyone) || + (item.RoleName == RoleNames.Unauthenticated && userId == -1) || + roles.Split(';', StringSplitOptions.RemoveEmptyEntries).Contains(item.RoleName))) || (item.UserId != null && item.UserId.Value == userId))).Any(); } } @@ -74,7 +74,7 @@ private static bool IsAuthorized(int userId, string roles, List perm public static bool ContainsRole(List permissions, string permissionName, string roleName) { - return permissions.Any(item => item.PermissionName == permissionName && item.Role.Name == roleName); + return permissions.Any(item => item.PermissionName == permissionName && item.RoleName == roleName); } public static bool ContainsUser(List permissions, string permissionName, int userId)