Auth API changes - Async only and add challenge behavior #323

HaoK · 2015-06-03T20:26:30Z

Changes needed for Authentication changes in security:

IAuthenticationHandler is now mostly async, AuthenticationManager.Authencate is a sync wrapper
Http.Abstractions now depends on Http.Features to reuse AuthenticateContext in AuthManager
Nuke AuthenticateResult, with Authenticate api changed to use AuthenicateContext
Add sugar methods which just take scheme
Remove 401 setting behavior for challenge in manager (Handler will be responsible for everything)
Add ChallengeBehavior enum/property to ChallengeContext
Add new Forbidden API which maps to (Challenge with Forbidden(403) behavior)

cc @lodejard @davidfowl @Tratcher @divega

Tratcher · 2015-06-03T20:31:59Z

src/Microsoft.AspNet.Http.Abstractions/project.json

@@ -2,6 +2,7 @@
  "version": "1.0.0-*",
  "description": "ASP.NET 5 HTTP object model. HttpContext and family.",
  "dependencies": {
+    "Microsoft.AspNet.Http.Features": "1.0.0-*",


@kichalla Didn't you already add this?

@Tratcher : I did not check this in actually...i wanted to combine Session related changes with the checkin...

darrelmiller · 2015-06-03T23:08:03Z

Would it be possible for someone to explain the reasoning behind changing to this style of API? Personally I find the new style a major regression from the point of view of clarity, readability and discovery.

Instead of a function where we pass the Scheme to Authenticate() and get a result we now must construct a context object that will be mutated by the Authenticate method.

This approach forces me to ask a ton of questions about API:

How should the context object be constructed? Is the ctor parameter enough? Do I also need to set properties on it? Does it need to be passed to another method first for other mutations?
How do I determine failure? Will the method throw if Authentication fails? What property on the context reflects success?
What properties will success update on the context object? Which properties are inputs, which are outputs? Is that context object thread safe? What happens if I pass it elsewhere and some other code is also passing it to an another Authenticate method?

Functions/Methods have signatures for a reason. They have input parameters and they have return values. This allows to reason about the effects and non-effects of calling them. This use of context objects completely eliminates these benefits. Why are we doing it?

HaoK · 2015-06-03T23:11:02Z

Just to be clear, there still is the following API (which now directly gives you the principal):

ClaimsPrincipal Authenticate(string authenticationScheme)

Its only if you are using the less commonly accessed AuthenticationProperties/Description where you have to use the context

darrelmiller · 2015-06-03T23:14:41Z

@HaoK But that's syntactic sugar over the primary API which uses the context object, right?

HaoK · 2015-06-03T23:19:32Z

Depends on how far you want to take that line of argument, the whole AuthenticationManager API is 'sugar' on top of the HttpFeature which has always looked this way (AuthenticateContext)... The most commonly used API should be the new simpler ClaimsPrincipal Authenticate. We removed the intermediate API which returned a tuple of 3 things from the Context. The intent is to guide people to the new simplier API.

darrelmiller · 2015-06-03T23:31:23Z

I agree that the simpler ClaimsPrincipal Authenticate is better than the API that returned AuthenticationResult. And if the objective is to guide people towards the simpler API for the majority of cases then I would agree that is a good thing.
I'm just frustrated by context objects throughout the framework being the primary interaction method. Glad to hear that's not the case here.

davidfowl · 2015-06-04T04:30:30Z

src/Microsoft.AspNet.Http/Authentication/DefaultAuthenticationManager.cs

@@ -117,6 +95,18 @@ public override void Challenge(string authenticationScheme, AuthenticationProper
            }
        }

+        // You are not allowed access
+        public void Forbidden(string authenticationScheme, AuthenticationProperties properties)


Are we changing the ChallengeBehavior enum to match the naming as well?

kevinchalet · 2015-06-06T08:42:44Z

Functions/Methods have signatures for a reason. They have input parameters and they have return values. This allows to reason about the effects and non-effects of calling them. This use of context objects completely eliminates these benefits. Why are we doing it?

I couldn't agree more with you, sir 👍

HaoK · 2015-06-08T05:10:05Z

Renamed Forbidden => Forbid and added overloads to abstract base class to make it friendlier (optional AuthenticationProperties, and exposed the raw abstract Challenge overload which takes ChallengeBehavior which will make @lodejard happy)

HaoK · 2015-06-08T05:11:19Z

One thing I couldn't figure out how to do yet is remove the 401 setting inside of Challenge, @lodejard was adamant that this behavior belongs in the IAuthenticationHandler instead, I haven't quite gotten that to work yet.

HaoK · 2015-06-12T23:17:29Z

Per discussion with @Tratcher made AuthManager apis async to try and eliminate duplicate code paths in AuthHandlers.

kevinchalet · 2015-06-14T16:48:33Z

src/Microsoft.AspNet.Http.Abstractions/Authentication/AuthenticationManager.cs

