diff --git a/src/Ocelot/Authentication/Middleware/AuthenticationMiddleware.cs b/src/Ocelot/Authentication/Middleware/AuthenticationMiddleware.cs index 31ef6976..f6ecd654 100644 --- a/src/Ocelot/Authentication/Middleware/AuthenticationMiddleware.cs +++ b/src/Ocelot/Authentication/Middleware/AuthenticationMiddleware.cs @@ -36,7 +36,7 @@ namespace Ocelot.Authentication.Middleware { if (IsAuthenticatedRoute(DownstreamRoute.ReRoute)) { - _logger.LogDebug($"{context.Request.Path} is an authenticated route. {MiddlwareName} checking if client is authenticated"); + _logger.LogDebug($"{context.Request.Path} is an authenticated route. {MiddlewareName} checking if client is authenticated"); var authenticationHandler = _authHandlerFactory.Get(_app, DownstreamRoute.ReRoute.AuthenticationOptions); diff --git a/src/Ocelot/DownstreamRouteFinder/Middleware/DownstreamRouteFinderMiddleware.cs b/src/Ocelot/DownstreamRouteFinder/Middleware/DownstreamRouteFinderMiddleware.cs index 8fdd2a54..51cb3a0b 100644 --- a/src/Ocelot/DownstreamRouteFinder/Middleware/DownstreamRouteFinderMiddleware.cs +++ b/src/Ocelot/DownstreamRouteFinder/Middleware/DownstreamRouteFinderMiddleware.cs @@ -38,7 +38,7 @@ namespace Ocelot.DownstreamRouteFinder.Middleware if (downstreamRoute.IsError) { - _logger.LogError($"{MiddlwareName} setting pipeline errors. IDownstreamRouteFinder returned {downstreamRoute.Errors.ToErrorString()}"); + _logger.LogError($"{MiddlewareName} setting pipeline errors. IDownstreamRouteFinder returned {downstreamRoute.Errors.ToErrorString()}"); SetPipelineError(downstreamRoute.Errors); return; diff --git a/src/Ocelot/Middleware/OcelotMiddleware.cs b/src/Ocelot/Middleware/OcelotMiddleware.cs index af56891e..0060cbb3 100644 --- a/src/Ocelot/Middleware/OcelotMiddleware.cs +++ b/src/Ocelot/Middleware/OcelotMiddleware.cs @@ -13,10 +13,10 @@ namespace Ocelot.Middleware protected OcelotMiddleware(IRequestScopedDataRepository requestScopedDataRepository) { _requestScopedDataRepository = requestScopedDataRepository; - MiddlwareName = this.GetType().Name; + MiddlewareName = this.GetType().Name; } - public string MiddlwareName { get; } + public string MiddlewareName { get; } public bool PipelineError => _requestScopedDataRepository.Get("OcelotMiddlewareError").Data; diff --git a/src/Ocelot/Responder/Middleware/ResponderMiddleware.cs b/src/Ocelot/Responder/Middleware/ResponderMiddleware.cs index 2b6f52f1..b6ed2717 100644 --- a/src/Ocelot/Responder/Middleware/ResponderMiddleware.cs +++ b/src/Ocelot/Responder/Middleware/ResponderMiddleware.cs @@ -39,7 +39,7 @@ namespace Ocelot.Responder.Middleware if (PipelineError) { var errors = PipelineErrors; - _logger.LogError($"{errors.Count} pipeline errors found in {MiddlwareName}. Setting error response status code"); + _logger.LogError($"{PipelineErrors.Count} pipeline errors found in {MiddlewareName}. Setting error response status code"); SetErrorResponse(context, errors); } @@ -53,7 +53,6 @@ namespace Ocelot.Responder.Middleware private void SetErrorResponse(HttpContext context, List errors) { var statusCode = _codeMapper.Map(errors); - _responder.SetErrorResponseOnContext(context, statusCode); } } diff --git a/test/Ocelot.UnitTests/Responder/AnyError.cs b/test/Ocelot.UnitTests/Responder/AnyError.cs new file mode 100644 index 00000000..a4b35df0 --- /dev/null +++ b/test/Ocelot.UnitTests/Responder/AnyError.cs @@ -0,0 +1,15 @@ +using Ocelot.Errors; + +namespace Ocelot.UnitTests.Responder +{ + class AnyError : Error + { + public AnyError() : base("blahh", OcelotErrorCode.UnknownError) + { + } + + public AnyError(OcelotErrorCode errorCode) : base("blah", errorCode) + { + } + } +} diff --git a/test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs b/test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs index 3b79715e..b0455e38 100644 --- a/test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs +++ b/test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs @@ -1,8 +1,7 @@ using System; using System.Collections.Generic; +using System.Net; using Ocelot.Errors; -using Ocelot.Middleware; -using Ocelot.Requester; using Ocelot.Responder; using Shouldly; using TestStack.BDDfy; @@ -21,47 +20,127 @@ namespace Ocelot.UnitTests.Responder _codeMapper = new ErrorsToHttpStatusCodeMapper(); } - [Fact] - public void should_return_timeout() + [Theory] + [InlineData(OcelotErrorCode.UnauthenticatedError)] + public void should_return_unauthorized(OcelotErrorCode errorCode) { - this.Given(x => x.GivenThereAreErrors(new List - { - new RequestTimedOutError(new Exception()) - })) - .When(x => x.WhenIGetErrorStatusCode()) - .Then(x => x.ThenTheResponseIsStatusCodeIs(503)) - .BDDfy(); + ShouldMapErrorToStatusCode(errorCode, HttpStatusCode.Unauthorized); + } + + [Theory] + [InlineData(OcelotErrorCode.CannotFindClaimError)] + [InlineData(OcelotErrorCode.ClaimValueNotAuthorisedError)] + [InlineData(OcelotErrorCode.ScopeNotAuthorisedError)] + [InlineData(OcelotErrorCode.UnauthorizedError)] + [InlineData(OcelotErrorCode.UserDoesNotHaveClaimError)] + public void should_return_forbidden(OcelotErrorCode errorCode) + { + ShouldMapErrorToStatusCode(errorCode, HttpStatusCode.Forbidden); + } + + [Theory] + [InlineData(OcelotErrorCode.RequestTimedOutError)] + public void should_return_service_unavailable(OcelotErrorCode errorCode) + { + ShouldMapErrorToStatusCode(errorCode, HttpStatusCode.ServiceUnavailable); + } + + + [Theory] + [InlineData(OcelotErrorCode.CannotAddDataError)] + [InlineData(OcelotErrorCode.CannotFindDataError)] + [InlineData(OcelotErrorCode.DownstreamHostNullOrEmptyError)] + [InlineData(OcelotErrorCode.DownstreamPathNullOrEmptyError)] + [InlineData(OcelotErrorCode.DownstreampathTemplateAlreadyUsedError)] + [InlineData(OcelotErrorCode.DownstreamPathTemplateContainsSchemeError)] + [InlineData(OcelotErrorCode.DownstreamSchemeNullOrEmptyError)] + [InlineData(OcelotErrorCode.InstructionNotForClaimsError)] + [InlineData(OcelotErrorCode.NoInstructionsError)] + [InlineData(OcelotErrorCode.ParsingConfigurationHeaderError)] + [InlineData(OcelotErrorCode.RateLimitOptionsError)] + [InlineData(OcelotErrorCode.ServicesAreEmptyError)] + [InlineData(OcelotErrorCode.ServicesAreNullError)] + [InlineData(OcelotErrorCode.UnableToCompleteRequestError)] + [InlineData(OcelotErrorCode.UnableToCreateAuthenticationHandlerError)] + [InlineData(OcelotErrorCode.UnableToFindDownstreamRouteError)] + [InlineData(OcelotErrorCode.UnableToFindLoadBalancerError)] + [InlineData(OcelotErrorCode.UnableToFindServiceDiscoveryProviderError)] + [InlineData(OcelotErrorCode.UnableToFindQoSProviderError)] + [InlineData(OcelotErrorCode.UnableToSetConfigInConsulError)] + [InlineData(OcelotErrorCode.UnknownError)] + [InlineData(OcelotErrorCode.UnmappableRequestError)] + [InlineData(OcelotErrorCode.UnsupportedAuthenticationProviderError)] + public void should_return_not_found(OcelotErrorCode errorCode) + { + ShouldMapErrorToStatusCode(errorCode, HttpStatusCode.NotFound); } [Fact] - public void should_create_unauthenticated_response_code() + public void AuthenticationErrorsHaveHighestPriority() { - this.Given(x => x.GivenThereAreErrors(new List - { - new UnauthenticatedError("no matter") - })) - .When(x => x.WhenIGetErrorStatusCode()) - .Then(x => x.ThenTheResponseIsStatusCodeIs(401)) - .BDDfy(); - } - - [Fact] - public void should_create_not_found_response_response_code() - { - this.Given(x => x.GivenThereAreErrors(new List - { - new AnyError() - })) - .When(x => x.WhenIGetErrorStatusCode()) - .Then(x => x.ThenTheResponseIsStatusCodeIs(404)) - .BDDfy(); - } - - class AnyError : Error - { - public AnyError() : base("blahh", OcelotErrorCode.UnknownError) + var errors = new List { + OcelotErrorCode.CannotAddDataError, + OcelotErrorCode.CannotFindClaimError, + OcelotErrorCode.UnauthenticatedError, + OcelotErrorCode.RequestTimedOutError, + }; + + ShouldMapErrorsToStatusCode(errors, HttpStatusCode.Unauthorized); + } + + [Fact] + public void AuthorisationErrorsHaveSecondHighestPriority() + { + var errors = new List + { + OcelotErrorCode.CannotAddDataError, + OcelotErrorCode.CannotFindClaimError, + OcelotErrorCode.RequestTimedOutError + }; + + ShouldMapErrorsToStatusCode(errors, HttpStatusCode.Forbidden); + } + + [Fact] + public void ServiceUnavailableErrorsHaveThirdHighestPriority() + { + var errors = new List + { + OcelotErrorCode.CannotAddDataError, + OcelotErrorCode.RequestTimedOutError + }; + + ShouldMapErrorsToStatusCode(errors, HttpStatusCode.ServiceUnavailable); + } + + [Fact] + public void check_we_have_considered_all_errors_in_these_tests() + { + // If this test fails then it's because the number of error codes has changed. + // You should make the appropriate changes to the test cases here to ensure + // they cover all the error codes, and then modify this assertion. + Enum.GetNames(typeof(OcelotErrorCode)).Length.ShouldBe(30, "Looks like the number of error codes has changed. Do you need to modify ErrorsToHttpStatusCodeMapper?"); + } + + private void ShouldMapErrorToStatusCode(OcelotErrorCode errorCode, HttpStatusCode expectedHttpStatusCode) + { + ShouldMapErrorsToStatusCode(new List { errorCode }, expectedHttpStatusCode); + } + + private void ShouldMapErrorsToStatusCode(List errorCodes, HttpStatusCode expectedHttpStatusCode) + { + var errors = new List(); + + foreach(var errorCode in errorCodes) + { + errors.Add(new AnyError(errorCode)); } + + this.Given(x => x.GivenThereAreErrors(errors)) + .When(x => x.WhenIGetErrorStatusCode()) + .Then(x => x.ThenTheResponseIsStatusCodeIs(expectedHttpStatusCode)) + .BDDfy(); } private void GivenThereAreErrors(List errors) @@ -77,6 +156,11 @@ namespace Ocelot.UnitTests.Responder private void ThenTheResponseIsStatusCodeIs(int expectedCode) { _result.ShouldBe(expectedCode); - } + } + + private void ThenTheResponseIsStatusCodeIs(HttpStatusCode expectedCode) + { + _result.ShouldBe((int)expectedCode); + } } } diff --git a/test/Ocelot.UnitTests/Responder/ResponderMiddlewareTestsV2.cs b/test/Ocelot.UnitTests/Responder/ResponderMiddlewareTestsV2.cs new file mode 100644 index 00000000..28c3e9fa --- /dev/null +++ b/test/Ocelot.UnitTests/Responder/ResponderMiddlewareTestsV2.cs @@ -0,0 +1,140 @@ +using System.Collections.Generic; +using System.Net.Http; +using Microsoft.AspNetCore.Http; +using Moq; +using Ocelot.Infrastructure.RequestData; +using Ocelot.Errors; +using Ocelot.Logging; +using Ocelot.Responder; +using Ocelot.Responder.Middleware; +using Ocelot.Responses; +using TestStack.BDDfy; +using Xunit; + +namespace Ocelot.UnitTests.Responder +{ + public class ResponderMiddlewareTestsV2 + { + private readonly Mock _responder; + private readonly Mock _scopedRepository; + private readonly Mock _codeMapper; + private readonly Mock _next; + private readonly Mock _loggerFactory; + private readonly Mock _logger; + private readonly Mock _httpContext; + private ResponderMiddleware _middleware; + private OkResponse _response; + private int _mappedStatusCode; + private List _pipelineErrors; + + public ResponderMiddlewareTestsV2() + { + _responder = new Mock(); + _codeMapper = new Mock(); + _next = new Mock(); + _logger = new Mock(); + _scopedRepository = new Mock(); + _loggerFactory = new Mock(); + _httpContext = new Mock(); + + _loggerFactory + .Setup(lf => lf.CreateLogger()) + .Returns(_logger.Object); + + _middleware = new ResponderMiddleware(_next.Object, _responder.Object, _loggerFactory.Object, _scopedRepository.Object, _codeMapper.Object); + + GivenTheHttpResponseMessageIs(new HttpResponseMessage()); + } + + [Fact] + public void NoPipelineErrors() + { + this.Given(x => x.GivenThereAreNoPipelineErrors()) + .When(x => x.WhenICallTheMiddleware()) + .Then(_ => ThenTheNextMiddlewareIsCalled()) + .And(x => x.ThenThereAreNoErrorsOnTheHttpContext()) + .BDDfy(); + } + + [Fact] + public void PipelineErrors() + { + this.Given(_ => GivenThereArePipelineErrors()) + .And(_ => GivenTheErrorsCanBeMappedToAStatusCode()) + .When(_ => WhenICallTheMiddleware()) + .Then(_ => ThenTheNextMiddlewareIsCalled()) + .And(x => x.ThenTheErrorsAreLogged()) + .And(_ => ThenTheErrorsAreMappedToAnHttpStatus()) + .And(_ => ThenAnErrorResponseIsSetOnTheHttpContext()) + .BDDfy(); + } + + private void GivenTheHttpResponseMessageIs(HttpResponseMessage response) + { + _response = new OkResponse(response); + _scopedRepository + .Setup(x => x.Get(It.IsAny())) + .Returns(_response); + } + + private void GivenThereAreNoPipelineErrors() + { + GivenThereArePipelineErrors(new List()); + } + + private void GivenThereArePipelineErrors() + { + GivenThereArePipelineErrors(new List() { new AnyError() }); + } + + private void GivenThereArePipelineErrors(List pipelineErrors) + { + _pipelineErrors = pipelineErrors; + + _scopedRepository + .Setup(x => x.Get("OcelotMiddlewareError")) + .Returns(new OkResponse(_pipelineErrors.Count != 0)); + + _scopedRepository + .Setup(sr => sr.Get>("OcelotMiddlewareErrors")) + .Returns(new OkResponse>(_pipelineErrors)); + } + + private void GivenTheErrorsCanBeMappedToAStatusCode() + { + _mappedStatusCode = 500; //TODO: autofixture + _codeMapper.Setup(cm => cm.Map(It.IsAny>())) + .Returns(_mappedStatusCode); + } + + private void WhenICallTheMiddleware() + { + _middleware.Invoke(_httpContext.Object).GetAwaiter().GetResult(); + } + + private void ThenTheNextMiddlewareIsCalled() + { + _next.Verify(n => n(_httpContext.Object), Times.Once); + } + + private void ThenTheErrorsAreMappedToAnHttpStatus() + { + _codeMapper.Verify(cm => cm.Map(_pipelineErrors), Times.Once); + } + + private void ThenTheErrorsAreLogged() + { + _logger.Verify(l => l.LogError($"{_pipelineErrors.Count} pipeline errors found in ResponderMiddleware. Setting error response status code"), Times.Once); + } + + private void ThenThereAreNoErrorsOnTheHttpContext() + { + _responder.Verify(r => r.SetErrorResponseOnContext(It.IsAny(), It.IsAny()), Times.Never); + } + + private void ThenAnErrorResponseIsSetOnTheHttpContext() + { + _responder.Verify(r => r.SetErrorResponseOnContext(_httpContext.Object, _mappedStatusCode), Times.Once); + } + } +}