search

aspnet / Announcements

Subscribe to this repo to be notified about major changes in ASP.NET Core and Entity Framework Core
Other
1.66k stars 80 forks source link

Microsoft.AspNet.Authentication.Cookies.CookieAuthenticationOptions now has a new AccessDeniedPath #40

Open blowdart opened 9 years ago

blowdart commented 9 years ago

Rather than have a single LoginPath browsers are redirected to with cookie based authorization fails we have introduced a new AccessDeniedPath property on CookieAuthenticationOptions.

If the LoginPath property is set and a request generates an http error with a status code 401 the request will be redirected to the path specified.

If the AccessDeniedPath property is set and a request generates an http error with a status code 403 the request will be redirected to the path specified.

If the properties are not set then the status codes are returned to the browser.

dmccaffery commented 9 years ago

I like it!

guardrex commented 9 years ago

Setting AccessDeniedPath doesn't stop OpenIdConnect middleware from redirecting to AAD login. Where can I file a request for a similar feature for an AccessDeniedPath option for app.UseOpenIdConnectAuthentication? ... aspnet/Identity? ... aspnet/Security? ... elsewhere?

blowdart commented 9 years ago

It only applies to cookie middleware. If you wanted the same in other middleware you should file a feature request in aspnet/Security.

brockallen commented 9 years ago

trolling Not ForbiddenPath? /trolling

LandryDubus commented 9 years ago

Yeah, this new configuration option should be named ForbiddenPath and not AccessDeniedPath according to the specified http status code handled.

This is in fact quite misleading as we would suspect the Authorize attribute or any custom AuthorizationFilterAttribute to redirect to this page (instead of the login page) when the autorization fails because the user does not have the right credentials. This is not the case, it stills redirect to the login page because when the OnAuthorization method fails, a 401 status code is returned and this is quite right as this is the right status code to return in this case according to http standards: http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html

According to http standards, a 401 status code should be returned when a user is not authorized either because he is not authenticated or because he does not have the sufficient credentials.

In fact, there should be two configuration options handling 401 status codes.

This could be distinguished based on WWW-Authenticate Response Header that comes with the 401 status code in the response (more precisely with the stale and nonce headers): http://www.ietf.org/rfc/rfc2617.txt

This could be really helpful in applications that wishes to differentiate between the two. We could redirect to login page if the user is not logged in and redirect to a default page (Home for instance) if the user is logged in but does not have sufficient credentials to acces a requested action. This would be particularly helpful when creating custom AuthorizationFilterAttribute.

blowdart commented 9 years ago

Unfortunately we're stuck with AccessDeniedPath for back compat reasons.

As for making AccessDeniedPath for 401 that is arguably wrong. In those cases middleware should be returning a 403, which is why we added the new option.

As I've said elsewhere with the new Policy based authorization approach we would hope the need for creating your own AuthorizationFilterAttribute has gone away. If a policy fails we go the 403 route.

LandryDubus commented 9 years ago

I think you misunderstood what I was saying because I used the same AccessDeniedPath name for the option I proposed and the option you have already offered.

My point was that there shoud be 3 configuration options:

I understands that there could be back compatibility reasons that do not allow you to use such names for the configuration options I propose, but my point is not really about the naming but more about the middleware behaviour that seems wrong to me based on http standards.

A 403 code should not be issued because the user do not have sufficient credentials according to a policy but because the action requested is unauthorized for any users whatever there credentials are.

The thing about the AuthorizationFilterAttribute is irrelevant as this is in my opinion a design flow in the behaviour of the middleware.

IMO, if a policy fail, we should not go to 403 but to 401 with the right WWW-Authenticate Response Header according to the situation.

As I said, this is but my opinion.

I understand I could be wrong.^^

No harm intended.

blowdart commented 9 years ago

I disagree with the 401 and Authenticate response I'm afraid. All that tells you is what authentication type is expected, although in the case of forms based auth the middleware will intercept and change it to a redirection.

There was some added clarity to the RFCs

The 401 (Unauthorized) status code indicates that the request has not been applied because it lacks valid authentication credentials for the target resource. The server generating a 401 response MUST send a WWW-Authenticate header field (Section 4.1) containing at least one challenge applicable to the target resource.

A server that receives valid credentials that are not adequate to gain access ought to respond with the 403 (Forbidden) status code.

We view having an identity but not enough rights as valid, but not adequate. However your middleware could always override this and set its own status code, but I'd hazard a guess that's going to confuse the vast majority of people as it's veering from the common understanding.

This discussion is probably best taken to an issue on the security repo, rather than in announcements, as people are going to get spammed with the issue updates. If you want to do that I'll take the discussion there. For now, to avoid the spam, I'm going to lock the issue here as that's what we've decided to do with new announcements.