+        private static TResult RunSync<TResult>(Func<Task<TResult>> func)
+        {
+            // REVIEW: Do we need to flow culture info still?
+            return _myTaskFactory.StartNew(() => func()).Unwrap().GetAwaiter().GetResult();


OMFG, what's that? 😱

It's bad, bad, bad, horribly bad: you're offloading an async delegate (() => func()) to the thread pool before unwrapping it and then blocking on this new task 😄

This clearly creates unnecessary pressure on the thread pool, specially since you use StartNew on an Task-returning delegate, which is supposed to be non-blocking.

Yeah well this was the only safe way to do this historically in asp.net (calling async methods synch). Other less nasty ways would deadlock in some situations. I'm not sure this is still needed in the new world.

I'd be really surprised if this was still necessary now that we no longer have the System.Web call context.

Ok cool, so its definitely not as gross with out this...

@davidfowl What do you think about this change? (Making everything async only for auth, and only having a sync public wrapper in the manager?

HaoK · 2015-06-18T21:52:15Z

After discussing with @lodejard @Tratcher @Eilon concluded that we are going with async only, removing prototype tag, these should be the changes needed for security.

kevinchalet · 2015-06-18T21:56:14Z

src/Microsoft.AspNet.Owin/OwinEnvironment.cs

@@ -61,7 +61,7 @@ public OwinEnvironment(HttpContext context)
                { OwinConstants.ResponseReasonPhrase, new FeatureMap<IHttpResponseFeature>(feature => feature.ReasonPhrase, (feature, value) => feature.ReasonPhrase = Convert.ToString(value)) },
                { OwinConstants.ResponseHeaders, new FeatureMap<IHttpResponseFeature>(feature => feature.Headers, (feature, value) => feature.Headers = (IDictionary<string, string[]>)value) },
                { OwinConstants.ResponseBody, new FeatureMap<IHttpResponseFeature>(feature => feature.Body, () => Stream.Null, (feature, value) => feature.Body = (Stream)value) },
-                { OwinConstants.CommonKeys.OnSendingHeaders, new FeatureMap<IHttpResponseFeature>(feature => new Action<Action<object>, object>(feature.OnSendingHeaders)) },
+                { OwinConstants.CommonKeys.OnSendingHeaders, new FeatureMap<IHttpResponseFeature>(feature => new Action<Func<object, Task>, object>(feature.OnResponseStarting)) },


I'm not sure you can change the signature of server.OnSendingHeaders without breaking tons of OWIN middleware that expect a Action<Action<object>, object>.

http://owin.org/spec/spec/CommonKeys.html

Tratcher · 2015-06-19T15:24:20Z

src/Microsoft.AspNet.Http.Abstractions/Authentication/AuthenticationManager.cs

        }

-        public abstract void Challenge(string authenticationScheme, AuthenticationProperties properties);
+        // Leave it up to authentication handler to do the right thing for the challenge
+        public Task ChallengeAsync([NotNull] string authenticationScheme, AuthenticationProperties properties)


You're using [NotNull] here but then passing in null from the overloads. This can only end well...

Whoops I had a bad feeling when I added them yesterday while rebasing to dev. Now I know why! Good catch :)

Updated PR to use [NotNull] everywhere, Challenge sugar methods now pass string.Empty since we were already doing string.IsNullOrWhitespace checks, this shouldn't affect anything. Also nuked the overloads of SignIn/SignOut that didn't require a scheme, this was on the todo list from earlier API reviews with Lou

IMHO, using string.Empty is a bit ugly. And removing SignIn/SignOut overloads that don't take a specific scheme while keeping the Challenge method that takes no parameter is a bit weird, no?

Naked Challenge is required for naked Authorize which is pretty important for MVC.

SignIn/SignOut with no arguments where any automatic middleware picks that up felt like a pretty unnecessary feature.

We are going to try and make it explicit so authenticationScheme is more understood since its always there...

Well, SignIn/SignOut with no arguments are a great way for the underlying application to keep the separation of concerns clear. For instance, controllers shouldn't have to know which schemes have been configured in Startup.cs request to globally sign in or sign out the user.

That's what identity is for. The feedback has been pretty consistent that AutomaticAuthentication is confusing. We need it for naked challenge and the automatic cookie/bearer flow. But for everything else we want to dial that back is our current thinking.

NotNull everywhere ("" for challenge overloads) No more automatic sign in/sign out, requires specifying a scheme always

HaoK · 2015-06-23T23:04:06Z

Added a new DisposeOnCompleted(IDisposable) since MVC was using this all over. Seemed generally useful. @Tratcher what do you think about shortening the name of the OnResponseXyz methods to simply OnXyz()? They already hang off of httpResponse, it seems a bit redundant to say response.OnResponseStarted/Completed.

response.OnStarted/OnCompleted seems just as clear... I'm touching all of the signatures already, so if we want to rename these, its a free opportunity

Tratcher · 2015-06-23T23:16:15Z

Yes, OnStarted/Completed is better.

