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

#913 #1066 ScopesAuthorizer refactoring #1478

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
+18 −10
Open

#913 #1066 ScopesAuthorizer refactoring #1478

Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c85b7c1
ScopesAuthorizer refactoring
mehyaa May 31, 2021
5090cbc
Useless block removed
mehyaa May 31, 2021
63f8973
Update ScopesAuthorizer.cs
mehyaa Aug 11, 2023
3c9fc3d
Fix ScopesAuthorizer
mehyaa Aug 11, 2023
a05514c
Fix ScopesAuthorizer
mehyaa Aug 11, 2023
d8510a5
Refactoring
mehyaa Aug 14, 2023
659cb1e
Fix
mehyaa Aug 14, 2023
708c321
Fix errors and messages
raman-m Aug 24, 2023
f1f613f
Tests fixed
mehyaa Sep 14, 2023
ecb22c7
Test names corrected
mehyaa Sep 14, 2023
528db09
Tiny change
mehyaa Sep 15, 2023
d9fd3bd
Revert variable names to their previous identifiers to compare the ch…
raman-m Nov 5, 2024
d9e8e74
Merge branch 'develop' into patch-1
raman-m Nov 6, 2024
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
18 changes: 13 additions & 5 deletions src/Ocelot/Authorization/ScopesAuthorizer.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using Ocelot.Infrastructure.Claims.Parser;
using Ocelot.Infrastructure.Claims.Parser;
using Ocelot.Responses;
using System.Security.Claims;

@@ -28,14 +28,22 @@ public Response<bool> Authorize(ClaimsPrincipal claimsPrincipal, List<string> ro
return new ErrorResponse<bool>(values.Errors);
}

var userScopes = values.Data;
IList<string> userScopes = values.Data;

var matchesScopes = routeAllowedScopes.Intersect(userScopes);
if (userScopes.Count == 1)
Copy link
Member

Choose a reason for hiding this comment

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

The if-block ought to be relocated to IClaimsParser to preserve the existing logic intact. Consequently, you must inject your specialized IClaimsParser service to generate the precise list of claims.

{
var scope = userScopes[0];

if (scope.Contains(' '))
{
userScopes = scope.Split(' ', StringSplitOptions.RemoveEmptyEntries);
}
Comment on lines +37 to +40
Copy link
Member

Choose a reason for hiding this comment

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

It's impossible to predict the body (serialized data) of the token from an unknown Auth-provider.
May I ask which Auth provider you utilize in your project?

}

if (!matchesScopes.Any())
if (routeAllowedScopes.Except(userScopes).Any())
Copy link
Member

@raman-m raman-m Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This logic inversion is feasible, yet it appears redundant. It seems to be a minor refactoring aimed at reducing the number of lines in the code.
Finally, it is useless change!

Additionally, the valuable suggestion from the previous code review was overlooked. This recommendation is more logical than the favored Except helper.

{
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 route scopes: '{string.Join(',', routeAllowedScopes)}'"));
}

return new OkResponse<bool>(true);
2 changes: 1 addition & 1 deletion test/Ocelot.AcceptanceTests/AuthorizationTests.cs
View file
Original file line number Diff line number Diff line change
@@ -177,7 +177,7 @@ public void should_return_response_200_using_identity_server_with_allowed_scope(
AuthenticationOptions = new FileAuthenticationOptions
{
AuthenticationProviderKey = "Test",
AllowedScopes = new List<string>{ "api", "api.readOnly", "openid", "offline_access" },
AllowedScopes = new List<string>{ "api", "api.readOnly" },
},
},
},
2 changes: 1 addition & 1 deletion test/Ocelot.AcceptanceTests/ClaimsToDownstreamPathTests.cs
View file
Original file line number Diff line number Diff line change
@@ -68,7 +68,7 @@ public void should_return_200_and_change_downstream_path()
AuthenticationProviderKey = "Test",
AllowedScopes = new List<string>
{
"openid", "offline_access", "api",
"api",
},
},
ChangeDownstreamPathTemplate =
2 changes: 1 addition & 1 deletion test/Ocelot.AcceptanceTests/ClaimsToHeadersForwardingTests.cs
View file
Original file line number Diff line number Diff line change
@@ -76,7 +76,7 @@ public void should_return_response_200_and_foward_claim_as_header()
AuthenticationProviderKey = "Test",
AllowedScopes = new List<string>
{
"openid", "offline_access", "api",
"api",
},
},
AddHeadersToRequest =
4 changes: 2 additions & 2 deletions test/Ocelot.AcceptanceTests/ClaimsToQueryStringForwardingTests.cs
View file
Original file line number Diff line number Diff line change
@@ -74,7 +74,7 @@ public void should_return_response_200_and_foward_claim_as_query_string()
AuthenticationProviderKey = "Test",
AllowedScopes = new List<string>
{
"openid", "offline_access", "api",
"api",
},
},
AddQueriesToRequest =
@@ -140,7 +140,7 @@ public void should_return_response_200_and_foward_claim_as_query_string_and_pres
AuthenticationProviderKey = "Test",
AllowedScopes = new List<string>
{
"openid", "offline_access", "api",
"api",
},
},
AddQueriesToRequest =