Revisit OAuthAuthenticationHandler and add a new SaveTokensAsClaims option #257

kevinchalet · 2015-05-14T14:28:10Z

In the current bits, the OAuth2 client middleware automagically saves access and refresh tokens in the ClaimsPrincipal it issues, but this logic disappears when you override the default OnGetUserInformation notification: https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OAuth/OAuthAuthenticationDefaults.cs#L13

This pull request removes DefaultOnGetUserInformationAsync, moves its logic directly to OAuthAuthenticationHandler and adds a new SaveTokens property to OAuthAuthenticationOptions to decide whether tokens should be stored in ClaimsPrincipal (and in the final authentication cookie) or not, independently of the OnGetUserInformation notification.

This will also make OAuthAuthenticationHandler inheritors more DRY, as they won't have to repeat the same snippet again and again: https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/blob/dev/src/AspNet.Security.OAuth.GitHub/GitHubAuthenticationHandler.cs#L52-L70

/cc @HaoK @Tratcher

kevinchalet · 2015-05-14T14:30:13Z

@Eilon please tell me I won't have to sign a third CLA 😄

(FYI, I submitted a PR two months ago: #168)

kevinchalet · 2015-05-14T14:32:56Z

samples/SocialSample/Startup.cs

@@ -218,7 +215,7 @@ 127.0.0.1 MsSecSample.localhost.this
            // Deny anonymous request beyond this point.
            app.Use(async (context, next) =>
            {
-                if (string.IsNullOrEmpty(context.User.Identity.Name))
+                if (context.User == null || !context.User.Identities.Any(identity => identity.IsAuthenticated))


Unrelated change, but I fixed it because the "GitHub-AccessToken" sample didn't work properly.

context.User is guaranteed never to be null.

Ah nice, I'll remove the unnecessary check then 😄

Eilon · 2015-05-14T16:11:46Z

@PinpointTownes you do need to sign a .NET Foundation CLA (we just changed the projects from MS Open Tech), but this is the last CLA we plan to have! Also, this new CLA is much simpler and quicker than the previous CLA. Sorry for the hassle!

kevinchalet · 2015-05-14T16:16:32Z

@Eilon okay, done (indeed, it was rather quick 😄)

(btw, it seems that the Contribution page has not been updated for every branch: https://github.com/aspnet/Home/blob/release/CONTRIBUTING.md#contributing-code-and-content)

Eilon · 2015-05-14T16:41:29Z

@PinpointTownes sorry that link is out-of-date... I fixed it this morning. You want to use the fully automated system at https://cla2.dotnetfoundation.org/ .

dnfclas · 2015-05-14T16:46:21Z

@PinpointTownes, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

kevinchalet · 2015-05-14T16:46:56Z

Ah. I had filed the wrong form: https://cla.dotnetfoundation.org/ (the first result on Google...)

@dnfclas thanks, dear bot 😄

Eilon · 2015-05-14T16:58:25Z

Thanks! BTW I contacted the bot operators to have the CLA sites clarified as to which site is for which. Hopefully they can get that resolved soon!

kevinchalet · 2015-05-14T16:59:17Z

@Eilon wonderful, thanks!

kevinchalet · 2015-05-15T19:07:22Z

@HaoK @Tratcher any remark concerning this PR? 😰

Tratcher · 2015-05-15T19:24:39Z

src/Microsoft.AspNet.Authentication.OAuth/OAuthAuthenticationHandler.cs

-            await Options.Notifications.GetUserInformationAsync(context);
-            return new AuthenticationTicket(context.Principal, context.Properties, Options.AuthenticationScheme);
+
+            await Options.Notifications.GetUserInformationAsync(notification);


Should this also be renamed?

It's not necessary but yep, it would be ideal.

How about CreateTicket/OnCreateTicket?

Yes, OnCreateTicket.

Note that the deriving providers just call OnAuthenticated, they don't call this one directly. It would be nice if we could merge the two.

I'm not sure how to do that yet. *Authenticated events expose strongly-typed properties that we couldn't have in the generic CreateTicket notification.

HaoK · 2015-05-15T21:08:18Z

src/Microsoft.AspNet.Authentication.OAuth/OAuthAuthenticationHandler.cs

        {
-            var context = new OAuthGetUserInformationContext(Context, Options, Backchannel, tokens)
+            var identity = new ClaimsIdentity(Options.ClaimsIssuer);


I'm not opposed to changing this back to AuthenticationScheme if we think that's more appropriate. We don't really use AuthenticationType for anything anymore, other than null or not for IsAuthenticated

IMHO, AuthenticationScheme definitely sounds more appropriate, @Tratcher thinks we should use Options.ClaimsIssuer instead.

I think @Tratcher mostly was arguing consistency since I changed everything to use ClaimsIssuer. I recall that was something I questioned myself when doing it, so now is a good time to revisit to make sure we pick the most appropriate one to use...

Only because that's what we changed all of the others to. That's a debate I don't want to restart in this PR. If it's out of date file a different bug.

Tratcher · 2015-05-18T17:13:22Z

Possibly related: #101

kevinchalet · 2015-05-18T18:01:29Z

@Tratcher I'm testing a new approach that merges OnAuthenticated and OnCreateTicket, as suggested. Feel free to tell me if you prefer the new design or the old one: https://github.com/PinpointTownes/Security/commit/517125d8b82ac89de1d869e4f88c949d9931032c

Regarding #101, I'll fix it in a future commit, when we're done with the general design.

Tratcher · 2015-05-19T16:35:04Z

src/Microsoft.AspNet.Authentication.OAuth/OAuthAuthenticationOptions.cs

+        /// after a successful authentication. You can set this property to <see cref="false"/> to reduce
+        /// the size of the final authentication cookie.
+        /// </summary>
+        public bool SaveTokensAsClaims { get; set; } = true;


Should this really be enabled by default? It's not needed in the default app flow, and we're already having major cookie size problems.

I opted for true because it was the default behavior of OnGetUserInformationAsync, but yeah, it's probably better to disable it by default.

Actually, would it make sense to set it to true in the base options and false in the derived options? Otherwise the default behavior of the base class is an empty identity, no?

It might be a bit surprising from a configuration perspective, but as long as it is correctly documented, it shouldn't be an issue.

I'll fix that. Am I allowed to use a virtual property? 😄

Try it and we'll see :-)

Tratcher · 2015-06-16T21:04:37Z

Looks good. @hoak can you check it and merge it?

kevinchalet · 2015-06-23T14:05:34Z

@HaoK any chance you could have a look? 😄

HaoK · 2015-06-23T17:43:15Z

Yeah I need to review it again since Lou had some potential concerns which I think we avoided but I'll need to double check

kevinchalet · 2015-06-24T18:13:59Z

@HaoK any chance you could share more info concerning these potential concerns? 😄

HaoK · 2015-06-26T02:33:31Z

So he is fine with us nuking all of the derived generics/options/interfaces. His main concern is that we must keep the core INotifications interface so you can replace the workers on the options via DI.

Also when you get a chance, can you rebase this against dev? I sort of blew all sorts of stuff up :)

kevinchalet · 2015-06-26T07:51:34Z

So he is fine with us nuking all of the derived generics/options/interfaces. His main concern is that we must keep the core INotifications interface so you can replace the workers on the options via DI.

Ah yeah, I couldn't agree more. You'll have to convince @Tratcher though: #47 (comment) 😄

Also when you get a chance, can you rebase this against dev? I sort of blew all sorts of stuff up :)

Sure. Any idea when the CI builds including your changes will be ready? It would be easier than hacking global.json 😄

HaoK · 2015-06-26T09:41:58Z

CI might take a day or two, you can try using the dev volatile feed which is what we use internally:
https://www.myget.org/F/aspnetvolatile/

I wouldn't recommend doing this in general, unless you like being broken...a lot :)

HaoK · 2015-06-26T09:48:21Z

Well the status quo wins in general, its more like we have to convince @lodejard that this is the right thing to do, he felt pretty strongly against removing the interface, and if you actually agree with him, then that's 2:1 in favor of the status quo. I mostly wanted the generics gone, so I don't have a strong opinion either way on how this goes, which in practice means I'm leaning towards leaving the interface alone as well then...

kevinchalet · 2015-06-26T14:55:58Z

@HaoK well yeah, but the current situation is not satisfactory, as we have both patterns in the code base, which is terribly inconsistent.

kevinchalet · 2015-06-26T15:02:26Z

samples/SocialSample/Startup.cs

                    },
                };
            });

            // Choose an authentication type
