Save tokens in auth properties instead of claims #698

HaoK · 2016-02-16T20:08:12Z

No description provided.

dnfclas · 2016-02-16T20:08:15Z

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;

Tratcher · 2016-02-16T20:09:36Z

samples/SocialSample/Startup.cs

@@ -90,7 +90,7 @@ public void Configure(IApplicationBuilder app, ILoggerFactory loggerfactory)
                AuthorizationEndpoint = GoogleDefaults.AuthorizationEndpoint,
                TokenEndpoint = GoogleDefaults.TokenEndpoint,
                Scope = { "openid", "profile", "email" },
-                SaveTokensAsClaims = true
+                SaveTokensInAuthenticationProperties = true


SaveTokens. We can say where in the comments.

Tratcher · 2016-02-16T20:10:31Z

What, no usage sample?

Tratcher · 2016-02-16T20:11:36Z

src/Microsoft.AspNetCore.Authentication.OAuth/OAuthHandler.cs

            {
-                identity.AddClaim(new Claim("access_token", tokens.AccessToken,
-                                            ClaimValueTypes.String, Options.ClaimsIssuer));
+                var toks = new List<AuthenticationToken>();


sp, tokens.

At least this way the tokens are grouped together.

HaoK · 2016-02-16T20:14:29Z

Well the usage without identity is going to be pretty gross, so I didn't update the samples, it'd basically be this:

                            var authContext = new AuthenticateContext("<the sign in as scheme">);
                            await context.Authentication.AuthenticateAsync(authContext);
                            var tokens = AuthenticationToken.GetTokens(new AuthenticationProperties(authContext.Properties));

We definitely need to add some sugar around extracting an individual token

Tratcher · 2016-02-16T20:16:37Z

Please update the sample and propose some sugar. "This is Hard" is one of the main complaints we're trying to address.

HaoK · 2016-02-16T20:18:10Z

Sure, but the difference in the fix is whether its identity that makes it easier or security. The feedback from the last PR I thought was that the token APIs belong in identity. I don't think we should try to fix this at the security layer. I think the basic pieces to build the sugar should be exposed in Security, but its identity that makes the end to end nice.

Tratcher · 2016-02-16T20:18:26Z

src/Microsoft.AspNetCore.Authentication/AuthenticationToken.cs

+
+        private static string TokenNamesKey = ".TokenNames";
+
+        public static void StoreTokens(AuthenticationProperties properties, IEnumerable<AuthenticationToken> tokens)


It's a little odd to have these as static here. I'd expect them to be extension methods.

Yeah could go either way here, I left them as helpers for now, extension methods work just as well...

Tratcher · 2016-02-16T20:21:21Z

Yes Identity needs to provide support for some of the larger end-to-end scenarios (multiple providers, local account, etc.), but we also need a usable solution that does not require Identity for the basic scenarios (single provider).

HaoK · 2016-02-16T20:24:50Z

So the whole remote authentication flow is non trivial, but I can certainly add sugar to enable something like this:

   httpContext.Authentication.GetToken("Cookies", "access_token")

The one bit of weirdness is that the authentication scheme passed in needs to be the SignInAsScheme and not the actual scheme since remote auth schemes do not support Authenticate anymore...

HaoK · 2016-02-16T21:00:50Z

Updated the social sample with usage, I can flip the helpers on AuthenticationToken to extension methods if we want, they do feel a bit clunky this way. But you get the idea anyways

leastprivilege · 2016-02-16T21:03:10Z

Most of our customers don't and won't use identity. So design the api without identity in mind.

HaoK · 2016-02-16T21:17:40Z

The current direction in this iteration of the PRs is that the AuthN layer exposes a few 'nicer' apis that easily let you extract/store named tokens from AuthenticationProperties. Its up to the caller to manage these/associate these with users. Identity will do this, but if you aren't using identity, you or your customers will need to associate/manage the tokens somehow on your own

HaoK · 2016-02-16T21:19:50Z

Basically this PR just exposes a simpler way to get a token out of facebook/google, via.
AuthenticationToken.GetToken(context, "Cookie", "access_token") or context.Authentication.GetToken("Cookie", "access_token"). What you do with this token isn't really addressed

Tratcher · 2016-02-16T21:40:58Z

samples/SocialSample/Startup.cs

+                await context.Response.WriteAsync("Access Token: " + await AuthenticationToken.GetTokenAsync(context, CookieAuthenticationDefaults.AuthenticationScheme, "access_token") + "<br>");
+                await context.Response.WriteAsync("Refresh Token: " + await AuthenticationToken.GetTokenAsync(context, CookieAuthenticationDefaults.AuthenticationScheme, "refresh_token") + "<br>");
+                await context.Response.WriteAsync("Token Type: " + await AuthenticationToken.GetTokenAsync(context, CookieAuthenticationDefaults.AuthenticationScheme, "token_type") + "<br>");
+                await context.Response.WriteAsync("expires_at: " + await AuthenticationToken.GetTokenAsync(context, CookieAuthenticationDefaults.AuthenticationScheme, "expires_at") + "<br>");


And the flow if you need to update any tokens is as follows?
AuthenticateAsync(SignInScheme) to get the ClaimsPrincipal and AuthProperties,
Read the tokens from AuthProperties
Refresh the tokens
Store the tokens back in AuthProperties (or create a new instance?)
Call SignIn(SignInScheme) to persist the changes?

How about context.Authentication.GetTokenAsync("access_token") that used Automatic by default? That would avoid the weirdness of specifying signin-scheme.

Updating the tokens that way seems pretty awkward. But I guess so at this layer.

Sure I can add overloads with the automatic scheme, that does make things a lot nicer.

HaoK · 2016-02-16T22:00:41Z

Updated with new extension methods, and AutomaticScheme overload which simplifies the calling code a bit

Tratcher · 2016-02-16T22:10:51Z

src/Microsoft.AspNetCore.Authentication/TokenExtensions.cs

+        }
+
+
+        public static async Task<string> GetTokenAsync(this AuthenticationManager manager, string signedInScheme, string tokenName)


