diff --git a/backend/src/Squidex.Web/ApiExceptionConverter.cs b/backend/src/Squidex.Web/ApiExceptionConverter.cs index 95dd28303..bbc05f433 100644 --- a/backend/src/Squidex.Web/ApiExceptionConverter.cs +++ b/backend/src/Squidex.Web/ApiExceptionConverter.cs @@ -12,6 +12,7 @@ using System.Linq; using System.Security; using System.Text; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; using Squidex.Infrastructure; using Squidex.Infrastructure.Validation; @@ -19,7 +20,6 @@ namespace Squidex.Web { public static class ApiExceptionConverter { - private static readonly List> Handlers = new List>(); private static readonly Dictionary Links = new Dictionary { [400] = "https://tools.ietf.org/html/rfc7231#section-6.5.1", @@ -34,84 +34,104 @@ namespace Squidex.Web [500] = "https://tools.ietf.org/html/rfc7231#section-6.6.1" }; - private static void AddHandler(Func handler) where T : Exception + public static (ErrorDto Error, bool WellKnown) ToErrorDto(this ProblemDetails problem, HttpContext? httpContext) { - Handlers.Add(ex => ex is T typed ? handler(typed) : null); - } + Guard.NotNull(problem); - static ApiExceptionConverter() - { - AddHandler(OnValidationException); - AddHandler(OnDecoderException); - AddHandler(OnDomainObjectNotFoundException); - AddHandler(OnDomainObjectVersionException); - AddHandler(OnDomainForbiddenException); - AddHandler(OnDomainException); - AddHandler(OnSecurityException); + var error = new ErrorDto { Message = problem.Title, StatusCode = problem.Status }; + + Enrich(httpContext, error); + + return (error, true); } - public static ErrorDto ToErrorDto(this Exception exception, HttpContext? httpContext) + public static (ErrorDto Error, bool WellKnown) ToErrorDto(this Exception exception, HttpContext? httpContext) { Guard.NotNull(exception); - foreach (var handler in Handlers) - { - var result = handler(exception); - - if (result != null) - { - result.TraceId = Activity.Current?.Id ?? httpContext?.TraceIdentifier; - - if (result.StatusCode.HasValue) - { - result.Type = Links.GetOrDefault(result.StatusCode.Value); - } + var result = CreateError(exception); - return result; - } - } + Enrich(httpContext, result.Error); - return new ErrorDto { StatusCode = 500 }; + return result; } - private static ErrorDto OnDecoderException(DecoderFallbackException ex) + private static void Enrich(HttpContext? httpContext, ErrorDto error) { - return new ErrorDto { StatusCode = 400, Message = ex.Message }; - } + error.TraceId = Activity.Current?.Id ?? httpContext?.TraceIdentifier; - private static ErrorDto OnDomainObjectNotFoundException(DomainObjectNotFoundException ex) - { - return new ErrorDto { StatusCode = 404 }; + if (error.StatusCode.HasValue) + { + error.Type = Links.GetOrDefault(error.StatusCode.Value); + } } - private static ErrorDto OnDomainObjectVersionException(DomainObjectVersionException ex) + private static (ErrorDto Error, bool WellKnown) CreateError(Exception exception) { - return new ErrorDto { StatusCode = 412, Message = ex.Message }; - } + switch (exception) + { + case ValidationException ex: + return (new ErrorDto + { + StatusCode = 400, + Message = ex.Summary, + Details = ToDetails(ex) + }, true); - private static ErrorDto OnDomainException(DomainException ex) - { - return new ErrorDto { StatusCode = 400, Message = ex.Message }; - } + case DomainObjectNotFoundException _: + return (new ErrorDto + { + StatusCode = 404, + Message = null! + }, true); - private static ErrorDto OnDomainForbiddenException(DomainForbiddenException ex) - { - return new ErrorDto { StatusCode = 403, Message = ex.Message }; - } + case DomainObjectVersionException _: + return (new ErrorDto + { + StatusCode = 412, + Message = exception.Message + }, true); - private static ErrorDto OnSecurityException(SecurityException ex) - { - return new ErrorDto { StatusCode = 403, Message = ex.Message }; - } + case DomainForbiddenException _: + return (new ErrorDto + { + StatusCode = 403, + Message = exception.Message + }, true); - private static ErrorDto OnValidationException(ValidationException ex) - { - return new ErrorDto { StatusCode = 400, Message = ex.Summary, Details = ToDetails(ex) }; + case DomainException _: + return (new ErrorDto + { + StatusCode = 400, + Message = exception.Message + }, true); + + case SecurityException _: + return (new ErrorDto + { + StatusCode = 403, + Message = "Forbidden" + }, false); + + case DecoderFallbackException _: + return (new ErrorDto + { + StatusCode = 400, + Message = exception.Message + }, true); + + default: + return (new ErrorDto + { + StatusCode = 500, + Message = "Server Error" + }, false); + } } private static string[] ToDetails(ValidationException ex) { - return ex.Errors?.Select(e => + return ex.Errors.Select(e => { if (e.PropertyNames?.Any() == true) { @@ -121,7 +141,7 @@ namespace Squidex.Web { return e.Message; } - }).ToArray() ?? new string[0]; + }).ToArray(); } } } diff --git a/backend/src/Squidex.Web/ApiExceptionFilterAttribute.cs b/backend/src/Squidex.Web/ApiExceptionFilterAttribute.cs index a24809644..41df439aa 100644 --- a/backend/src/Squidex.Web/ApiExceptionFilterAttribute.cs +++ b/backend/src/Squidex.Web/ApiExceptionFilterAttribute.cs @@ -8,6 +8,8 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Filters; +using Microsoft.Extensions.DependencyInjection; +using Squidex.Infrastructure.Log; namespace Squidex.Web { @@ -19,32 +21,40 @@ namespace Squidex.Web if (resultContext.Result is ObjectResult objectResult && objectResult.Value is ProblemDetails problem) { - var error = new ErrorDto { Message = problem.Title, Type = problem.Type, StatusCode = problem.Status }; + var (error, _) = problem.ToErrorDto(context.HttpContext); - if (problem.Extensions.TryGetValue("traceId", out var temp) && temp is string traceId) - { - error.TraceId = traceId; - } - - objectResult.Value = error; + context.Result = GetResult(error); } } public void OnException(ExceptionContext context) { - var error = context.Exception.ToErrorDto(context.HttpContext); + var (error, wellKnown) = context.Exception.ToErrorDto(context.HttpContext); - if (error.StatusCode == 404) + if (!wellKnown) { - context.Result = new NotFoundResult(); + var log = context.HttpContext.RequestServices.GetService(); + + if (log != null) + { + log.LogError(context.Exception, w => w.WriteProperty("status", "UnhandledException")); + } } - else + + context.Result = GetResult(error); + } + + private static IActionResult GetResult(ErrorDto error) + { + if (error.StatusCode == 404) { - context.Result = new ObjectResult(error) - { - StatusCode = error.StatusCode - }; + return new NotFoundResult(); } + + return new ObjectResult(error) + { + StatusCode = error.StatusCode + }; } } } diff --git a/backend/src/Squidex.Web/CommandMiddlewares/EnrichWithActorCommandMiddleware.cs b/backend/src/Squidex.Web/CommandMiddlewares/EnrichWithActorCommandMiddleware.cs index f56ec9dbd..4cdbce101 100644 --- a/backend/src/Squidex.Web/CommandMiddlewares/EnrichWithActorCommandMiddleware.cs +++ b/backend/src/Squidex.Web/CommandMiddlewares/EnrichWithActorCommandMiddleware.cs @@ -41,7 +41,7 @@ namespace Squidex.Web.CommandMiddlewares { var actorToken = user.Token(); - squidexCommand.Actor = actorToken ?? throw new SecurityException("No actor with subject or client id available."); + squidexCommand.Actor = actorToken ?? throw new DomainForbiddenException("No actor with subject or client id available."); } if (squidexCommand.User == null) diff --git a/backend/src/Squidex.Web/Pipeline/RequestLogPerformanceMiddleware.cs b/backend/src/Squidex.Web/Pipeline/RequestLogPerformanceMiddleware.cs index 27e3d39f3..e18e678eb 100644 --- a/backend/src/Squidex.Web/Pipeline/RequestLogPerformanceMiddleware.cs +++ b/backend/src/Squidex.Web/Pipeline/RequestLogPerformanceMiddleware.cs @@ -5,6 +5,7 @@ // All rights reserved. Licensed under the MIT license. // ========================================================================== +using System; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Options; @@ -39,6 +40,12 @@ namespace Squidex.Web.Pipeline { await next(context); } + catch (Exception ex) + { + log.LogError(ex, w => w.WriteProperty("status", "UnhandledException")); + + context.Response.StatusCode = 500; + } finally { var elapsedMs = watch.Stop(); diff --git a/backend/src/Squidex/Areas/Api/Controllers/Contents/Models/ImportResultDto.cs b/backend/src/Squidex/Areas/Api/Controllers/Contents/Models/ImportResultDto.cs index 25a32acde..7ecc1eeba 100644 --- a/backend/src/Squidex/Areas/Api/Controllers/Contents/Models/ImportResultDto.cs +++ b/backend/src/Squidex/Areas/Api/Controllers/Contents/Models/ImportResultDto.cs @@ -26,7 +26,9 @@ namespace Squidex.Areas.Api.Controllers.Contents.Models public static ImportResultDto FromImportResult(ImportResultItem result, HttpContext httpContext) { - return new ImportResultDto { ContentId = result.ContentId, Error = result.Exception?.ToErrorDto(httpContext) }; + var error = result.Exception?.ToErrorDto(httpContext).Error; + + return new ImportResultDto { ContentId = result.ContentId, Error = error }; } } } diff --git a/backend/src/Squidex/Areas/IdentityServer/Controllers/Account/AccountController.cs b/backend/src/Squidex/Areas/IdentityServer/Controllers/Account/AccountController.cs index b0008ae64..0d29849b8 100644 --- a/backend/src/Squidex/Areas/IdentityServer/Controllers/Account/AccountController.cs +++ b/backend/src/Squidex/Areas/IdentityServer/Controllers/Account/AccountController.cs @@ -71,7 +71,7 @@ namespace Squidex.Areas.IdentityServer.Controllers.Account [Route("account/forbidden/")] public IActionResult Forbidden() { - throw new SecurityException("User is not allowed to login."); + throw new DomainForbiddenException("User is not allowed to login."); } [HttpGet] diff --git a/backend/tests/Squidex.Web.Tests/ApiExceptionFilterAttributeTests.cs b/backend/tests/Squidex.Web.Tests/ApiExceptionFilterAttributeTests.cs index 85bccda83..e2eea0a51 100644 --- a/backend/tests/Squidex.Web.Tests/ApiExceptionFilterAttributeTests.cs +++ b/backend/tests/Squidex.Web.Tests/ApiExceptionFilterAttributeTests.cs @@ -8,13 +8,16 @@ using System; using System.Collections.Generic; using System.Security; +using System.Text; using System.Threading.Tasks; +using FakeItEasy; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Routing; using Squidex.Infrastructure; +using Squidex.Infrastructure.Log; using Squidex.Infrastructure.Validation; using Xunit; @@ -22,18 +25,9 @@ namespace Squidex.Web { public class ApiExceptionFilterAttributeTests { + private readonly ISemanticLog log = A.Fake(); private readonly ApiExceptionFilterAttribute sut = new ApiExceptionFilterAttribute(); - [Fact] - public void Should_generate_404_for_DomainObjectNotFoundException() - { - var context = E(new DomainObjectNotFoundException("1", typeof(object))); - - sut.OnException(context); - - Assert.IsType(context.Result); - } - [Fact] public void Should_generate_400_for_ValidationException() { @@ -42,7 +36,7 @@ namespace Squidex.Web new ValidationError("Error2", "P"), new ValidationError("Error3", "P1", "P2")); - var context = E(ex); + var context = Error(ex); sut.OnException(context); @@ -54,73 +48,134 @@ namespace Squidex.Web Assert.Equal(ex.Summary, ((ErrorDto)result.Value).Message); Assert.Equal(new[] { "Error1", "P: Error2", "P1, P2: Error3" }, ((ErrorDto)result.Value).Details); + + A.CallTo(() => log.Log(SemanticLogLevel.Error, None.Value, A>._)) + .MustNotHaveHappened(); + } + + [Fact] + public void Should_generate_404_for_DomainObjectNotFoundException() + { + var context = Error(new DomainObjectNotFoundException("1", typeof(object))); + + sut.OnException(context); + + Assert.IsType(context.Result); + + A.CallTo(() => log.Log(SemanticLogLevel.Error, None.Value, A>._)) + .MustNotHaveHappened(); + } + + [Fact] + public void Should_generate_500_and_log_for_unknown_error() + { + var context = Error(new InvalidOperationException()); + + sut.OnException(context); + + Validate(500, context.Result, null); + + A.CallTo(() => log.Log(SemanticLogLevel.Error, None.Value, A>._)) + .MustHaveHappened(); } [Fact] public void Should_generate_400_for_DomainException() { - var context = E(new DomainException("NotAllowed")); + var context = Error(new DomainException("NotAllowed")); sut.OnException(context); Validate(400, context.Result, context.Exception); + + A.CallTo(() => log.Log(SemanticLogLevel.Error, None.Value, A>._)) + .MustNotHaveHappened(); + } + + [Fact] + public void Should_generate_400_for_DecoderFallbackException() + { + var context = Error(new DecoderFallbackException("Decoder")); + + sut.OnException(context); + + Validate(400, context.Result, context.Exception); + + A.CallTo(() => log.Log(SemanticLogLevel.Error, None.Value, A>._)) + .MustNotHaveHappened(); } [Fact] public void Should_generate_412_for_DomainObjectVersionException() { - var context = E(new DomainObjectVersionException("1", typeof(object), 1, 2)); + var context = Error(new DomainObjectVersionException("1", typeof(object), 1, 2)); sut.OnException(context); Validate(412, context.Result, context.Exception); + + A.CallTo(() => log.Log(SemanticLogLevel.Error, None.Value, A>._)) + .MustNotHaveHappened(); } [Fact] public void Should_generate_403_for_DomainForbiddenException() { - var context = E(new DomainForbiddenException("Forbidden")); + var context = Error(new DomainForbiddenException("Forbidden")); sut.OnException(context); Validate(403, context.Result, context.Exception); + + A.CallTo(() => log.Log(SemanticLogLevel.Error, None.Value, A>._)) + .MustNotHaveHappened(); } [Fact] - public void Should_generate_403_for_SecurityException() + public void Should_generate_403_and_log_for_SecurityException() { - var context = E(new SecurityException("Forbidden")); + var context = Error(new SecurityException()); sut.OnException(context); - Validate(403, context.Result, context.Exception); + Validate(403, context.Result, null); + + A.CallTo(() => log.Log(SemanticLogLevel.Error, None.Value, A>._)) + .MustHaveHappened(); } [Fact] public async Task Should_unify_errror() { - var context = R(new ProblemDetails { Status = 403, Type = "type" }); + var context = Problem(new ProblemDetails { Status = 403, Type = "type" }); await sut.OnResultExecutionAsync(context, () => Task.FromResult(Result(context))); Validate(403, context.Result, null); + + A.CallTo(() => log.Log(SemanticLogLevel.Error, None.Value, A>._)) + .MustNotHaveHappened(); } - private static ResultExecutedContext Result(ResultExecutingContext context) + private ResultExecutedContext Result(ResultExecutingContext context) { var actionContext = ActionContext(); - return new ResultExecutedContext(actionContext, new List(), context.Result, context.Controller); + var result = context.Result; + + return new ResultExecutedContext(actionContext, new List(), result, context.Controller); } - private static ResultExecutingContext R(ProblemDetails problem) + private ResultExecutingContext Problem(ProblemDetails problem) { var actionContext = ActionContext(); - return new ResultExecutingContext(actionContext, new List(), new ObjectResult(problem) { StatusCode = problem.Status }, null); + var result = new ObjectResult(problem) { StatusCode = problem.Status }; + + return new ResultExecutingContext(actionContext, new List(), result, null); } - private static ExceptionContext E(Exception exception) + private ExceptionContext Error(Exception exception) { var actionContext = ActionContext(); @@ -130,9 +185,17 @@ namespace Squidex.Web }; } - private static ActionContext ActionContext() + private ActionContext ActionContext() { - var httpContext = new DefaultHttpContext(); + var services = A.Fake(); + + A.CallTo(() => services.GetService(typeof(ISemanticLog))) + .Returns(log); + + var httpContext = new DefaultHttpContext + { + RequestServices = services + }; var actionContext = new ActionContext(httpContext, new RouteData(), new ActionDescriptor { diff --git a/backend/tests/Squidex.Web.Tests/CommandMiddlewares/EnrichWithActorCommandMiddlewareTests.cs b/backend/tests/Squidex.Web.Tests/CommandMiddlewares/EnrichWithActorCommandMiddlewareTests.cs index ec4bc4794..de55b771e 100644 --- a/backend/tests/Squidex.Web.Tests/CommandMiddlewares/EnrichWithActorCommandMiddlewareTests.cs +++ b/backend/tests/Squidex.Web.Tests/CommandMiddlewares/EnrichWithActorCommandMiddlewareTests.cs @@ -39,7 +39,7 @@ namespace Squidex.Web.CommandMiddlewares var command = new CreateContent(); var context = Ctx(command); - await Assert.ThrowsAsync(() => sut.HandleAsync(context)); + await Assert.ThrowsAsync(() => sut.HandleAsync(context)); } [Fact]