Add AuthenticationProperties to AuthenticateResult for failures. #889

Tratcher · 2017-07-06T19:28:37Z

aspnet/Security#1188
See the matching Security PR.

@HaoK how crazy is this? I know you just finished cleaning these up.

HaoK · 2017-07-06T20:05:41Z

Bleh why don't you create a real first class AuthenticationError class with the things you want, that can contain a Properties if you want, but not just a top level properties

Tratcher · 2017-07-06T23:29:25Z

How's this?

HaoK · 2017-07-06T23:35:02Z

Do we really need properties, or just a dictionary bag? Or are we storing original properties?

davidfowl · 2017-07-06T23:39:34Z

src/Microsoft.AspNetCore.Authentication.Abstractions/AuthenticateResult.cs

@@ -31,12 +31,17 @@ public class AuthenticateResult
        /// <summary>
        /// Additional state values for the authentication session.
        /// </summary>
-        public AuthenticationProperties Properties => Ticket?.Properties;
+        public AuthenticationProperties Properties => Ticket?.Properties ?? Error?.Properties;


Will both of these ever be set?

Ugh, get rid of this fallback, I think Properties should only be Ticket.Properties, make them go Error.Properties if they want the error ones.

Or we should just get rid of this property since there's potentially two Properties now

Yea, this looks like a disaster waiting to happen.

There will never be both a Ticket and an Error, and the Properties collection is actually the same either way.

Then maybe instead of an Error property, we should have ErrorResult derive from AuthenticateResult, have Ticket return null, Properties return the error properties.

So basically instead of a new standalone AuthenticationError class, have an ErrorResult : AuthenticateResult instead which sets the Result up properly (no ticket, properties only)

The AuthError gives me a mechanic to solve the next problem (#1178) flowing error details.

Sure but the Error is just an exception + properties, can't you do the same with an ErrorResult (which also has Failure exception + Properties)? Where's the relevant code in the associated PR today?

Tratcher · 2017-07-07T17:43:10Z

In this case it's the original auth properties from the user's challenge.

Tratcher · 2017-09-25T21:40:35Z

bump rebased and revived for 2.1. We now have to be more careful that the changes are strictly additive.

HaoK · 2017-09-26T17:41:42Z

I think I'd prefer we added something like a generic bag to AuthenticateResult:

   public Dictionary<string, object> CustomData { get } = new Dictionary<string, object>();

And then just have Security/RemoteAuthHandler put whatever it needs into the CustomData to flow the extra error info you need. AuthenticateError looks way too similar to AuthenticateResult...

Tratcher · 2017-09-26T17:45:50Z

AuthProperties aren't custom data, they're auth flow state that we surface for success but not for failures.

Your suggestion applies more to aspnet/Security#1178 (comment) than the current issue.

HaoK · 2017-09-26T17:47:23Z

There's already a Properties on AuthenticateResult

HaoK · 2017-09-26T17:48:28Z

Why can't security just manufacture an error AuthenticateResult with the correct Properties inside?

HaoK · 2017-09-26T17:51:00Z

src/Microsoft.AspNetCore.Authentication.Abstractions/AuthenticateResult.cs

        {
-            return new AuthenticateResult() { Failure = new Exception(failureMessage) };
+            return new AuthenticateResult() { Error = new AuthenticationError(failure, properties) };


Basically instead of adding an Error, just add a new _errorProperties field to use here, and return that instead of _Error.Properties

Actually scratch that, I'd just leave Properties to mean Ticket.Properties, and add a new FailureProperties so its clear they aren't the same thing...no confusing fallback

They are the same properties. We just don't have a way to surface them for failure scenarios.

A field can still work, just copy the properties into the field in both Success and Failure instead of calling through to the Ticket/Error.

My main pushback is AuthenticateResult is already meant to handle errors. We don't need another AuthenticationError class

Tratcher · 2017-09-26T17:54:33Z

AuthenticateResult.Properties requires an AuthTicket, which requires a ClaimsPrincipal.

Tratcher · 2017-09-26T22:16:12Z

Updated, this version's a little simpler.

HaoK

Much better :)

Tratcher added this to the 2.0.0 milestone Jul 6, 2017

Tratcher self-assigned this Jul 6, 2017

Tratcher requested a review from HaoK July 6, 2017 19:28

dnfclas added the cla-already-signed label Jul 6, 2017

Tratcher mentioned this pull request Jul 6, 2017

Add AuthenticationProperties to HandleRequestResult and RemoteFailureContext aspnet/Security#1299

Merged

Tratcher force-pushed the tratcher/fail branch from 71ef50b to fa3599d Compare July 6, 2017 23:28

davidfowl reviewed Jul 6, 2017

View reviewed changes

Tratcher force-pushed the tratcher/fail branch from fa3599d to fd597b9 Compare July 7, 2017 17:38

Tratcher modified the milestones: 2.1.0, 2.0.0 Jul 7, 2017

Tratcher force-pushed the tratcher/fail branch from fd597b9 to 8237e8a Compare July 10, 2017 15:58

Tratcher force-pushed the tratcher/fail branch from 8237e8a to 508b1d6 Compare September 25, 2017 19:51

HaoK reviewed Sep 26, 2017

View reviewed changes

Tratcher force-pushed the tratcher/fail branch from 508b1d6 to 95a7a6a Compare September 26, 2017 22:14

HaoK approved these changes Sep 26, 2017

View reviewed changes

Add AuthenticationProperties to AuthenticateResult for failures.

321639b

Tratcher force-pushed the tratcher/fail branch from 95a7a6a to 321639b Compare September 27, 2017 21:17

Tratcher merged commit 321639b into dev Sep 27, 2017

Tratcher deleted the tratcher/fail branch September 27, 2017 21:17

Add AuthenticationProperties to AuthenticateResult for failures. #889

Add AuthenticationProperties to AuthenticateResult for failures. #889

Uh oh!

Conversation

Tratcher commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HaoK commented Jul 6, 2017

Uh oh!

Tratcher commented Jul 6, 2017

Uh oh!

HaoK commented Jul 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Jul 7, 2017

Uh oh!

Tratcher commented Sep 25, 2017

Uh oh!

HaoK commented Sep 26, 2017

Uh oh!

Tratcher commented Sep 26, 2017

Uh oh!

HaoK commented Sep 26, 2017

Uh oh!

HaoK commented Sep 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaoK Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Sep 26, 2017

Uh oh!

Tratcher commented Sep 26, 2017

Uh oh!

HaoK left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Tratcher commented Jul 6, 2017 •

edited

Loading

HaoK Sep 26, 2017 •

edited

Loading