Optimize role authorization lookup to O(1) using HashSet #1409
Conversation
…t role membership checks.
There was a problem hiding this comment.
Pull request overview
This PR optimizes the role authorization lookup in the AuthorizationBehaviour class by replacing nested loops with HashSet-based comparisons, reducing the time complexity from O(n×m) to O(n+m).
Key Changes:
- Replaces nested foreach loops with LINQ operations to build a HashSet of required roles
- Uses HashSet for user roles to enable efficient membership checks
- Adds trimming and filtering of whitespace from role names
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var requiredRoles = authorizeAttributesWithRoles | ||
| .Where(a => a.Roles != null) | ||
| .SelectMany(a => a.Roles.Split(',')) | ||
| .Select(r => r.Trim()) | ||
| .Where(r => !string.IsNullOrWhiteSpace(r)) | ||
| .ToHashSet(StringComparer.OrdinalIgnoreCase); | ||
|
|
||
| foreach (var roles in authorizeAttributesWithRoles.Select(a => a.Roles.Split(','))) | ||
| { | ||
| foreach (var role in roles) | ||
| { | ||
| var isInRole = _user.Roles?.Any(x => role == x)??false; | ||
| if (isInRole) | ||
| { | ||
| authorized = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| var userRoles = _user.Roles?.ToHashSet(StringComparer.OrdinalIgnoreCase) ?? new HashSet<string>(StringComparer.OrdinalIgnoreCase); | ||
| var authorized = requiredRoles.Any(role => userRoles.Contains(role)); |
There was a problem hiding this comment.
This change introduces case-insensitive role comparison where the original implementation was case-sensitive. The old code used direct string equality (role == x), while the new code uses StringComparer.OrdinalIgnoreCase. This means roles like "Admin" and "admin" will now be treated as equivalent, which is a breaking behavioral change that could affect authorization logic across the application.
| } | ||
| } | ||
| } | ||
| var userRoles = _user.Roles?.ToHashSet(StringComparer.OrdinalIgnoreCase) ?? new HashSet<string>(StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
The new implementation trims whitespace from required role names but does not trim user roles from _user.Roles. This creates an inconsistency: if a user has role " Admin " (with spaces) in their roles list, and the required role is "Admin" (trimmed), the authorization will fail even though it should succeed. Either both should be trimmed or neither should be trimmed to maintain consistent behavior.
| var userRoles = _user.Roles?.ToHashSet(StringComparer.OrdinalIgnoreCase) ?? new HashSet<string>(StringComparer.OrdinalIgnoreCase); | |
| var userRoles = _user.Roles? | |
| .Select(r => r?.Trim()) | |
| .Where(r => !string.IsNullOrWhiteSpace(r)) | |
| .ToHashSet(StringComparer.OrdinalIgnoreCase) | |
| ?? new HashSet<string>(StringComparer.OrdinalIgnoreCase); |
| { | ||
| var authorized = false; | ||
| var requiredRoles = authorizeAttributesWithRoles | ||
| .Where(a => a.Roles != null) |
There was a problem hiding this comment.
This null check is redundant because line 35 already filters for !string.IsNullOrWhiteSpace(a.Roles), which ensures that a.Roles is neither null nor whitespace. The Where(a => a.Roles != null) check can be removed for cleaner code.
| .Where(a => a.Roles != null) |
1 participant
Optimize role authorization lookup to O(1) using HashSet for efficient role membership checks.