It's just called signInScheme, not signedInScheme.

Tratcher · 2016-02-16T22:14:30Z

⌚ Curious what others think, and what it looks like in Identity.

leastprivilege · 2016-02-17T05:14:17Z

This seems like a basic enough abstraction.

kevinchalet · 2016-02-17T17:51:30Z

I like this approach, though it requires too much plumbing to my taste 😄

IMHO, updating AuthenticationManager.AuthenticateAsync to simply return an AuthenticationTicket would be much simpler (the current signature is ABSOLUTELY insane anyway).

var ticket = context.Authentication.AuthenticateAsync("Cookies");
var token = ticket?.Properties?.Items?["access_token"];

We could also provide a GetProperty helper to make the properties easier to extract. Here's the one used by ASOS:

/// <summary>
/// Gets a given property from the authentication ticket.
/// </summary>
/// <param name="ticket">The authentication ticket.</param>
/// <param name="property">The specific property to look for.</param>
/// <returns>The value corresponding to the property, or <c>null</c> if the property cannot be found.</returns>
public static string GetProperty(this AuthenticationTicket ticket, string property) {
    if (ticket == null) {
        throw new ArgumentNullException(nameof(ticket));
    }

    string value;
    if (!ticket.Properties.Items.TryGetValue(property, out value)) {
        return null;
    }

    return value;
}

var ticket = context.Authentication.AuthenticateAsync("Cookies");
var token = ticket?.GetProperty("access_token");

kevinchalet · 2016-02-17T17:58:41Z

test/Microsoft.AspNetCore.Authentication.Test/Google/GoogleMiddlewareTests.cs