-            app.Map("/login", signoutApp =>


Definitely not something I wanted to change, but it sounds like @HaoK's recent changes broke something here:

It seems that the redirect_uri is now computed using the PathBase of the handler/middleware that triggers the Challenge call, which of course, produces an incorrect redirect_uri (http://localhost:54540/login/signin-google vs http://localhost:54540/signin-google).

Very possible i regressed something until the ci goes green. Music store isn't green yet due to lots of issues so I will probably fix this today

Hmm, the middleware may need to snapshot the base address when the request first arrives.

What is the correct url that is registered for these apps? I'm seeing errors saying: "http://localhost:54540/login/signin-google-token" did not match a registered redirect uri

nm, I should scroll up :)

I'm going to port this fix manually to a standalone so social samples actually work again

Thanks to your fix, I can now revert this change 😄

Tratcher · 2015-06-26T15:06:29Z

I primarily want to reduce it from triple extensibility to just one pattern. I'm less concerned which pattern, though I do think the delegate pattern we use in OIDC is the simplest.

HaoK · 2015-06-26T18:02:51Z

Ok so sounds like we have consensus to add back just the base IOAuthAuthenticationNotifications (RIP Generics and derived notifications), and use that as the pattern everywhere?

kevinchalet · 2015-06-26T19:41:00Z

Ok so sounds like we have consensus to add back just the base IOAuthAuthenticationNotifications (RIP Generics and derived notifications), and use that as the pattern everywhere?

Yup! I haven't removed IOAuthAuthenticationNotifications, so we're fine for this PR, but we'll definitely need to fix the other providers in a future PR 😄

…ption

kevinchalet · 2015-06-30T17:40:45Z

Rebased, squashed. I also removed the volatile feed from NuGet.Config.

HaoK · 2015-07-01T22:12:00Z

merged 9bb8b61 thanks for grinding through this and all the rebases :)

