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.

Sign up for GitHub

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

Jump to bottom

#913 #1066 ScopesAuthorizer refactoring #1478

Open
wants to merge 11 commits into
base: release/24.0
Choose a base branch
from
Open

#913 #1066 ScopesAuthorizer refactoring #1478

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1ef19de
ScopesAuthorizer refactoring
mehyaa May 31, 2021
8940aa5
Useless block removed
mehyaa May 31, 2021
d7153b0
Update ScopesAuthorizer.cs
mehyaa Aug 11, 2023
6206f16
Fix ScopesAuthorizer
mehyaa Aug 11, 2023
0b70324
Fix ScopesAuthorizer
mehyaa Aug 11, 2023
11007f5
Refactoring
mehyaa Aug 14, 2023
e4b7738
Fix
mehyaa Aug 14, 2023
d87a324
Fix errors and messages
raman-m Aug 24, 2023
8f5bcde
Tests fixed
mehyaa Sep 14, 2023
c72ed76
Test names corrected
mehyaa Sep 14, 2023
5b500af
Tiny change
mehyaa Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 23 additions & 17 deletions src/Ocelot/Authorization/ScopesAuthorizer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,13 @@
using Ocelot.Infrastructure.Claims.Parser;
using Ocelot.Infrastructure.Claims.Parser;
using Ocelot.Responses;
using System.Security.Claims;

namespace Ocelot.Authorization
{
public class ScopesAuthorizer : IScopesAuthorizer
{
private const string ScopeClaimKey = "scope";
private readonly IClaimsParser _claimsParser;
private const string Scope = "scope";

public ScopesAuthorizer(IClaimsParser claimsParser)
{
_claimsParser = claimsParser;
}

public Response<bool> Authorize(ClaimsPrincipal claimsPrincipal, List<string> routeAllowedScopes)
{
Expand All @@ -21,21 +16,32 @@ public Response<bool> Authorize(ClaimsPrincipal claimsPrincipal, List<string> ro
return new OkResponse<bool>(true);
}

var values = _claimsParser.GetValuesByClaimType(claimsPrincipal.Claims, Scope);
var userScopes =
claimsPrincipal.Claims
.Where(x => x.Type == ScopeClaimKey)
.Select(x => x.Value)
.ToArray();

if (values.IsError)
if (userScopes.Length == 1)
{
return new ErrorResponse<bool>(values.Errors);
var userScope = userScopes[0];

var hasMultipleValues = userScope.Contains(" ");

if (hasMultipleValues)
{
userScopes = userScope.Split(" ", StringSplitOptions.RemoveEmptyEntries);
}
else
{
userScopes = new[] { userScope };
}
}

var userScopes = values.Data;

var matchesScopes = routeAllowedScopes.Intersect(userScopes);

if (!matchesScopes.Any())
if (routeAllowedScopes.Any(s => !userScopes.Contains(s)))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Denial is more costly:

To make scope checking more efficient and less costly, a good practice is to invert the condition.

You can also use a method that minimizes the number of iterations required in the collections, making sure that all required scopes are present in the user scopes.

if (routeAllowedScopes.TrueForAll(userScopes.Contains))
{
    return new OkResponse<bool>(true);
}

return new ErrorResponse<bool>(
    new ScopeNotAuthorizedError($"User scopes: '{string.Join(",", userScopes)}' do not have all allowed scopes: '{string.Join(",", routeAllowedScopes)}'"));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! Thanks!
It makes sense to invert the condition.

{
return new ErrorResponse<bool>(
new ScopeNotAuthorizedError($"no one user scope: '{string.Join(',', userScopes)}' match with some allowed scope: '{string.Join(',', routeAllowedScopes)}'"));
new ScopeNotAuthorizedError($"User scopes: '{string.Join(",", userScopes)}' do not have all allowed scopes: '{string.Join(",", routeAllowedScopes)}'"));
}

return new OkResponse<bool>(true);
Expand Down