@@ -781,6 +789,13 @@ private static TestServer CreateServer(GoogleOptions options, Func<HttpContext,
                        {
                            await context.Authentication.ChallengeAsync("Google");
                        }
+                        else if (req.Path == new PathString("/tokens"))
+                        {
+                            var authContext = new AuthenticateContext(TestExtensions.CookieAuthenticationScheme);


What's the gun for? I can't nuke the scheme because its a required argument, I could switch it to automatic scheme but that's just less clear I think

The gun is for me, this syntax is literally killing me (though it's absolutely not specific to this PR).

Le 17 févr. 2016 à 21:44, Hao Kung notifications@github.com a écrit :

In test/Microsoft.AspNetCore.Authentication.Test/Google/GoogleMiddlewareTests.cs:

@@ -781,6 +789,13 @@ private static TestServer CreateServer(GoogleOptions options, Func<HttpContext,
{
await context.Authentication.ChallengeAsync("Google");
}

else if (req.Path == new PathString("/tokens"))

{

var authContext = new AuthenticateContext(TestExtensions.CookieAuthenticationScheme);
What's the gun for? I can't nuke the scheme because its a required argument, I could switch it to automatic scheme but that's just less clear I think

—
Reply to this email directly or view it on GitHub.

Ah okay... Yeah its clunky yes, this is the raw no sugar api :)

Internally we have a fairly widespread pattern of creating a context, and passing it to a method, I don't think its THAT bad. But its not the best public API no :)

Well, I'm totally fine using this pattern with protected members you override (it's not really different from calling a method on event handler args) but I think it's a terrible pattern for public APIs, as it's counterintuitive: instantiating a context object and expect the method you call to alter it is rather unusual.

Le 17 févr. 2016 à 21:55, Hao Kung notifications@github.com a écrit :

In test/Microsoft.AspNetCore.Authentication.Test/Google/GoogleMiddlewareTests.cs:

@@ -781,6 +789,13 @@ private static TestServer CreateServer(GoogleOptions options, Func<HttpContext,
{
await context.Authentication.ChallengeAsync("Google");
}

else if (req.Path == new PathString("/tokens"))

{

var authContext = new AuthenticateContext(TestExtensions.CookieAuthenticationScheme);
Internally we have a fairly widespread pattern of creating a context, and passing it to a method, I don't think its THAT bad. But its not the best public API no :)

—
Reply to this email directly or view it on GitHub.

Yeah I agree, basically its the internal guts exposed, which is why it feels a bit gross...

But this actually IS the public API between the lower HttpAbstractions layer and IAuthenticationHandlers :) Its just usually sugar'd over by the AuthenticationManager. I think the reason why we felt this API didn't need sugar is because the developers who are using AuthenticationProperties are typically the ones who would be familiar enough with this pattern.

HaoK · 2016-02-17T20:39:36Z

updating AuthenticationManager.AuthenticateAsync to simply return an AuthenticationTicket would be much simpler (the current signature is ABSOLUTELY insane anyway).

I don't think AuthenticationProperties is really used much except for shuttling data around (identity uses it a bit, and now we stick tokens there), but in general it should be hidden. That said, go ahead and file a new issue to discuss this if you want, but I don't want rip open any lower layers as part of this change if I don't have to.

Tratcher · 2016-02-17T21:45:57Z

@HaoK sounds like there are no major objections so far. Want to try filling in the other providers, and give Jwt a try while you're at it?

HaoK · 2016-02-18T00:14:54Z

-Updated the rest of the middleware
-store JwtBearer token as Bearer
-StoreTokens deletes previous tokens, so last store wins
-Added tests

kevinchalet · 2016-02-18T00:16:53Z

src/Microsoft.AspNetCore.Authentication/AuthenticationOptions.cs

+        /// This property is set to <c>false</c> by default to reduce
+        /// the size of the final authentication cookie.
+        /// </summary>
+        public bool SaveTokens { get; set; }


Hum, I don't think this property is generic enough to be in the AuthenticationOptions base class.

This could be true by default for Bearer as it wouldn't have the cookie size concerns.

So the only one that doesn't use this now is Cookies... And there's nothing preventing that from saving tokens either...

Its basically a flag that tells the auth middleware to save tokens into AuthenticationProperties if it happens to have any tokens. If JwtBearer can use this flag, I don't see why its not generically applicable. AuthenticationMiddleware who don't have any tokens can just ignore this option...

RemoteAuth + Bearer, right?

Not Cookies, not Basic, not cert auth, not Windows, etc..

Made it true by default for JwtBearer,

Okay, moved SaveTokens back to RemoteAuth and added a new SaveToken which defaults to True for JwtBearer

Tratcher · 2016-02-18T17:41:32Z

👍
How's it looking in Identity?

HaoK · 2016-02-18T22:48:30Z

Identity PR here: aspnet/Identity#748

HaoK · 2016-02-23T22:29:00Z

@Tratcher want to take a final look over this before I merge?

kevinchalet · 2016-02-23T22:30:15Z

src/Microsoft.AspNetCore.Authentication.OAuth/OAuthHandler.cs

+                        var expiresAt = Options.SystemClock.UtcNow + TimeSpan.FromSeconds(value);
+                        authTokens.Add(new AuthenticationToken
+                        {
+                            Name = "expires_at",


So, no plan to make it a property on AuthenticationToken?

Doesn't sound like you've convinced @Tratcher and it definitely makes things more complicated, so my default is no for now as well, you can file an issue to consider adding it later

Okay, will do.

kevinchalet · 2016-02-23T22:31:25Z

@HaoK don't forget to update HandleSignOutAsync: https://github.com/aspnet/Security/blob/haok/2-16tok/src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs#L108-L109.

HaoK · 2016-02-23T22:32:58Z

Thanks @PinpointTownes oh how I love lack of test coverage :)

kevinchalet · 2016-02-24T00:49:49Z

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

+                        {
+                            ticket.Properties.StoreTokens(new[]
+                            {
+                                new AuthenticationToken { Name = "Bearer", Value = token }


Should we name it access_token for consistency?

I don't have enough context to say, I would say we should name it 'correctly' in the context of JwtBearer as opposed to consistently. I think it's better for it to be as clear as possible what this token is based on the name. That said, if access_token is correct-ish, consistency is great :)

Yep, it's correct (it's the official name used by the specs: https://tools.ietf.org/html/rfc6750#section-2.3)

Tratcher · 2016-03-03T18:36:07Z

HaoK · 2016-03-03T20:52:13Z

ace166f

dnfclas added the cla-already-signed label Feb 16, 2016

HaoK mentioned this pull request Feb 16, 2016

Replace SaveTokensAsClaims with ITokenStore #696

Closed

Tratcher reviewed Feb 16, 2016
View reviewed changes

kevinchalet reviewed Feb 17, 2016
View reviewed changes

kevinchalet reviewed Feb 18, 2016
View reviewed changes

HaoK mentioned this pull request Feb 22, 2016

Initial token schema changes and API aspnet/Identity#748

Closed

kevinchalet reviewed Feb 23, 2016
View reviewed changes

kevinchalet reviewed Feb 24, 2016
View reviewed changes

kevinchalet mentioned this pull request Feb 24, 2016

Update AuthenticationToken to include an expiration date #709

Closed

HaoK added 8 commits February 25, 2016 13:20

Save tokens in auth properties instead of claims

f0b16e5

Update samples and rename property

66c9882

Switch to extension methods

1fc8d35

Update rest of the auth middleware add tests

8973833

PR feedback

f914567

Move SaveTokens into Remote/JwtBearerOptions

73614d1

Switch to token api for OIDC IdTokenHint

1d984be

Rename Bearer token name to access_token

7809f0a

HaoK force-pushed the haok/2-16tok branch from 5f28171 to 7809f0a Compare February 25, 2016 21:30

Remove review comment

5309445

HaoK closed this Mar 3, 2016

kevinchalet mentioned this pull request Apr 16, 2016

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

Closed

HaoK deleted the haok/2-16tok branch June 10, 2016 22:46


		private static string TokenNamesKey = ".TokenNames";

		public static void StoreTokens(AuthenticationProperties properties, IEnumerable<AuthenticationToken> tokens)

		}


		public static async Task<string> GetTokenAsync(this AuthenticationManager manager, string signedInScheme, string tokenName)

Save tokens in auth properties instead of claims #698

Save tokens in auth properties instead of claims #698

Conversation

HaoK commented Feb 16, 2016

dnfclas commented Feb 16, 2016

Choose a reason for hiding this comment

Tratcher commented Feb 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Feb 16, 2016

Tratcher commented Feb 16, 2016

HaoK commented Feb 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Feb 16, 2016

HaoK commented Feb 16, 2016

HaoK commented Feb 16, 2016

leastprivilege commented Feb 16, 2016

HaoK commented Feb 16, 2016

HaoK commented Feb 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Feb 16, 2016

Choose a reason for hiding this comment

Tratcher commented Feb 16, 2016

leastprivilege commented Feb 17, 2016

kevinchalet commented Feb 17, 2016

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 Feb 17, 2016

Tratcher commented Feb 17, 2016

HaoK commented Feb 18, 2016

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 Feb 18, 2016

HaoK commented Feb 18, 2016

HaoK commented Feb 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinchalet commented Feb 23, 2016

HaoK commented Feb 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Mar 3, 2016

HaoK commented Mar 3, 2016