kevinchalet · 2015-07-01T22:16:31Z

@HaoK wooo, thank you for merging it! 🙇

dnfclas added the cla-required label May 14, 2015

kevinchalet reviewed May 14, 2015
View reviewed changes

kevinchalet changed the title ~~Add a new SaveTokens options to save access and tokens directly in OAuthAuthenticationHandler~~ Add a new SaveTokens option to save access and tokens directly in OAuthAuthenticationHandler May 14, 2015

dnfclas added cla-signed and removed cla-required labels May 14, 2015

Tratcher reviewed May 15, 2015
View reviewed changes

kevinchalet changed the title ~~Add a new SaveTokens option to save access and tokens directly in OAuthAuthenticationHandler~~ Add a new SaveTokens option to save tokens directly in OAuthAuthenticationHandler May 15, 2015

HaoK reviewed May 15, 2015
View reviewed changes

Tratcher reviewed May 19, 2015
View reviewed changes

kevinchalet mentioned this pull request Jun 23, 2015

Update the GitHub provider to use the new OAuth2 base middleware design aspnet-contrib/AspNet.Security.OAuth.Providers#26

Merged

kevinchalet reviewed Jun 26, 2015
View reviewed changes

HaoK mentioned this pull request Jun 26, 2015

Fix social samples #304

Closed

This was referenced Jun 29, 2015

State and tests #301

Closed

Remove default tasks for OIDC notifications, perform null check before any work. #307

Closed

AppVoyer & Travis show errors in "React to OnStarting rename" PR aspnet/Session#45

Closed

Revisit OAuthAuthenticationHandler and add a new SaveTokensAsClaims o…

5261717

…ption

HaoK closed this Jul 1, 2015

kevinchalet deleted the oauth branch July 3, 2015 23:19

kevinchalet mentioned this pull request Jul 13, 2015

AuthenticationManager.SignOutAsync is not signing out with the IDP (AAD in my case) #356

Closed

kevinchalet mentioned this pull request Jul 29, 2015

Announcement Discussion: AuthenticationHandler<T> is now only async, AuthenticateCore() renamed to HandleAuthenticateAsync() #379

Closed

kevinchalet mentioned this pull request Dec 4, 2015

SaveTokensAsClaims defaults #526

Closed

kevinchalet mentioned this pull request Dec 14, 2015

Discussion for Scope being a list and not a string? #614

Closed

kevinchalet mentioned this pull request Apr 16, 2016

How to get additional fields using the Facebook provider in RC1? #783

Closed

Revisit OAuthAuthenticationHandler and add a new SaveTokensAsClaims option #257

Revisit OAuthAuthenticationHandler and add a new SaveTokensAsClaims option #257

Conversation

kevinchalet commented May 14, 2015

kevinchalet commented May 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eilon commented May 14, 2015

kevinchalet commented May 14, 2015

Eilon commented May 14, 2015

dnfclas commented May 14, 2015

kevinchalet commented May 14, 2015

Eilon commented May 14, 2015

kevinchalet commented May 14, 2015

kevinchalet commented May 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented May 18, 2015

kevinchalet commented May 18, 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

Tratcher commented Jun 16, 2015

kevinchalet commented Jun 23, 2015

HaoK commented Jun 23, 2015

kevinchalet commented Jun 24, 2015

HaoK commented Jun 26, 2015

kevinchalet commented Jun 26, 2015

HaoK commented Jun 26, 2015

HaoK commented Jun 26, 2015

kevinchalet commented Jun 26, 2015

Choose a reason for hiding this comment

HaoK Jun 26, 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

Tratcher commented Jun 26, 2015

HaoK commented Jun 26, 2015

kevinchalet commented Jun 26, 2015

kevinchalet commented Jun 30, 2015

HaoK commented Jul 1, 2015

kevinchalet commented Jul 1, 2015