♻️ Refactored permission service

2025-11-08 21:03:03 +08:00
parent 637cc0cfa4
commit 0123c74ab8
3 changed files with 447 additions and 80 deletions
--- a/DysonNetwork.Pass/Permission/PermissionMiddleware.cs
+++ b/DysonNetwork.Pass/Permission/PermissionMiddleware.cs
@@ -1,6 +1,7 @@
 namespace DysonNetwork.Pass.Permission;

 using System;
+using Microsoft.Extensions.Logging;
 using DysonNetwork.Shared.Models;

 [AttributeUsage(AttributeTargets.Method)]
@@ -10,8 +11,11 @@ public class RequiredPermissionAttribute(string area, string key) : Attribute
    public string Key { get; } = key;
 }

-public class PermissionMiddleware(RequestDelegate next)
+public class PermissionMiddleware(RequestDelegate next, ILogger<PermissionMiddleware> logger)
 {
+    private const string ForbiddenMessage = "Insufficient permissions";
+    private const string UnauthorizedMessage = "Authentication required";
+
    public async Task InvokeAsync(HttpContext httpContext, PermissionService pm)
    {
        var endpoint = httpContext.GetEndpoint();
@@ -22,27 +26,55 @@ public class PermissionMiddleware(RequestDelegate next)

        if (attr != null)
        {
+            // Validate permission attributes
+            if (string.IsNullOrWhiteSpace(attr.Area) || string.IsNullOrWhiteSpace(attr.Key))
+            {
+                logger.LogWarning("Invalid permission attribute: Area='{Area}', Key='{Key}'", attr.Area, attr.Key);
+                httpContext.Response.StatusCode = StatusCodes.Status500InternalServerError;
+                await httpContext.Response.WriteAsync("Server configuration error");
+                return;
+            }
+
            if (httpContext.Items["CurrentUser"] is not SnAccount currentUser)
            {
-                httpContext.Response.StatusCode = StatusCodes.Status403Forbidden;
-                await httpContext.Response.WriteAsync("Unauthorized");
+                logger.LogWarning("Permission check failed: No authenticated user for {Area}/{Key}", attr.Area, attr.Key);
+                httpContext.Response.StatusCode = StatusCodes.Status401Unauthorized;
+                await httpContext.Response.WriteAsync(UnauthorizedMessage);
                return;
            }

            if (currentUser.IsSuperuser)
            {
                // Bypass the permission check for performance
+                logger.LogDebug("Superuser {UserId} bypassing permission check for {Area}/{Key}",
+                    currentUser.Id, attr.Area, attr.Key);
                await next(httpContext);
                return;
            }

            var actor = $"user:{currentUser.Id}";
+            try
+            {
                var permNode = await pm.GetPermissionAsync<bool>(actor, attr.Area, attr.Key);

                if (!permNode)
                {
+                    logger.LogWarning("Permission denied for user {UserId}: {Area}/{Key}",
+                        currentUser.Id, attr.Area, attr.Key);
                    httpContext.Response.StatusCode = StatusCodes.Status403Forbidden;
-                await httpContext.Response.WriteAsync($"Permission {attr.Area}/{attr.Key} = {true} was required.");
+                    await httpContext.Response.WriteAsync(ForbiddenMessage);
+                    return;
+                }
+
+                logger.LogDebug("Permission granted for user {UserId}: {Area}/{Key}",
+                    currentUser.Id, attr.Area, attr.Key);
+            }
+            catch (Exception ex)
+            {
+                logger.LogError(ex, "Error checking permission for user {UserId}: {Area}/{Key}",
+                    currentUser.Id, attr.Area, attr.Key);
+                httpContext.Response.StatusCode = StatusCodes.Status500InternalServerError;
+                await httpContext.Response.WriteAsync("Permission check failed");
                return;
            }
        }
--- a/DysonNetwork.Pass/Permission/PermissionService.cs
+++ b/DysonNetwork.Pass/Permission/PermissionService.cs
@@ -1,4 +1,6 @@
 using Microsoft.EntityFrameworkCore;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Options;
 using NodaTime;
 using System.Text.Json;
 using DysonNetwork.Shared.Cache;
@@ -6,24 +8,33 @@ using DysonNetwork.Shared.Models;

 namespace DysonNetwork.Pass.Permission;

+public class PermissionServiceOptions
+{
+    public TimeSpan CacheExpiration { get; set; } = TimeSpan.FromMinutes(1);
+    public bool EnableWildcardMatching { get; set; } = true;
+    public int MaxWildcardMatches { get; set; } = 100;
+}
+
 public class PermissionService(
    AppDatabase db,
-    ICacheService cache
+    ICacheService cache,
+    ILogger<PermissionService> logger,
+    IOptions<PermissionServiceOptions> options
 )
 {
-    private static readonly TimeSpan CacheExpiration = TimeSpan.FromMinutes(1);
+    private readonly PermissionServiceOptions _options = options.Value;

-    private const string PermCacheKeyPrefix = "perm:";
-    private const string PermGroupCacheKeyPrefix = "perm-cg:";
+    private const string PermissionCacheKeyPrefix = "perm:";
+    private const string PermissionGroupCacheKeyPrefix = "perm-cg:";
    private const string PermissionGroupPrefix = "perm-g:";

-    private static string _GetPermissionCacheKey(string actor, string area, string key) =>
-        PermCacheKeyPrefix + actor + ":" + area + ":" + key;
+    private static string GetPermissionCacheKey(string actor, string area, string key) =>
+        PermissionCacheKeyPrefix + actor + ":" + area + ":" + key;

-    private static string _GetGroupsCacheKey(string actor) =>
-        PermGroupCacheKeyPrefix + actor;
+    private static string GetGroupsCacheKey(string actor) =>
+        PermissionGroupCacheKeyPrefix + actor;

-    private static string _GetPermissionGroupKey(string actor) =>
+    private static string GetPermissionGroupKey(string actor) =>
        PermissionGroupPrefix + actor;

    public async Task<bool> HasPermissionAsync(string actor, string area, string key)
@@ -34,14 +45,49 @@ public class PermissionService(

    public async Task<T?> GetPermissionAsync<T>(string actor, string area, string key)
    {
-        var cacheKey = _GetPermissionCacheKey(actor, area, key);
+        // Input validation
+        if (string.IsNullOrWhiteSpace(actor))
+            throw new ArgumentException("Actor cannot be null or empty", nameof(actor));
+        if (string.IsNullOrWhiteSpace(area))
+            throw new ArgumentException("Area cannot be null or empty", nameof(area));
+        if (string.IsNullOrWhiteSpace(key))
+            throw new ArgumentException("Key cannot be null or empty", nameof(key));

+        var cacheKey = GetPermissionCacheKey(actor, area, key);
+
+        try
+        {
            var (hit, cachedValue) = await cache.GetAsyncWithStatus<T>(cacheKey);
            if (hit)
+            {
+                logger.LogDebug("Permission cache hit for {Actor}:{Area}:{Key}", actor, area, key);
                return cachedValue;
+            }

            var now = SystemClock.Instance.GetCurrentInstant();
-        var groupsKey = _GetGroupsCacheKey(actor);
+            var groupsId = await GetOrCacheUserGroupsAsync(actor, now);
+
+            var permission = await FindPermissionNodeAsync(actor, area, key, groupsId, now);
+            var result = permission != null ? DeserializePermissionValue<T>(permission.Value) : default;
+
+            await cache.SetWithGroupsAsync(cacheKey, result,
+                [GetPermissionGroupKey(actor)],
+                _options.CacheExpiration);
+
+            logger.LogDebug("Permission resolved for {Actor}:{Area}:{Key} = {Result}",
+                actor, area, key, result != null);
+            return result;
+        }
+        catch (Exception ex)
+        {
+            logger.LogError(ex, "Error retrieving permission for {Actor}:{Area}:{Key}", actor, area, key);
+            throw;
+        }
+    }
+
+    private async Task<List<Guid>> GetOrCacheUserGroupsAsync(string actor, Instant now)
+    {
+        var groupsKey = GetGroupsCacheKey(actor);

        var groupsId = await cache.GetAsync<List<Guid>>(groupsKey);
        if (groupsId == null)
@@ -54,11 +100,20 @@ public class PermissionService(
                .ToListAsync();

            await cache.SetWithGroupsAsync(groupsKey, groupsId,
-                [_GetPermissionGroupKey(actor)],
-                CacheExpiration);
+                [GetPermissionGroupKey(actor)],
+                _options.CacheExpiration);
+
+            logger.LogDebug("Cached {Count} groups for actor {Actor}", groupsId.Count, actor);
        }

-        var permission = await db.PermissionNodes
+        return groupsId;
+    }
+
+    private async Task<SnPermissionNode?> FindPermissionNodeAsync(string actor, string area, string key,
+        List<Guid> groupsId, Instant now)
+    {
+        // First try exact match (highest priority)
+        var exactMatch = await db.PermissionNodes
            .Where(n => (n.GroupId == null && n.Actor == actor) ||
                        (n.GroupId != null && groupsId.Contains(n.GroupId.Value)))
            .Where(n => n.Key == key && n.Area == area)
@@ -66,13 +121,85 @@ public class PermissionService(
            .Where(n => n.AffectedAt == null || n.AffectedAt <= now)
            .FirstOrDefaultAsync();

-        var result = permission is not null ? _DeserializePermissionValue<T>(permission.Value) : default;
+        if (exactMatch != null)
+        {
+            return exactMatch;
+        }

-        await cache.SetWithGroupsAsync(cacheKey, result,
-            [_GetPermissionGroupKey(actor)],
-            CacheExpiration);
+        // If no exact match and wildcards are enabled, try wildcard matches
+        if (!_options.EnableWildcardMatching)
+        {
+            return null;
+        }

-        return result;
+        var wildcardMatches = await db.PermissionNodes
+            .Where(n => (n.GroupId == null && n.Actor == actor) ||
+                        (n.GroupId != null && groupsId.Contains(n.GroupId.Value)))
+            .Where(n => (n.Key.Contains("*") || n.Area.Contains("*")))
+            .Where(n => n.ExpiredAt == null || n.ExpiredAt > now)
+            .Where(n => n.AffectedAt == null || n.AffectedAt <= now)
+            .Take(_options.MaxWildcardMatches)
+            .ToListAsync();
+
+        // Find the best wildcard match
+        SnPermissionNode? bestMatch = null;
+        var bestMatchScore = -1;
+
+        foreach (var node in wildcardMatches)
+        {
+            var score = CalculateWildcardMatchScore(node.Area, node.Key, area, key);
+            if (score > bestMatchScore)
+            {
+                bestMatch = node;
+                bestMatchScore = score;
+            }
+        }
+
+        if (bestMatch != null)
+        {
+            logger.LogDebug("Found wildcard permission match: {NodeArea}:{NodeKey} for {Area}:{Key}",
+                bestMatch.Area, bestMatch.Key, area, key);
+        }
+
+        return bestMatch;
+    }
+
+    private static int CalculateWildcardMatchScore(string nodeArea, string nodeKey, string targetArea, string targetKey)
+    {
+        // Calculate how well the wildcard pattern matches
+        // Higher score = better match
+        var areaScore = CalculatePatternMatchScore(nodeArea, targetArea);
+        var keyScore = CalculatePatternMatchScore(nodeKey, targetKey);
+
+        // Perfect match gets highest score
+        if (areaScore == int.MaxValue && keyScore == int.MaxValue)
+            return int.MaxValue;
+
+        // Prefer area matches over key matches, more specific patterns over general ones
+        return (areaScore * 1000) + keyScore;
+    }
+
+    private static int CalculatePatternMatchScore(string pattern, string target)
+    {
+        if (pattern == target)
+            return int.MaxValue; // Exact match
+
+        if (!pattern.Contains("*"))
+            return -1; // No wildcard, not a match
+
+        // Simple wildcard matching: * matches any sequence of characters
+        var regexPattern = "^" + System.Text.RegularExpressions.Regex.Escape(pattern).Replace("\\*", ".*") + "$";
+        var regex = new System.Text.RegularExpressions.Regex(regexPattern, System.Text.RegularExpressions.RegexOptions.IgnoreCase);
+
+        if (regex.IsMatch(target))
+        {
+            // Score based on specificity (shorter patterns are less specific)
+            var wildcardCount = pattern.Count(c => c == '*');
+            var length = pattern.Length;
+            return Math.Max(1, 1000 - (wildcardCount * 100) - length);
+        }
+
+        return -1; // No match
    }

    public async Task<SnPermissionNode> AddPermissionNode<T>(
@@ -91,7 +218,7 @@ public class PermissionService(
            Actor = actor,
            Key = key,
            Area = area,
-            Value = _SerializePermissionValue(value),
+            Value = SerializePermissionValue(value),
            ExpiredAt = expiredAt,
            AffectedAt = affectedAt
        };
@@ -122,7 +249,7 @@ public class PermissionService(
            Actor = actor,
            Key = key,
            Area = area,
-            Value = _SerializePermissionValue(value),
+            Value = SerializePermissionValue(value),
            ExpiredAt = expiredAt,
            AffectedAt = affectedAt,
            Group = group,
@@ -134,8 +261,8 @@ public class PermissionService(

        // Invalidate related caches
        await InvalidatePermissionCacheAsync(actor, area, key);
-        await cache.RemoveAsync(_GetGroupsCacheKey(actor));
-        await cache.RemoveGroupAsync(_GetPermissionGroupKey(actor));
+        await cache.RemoveAsync(GetGroupsCacheKey(actor));
+        await cache.RemoveGroupAsync(GetPermissionGroupKey(actor));

        return node;
    }
@@ -164,22 +291,22 @@ public class PermissionService(

        // Invalidate caches
        await InvalidatePermissionCacheAsync(actor, area, key);
-        await cache.RemoveAsync(_GetGroupsCacheKey(actor));
-        await cache.RemoveGroupAsync(_GetPermissionGroupKey(actor));
+        await cache.RemoveAsync(GetGroupsCacheKey(actor));
+        await cache.RemoveGroupAsync(GetPermissionGroupKey(actor));
    }

    private async Task InvalidatePermissionCacheAsync(string actor, string area, string key)
    {
-        var cacheKey = _GetPermissionCacheKey(actor, area, key);
+        var cacheKey = GetPermissionCacheKey(actor, area, key);
        await cache.RemoveAsync(cacheKey);
    }

-    private static T? _DeserializePermissionValue<T>(JsonDocument json)
+    private static T? DeserializePermissionValue<T>(JsonDocument json)
    {
        return JsonSerializer.Deserialize<T>(json.RootElement.GetRawText());
    }

-    private static JsonDocument _SerializePermissionValue<T>(T obj)
+    private static JsonDocument SerializePermissionValue<T>(T obj)
    {
        var str = JsonSerializer.Serialize(obj);
        return JsonDocument.Parse(str);
@@ -192,7 +319,109 @@ public class PermissionService(
            Actor = actor,
            Area = area,
            Key = key,
-            Value = _SerializePermissionValue(value),
+            Value = SerializePermissionValue(value),
        };
    }
+
+    /// <summary>
+    /// Lists all effective permissions for an actor (including group permissions)
+    /// </summary>
+    public async Task<List<SnPermissionNode>> ListEffectivePermissionsAsync(string actor)
+    {
+        if (string.IsNullOrWhiteSpace(actor))
+            throw new ArgumentException("Actor cannot be null or empty", nameof(actor));
+
+        try
+        {
+            var now = SystemClock.Instance.GetCurrentInstant();
+            var groupsId = await GetOrCacheUserGroupsAsync(actor, now);
+
+            var permissions = await db.PermissionNodes
+                .Where(n => (n.GroupId == null && n.Actor == actor) ||
+                            (n.GroupId != null && groupsId.Contains(n.GroupId.Value)))
+                .Where(n => n.ExpiredAt == null || n.ExpiredAt > now)
+                .Where(n => n.AffectedAt == null || n.AffectedAt <= now)
+                .OrderBy(n => n.Area)
+                .ThenBy(n => n.Key)
+                .ToListAsync();
+
+            logger.LogDebug("Listed {Count} effective permissions for actor {Actor}", permissions.Count, actor);
+            return permissions;
+        }
+        catch (Exception ex)
+        {
+            logger.LogError(ex, "Error listing permissions for actor {Actor}", actor);
+            throw;
+        }
+    }
+
+    /// <summary>
+    /// Lists all direct permissions for an actor (excluding group permissions)
+    /// </summary>
+    public async Task<List<SnPermissionNode>> ListDirectPermissionsAsync(string actor)
+    {
+        if (string.IsNullOrWhiteSpace(actor))
+            throw new ArgumentException("Actor cannot be null or empty", nameof(actor));
+
+        try
+        {
+            var now = SystemClock.Instance.GetCurrentInstant();
+            var permissions = await db.PermissionNodes
+                .Where(n => n.GroupId == null && n.Actor == actor)
+                .Where(n => n.ExpiredAt == null || n.ExpiredAt > now)
+                .Where(n => n.AffectedAt == null || n.AffectedAt <= now)
+                .OrderBy(n => n.Area)
+                .ThenBy(n => n.Key)
+                .ToListAsync();
+
+            logger.LogDebug("Listed {Count} direct permissions for actor {Actor}", permissions.Count, actor);
+            return permissions;
+        }
+        catch (Exception ex)
+        {
+            logger.LogError(ex, "Error listing direct permissions for actor {Actor}", actor);
+            throw;
+        }
+    }
+
+    /// <summary>
+    /// Validates a permission pattern for wildcard usage
+    /// </summary>
+    public static bool IsValidPermissionPattern(string pattern)
+    {
+        if (string.IsNullOrWhiteSpace(pattern))
+            return false;
+
+        // Basic validation: no consecutive wildcards, no leading/trailing wildcards in some cases
+        if (pattern.Contains("**") || pattern.StartsWith("*") || pattern.EndsWith("*"))
+            return false;
+
+        // Check for valid characters (alphanumeric, underscore, dash, dot, star)
+        return pattern.All(c => char.IsLetterOrDigit(c) || c == '_' || c == '-' || c == '.' || c == '*' || c == ':');
+    }
+
+    /// <summary>
+    /// Clears all cached permissions for an actor
+    /// </summary>
+    public async Task ClearActorCacheAsync(string actor)
+    {
+        if (string.IsNullOrWhiteSpace(actor))
+            throw new ArgumentException("Actor cannot be null or empty", nameof(actor));
+
+        try
+        {
+            var groupsKey = GetGroupsCacheKey(actor);
+            var permissionGroupKey = GetPermissionGroupKey(actor);
+
+            await cache.RemoveAsync(groupsKey);
+            await cache.RemoveGroupAsync(permissionGroupKey);
+
+            logger.LogInformation("Cleared cache for actor {Actor}", actor);
+        }
+        catch (Exception ex)
+        {
+            logger.LogError(ex, "Error clearing cache for actor {Actor}", actor);
+            throw;
+        }
+    }
 }
--- a/DysonNetwork.Pass/Permission/PermissionServiceGrpc.cs
+++ b/DysonNetwork.Pass/Permission/PermissionServiceGrpc.cs
@@ -1,6 +1,7 @@
 using Grpc.Core;
 using Microsoft.EntityFrameworkCore;
 using DysonNetwork.Shared.Proto;
+using DysonNetwork.Shared.Models;
 using Google.Protobuf.WellKnownTypes;
 using System.Text.Json;
 using NodaTime.Serialization.Protobuf;
@@ -9,40 +10,101 @@ namespace DysonNetwork.Pass.Permission;

 public class PermissionServiceGrpc(
    PermissionService permissionService,
-    AppDatabase db
+    AppDatabase db,
+    ILogger<PermissionServiceGrpc> logger
 ) : DysonNetwork.Shared.Proto.PermissionService.PermissionServiceBase
 {
    public override async Task<HasPermissionResponse> HasPermission(HasPermissionRequest request, ServerCallContext context)
+    {
+        try
        {
            var hasPermission = await permissionService.HasPermissionAsync(request.Actor, request.Area, request.Key);
            return new HasPermissionResponse { HasPermission = hasPermission };
        }
+        catch (Exception ex)
+        {
+            logger.LogError(ex, "Error checking permission for actor {Actor}, area {Area}, key {Key}",
+                request.Actor, request.Area, request.Key);
+            throw new RpcException(new Status(StatusCode.Internal, "Permission check failed"));
+        }
+    }

    public override async Task<GetPermissionResponse> GetPermission(GetPermissionRequest request, ServerCallContext context)
+    {
+        try
        {
            var permissionValue = await permissionService.GetPermissionAsync<JsonDocument>(request.Actor, request.Area, request.Key);
-        return new GetPermissionResponse { Value = permissionValue != null ? Value.Parser.ParseJson(permissionValue.RootElement.GetRawText()) : null };
+            return new GetPermissionResponse
+            {
+                Value = permissionValue != null ? Value.Parser.ParseJson(permissionValue.RootElement.GetRawText()) : null
+            };
+        }
+        catch (Exception ex)
+        {
+            logger.LogError(ex, "Error getting permission for actor {Actor}, area {Area}, key {Key}",
+                request.Actor, request.Area, request.Key);
+            throw new RpcException(new Status(StatusCode.Internal, "Failed to retrieve permission"));
+        }
    }

    public override async Task<AddPermissionNodeResponse> AddPermissionNode(AddPermissionNodeRequest request, ServerCallContext context)
    {
+        try
+        {
+            JsonDocument jsonValue;
+            try
+            {
+                jsonValue = JsonDocument.Parse(request.Value.ToString());
+            }
+            catch (JsonException ex)
+            {
+                logger.LogWarning(ex, "Invalid JSON in permission value for actor {Actor}, area {Area}, key {Key}",
+                    request.Actor, request.Area, request.Key);
+                throw new RpcException(new Status(StatusCode.InvalidArgument, "Invalid permission value format"));
+            }
+
            var node = await permissionService.AddPermissionNode(
                request.Actor,
                request.Area,
                request.Key,
-            JsonDocument.Parse(request.Value.ToString()), // Convert Value to JsonDocument
+                jsonValue,
                request.ExpiredAt?.ToInstant(),
                request.AffectedAt?.ToInstant()
            );
            return new AddPermissionNodeResponse { Node = node.ToProtoValue() };
        }
+        catch (RpcException)
+        {
+            throw;
+        }
+        catch (Exception ex)
+        {
+            logger.LogError(ex, "Error adding permission node for actor {Actor}, area {Area}, key {Key}",
+                request.Actor, request.Area, request.Key);
+            throw new RpcException(new Status(StatusCode.Internal, "Failed to add permission node"));
+        }
+    }

    public override async Task<AddPermissionNodeToGroupResponse> AddPermissionNodeToGroup(AddPermissionNodeToGroupRequest request, ServerCallContext context)
    {
-        var group = await db.PermissionGroups.FirstOrDefaultAsync(g => g.Id == Guid.Parse(request.Group.Id));
+        try
+        {
+            var group = await FindPermissionGroupAsync(request.Group.Id);
            if (group == null)
            {
-            throw new RpcException(new Status(StatusCode.NotFound, "Permission group not found."));
+                throw new RpcException(new Status(StatusCode.NotFound, "Permission group not found"));
+            }
+
+            JsonDocument jsonValue;
+            try
+            {
+                jsonValue = JsonDocument.Parse(request.Value.ToString());
+            }
+            catch (JsonException ex)
+            {
+                logger.LogWarning(ex, "Invalid JSON in permission value for group {GroupId}, actor {Actor}, area {Area}, key {Key}",
+                    request.Group.Id, request.Actor, request.Area, request.Key);
+                throw new RpcException(new Status(StatusCode.InvalidArgument, "Invalid permission value format"));
            }

            var node = await permissionService.AddPermissionNodeToGroup(
@@ -50,28 +112,72 @@ public class PermissionServiceGrpc(
                request.Actor,
                request.Area,
                request.Key,
-            JsonDocument.Parse(request.Value.ToString()), // Convert Value to JsonDocument
+                jsonValue,
                request.ExpiredAt?.ToInstant(),
                request.AffectedAt?.ToInstant()
            );
            return new AddPermissionNodeToGroupResponse { Node = node.ToProtoValue() };
        }
+        catch (RpcException)
+        {
+            throw;
+        }
+        catch (Exception ex)
+        {
+            logger.LogError(ex, "Error adding permission node to group {GroupId} for actor {Actor}, area {Area}, key {Key}",
+                request.Group.Id, request.Actor, request.Area, request.Key);
+            throw new RpcException(new Status(StatusCode.Internal, "Failed to add permission node to group"));
+        }
+    }

    public override async Task<RemovePermissionNodeResponse> RemovePermissionNode(RemovePermissionNodeRequest request, ServerCallContext context)
+    {
+        try
        {
            await permissionService.RemovePermissionNode(request.Actor, request.Area, request.Key);
            return new RemovePermissionNodeResponse { Success = true };
        }
+        catch (Exception ex)
+        {
+            logger.LogError(ex, "Error removing permission node for actor {Actor}, area {Area}, key {Key}",
+                request.Actor, request.Area, request.Key);
+            throw new RpcException(new Status(StatusCode.Internal, "Failed to remove permission node"));
+        }
+    }

    public override async Task<RemovePermissionNodeFromGroupResponse> RemovePermissionNodeFromGroup(RemovePermissionNodeFromGroupRequest request, ServerCallContext context)
    {
-        var group = await db.PermissionGroups.FirstOrDefaultAsync(g => g.Id == Guid.Parse(request.Group.Id));
+        try
+        {
+            var group = await FindPermissionGroupAsync(request.Group.Id);
            if (group == null)
            {
-            throw new RpcException(new Status(StatusCode.NotFound, "Permission group not found."));
+                throw new RpcException(new Status(StatusCode.NotFound, "Permission group not found"));
            }

            await permissionService.RemovePermissionNodeFromGroup<JsonDocument>(group, request.Actor, request.Area, request.Key);
            return new RemovePermissionNodeFromGroupResponse { Success = true };
        }
+        catch (RpcException)
+        {
+            throw;
+        }
+        catch (Exception ex)
+        {
+            logger.LogError(ex, "Error removing permission node from group {GroupId} for actor {Actor}, area {Area}, key {Key}",
+                request.Group.Id, request.Actor, request.Area, request.Key);
+            throw new RpcException(new Status(StatusCode.Internal, "Failed to remove permission node from group"));
+        }
+    }
+
+    private async Task<SnPermissionGroup?> FindPermissionGroupAsync(string groupId)
+    {
+        if (!Guid.TryParse(groupId, out var guid))
+        {
+            logger.LogWarning("Invalid GUID format for group ID: {GroupId}", groupId);
+            return null;
+        }
+
+        return await db.PermissionGroups.FirstOrDefaultAsync(g => g.Id == guid);
+    }
 }