Add RemoteAuthenticationHandler base (support for error handling) #495

HaoK · 2015-09-25T23:31:02Z

dnfclas · 2015-09-25T23:31:05Z

Hi @HaoK, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

HaoK · 2015-09-25T23:32:08Z

Not sure where best to integrate this with ODIC yet. Also still need to see if this makes sense in Cookies anywhere

kevinchalet · 2015-09-25T23:55:18Z

src/Microsoft.AspNet.Authentication/AuthenticationOptions.cs

+        /// The request path within the application's base path where the user-agent will be returned
+        /// when a failure occurs.
+        /// </summary>
+        public PathString ErrorHandlerPath { get; set; }


I'm not sure this property is general enough to put in the base AuthenticationOptions.

Where else could it live if we want the common error handler logic in the base handler?

The question is: do we want to have that in the base handler? This error handling is only applicable to authentication handlers implementing InvokeAsync (since you can't control the flow elsewhere) and I'm not sure all middleware will implement it.

Yeah well the other option is to introduce a base class for all of the Invoke style middlewares (not cookie/bearer) basically. I've been leaning towards that anyways, I'm not opposed to taking a stab at that and moving this error handling into the new common base. We need to rename InvokeAsync anyways, that name is terrible.

Perhaps RedirectingAuthenticationMiddleware as the name of the new shared base class for OAuth/OIDC/Twitter

What do you mean exactly by "common flows"? It sounds like a crazy idea 😄

Yeah you and @Tratcher both think I'm crazy :)

@PinpointTownes see new RemoteAuthenticationHandler shared base class, @Tratcher has grudgingly come around to its value... standardizes Invoke path for OAuth/Twitter/OIDC

This isn't needed anymore, correct?

HaoK · 2015-09-29T01:05:03Z

Updated with major changes:

Depends on HttpAbstractions: PR Auth changes: error handling + make error txt less sucky HttpAbstractions#421
Removed bools from protected Handle methods and use context.IsResponseCompleted
HandleAuthenticate now returns AuthenticateResult which contains Ticket and ErrorContext
ErrorContext contains an error message and optional error uri for redirect

HaoK · 2015-09-30T00:42:38Z

Updated with a stab at moving InvokeReplyPath to a new RemoteAuthenticationHandler base for OAuth and Twitter, I also sucked some of the common options into here as well like (BackChannel/CallbackPath). I also moved SigningIn to this base class/options, but I'm not even sure we really need this event anymore...

kevinchalet · 2015-10-01T00:23:55Z

src/Microsoft.AspNet.Authentication.JwtBearer/JwtBearerHandler.cs

                }

                if (authenticationFailedContext.Skipped)
                {
-                    return null;
+                    return new AuthenticateResult();
                }

                throw;


I'm not opposed to re-throwing exceptions by default at this stage. But they MUST be caught somewhere: we can't afford returning a 500 response for a simple invalid/expired token.

Make sure we don't forget about this. Once the higher level changes are agreed on, the fun process of going through each of the specific error/exception paths needs to be reviewed :/

HaoK · 2015-10-02T01:49:35Z

New iteration changes:
RemoteAuthorizationOptions is now the base class for OAuth/Twitter/OpenIdConnectOptions. It has a separate IRemoteEvents property which contains the Error and SigningIn events which are only invoked via the newly renamed HandleRemoteCallbackAsync. The old handler specific events are unchanged and continue to live in their stand alone Events property.

HandleErrorAsync is a no-op in the base AuthHandler, and overridden in RemoteHandler to invoke the Error event on RemoteOptions.RemoteEvents.

HaoK · 2015-10-09T01:20:02Z

Update:

Only RemoteAuthenticationHandler/Invoke code paths use and inspect the ErrorContext.
No events for TicketReceived/RemoteError, as that would pose issues with the derived handlers having to cast, or using new. (I think we should just nuke all of the event classes and switch to simple delegates for each on the options directly).
Nuked generics off of the base context types, instead each group (OAuth/Twitter/OIDC/Cookies) defines the correctly typed TOption lower down in their respective places.
OAuth/OIDC now return error authentication results instead of null tickets
Convenience methods of AuthenticationResult.Success/Failed

TBD: Renaming OIDC events, and adding control flow to cookies to see if BaseContext can be merged with BaseControlContext

kevinchalet · 2015-10-09T09:53:32Z

Nice 😄

@PinpointTownes see new RemoteAuthenticationHandler shared base class, @Tratcher has grudgingly come around to its value... standardizes Invoke path for OAuth/Twitter/OIDC

Actually, it doesn't look that bad but... 😄

No events for TicketReceived/RemoteError, as that would pose issues with the derived handlers having to cast, or using new. (I think we should just nuke all of the event classes and switch to simple delegates for each on the options directly).

Outch, it's a horrible pattern that will kill hundreds of poor kittens 😢
If this is the price to pay for having a new RemoteAuthenticationHandler, I'm not sure it's really worth it: events are for advanced flow control. Merging them with the options would create too much noise and make the real events harder to discover.

