Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

further optimization of permissions - removed reference to Role to reduce API payload and minimize information disclosure #2660

Merged
merged 1 commit into from
Mar 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions Oqtane.Client/Modules/Controls/PermissionGrid.razor
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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));
}
Expand Down
12 changes: 6 additions & 6 deletions Oqtane.Client/Themes/Controls/Container/ModuleActionsBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,11 @@ private async Task<string> Settings(string url, PageModule pagemodule)
private async Task<string> 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));
}
Expand All @@ -153,13 +153,13 @@ private async Task<string> Publish(string url, PageModule pagemodule)
private async Task<string> 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);
Expand Down
4 changes: 2 additions & 2 deletions Oqtane.Client/Themes/Controls/Theme/ControlPanel.razor
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
2 changes: 1 addition & 1 deletion Oqtane.Server/Repository/ModuleDefinitionRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ private List<Permission> ClonePermissions(int siteId, List<Permission> 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);
Expand Down
20 changes: 12 additions & 8 deletions Oqtane.Server/Repository/PermissionRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,17 @@ public IEnumerable<Permission> 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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand Down
41 changes: 22 additions & 19 deletions Oqtane.Shared/Models/Permission.cs
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
using System.ComponentModel.DataAnnotations.Schema;
using System.Text.Json.Serialization;
using System;

namespace Oqtane.Models
{
/// <summary>
Expand All @@ -17,46 +21,40 @@ public class Permission : ModelBase
public int SiteId { get; set; }

/// <summary>
/// Name of the Entity these permissions apply to.
/// Name of the Entity these permissions apply to (ie. Module )
/// </summary>
public string EntityName { get; set; }

/// <summary>
/// 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)
/// </summary>
public int EntityId { get; set; }

/// <summary>
/// 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)
/// </summary>
public string PermissionName { get; set; }

/// <summary>
/// <see cref="Role"/> 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 cref="User"/> (see <see cref="UserId"/>).
/// <see cref="Role"/> this permission applies to. If null then this is a <see cref="User"/> permission.
/// </summary>
public int? RoleId { get; set; }


/// <summary>
/// <see cref="User"/> this permission applies to.
/// If null, then the permission doesn't target a User but probably a <see cref="Role"/> (see <see cref="RoleId"/>).
/// The role name associated to the RoleId.
/// </summary>
public int? UserId { get; set; }
[NotMapped]
public string RoleName { get; set; }

/// <summary>
/// Determines if Authorization is sufficient to receive this permission.
/// <see cref="User"/> this permission applies to. If null then this is a <see cref="Role"/> permission.
/// </summary>
public bool IsAuthorized { get; set; }
public int? UserId { get; set; }

/// <summary>
/// Reference to the <see cref="Role"/> based on the <see cref="RoleId"/> - can be nullable.
/// The type of permission (ie. grant = true, deny = false)
/// </summary>
/// <remarks>
/// It's not certain if this will always be populated. TODO: todoc/verify
/// </remarks>
public Role Role { get; set; }
public bool IsAuthorized { get; set; }

public Permission()
{
Expand Down Expand Up @@ -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; }
}
}
18 changes: 9 additions & 9 deletions Oqtane.Shared/Security/UserSecurity.cs
Original file line number Diff line number Diff line change
Expand Up @@ -51,20 +51,20 @@ private static bool IsAuthorized(int userId, string roles, List<Permission> 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();
}
}
Expand All @@ -74,7 +74,7 @@ private static bool IsAuthorized(int userId, string roles, List<Permission> perm

public static bool ContainsRole(List<Permission> 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<Permission> permissions, string permissionName, int userId)
Expand Down