Cookie auth always returns 302 Found

[jods4]

I upgraded from beta-7 to rc1.
Following the work done in issue #458 I rewrote my code like this:

      app.UseCookieAuthentication(new CookieAuthenticationOptions
      {
        Events = new CookieAuthenticationEvents
        {
          OnRedirectToAccessDenied = context => { context.Response.StatusCode = 403; return Task.CompletedTask; },
          OnRedirectToLogin = context => { context.Response.StatusCode = 401; return Task.CompletedTask; },
        }
      });

But when sending an HTTP request without the auth cookie, I receive a 302 found that has Location set to login page rather that 401.

Putting a breakpoint in my delegate, it seems it is not called at all (makes sense then...).

What am I doing wrong?

[Eilon]

@HaoK can you try to repro this?

[tmm360]

I've no events, some calls return me 200 (how should be), others return me 302 Found.
This looks related to transport encoding. If server returns "Content-Encoding: gzip" I get 200, otherwise if plain text I get 302.
EDIT: It's not related to transport encoding, the first call is compressed and the second (and so on) near to the first is not compressed. The same is with this problem. The first returns 302, others near to the first return 200.

[tmm360]

I've identified the root cause for my problem: aspnet/KestrelHttpServer#365
Some calls don't recognize the https protocol, so a my (wrong) redirect code handles the request and returns 302 pointing to the secure URI.

[HaoK]

I cannot repro this on current bits. If you can repro this with RC2 bits, please reopen the issue...

I tried:

            options.Events = new CookieAuthenticationEvents
            {
                OnRedirectToAccessDenied = context => {
                    context.Response.StatusCode = 403; return Task.FromResult(0);
                },
                OnRedirectToLogin = context => {
                    context.Response.StatusCode = 405; return Task.FromResult(0);
                },
            };

And I was able to verify that the response code was 405 MethodNotAllowed so its getting called

[jods4]

@HaoK I'll try again when the RC2 bits are out and let you know.

[jods4]

Today I understood (by accident) what is actually happening.
It seems none of my CookieAuthenticationOptions are handled, apparently the options are completely dropped from config.

My code looks like this, any idea?

app.UseCookieAuthentication(new CookieAuthenticationOptions
      {
        CookieName = "Test",
        SessionStore = new MemoryCacheTicketStore(),
        Events = new CookieAuthenticationEvents
        {
          // TODO: not working in RC1?
          OnRedirectToAccessDenied = context => { context.Response.StatusCode = 403; return Task.CompletedTask; },
          OnRedirectToLogin = context => { context.Response.StatusCode = 401; return Task.CompletedTask; },
        }
      });

[jods4]

OK some final feedback on this ticket.

So the actual problem was that I'm using ASP.NET Identity. When I thought that I was configuring my cookies, it did not have any effect because the cookie middleware used is in fact the one linked to the application auth. scheme, as defined by Identity.
The proper way is to configure options.Cookies inside services.AddIdentity().
So: problem was in my code.

Some feedback on all this, though. The configuration of this stuff is rather confusing and being wrong is totally silent. Some config APIs are really not consistent, not intuitive and easy to get wrong. Case in point: AddIdentity.

(1) most middleware accept an option instance, but AddIdentity requires a callback. Not too bad, although not consistent + passing an instance with C# initializer syntax is nicer.

(2) you will be tempted (I was) to do
options.Cookies.ApplicationCookie = new CookieAuthenticationOptions { ... };
But doing that is a sure way to shoot yourself in the foot. Identity has a few non-default values in there that you ought to respect and copy in your instance should you choose this way (esp. the AuthenticationScheme, otherwise nothing works).
So you go on with the following model, which isn't especially nice:

services.AddIdentity(options =>
{
  options.Lockout = new LockoutOptions() { ... }; // Initializer seems fine.
  var appCookie = options.Cookies.ApplicationCookie; // Initializer is not fine here
  appCookie.CookieName = "MyApp.Auth";

Then comes appCookie.Events. And the pattern breaks again, because there are no setters on those properties (why not?).
So at this point you have to switch back to a new instance:

  appCookie.Events = new CookieAuthenticationEvents
  {
    OnRedirectToAccessDenied = context => { context.Response.StatusCode = 403; return Task.CompletedTask;

And now you did a mistake again! Because Identity has a default events to validate the security stamp. So if you want to faithfully enhance the default options, you have to know that you should add the following line to your config:

    OnValidatePrincipal = SecurityStampValidator.ValidatePrincipalAsync
  }
});

Honestly, without digging deep into the sources, there is no way you can come up with the correct configuration on your own (by correct I mean: take the default config and tweak just one or two options to fit your needs).

This code is not intuitive at all, is ugly and most "mistakes" are silent.

[Tratcher]

Sounds like at a minimum we need to remove the setter on options.Cookies.ApplicationCookie. @HaoK

[Eilon]

@Tratcher log a new bug for this?

[HaoK]

I'm not sure removing the setter is the right thing. I do agree this is pretty complex but basically it comes down to two different scenarios for when to tweak vs replace via the setter.

1. If you just want to tweak the default identity cookie options, you shouldn't set, you should just modify the existing instance.
2. For users that want full control and want to change lots of settings it's probably far easier to new up your own cookie options instance and set it rather than unsetting/clearing all the potentially unwanted identity defaults.

AddIdentity is also a bit weird since unlike UseCookie and the other middleware, the cookie options are configured via identity options because UseIdentity is what eventually calls UseCookie(options.Cookies.Application)

[jods4]

@HaoK not sure what the right thing is, but this is too complex to use. Given it's security and may silently not do what you expect this is bad.

Modifying the instance will surely get a lot easier if C# 7 gets with to modify multiple properties at once.

Setting a new instance "feels" right and since I modified 6 properties, I thought it may qualify as "lots" as you said. The problem is that when you do new CookieAuthenticationOptions() you get none of the Identity default options. Maybe it's not too bad, except for AuthenticationScheme. The problem with this one is that it's also in Cookies.ApplicationCookieAuthenticationScheme. Maybe the solution is to remove this redundant property (use ApplicationCookie.AuthenticationScheme) or have Identity copy it into the cookie config to make sure it's consistent.

The other trouble point is CookieAuthenticationOptions.Events. No setter on those properties, so you have to new it up. Then it's easy to miss that there is a default OnValidatePrincipal = SecurityStampValidator.ValidatePrincipalAsync. That's a kind of easy, silent, "not safe by default" regression.

[Tratcher]

At this point I recommend taking this discussion to aspnet/Identity#744

[HaoK]

Yeah lets take the identity specific stuff to that repo. In regards to the Events pattern, I personally do not like that at all either, but I lost that battle...

I would have preferred having no interface and a very simple Events class with a get/set delegate property for each event