[AuthN] Clean up 401-403 AuthenticateCalled

[HaoK]

Currently for 401-403 to work, the AuthenticateCalled flag must be set. This is a pretty obscure thing for subclasses to be required to do and is very easy to miss. Cookie was missing this and as a result broke 401-403. We should revisit this and see if there's anyway to make this more seamless...

@blowdart

[HaoK]

Summary of plan:

HttpAbstractions:

public enum ChallengeBehavior { Automatic, Unauthorized, Forbidden }

Add ChallengeBehavior property to ChallengeContext

new methods on AuthenticationManager:

// calls Challenge with Forbidden behavior
public void Forbidden(string authenticationScheme, AuthenticationProperties properties)

// Existing challenge uses Automatic behavior (where the middleware decides challenge behavior

AuthenticationHandler changes:

• AuthenticationScheme is required for SIgnIn/SignOut, no more overloads that sign in/sign out all automaticAuthentication middlewares (this setting now only affects ApplyResponseChallenge automatically getting called on the way out, and Authenticate getting called on the way in)
• Immediately change status codes/set headers/cookies for SignIn/SignOut/Challenge instead of deferring the changes until ApplyResponse
• ApplyResponseChallenge will still need to check for the AutomaticAuthentication chase which will then call handler's Challenge method explicitly (the non chained version)
• There will be a helper method in AuthenticationHandler which uses new ChallengeBehavior property:

        public virtual void Challenge(ChallengeContext context)
        {
            if (ShouldHandleScheme(context.AuthenticationScheme))
            {
                HandleChallenge(context)
                context.Accept();
            }

            if (PriorHandler != null)
            {
                PriorHandler.Challenge(context);
            }
        }

protected virtual void HandleChallenge(ChallengeContext context) {
   switch (context.ChallengeBehavior) {
     case ChallengeBehavior.Automatic:
        // No op the status code has already been modified by someone else
        if (Response.StatusCode != 401) return;
        var ticket = Authenticate(options.AuthenticationScheme);

        // If there's principal in the ticket, AuthZ failed/Forbidden, otherwise its Unauthorized
        if (ticket.Principal != null) HandleForbidden();
        else HandleUnauthorized();
        return;
     case ChallengeBehavior.Forbidden:
         HandleForbidden()
         return;
     case ChallengeBehavior.Unauthorized:
         HandleUnauthorized();
         return;
   }
}

// Cookie Middleware would override this to 302 to AccessDenied if options set
public virtual void HandleForbidden() {
    Response.StatusCode = 403;
}

// Cookie Middleware would override this to 302 to LoginPage if options set
public virtual void HandleUnauthorized() {
    Response.StatusCode = 401;
}

@lodejard is this what you had in mind?

cc @blowdart @davidfowl @Tratcher @divega

[kevinchalet]

Kewl, another totally insane design for 403 handling 😄

I'll probably repeat myself, but authentication middleware are NOT the right place for that. The weirdest thing with this new approach is that you (= the ASP.NET team) don't seem to realize that it is semantically aberrant: a "challenge" operation (or whatever you call it) cannot result in a 403 response (like your snippets suggest): a challenge is ALWAYS the consequence of a 401 response: http://tools.ietf.org/html/rfc2617

AuthenticationScheme is required for SIgnIn/SignOut, no more overloads that sign in/sign out all automaticAuthentication middlewares (this setting now only affects ApplyResponseChallenge automatically getting called on the way out, and Authenticate getting called on the way in)

Immediately change status codes/set headers/cookies for SignIn/SignOut/Challenge instead of deferring the changes until ApplyResponse

Interesting. What's the reason behind these changes?

[kevinchalet]

@HaoK FYI, I'm prototyping a new authorization middleware that reuses the (excellent) CORS approach and offers a far better way (at least, IMO) to handle authorization and set the appropriate response status code: https://github.com/PinpointTownes/Security/commit/7ac1a2515015a98dc6f4919f28baa9db33beb50a#diff-7bf20fd130932f29f2028b7126281f84R58

(it doesn't build as I haven't updated the tests but you should get the idea)

[HaoK]

#283