intelligentplant / ProblemDetails.WebApi

Adds RFC 7807 support to ASP.NET 4.x Web API applications.
Apache License 2.0
1 stars 3 forks source link

Owin Middleware Exception Handling #1

Closed agilenut closed 3 years ago

agilenut commented 3 years ago

Is it possible to have the Owin Middleware do the exception handling?

Currently, it looks like you can either use the middleware on its own which doesn't convert the exception to a Server Error Problem Detail. Or you can add the middleware and the error filter which will handle the exception near the action.

The issue with the latter approach is that this prevents the exception from bubbling up the stack and being handled by other middleware that I have.

Preferably, I could just add the middleware, allow it to convert any exceptions to problem details. A bonus would be if I could configure custom exception predicates to map exceptions to the correct problem detail response.

wazzamatazz commented 3 years ago

Hi,

Yes, at the moment, exceptions are intended to be handled by the Web API filters, as this was our use case. I don't think it would be too much effort to add in the capabilities you are asking for though - will see what I can do when I get a chance!

wazzamatazz commented 3 years ago

This actually turned out to be straightforward - see PR #2. Will get it merged and published later!

agilenut commented 3 years ago

Any idea how long it takes to get the new version pushed to nuget.org? I've been checking throughout the day.

wazzamatazz commented 3 years ago

Sorry, I didn't get around to it until now. Check out #3 as well - I figured out how to get the Web API exceptions to bubble up to the OWIN pipeline, so there is now an optional handleExceptions parameter when you register the problem details functionality with your Web API configuration, which should allow you to put everything through common OWIN error handling before creating the problem details responses.

agilenut commented 3 years ago

Nice! I tested something like this too and saw that it worked.

I also tried this: https://blog.jayway.com/2016/01/08/improving-error-handling-asp-net-web-api-2-1-owin/

He has a link to a stack overflow question where he was trying to avoid the debugger from breaking in the handler. He even had a bug bounty open for a while. I noticed your approach seems to address that.

On a different note, it would also be cool if there was an action we could set on the options class that was called when a problem details was created that allowed a user to add additional properties. We would like to include a trace id to correlate to logs obtained from Activity.Current.Id that meets the W3C Trace Context spec. An action could let us add this in startup.

agilenut commented 3 years ago

Also, I just tried your new changes. I think there may be a bug. It does not seem to set the status code of the response. The problem details has the correct status code but the actual response returned is always 200. I can work around this by using the IOwinContext to set the response status code manually but that seems unnecessary.

case AccessDeniedException _:
    context.Response.StatusCode = (int)HttpStatusCode.Forbidden; // Redundant
    result = factory.CreateProblemDetails(context, (int)HttpStatusCode.Forbidden);
    break;

Even the controller extension methods seem to set the wrong status code. If I pass in a NotFound to CreateProblemDetails method, I get a 400 instead of a 404 on the response.

This is the work-around I'm having to do now:

if (result == null) {
    var notFound = this.CreateProblemDetailsResponse(HttpStatusCode.NotFound); // Has 400 instead of 404
    var notFoundMsg = notFound.ExecuteAsync(CancellationToken.None).Result;
    notFoundMsg.StatusCode = HttpStatusCode.NotFound;
    return new ResponseMessageResult(notFoundMsg);
}
wazzamatazz commented 3 years ago

Nice! I tested something like this too and saw that it worked.

I also tried this: https://blog.jayway.com/2016/01/08/improving-error-handling-asp-net-web-api-2-1-owin/

He has a link to a stack overflow question where he was trying to avoid the debugger from breaking in the handler. He even had a bug bounty open for a while. I noticed your approach seems to address that.

On a different note, it would also be cool if there was an action we could set on the options class that was called when a problem details was created that allowed a user to add additional properties. We would like to include a trace id to correlate to logs obtained from Activity.Current.Id that meets the W3C Trace Context spec. An action could let us add this in startup.

You can do this already, by assigning values to the Extensions property on the ProblemDetails object. For example:

app.UseProblemDetails(new IntelligentPlant.ProblemDetails.Owin.ProblemDetailsMiddlewareOptions() { 
    IncludePaths = new[] { new PathString("/") },
    ExceptionHandler = (context, error, factory) => {
        var result = factory.CreateServerErrorProblemDetails(context, detail: error.Message);
        result.Extensions["request-id"] = "some-id";
        result.Extensions["utc-time"] = DateTime.UtcNow.ToString("yyyy-MM-ddTHH:mm:ss.fffffffZ");
        return result;
    }
});

JSON.NET will create a new property in the serialized JSON for every item in the Extensions dictionary:

{
  "type": "https://tools.ietf.org/html/rfc7231#section-6.6.1",
  "title": "Internal Server Error",
  "status": 500,
  "detail": "An error occurred!",
  "instance": "/example/error",
  "request-id": "some-id",
  "utc-time": "2020-12-10T18:05:13.0475861Z"
}
agilenut commented 3 years ago

Nice! I tested something like this too and saw that it worked. I also tried this: https://blog.jayway.com/2016/01/08/improving-error-handling-asp-net-web-api-2-1-owin/ He has a link to a stack overflow question where he was trying to avoid the debugger from breaking in the handler. He even had a bug bounty open for a while. I noticed your approach seems to address that. On a different note, it would also be cool if there was an action we could set on the options class that was called when a problem details was created that allowed a user to add additional properties. We would like to include a trace id to correlate to logs obtained from Activity.Current.Id that meets the W3C Trace Context spec. An action could let us add this in startup.

You can do this already, by assigning values to the Extensions property on the ProblemDetails object. For example:

app.UseProblemDetails(new IntelligentPlant.ProblemDetails.Owin.ProblemDetailsMiddlewareOptions() { 
    IncludePaths = new[] { new PathString("/") },
    ExceptionHandler = (context, error, factory) => {
        var result = factory.CreateServerErrorProblemDetails(context, error, detail: error.Message);
        result.Extensions["request-id"] = "some-id";
        result.Extensions["utc-time"] = DateTime.UtcNow.ToString("yyyy-MM-ddTHH:mm:ss.fffffffZ");
        return result;
    }
});

JSON.NET will create a new property in the serialized JSON for every item in the Extensions dictionary:

{
  "type": "https://tools.ietf.org/html/rfc7231#section-6.5.1",
  "title": "Bad Request",
  "status": 400,
  "detail": "Value does not fall within the expected range.",
  "instance": "/owin/invalid-argument",
  "request-id": "some-id",
  "utc-time": "2020-12-10T18:02:37.4847592Z"
}

True. But how do I do it for responses that are not causing an exception? I see no other way to gain access to that property without implementing my own factory and passing it around.

wazzamatazz commented 3 years ago

Have just published 2.1.1 release to NuGet, which fixes the status code bug and allows you to pass in a custom factory to the OWIN middleware and Web API configuration that can be used to customise all problem details objects it creates. See the example project for an example of this.

I think at this point, I'm done updating the project for the time being. Feel free to clone or submit your own PRs for any additional features!

agilenut commented 3 years ago

I completely understand. Thank you for all your hard work and for sharing this code with the community.

I can confirm the exception handling status code bug is fixed. I'm still getting the bug with the status code in the controller methods. I'll see if I can create a PR shortly.

agilenut commented 3 years ago

Please checkout #5 and #6

wazzamatazz commented 3 years ago

Thanks for your contributions! Version 2.2.0 has been published to NuGet.