diff --git a/src/Ocelot/Authorization/IRolesAuthorizer.cs b/src/Ocelot/Authorization/IRolesAuthorizer.cs index 88ac7ac093..472359ac00 100644 --- a/src/Ocelot/Authorization/IRolesAuthorizer.cs +++ b/src/Ocelot/Authorization/IRolesAuthorizer.cs @@ -1,11 +1,9 @@ -using System.Collections.Generic; +using Ocelot.Responses; using System.Security.Claims; -using Ocelot.Responses; -namespace Ocelot.Authorization +namespace Ocelot.Authorization; + +public interface IRolesAuthorizer { - public interface IRolesAuthorizer - { - Response Authorize(ClaimsPrincipal claimsPrincipal, List routeRequiredRole, string roleKey); - } + Response Authorize(ClaimsPrincipal claimsPrincipal, List routeRequiredRole, string roleKey); } diff --git a/src/Ocelot/Authorization/IScopesAuthorizer.cs b/src/Ocelot/Authorization/IScopesAuthorizer.cs index c62b0ee63c..11aa3a6290 100644 --- a/src/Ocelot/Authorization/IScopesAuthorizer.cs +++ b/src/Ocelot/Authorization/IScopesAuthorizer.cs @@ -1,10 +1,10 @@ using Ocelot.Responses; using System.Security.Claims; -namespace Ocelot.Authorization +namespace Ocelot.Authorization; + +public interface IScopesAuthorizer { - public interface IScopesAuthorizer - { - Response Authorize(ClaimsPrincipal claimsPrincipal, List routeAllowedScopes, string scopeKey); - } + Response Authorize(ClaimsPrincipal claimsPrincipal, List routeAllowedScopes, + string scopeKey); } diff --git a/src/Ocelot/Authorization/Middleware/AuthorizationMiddleware.cs b/src/Ocelot/Authorization/Middleware/AuthorizationMiddleware.cs index ed2226c2f8..b83446f7c5 100644 --- a/src/Ocelot/Authorization/Middleware/AuthorizationMiddleware.cs +++ b/src/Ocelot/Authorization/Middleware/AuthorizationMiddleware.cs @@ -28,16 +28,16 @@ public AuthorizationMiddleware(RequestDelegate next, // Note roles is a duplicate of scopes - should refactor based on type // Note scopes and roles are processed as OR - // todo create logic to process policies that we use in the API + // TODO: Create logic to process policies that we use in the API public async Task Invoke(HttpContext httpContext) { var downstreamRoute = httpContext.Items.DownstreamRoute(); - + var options = downstreamRoute.AuthenticationOptions; if (!IsOptionsHttpMethod(httpContext) && IsAuthenticatedRoute(downstreamRoute)) { Logger.LogInformation("route is authenticated scopes must be checked"); - var authorized = _scopesAuthorizer.Authorize(httpContext.User, downstreamRoute.AuthenticationOptions.AllowedScopes, downstreamRoute.AuthenticationOptions.ScopeKey); + var authorized = _scopesAuthorizer.Authorize(httpContext.User, options.AllowedScopes, options.ScopeKey); if (authorized.IsError) { @@ -64,7 +64,7 @@ public async Task Invoke(HttpContext httpContext) { Logger.LogInformation("route and scope is authenticated role must be checked"); - var authorizedRole = _rolesAuthorizer.Authorize(httpContext.User, downstreamRoute.AuthenticationOptions.RequiredRole, downstreamRoute.AuthenticationOptions.RoleKey); + var authorizedRole = _rolesAuthorizer.Authorize(httpContext.User, options.RequiredRole, options.RoleKey); if (authorizedRole.IsError) { diff --git a/src/Ocelot/Authorization/RolesAuthorizer.cs b/src/Ocelot/Authorization/RolesAuthorizer.cs index ea939627dc..62925094f7 100644 --- a/src/Ocelot/Authorization/RolesAuthorizer.cs +++ b/src/Ocelot/Authorization/RolesAuthorizer.cs @@ -1,47 +1,44 @@ -using System.Collections.Generic; -using System.Linq; -using System.Security.Claims; -using Ocelot.Infrastructure.Claims.Parser; +using Ocelot.Infrastructure.Claims.Parser; using Ocelot.Responses; +using System.Security.Claims; -namespace Ocelot.Authorization +namespace Ocelot.Authorization; + +public class RolesAuthorizer : IRolesAuthorizer { - public class RolesAuthorizer : IRolesAuthorizer + private readonly IClaimsParser _claimsParser; + + public RolesAuthorizer(IClaimsParser claimsParser) { - private readonly IClaimsParser _claimsParser; + _claimsParser = claimsParser; + } - public RolesAuthorizer(IClaimsParser claimsParser) + public Response Authorize(ClaimsPrincipal claimsPrincipal, List routeRequiredRole, string roleKey) + { + if (routeRequiredRole == null || routeRequiredRole.Count == 0) { - _claimsParser = claimsParser; + return new OkResponse(true); } - public Response Authorize(ClaimsPrincipal claimsPrincipal, List routeRequiredRole, string roleKey) - { - if (routeRequiredRole == null || routeRequiredRole.Count == 0) - { - return new OkResponse(true); - } + roleKey ??= "role"; - roleKey ??= "role"; + var values = _claimsParser.GetValuesByClaimType(claimsPrincipal.Claims, roleKey); - var values = _claimsParser.GetValuesByClaimType(claimsPrincipal.Claims, roleKey); - - if (values.IsError) - { - return new ErrorResponse(values.Errors); - } - - var userRoles = values.Data; + if (values.IsError) + { + return new ErrorResponse(values.Errors); + } - var matchedRoles = routeRequiredRole.Intersect(userRoles).ToList(); // Note this is an OR + var userRoles = values.Data; - if (matchedRoles.Count == 0) - { - return new ErrorResponse( - new ScopeNotAuthorizedError($"no one user role: '{string.Join(",", userRoles)}' match with some allowed role: '{string.Join(",", routeRequiredRole)}'")); - } + var matchedRoles = routeRequiredRole.Intersect(userRoles).ToList(); // Note this is an OR - return new OkResponse(true); + if (matchedRoles.Count == 0) + { + return new ErrorResponse( + new ScopeNotAuthorizedError($"no one user role: '{string.Join(",", userRoles)}' match with some allowed role: '{string.Join(",", routeRequiredRole)}'")); } + + return new OkResponse(true); } } diff --git a/src/Ocelot/Configuration/AuthenticationOptions.cs b/src/Ocelot/Configuration/AuthenticationOptions.cs index 77ff959eac..0021a80347 100644 --- a/src/Ocelot/Configuration/AuthenticationOptions.cs +++ b/src/Ocelot/Configuration/AuthenticationOptions.cs @@ -1,5 +1,4 @@ using Ocelot.Configuration.File; -using System; namespace Ocelot.Configuration { diff --git a/src/Ocelot/Configuration/Builder/AuthenticationOptionsBuilder.cs b/src/Ocelot/Configuration/Builder/AuthenticationOptionsBuilder.cs index 85faf259e6..7eb07442f2 100644 --- a/src/Ocelot/Configuration/Builder/AuthenticationOptionsBuilder.cs +++ b/src/Ocelot/Configuration/Builder/AuthenticationOptionsBuilder.cs @@ -1,5 +1,3 @@ -using System; - namespace Ocelot.Configuration.Builder { public class AuthenticationOptionsBuilder diff --git a/src/Ocelot/Configuration/Creator/AuthenticationOptionsCreator.cs b/src/Ocelot/Configuration/Creator/AuthenticationOptionsCreator.cs index bb736cbf03..86d0ea43c0 100644 --- a/src/Ocelot/Configuration/Creator/AuthenticationOptionsCreator.cs +++ b/src/Ocelot/Configuration/Creator/AuthenticationOptionsCreator.cs @@ -1,12 +1,8 @@ using Ocelot.Configuration.File; -namespace Ocelot.Configuration.Creator +namespace Ocelot.Configuration.Creator; + +public class AuthenticationOptionsCreator : IAuthenticationOptionsCreator { - public class AuthenticationOptionsCreator : IAuthenticationOptionsCreator - { - public AuthenticationOptions Create(FileRoute route) - { - return new AuthenticationOptions(route.AuthenticationOptions); - } - } + public AuthenticationOptions Create(FileRoute route) => new(route.AuthenticationOptions); } diff --git a/test/Ocelot.UnitTests/Infrastructure/RoleAuthorizerTests.cs b/test/Ocelot.UnitTests/Infrastructure/RoleAuthorizerTests.cs index 8dd6db9d00..9d3ffecf90 100644 --- a/test/Ocelot.UnitTests/Infrastructure/RoleAuthorizerTests.cs +++ b/test/Ocelot.UnitTests/Infrastructure/RoleAuthorizerTests.cs @@ -1,116 +1,109 @@ -using Moq; using Ocelot.Authorization; -using Ocelot.Errors; using Ocelot.Infrastructure.Claims.Parser; using Ocelot.Responses; -using Shouldly; -using System.Collections.Generic; using System.Security.Claims; -using TestStack.BDDfy; -using Xunit; -namespace Ocelot.UnitTests.Infrastructure +namespace Ocelot.UnitTests.Infrastructure; + +public class RoleAuthorizerTests : UnitTest { - public class RoleAuthorizerTests - { - private readonly RolesAuthorizer _authorizer; - private readonly Mock _parser; - private ClaimsPrincipal _principal; - private List _requiredRole; - private Response _result; + private readonly RolesAuthorizer _authorizer; + private readonly Mock _parser; + private ClaimsPrincipal _principal; + private List _requiredRole; + private Response _result; - public RoleAuthorizerTests() - { - _parser = new Mock(); - _authorizer = new RolesAuthorizer(_parser.Object); - } + public RoleAuthorizerTests() + { + _parser = new Mock(); + _authorizer = new RolesAuthorizer(_parser.Object); + } - [Fact] - public void Should_return_ok_if_no_allowed_scopes() - { - this.Given(_ => GivenTheFollowing(new ClaimsPrincipal())) - .And(_ => GivenTheFollowing(new List())) - .When(_ => WhenIAuthorize()) - .Then(_ => ThenTheFollowingIsReturned(new OkResponse(true))) - .BDDfy(); - } + [Fact] + public void Should_return_ok_if_no_allowed_scopes() + { + this.Given(_ => GivenTheFollowing(new ClaimsPrincipal())) + .And(_ => GivenTheFollowing(new List())) + .When(_ => WhenIAuthorize()) + .Then(_ => ThenTheFollowingIsReturned(new OkResponse(true))) + .BDDfy(); + } - [Fact] - public void Should_return_ok_if_null_allowed_scopes() - { - this.Given(_ => GivenTheFollowing(new ClaimsPrincipal())) - .And(_ => GivenTheFollowing((List)null)) - .When(_ => WhenIAuthorize()) - .Then(_ => ThenTheFollowingIsReturned(new OkResponse(true))) - .BDDfy(); - } + [Fact] + public void Should_return_ok_if_null_allowed_scopes() + { + this.Given(_ => GivenTheFollowing(new ClaimsPrincipal())) + .And(_ => GivenTheFollowing((List)null)) + .When(_ => WhenIAuthorize()) + .Then(_ => ThenTheFollowingIsReturned(new OkResponse(true))) + .BDDfy(); + } - [Fact] - public void Should_return_error_if_claims_parser_returns_error() - { - var fakeError = new FakeError(); - this.Given(_ => GivenTheFollowing(new ClaimsPrincipal())) - .And(_ => GivenTheParserReturns(new ErrorResponse>(fakeError))) - .And(_ => GivenTheFollowing(new List() { "doesntmatter" })) - .When(_ => WhenIAuthorize()) - .Then(_ => ThenTheFollowingIsReturned(new ErrorResponse(fakeError))) - .BDDfy(); - } + [Fact] + public void Should_return_error_if_claims_parser_returns_error() + { + var fakeError = new FakeError(); + this.Given(_ => GivenTheFollowing(new ClaimsPrincipal())) + .And(_ => GivenTheParserReturns(new ErrorResponse>(fakeError))) + .And(_ => GivenTheFollowing(new List() { "doesntmatter" })) + .When(_ => WhenIAuthorize()) + .Then(_ => ThenTheFollowingIsReturned(new ErrorResponse(fakeError))) + .BDDfy(); + } - [Fact] - public void Should_match_role_and_return_ok_result() - { - var claimsPrincipal = new ClaimsPrincipal(); - var requiredRole = new List() { "someRole" }; + [Fact] + public void Should_match_role_and_return_ok_result() + { + var claimsPrincipal = new ClaimsPrincipal(); + var requiredRole = new List() { "someRole" }; - this.Given(_ => GivenTheFollowing(claimsPrincipal)) - .And(_ => GivenTheParserReturns(new OkResponse>(requiredRole))) - .And(_ => GivenTheFollowing(requiredRole)) - .When(_ => WhenIAuthorize()) - .Then(_ => ThenTheFollowingIsReturned(new OkResponse(true))) - .BDDfy(); - } + this.Given(_ => GivenTheFollowing(claimsPrincipal)) + .And(_ => GivenTheParserReturns(new OkResponse>(requiredRole))) + .And(_ => GivenTheFollowing(requiredRole)) + .When(_ => WhenIAuthorize()) + .Then(_ => ThenTheFollowingIsReturned(new OkResponse(true))) + .BDDfy(); + } - [Fact] - public void Should_not_match_role_and_return_error_result() - { - var fakeError = new FakeError(); - var claimsPrincipal = new ClaimsPrincipal(); - var requiredRole = new List() { "someRole" }; - var userRoles = new List() { "anotherRole" }; + [Fact] + public void Should_not_match_role_and_return_error_result() + { + var fakeError = new FakeError(); + var claimsPrincipal = new ClaimsPrincipal(); + var requiredRole = new List() { "someRole" }; + var userRoles = new List() { "anotherRole" }; - this.Given(_ => GivenTheFollowing(claimsPrincipal)) - .And(_ => GivenTheParserReturns(new OkResponse>(userRoles))) - .And(_ => GivenTheFollowing(requiredRole)) - .When(_ => WhenIAuthorize()) - .Then(_ => ThenTheFollowingIsReturned(new ErrorResponse(fakeError))) - .BDDfy(); - } + this.Given(_ => GivenTheFollowing(claimsPrincipal)) + .And(_ => GivenTheParserReturns(new OkResponse>(userRoles))) + .And(_ => GivenTheFollowing(requiredRole)) + .When(_ => WhenIAuthorize()) + .Then(_ => ThenTheFollowingIsReturned(new ErrorResponse(fakeError))) + .BDDfy(); + } - private void GivenTheParserReturns(Response> response) - { - _parser.Setup(x => x.GetValuesByClaimType(It.IsAny>(), It.IsAny())).Returns(response); - } + private void GivenTheParserReturns(Response> response) + { + _parser.Setup(x => x.GetValuesByClaimType(It.IsAny>(), It.IsAny())).Returns(response); + } - private void GivenTheFollowing(ClaimsPrincipal principal) - { - _principal = principal; - } + private void GivenTheFollowing(ClaimsPrincipal principal) + { + _principal = principal; + } - private void GivenTheFollowing(List requiredRole) - { - _requiredRole = requiredRole; - } + private void GivenTheFollowing(List requiredRole) + { + _requiredRole = requiredRole; + } - private void WhenIAuthorize() - { - _result = _authorizer.Authorize(_principal, _requiredRole, null); - } + private void WhenIAuthorize() + { + _result = _authorizer.Authorize(_principal, _requiredRole, null); + } - private void ThenTheFollowingIsReturned(Response expected) - { - _result.Data.ShouldBe(expected.Data); - _result.IsError.ShouldBe(expected.IsError); - } + private void ThenTheFollowingIsReturned(Response expected) + { + _result.Data.ShouldBe(expected.Data); + _result.IsError.ShouldBe(expected.IsError); } }