HaoK · 2015-06-23T23:56:56Z

Updated with OnStarted/OnCompleted/OnCompetedDispose

Tratcher · 2015-06-24T18:14:14Z

src/Microsoft.AspNet.Http.Abstractions/HttpResponse.cs

-        }
+        public abstract void OnCompleted(Func<object, Task> callback, object state);
+
+        public virtual void OnCompletedDispose(IDisposable disposable) => OnStarting(_disposeDelegate, disposable);


This should be OnCompleted, not OnStarting.

That would explain the functional failures I was seeing :)

Tratcher · 2015-06-25T17:44:16Z

Yes, that looks right.

Tratcher · 2015-06-25T17:46:51Z

src/Microsoft.AspNet.Http.Abstractions/HttpResponse.cs

-        }
+        public abstract void OnCompleted(Func<object, Task> callback, object state);
+
+        public virtual void OnCompletedDispose(IDisposable disposable) => OnCompleted(_disposeDelegate, disposable);


[NotNull] for everything but state.

Tratcher · 2015-06-25T17:49:55Z

HaoK · 2015-06-26T00:10:59Z

5fe8037

dnfclas added the cla-already-signed label Jun 3, 2015

Tratcher reviewed Jun 3, 2015
View reviewed changes

davidfowl reviewed Jun 4, 2015
View reviewed changes

HaoK mentioned this pull request Jun 5, 2015

AuthenticationHandler now fully Async, SignOut/SignIn/Challenge API changes aspnet/Security#283

Closed

HaoK changed the title ~~Auth API changes~~ [Prototype] Auth API changes Jun 12, 2015

kevinchalet reviewed Jun 14, 2015
View reviewed changes

HaoK changed the title ~~[Prototype] Auth API changes~~ [Prototype] Make Auth interfaces Async only, AuthManager public API provides sync wrapper Jun 15, 2015

HaoK changed the title ~~[Prototype] Make Auth interfaces Async only, AuthManager public API provides sync wrapper~~ [Prototype] Auth API changes (async only, challenge behavior) Jun 15, 2015

HaoK changed the title ~~[Prototype] Auth API changes (async only, challenge behavior)~~ Auth API changes - Async only and add challenge behavior Jun 18, 2015

kevinchalet reviewed Jun 18, 2015
View reviewed changes

HaoK force-pushed the haok/6-3auth branch from 43e1691 to 107085c Compare June 18, 2015 22:36

Tratcher reviewed Jun 19, 2015
View reviewed changes

HaoK added 14 commits June 22, 2015 14:36

Simplify sync wrapper

ee6c9c2

Auth API changes

4ce288d

Cleanup comments

a5ff11f

Update public API

78e6ef2

Make AuthenticationManager async mostly

e7ba102

Remove sync helpers

7b025cd

Add NotNull

873b43d

Fixup

1b1e885

NotNull everywhere ("" for challenge overloads) No more automatic sign in/sign out, requires specifying a scheme always

Add stateless OnResponseStarting sugar

01334ab

Moar sugah

8ba2e47

Use C# 6 goodness

2287e05

Less better sugar

cf6d150

Owin tweak

de4ba9c

Add disposeOnCompleted for MVC

56e6760

HaoK force-pushed the haok/6-3auth branch from 208b55a to 56e6760 Compare June 23, 2015 23:01

Remove Response from OnStarting/Completed

f5c5ecd

HaoK mentioned this pull request Jun 24, 2015

React to Http changes aspnet/Session#38

Closed

Tratcher reviewed Jun 24, 2015
View reviewed changes

HaoK mentioned this pull request Jun 24, 2015

React to Security/Http changes aspnet/Mvc#2737

Closed

Fix Owin stuff (I think)

5e0b7a9

Tratcher reviewed Jun 25, 2015
View reviewed changes

HaoK closed this Jun 26, 2015

HaoK deleted the haok/6-3auth branch September 23, 2015 21:17

kevinchalet mentioned this pull request Feb 24, 2016

Update AuthenticationManager.AuthenticateAsync to return an AuthenticationTicket #572

Closed

Auth API changes - Async only and add challenge behavior #323

Auth API changes - Async only and add challenge behavior #323

Conversation

HaoK commented Jun 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darrelmiller commented Jun 3, 2015

HaoK commented Jun 3, 2015

darrelmiller commented Jun 3, 2015

HaoK commented Jun 3, 2015

darrelmiller commented Jun 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinchalet commented Jun 6, 2015

HaoK commented Jun 8, 2015

HaoK commented Jun 8, 2015

HaoK commented Jun 12, 2015

Choose a reason for hiding this comment

HaoK Jun 14, 2015 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Jun 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK Jun 19, 2015 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Jun 23, 2015

Tratcher commented Jun 23, 2015

HaoK commented Jun 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Jun 25, 2015

Choose a reason for hiding this comment

Tratcher commented Jun 25, 2015

HaoK commented Jun 26, 2015