aspnet / Security

[Archived] Middleware for security and authorization of web apps. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
1.27k stars 600 forks source link

Auth middleware /AuthorizeAttribute do not take into account AutomaticChallenge = false #687

Closed Alexander-Taran closed 8 years ago

Alexander-Taran commented 8 years ago

While trying to have 2 cookie auth middlewares with same cookie name but different reaction to unauthorized access (no auto challenge for api controolers) we stumbled upon a strange behavior:

When added 2 schemes to startup:

    app.UseCookieAuthentication(x =>
            {
                x.AuthenticationScheme = "API";
                x.CookieName = "sec";              
                x.LoginPath = PathString.Empty;              
                x.AutomaticChallenge = false;
                x.AutomaticAuthenticate = true;
            });

            app.UseCookieAuthentication(x =>
            {
                x.AuthenticationScheme = "MVC";
                x.CookieName = "sec";           
                x.LoginPath = PathString.Empty;         
                x.AutomaticChallenge = false;
                x.AutomaticAuthenticate = true;
            }); 

and adding authorize attribute with scheme on actions/controllers we get a redirect to /account/login even though we specified LoginPath as Empty.

attaching solution with repro: with 3 urls of interest : "/" (home/index) , "api/scheme-default" (plain authorize attribute) and "api/scheme-api" auth.zip

kevinchalet commented 8 years ago

By design: https://github.com/aspnet/Security/issues/508 https://github.com/aspnet/Security/issues/505.

I mentioned a few times this behavior was totally inappropriate and unnecessary, but so far, nothing has changed: https://github.com/aspnet/Security/issues/508#issuecomment-146686060.

blowdart commented 8 years ago

As @PinpointTownes , by design. Will not fix.

Once you start getting into multiple schemes you need to use the ActiveAuthenticationSchemes parameter on the Authorize attribute, or specify it in policy, via policy.AuthenticationSchemes.Add();

If you don't want the redirect then turn AutomaticAuthenticate to be false. LoginPath being empty has no effect here.

And to say it, as I always do, using cookies on an API controller isn't really best practice.

kevinchalet commented 8 years ago

If you don't want the redirect then turn AutomaticAuthenticate to be false. LoginPath being empty has no effect here.

That would be a valid suggestion if the authentication stack didn't throw an InvalidOperationException when calling ChallengeAsync() without having at least one middleware with AutomaticAuthenticate = true in the pipeline :trollface:

Sadly, it's not...

blowdart commented 8 years ago

Which is why in that case, like the authorize attribute needs a scheme, so does ChallengeAsync()

kevinchalet commented 8 years ago

But in this case, setting AutomaticAuthenticate to false is useless and doesn't solve the issue discussed here.

blowdart commented 8 years ago

Well the problem here is AutomaticAuthenticate is set to true on both. Which will, of course, confuse things, so they both run, not matter what, and lo, there's a redirect.

So set AutomaticAuthenticate to be false on both, then use the ActiveAuthenticationScheme in policy or attribute to choose which one fires on which controller.

Or am I horribly misunderstanding this?

kevinchalet commented 8 years ago

Well the problem here is AutomaticAuthenticate is set to true on both.

Well, not really. The problem here - not being able to nullify LoginPath to prevent the 401 -> 302 redirect - would be exactly the same with a single authentication middleware.

I think we all agree to say that using cookies for API is bad and risky (though the revamped Antiforgery block is truly promising, IMHO) but the whole point of this ticket is about being able to disable the automatic redirection, something the default events class does OTB for AJAX requests (without issuing a challenge header, BTW): https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication.Cookies/Events/CookieAuthenticationEvents.cs#L42-L51

Which will, of course, confuse things, so they both run, not matter what, and lo, there's a redirect.

Not exactly, actually: when the first active cookie middleware handles the challenge and replaces the 401 status code by a 302 response, the second instance is not invoked, even if it's configured to use automatic challenge (https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication/AuthenticationHandler.cs#L149).

blowdart commented 8 years ago

Hmm AutomaticChallenge doesn't see to be working here as I expect - @HaoK even if you set every middleware to automaticchallenge = false and automaticauthenticate = false a redirect is kicking in.

But having said that. If an active cookie middleware is invoked none of the others will get invoked, that's just how it works. One has already woken up and said "I'm handling this".

HaoK commented 8 years ago

Can you clarify exactly which behavior that you'd like me to investigate?

If there's a naked [Authorize] and there are no automatic challenge/authenticate middlewares, we still get a redirect kicking in?

HaoK commented 8 years ago

Oh, I figured out what I think the behavior is, you can't really clear out the Loginpath, if you set it to null or empty, we set it to the default value that you are seeing. But if you want to disable the redirects, you should just be overriding the event to make it not redirect:

Set the OnRedirectToLogin event to be a no op:

CookieAuthenticationEvents
        public Func<CookieRedirectContext, Task> OnRedirectToLogin { get; set; } = context =>
        {
            return Task.FromResult(0);
        };
kevinchalet commented 8 years ago

That's an option, but it's not really user friendly (events are really made for advanced cases).

I'm sorry to repeat it again and again, but we should be able to nullify the paths to disable redirections (which is the default behavior with AJAX requests)

HaoK commented 8 years ago

I get it, but we chose to make this behavior pretty much deterministic, if you have cookies, it will redirect on unauthorized/access denied. If you don't want this behavior (don't use cookies), or override the event. We chose not to use the path as a magic backdoor way to disable it.

HaoK commented 8 years ago

Its an orthogonal issue that events suck... :) I agree that they are way too hard to tweak/use...