Comments (22)
Proposal: Throw an exception and let the new ErrorHandler middleware deal with the failure.
from security.
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.
from security.
@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.
from security.
Developers are free to examine the exception and set their own status code. We just wanted the default to be 500.
from security.
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).
from security.
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
from security.
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.
from security.
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.
from security.
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.
from security.
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.
from security.
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:
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.
from security.
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.
from security.
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
from security.
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.
from security.
Related: http://katanaproject.codeplex.com/workitem/419
from security.
@lodejard @Tratcher @HaoK some notes to consider:
- 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.
- 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).
from security.
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.
from security.
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?
from security.
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.
from security.
I mean in reference to the the remove catch-all exception handling comment, and the as granular as possible recommendation
from security.
I'll send out the PR for this today, but got sucked into a config rename :/
from security.
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
from security.
Related Issues (20)
- About policy in Multiple Schemes 403 problem HOT 2
- AVRO serialization error - Could not find any matching known type for 'Newtonsoft.Json.Linq.JToken' using c# HOT 3
- How to Validate AccessToken in WebAPI core 2.0 getting from IdentityServer3 HOT 1
- Multiple refresh for authentication on server HOT 33
- Oauth MicrosoftAccount Token Request Issue HOT 6
- OAuth redirect_uri using http instead of https on Azure App Service Linux HOT 9
- Is there a way to Decrypt encrypted SAML token?? HOT 9
- Set redirect from authorization handler HOT 5
- JWT payload "unique_name" not mapped correctly HOT 2
- .
- HttpContext.SignInAsync sets null to Identity claims data HOT 3
- Handling incomplete remote signouts HOT 15
- .net core api authentication using ws-federation HOT 7
- AuthenticationProperties object is not passed to ChallengeAsync / ForbidAsync HOT 11
- [Net Core 2.0] AccessDeniedPath ignored in cookie authentication HOT 1
- Docker Swarm + nginx + WS-Federation: multiple redirection issue HOT 33
- SSO for Microsoft Outlook using auth0 HOT 6
- Redirect URI is ignored HOT 2
- Wctx parameter getting overridden, breaking functionality HOT 8
- ASP.NET Core OpenId Authentication in Container with TLS Termination in WAF HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from security.