Nuked generics off of the base context types, instead each group (OAuth/Twitter/OIDC/Cookies) defines the correctly typed TOption lower down in their respective places.

Curious: what's the benefit? 😮

Convenience methods of AuthenticationResult.Success/Failed

Oh yeah, it's much nicer 👍

That said, I know wonder why we need this class at all, since it simply exposes the ticket and an exception... why not simply returning the ticket and throwing exceptions? 😄
Also, I'm not sure how I feel when I see one of the Failed methods creating an exception on-the-fly without ever throwing it.

kevinchalet · 2015-10-09T09:54:14Z

src/Microsoft.AspNet.Authentication.Cookies/Events/CookieSignedInContext.cs

@@ -33,6 +33,8 @@ public class CookieSignedInContext : BaseContext<CookieAuthenticationOptions>
            Properties = properties;
        }

+        public new CookieAuthenticationOptions Options { get { return (CookieAuthenticationOptions)base.Options; } }


Is this needed?

Leftover turds from previous iterations, nuking thanks :)

HaoK · 2015-10-09T19:26:52Z

So the other option which I had previously was keeping the base remote events in a separate IRemoteEvents property on the base options so they don't interfere with the actual provider events.

options.RemoteEvents = new RemoteEvents() { OnError = ... }; (note I've come around fully to @Tratcher's view that the events should just be simple delegates without methods/interface)

HaoK · 2015-10-09T19:55:18Z

Updated: AuthenticationResult.Sucess to throw if a null ticket is provided, so the only time Ticket should be null is via the Failed method

Tratcher · 2015-10-09T21:31:27Z

src/Microsoft.AspNet.Authentication.JwtBearer/JwtBearerHandler.cs

                    }
                }

+                // REVIEW: this maybe return an error instead?


HaoK · 2015-10-09T23:26:42Z

Introduced new protected virtual HandleRemoteAuthenticateAsync to RemoteAuthenticationHandler and moved all the Invoke path logic there (old HandleAuthenticate method now returns a failed authenticated result). We should consider if its more appropriate to use a no-op successful empty ticket like before

HaoK · 2015-10-12T23:33:02Z

@Tratcher Updated with new iteration based on what we decided on:

Put events back into the base Event property and use it as a new base

Tratcher · 2015-10-13T15:33:02Z

src/Microsoft.AspNet.Authentication/AuthenticationHandler.cs

+        /// <summary>
+        /// </summary>
+        /// <param name="context"></param>
+        /// <returns>True if no other handlers should be called</returns>


Remove empty docs, doesn't return anything.

HaoK · 2015-10-14T01:39:49Z

Updated with the new iteration:

Split AutomaticAuthentication into AutomaticChallenge / AutomaticAuthenticate
Brought back bool control flow for Unauthorized/Challenge
OAuth throws with no catch which is a temporary behavior change until we revisit error handling in the next PR

@Tratcher

Tratcher · 2015-10-14T16:21:38Z

samples/SocialSample/Startup.cs

