Rationalize error handling

[Tratcher]

The original batch of social auth middleware (Facebook, Twitter, etc.) all swallowed exceptions. The second round (WsFed, ODIC) notified you (AuthenticationFailedNotification) and threw them. These should be rationalized.

Also, consider getting rid of EnsureSuccessStatusCode and offering a more useful error reporting by trying to build an exception using the JSON error payload when content type=app/json.

[Tratcher]

Proposal: Throw an exception and let the new ErrorHandler middleware deal with the failure.

[saravanandorai]

Regarding the exception logging, I updated the web.config to log the exceptions in a log file separate for Facebook, Twitter etc so that I can analyze and it works fine. This is a good option of using a ErrorHandler middleware.

Also, can we have the configuration of the End Point URI's in the app.config so that it is not required to change dll's in the event of an end point change.

[kevinchalet]

@Tratcher good idea 🌴

That said, the fact that your new error middleware always returns 500 responses might not be appropriate in every case, specially when the exception has been directly caused by a malformed request. In such scenarios, returning a 400 response would be probably better.

Of course, a special exception type allowing to set the status code would solve this kind of issue... but IIRC, you were not a huge fan of this approach.

[Tratcher]

Developers are free to examine the exception and set their own status code. We just wanted the default to be 500.

[kevinchalet]

Sure. But currently, this responsibility is delegated to the end developer, not to the middleware writer.

https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Security.OAuth/OAuthAuthenticationHandler.cs#L102
For instance, in this code path, you already know that the error has not been caused by a server error, but by an external party (the user, who refused to give his consent or the provider). Returning a 4** response might be more appropriate, but would require to play with the error handler to avoid applying the default 500 behavior (which might not be trivial if you wanted to deal with exceptions coming from very different middleware).

[Tratcher]

ErrorHandler is just for unhandled exceptions. Components that want to return status codes can do so. We're discussing a custom status codes middleware that would provide friendly content to go with status codes. Here's one I prototyped a while ago: http://tratcher.azurewebsites.net/StatusCodes

[kevinchalet]

But AFAICT, you can't set the status code while throwing an exception given that ErrorHandler overrides it later.

In my mind, a middleware should be able to set an appropriate status code to indicate that the requester made a bad request and throw an exception that could be caught in the middleware itself (using the OnException approach for instance), at the error handler level or even at the host level when no error handler has been configured. In all cases, the status code shouldn't be altered.

[Tratcher]

You're conflating two forms of control flow that really shouldn't be mixed. What you're asking for is analogous to a method that both returns a error code and throws an exception. A status code should be returned if it's the definitive answer (even if it's a 400). An exception should be thrown when you have no way of producing a definitive answer. If your code path does not have direct access to set the status code then you should be catching specific kinds of exceptions and converting them to status codes in a local scope (e.g. within you middleware/framework) where you have the context to make the conversion. Once an exception escapes into the pipeline then there is no way to guarantee how it will be handled.

[kevinchalet]

Interesting explanation, even if I don't see why the two forms couldn't be mixed (and HttpResponseException works like that: you throw an exception with a status code and some content and it's up to the rest of the pipeline to decide how it will be rendered).

This is an easy way for a middleware to prepare a standard response (i.e an appropriate status code and a default message) while allowing the rest of the pipeline to change the way the message is finally handled (by using MVC and Razor or by redirecting the user agent to the login path for instance) without necessarily changing the status code associated with the exception.

[Tratcher]

HttpResponseException fits my example above where it's handled within WebApi, it doesn't escape into the larger application pipeline. As far as I know, it's also indented to be used only in places where you can't just return an HttpResponseMessage. This is not something we want to try orchestrating across disparate middleware and frameworks.

[kevinchalet]

True, HttpResponseException is always caught by Web API, but my remark was about its ability to flow a status code and some content at the same time, not the fact it never escapes from Web API (BTW, I could find dozens of samples/tutorials where HRE is used in controllers. For instance: http://www.asp.net/web-api/overview/testing-and-debugging/exception-handling)

Here's a simple case to illustrate why a special exception could be convenient:

https://github.com/AspNet-OpenIdConnect-Server/Owin.Security.OpenIdConnect.Server/blob/dev/src/Owin.Security.OpenIdConnect.Server/OpenIdConnectServerHandler.cs#L102

In this code path, we want to make sure that the authorization request has been made using either GET or POST. If it's not the case, that's obviously a bad request and we want to inform the user with a 400 status code. Currently (and just like the OAuth2 server built in Katana does), we just return some HTML payload containing the error details.

Using the approach I'm suggesting, I could throw an exception containing a 400 status code and a standard page/message explaining what happened while - potentially - allowing the end developer to catch this exception using your ErrorHandler (to display a nice MVC/Razor view). If the exception was not caught, the host would simply have to return the response embedded in the exception.

[Tratcher]

If the exception was not caught, the host would simply have to return the response embedded in the exception. - Production servers should never display the content of an exception to the end user. Creating a special exception that is indented to be shown to the user is a non-starter.

I think your example scenario is better addressed with the new custom status code middleware we've been considering. Your middleware sets a status code (e.g. 400), and may include content. The custom status code middleware can then choose to let that content pass through, wrap it, or replace it. There's no reason to use exceptions to control this flow.

Exceptions are not for flow control, they are for uncontrolled failures. I don't know how many ways I can say it. When you thrown an exception you have no say in how it gets handled, that's entirely up to the caller.

[kevinchalet]

That's a valid point, even if I don't necessarily agree (while it's up to a caller to handle an exception, nothing prevents you from sharing custom data the caller could use to handle it).

Anyway, if you plan to create the kind of middleware you're mentioning, I won't fight to get a special exception type 😄

[brentschmaltz]

If a handler throws, will the next handler be called? I think not. Given that we need sites to handle multiple providers for the same protocol, throwing may not be the right thing say if the signature fails, since the next handler using different signing key would succeed.

[Tratcher]

Related: http://katanaproject.codeplex.com/workitem/419

[Eilon]

@lodejard @Tratcher @HaoK some notes to consider:

1. Look at all the places we have a try/catch and either remove them entirely, or "tighten them up" to be more constrained, log the error (maybe), and then rethrow.
2. Consider scenarios such a login failure loops and how an app developer can fall into the pit of success (i.e. don't have infinite loops by default).

[Tratcher]

Design recommendation:

• Remove catch-all exception handling from all the middleware. Let the ExceptionHandler middleware catch and deal with the exceptions.
• Handle known errors as granular as possible. (e.g. g+ login not enabled.)
• For OAuth/OIDC/Twitter protocol errors (e.g. we get back an "error" response when the use hits cancel), add a new ProtocolError event, where the default behavior is to redirect to an auth failed path with query parameters describing the error.

[blowdart]

Depending on the error detail we may need to squash the error into something more generic, e.g. crypto errors. Can someone draw up a list of the errors thrown for review?

[Tratcher]

The ProtocolError redirect would only be for error messages the 3rd party sent to us, e.g. the user pressed cancel. These were already sent to us via a client side redirect, so the client had the chance to see them. Crypto errors would be exceptions handled by the ExceptionHandler middleware.

[blowdart]

I mean in reference to the the remove catch-all exception handling comment, and the as granular as possible recommendation

[HaoK]

I'll send out the PR for this today, but got sucked into a config rename :/

[HaoK]

Closing this since we have the main changes/design in. Will use the other bugs to track resolving specific flows for each error condition. 409b502