@@ -29,7 +29,7 @@ public void Configure(IApplicationBuilder app, ILoggerFactory loggerfactory)

            app.UseCookieAuthentication(options =>
            {
-                options.AutomaticAuthentication = true;
+                options.AutomaticAuthenticate = true;


This one should also use AutomaticChallenge. See https://github.com/aspnet/Security/blob/haok9-25/errors/samples/SocialSample/Startup.cs#L225

Tratcher · 2015-10-14T16:52:38Z

Almost done ⌚

kevinchalet · 2015-10-14T18:45:19Z

src/Microsoft.AspNet.Authentication.Cookies/Events/BaseCookieContext.cs

+
+namespace Microsoft.AspNet.Authentication.Cookies
+{
+    public class BaseCookieContext : BaseContext


Curious: why do you need this?

We nuked Options/Generics off of the base contexts, and just added them all back in the specific OAuth/Cookies/Facebook contexts themselves.

public CookieAuthenticationOptions Options { get; }

HaoK · 2015-10-15T00:21:54Z

Updated, any last requests? I'll push this tomorrow in the afternoon, MVC/Identity changes look to be fairly small surprisingly :)

HaoK · 2015-10-15T06:10:59Z

409b502 will address any remaining issues in next iteration/PR

Tratcher · 2015-10-15T21:03:38Z

src/Microsoft.AspNet.Authentication/RemoteAuthenticationHandler.cs

+                return false;
+            }
+
+            if (context.Principal != null)


I'm still trying to understand how you would get to this point without a principal or ReturnUri. I would expect all of those cases to be error conditions handled above.

The event can simply null out the principal or return uri, its settable on the context today

That would mean you're using the context property values as a form of flow control. Since we have real flow control now I don't think we should support the old hacky approach.

Make the context properties read only for Ticket/Principal then? I'm fine with that :)

No, we still want to allow people to replace it, but I don't think we should support null values.

The problem is we allow AuthenticationTickets with null principals. So we can't easily block this unless we change that.

eriksendc · 2016-05-26T17:46:39Z

Hi All,

Before posting a new issue I wanted to see if perhaps this issue was about or related to an issue I'm having. I'm on RC1, When a user selects to authenticate w/Facebook, and they put in their user name and password correctly, but clicks the Cancel button on the "this is what the app is requesting" confirmation, then the user's browser is redirected to

https://localhost:44300/signin-facebook?error=access_denied&error_code=200&error_description=Permissions+error&error_reason=user_denied&state=CfDJ8BCOQ9uBPU9Cu4ITtOhOeD-LHOt2vhRmJu5_xD4rvL3zm4ka-ak_iYgNuIJOOjjMsO1Fg5p5Tb8Fll6CJ064ysTkRrC46xaeG_vu5sFQdpPKjjGPokfsPnndPsB84CnDmRIzjwLeGgYzbESqjjGyAFUmSACDAYLo9LmmA8HJNPcdJIxTvcrepdr0GfE2DhJtP_D0Z7wNxyzJw1Jf8pGprHP8P4Q130hpmFzOLA2UCwjXYPxijlOP9LpY6Pgz66PKVJt_nO-6e0yA7aStVJTCOH8ic-I2xRguFAWXpfoim8wlqaizDupXFBBgeBtw9RvopkxKENby2N89okirRZSFK6FTLZJaG7RKxEWv_aUZskoFKmrKrkZDm0Ur3DyWnH53fQ#_=_

Control doesn't seem to be flowing back to my ASP.NET web application (I never get back anything in any /Account/xxx account controller method.

Wanted to report as an issue if it isn't one yet, but didn't want to add a new issue if this has already been fixed.

Thanks,
-Brian Eriksen

kevinchalet · 2016-05-26T17:49:48Z

@eriksendc hey,

There's actually a ticket tracking that: #710.

eriksendc · 2016-05-26T17:57:28Z

Thx!

dnfclas added the cla-already-signed label Sep 25, 2015

kevinchalet reviewed Sep 25, 2015
View reviewed changes

HaoK force-pushed the haok9-25/errors branch from a0df401 to bfa245c Compare September 30, 2015 22:15

kevinchalet reviewed Oct 1, 2015
View reviewed changes

HaoK force-pushed the haok9-25/errors branch from e69f8d2 to 21dd52c Compare October 9, 2015 01:15

HaoK changed the title ~~First stab at error handling~~ Add RemoteAuthenticationHandler base (support for error handling) Oct 9, 2015

kevinchalet reviewed Oct 9, 2015
View reviewed changes

kevinchalet mentioned this pull request Oct 9, 2015

Update the OAuth2 handler to log error_description and error_uri when receiving an error #512

Closed

Tratcher reviewed Oct 9, 2015
View reviewed changes

src/Microsoft.AspNet.Authentication.JwtBearer/JwtBearerHandler.cs

}

}

// REVIEW: this maybe return an error instead?

Copy link

Member

Tratcher Oct 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

Tratcher mentioned this pull request Oct 13, 2015

Partly fix google access deny #516

Closed

Tratcher reviewed Oct 13, 2015
View reviewed changes

HaoK added 4 commits October 13, 2015 18:10

Use EnsureSuccessCode again

f4aabf1

Formatting

5c6e517

Tweaks

dc697ed

Tweaks to fix tests

cc41921

HaoK force-pushed the haok9-25/errors branch from ba1bf42 to cc41921 Compare October 14, 2015 01:37

Tratcher reviewed Oct 14, 2015
View reviewed changes

kevinchalet reviewed Oct 14, 2015
View reviewed changes

Fix tests and PR updates

4407e76

HaoK closed this Oct 15, 2015

Tratcher reviewed Oct 15, 2015
View reviewed changes

HaoK deleted the haok9-25/errors branch February 23, 2016 20:39

Add RemoteAuthenticationHandler base (support for error handling) #495

Add RemoteAuthenticationHandler base (support for error handling) #495

Conversation

HaoK commented Sep 25, 2015

dnfclas commented Sep 25, 2015

HaoK commented Sep 25, 2015

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

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 Sep 29, 2015

HaoK commented Sep 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Oct 2, 2015

HaoK commented Oct 9, 2015

kevinchalet commented Oct 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Oct 9, 2015

HaoK commented Oct 9, 2015

Choose a reason for hiding this comment

HaoK commented Oct 9, 2015

HaoK commented Oct 12, 2015

Choose a reason for hiding this comment

HaoK commented Oct 14, 2015

Choose a reason for hiding this comment

Tratcher commented Oct 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Oct 15, 2015

HaoK commented Oct 15, 2015

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

eriksendc commented May 26, 2016

kevinchalet commented May 26, 2016

eriksendc commented May 26, 2016