Skip to content
This repository was archived by the owner on Dec 13, 2018. It is now read-only.

Revisit OAuthAuthenticationHandler and add a new SaveTokensAsClaims option #257

Closed
wants to merge 1 commit into from
Closed

Revisit OAuthAuthenticationHandler and add a new SaveTokensAsClaims option #257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions samples/CookieSample/Startup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.Linq;
using System.Security.Claims;
using Microsoft.AspNet.Authentication.Cookies;
using Microsoft.AspNet.Builder;
Expand Down Expand Up @@ -25,10 +26,11 @@ public void Configure(IApplicationBuilder app, ILoggerFactory loggerfactory)

app.Run(async context =>
{
if (string.IsNullOrEmpty(context.User.Identity.Name))
if (!context.User.Identities.Any(identity => identity.IsAuthenticated))
{
var user = new ClaimsPrincipal(new ClaimsIdentity(new[] { new Claim(ClaimTypes.Name, "bob") }));
var user = new ClaimsPrincipal(new ClaimsIdentity(new[] { new Claim(ClaimTypes.Name, "bob") }, CookieAuthenticationDefaults.AuthenticationScheme));
await context.Authentication.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme, user);

context.Response.ContentType = "text/plain";
await context.Response.WriteAsync("Hello First timer");
return;
Expand Down
8 changes: 6 additions & 2 deletions samples/CookieSessionSample/Startup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Collections.Generic;
using System.Linq;
using System.Security.Claims;
using Microsoft.AspNet.Builder;
using Microsoft.AspNet.Http;
Expand Down Expand Up @@ -27,7 +28,7 @@ public void Configure(IApplicationBuilder app, ILoggerFactory loggerfactory)

app.Run(async context =>
{
if (string.IsNullOrEmpty(context.User.Identity.Name))
if (!context.User.Identities.Any(identity => identity.IsAuthenticated))
{
// Make a large identity
var claims = new List<Claim>(1001);
Expand All @@ -36,7 +37,10 @@ public void Configure(IApplicationBuilder app, ILoggerFactory loggerfactory)
{
claims.Add(new Claim(ClaimTypes.Role, "SomeRandomGroup" + i, ClaimValueTypes.String, "IssuedByBob", "OriginalIssuerJoe"));
}
await context.Authentication.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme, new ClaimsPrincipal(new ClaimsIdentity(claims)));

await context.Authentication.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme,
new ClaimsPrincipal(new ClaimsIdentity(claims, CookieAuthenticationDefaults.AuthenticationScheme)));

context.Response.ContentType = "text/plain";
await context.Response.WriteAsync("Hello First timer");
return;
Expand Down
5 changes: 3 additions & 2 deletions samples/OpenIdConnectSample/Startup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Microsoft.AspNet.Builder;
using System.Linq;
using Microsoft.AspNet.Builder;
using Microsoft.AspNet.Http;
using Microsoft.AspNet.Http.Authentication;
using Microsoft.AspNet.Authentication;
Expand Down Expand Up @@ -38,7 +39,7 @@ public void Configure(IApplicationBuilder app, ILoggerFactory loggerfactory)

app.Run(async context =>
{
if (string.IsNullOrEmpty(context.User.Identity.Name))
if (!context.User.Identities.Any(identity => identity.IsAuthenticated))
{
await context.Authentication.ChallengeAsync(OpenIdConnectAuthenticationDefaults.AuthenticationScheme, new AuthenticationProperties { RedirectUri = "/" });

Expand Down
67 changes: 37 additions & 30 deletions samples/SocialSample/Startup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.Linq;
using System.Net.Http;
using System.Net.Http.Headers;
using System.Security.Claims;
Expand Down Expand Up @@ -110,6 +111,7 @@ dnx . web
options.Caption = "MicrosoftAccount - Requires project changes";
options.ClientId = "00000000480FF62E";
options.ClientSecret = "bLw2JIvf8Y1TaToipPEqxTVlOeJwCUsr";
options.Scope.Add("wl.emails");
});

// https://github.com/settings/applications/
Expand All @@ -131,48 +133,53 @@ dnx . web
options.TokenEndpoint = "https://github.com/login/oauth/access_token";
options.UserInformationEndpoint = "https://api.github.com/user";
options.ClaimsIssuer = "OAuth2-Github";
options.SaveTokensAsClaims = false;
// Retrieving user information is unique to each provider.
options.Notifications = new OAuthAuthenticationNotifications()
options.Notifications = new OAuthAuthenticationNotifications
{
OnGetUserInformationAsync = async (context) =>
OnAuthenticated = async notification =>
{
// Get the GitHub user
var userRequest = new HttpRequestMessage(HttpMethod.Get, context.Options.UserInformationEndpoint);
userRequest.Headers.Authorization = new AuthenticationHeaderValue("Bearer", context.AccessToken);
userRequest.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
var userResponse = await context.Backchannel.SendAsync(userRequest, context.HttpContext.RequestAborted);
userResponse.EnsureSuccessStatusCode();
var text = await userResponse.Content.ReadAsStringAsync();
var user = JObject.Parse(text);

var identity = new ClaimsIdentity(
context.Options.AuthenticationScheme,
ClaimsIdentity.DefaultNameClaimType,
ClaimsIdentity.DefaultRoleClaimType);

JToken value;
var id = user.TryGetValue("id", out value) ? value.ToString() : null;
if (!string.IsNullOrEmpty(id))
var request = new HttpRequestMessage(HttpMethod.Get, notification.Options.UserInformationEndpoint);
request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", notification.AccessToken);
request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

var response = await notification.Backchannel.SendAsync(request, notification.HttpContext.RequestAborted);
response.EnsureSuccessStatusCode();

var user = JObject.Parse(await response.Content.ReadAsStringAsync());

var identifier = user.Value<string>("id");
if (!string.IsNullOrEmpty(identifier))
{
identity.AddClaim(new Claim(ClaimTypes.NameIdentifier, id, ClaimValueTypes.String, context.Options.ClaimsIssuer));
notification.Identity.AddClaim(new Claim(
ClaimTypes.NameIdentifier, identifier,
ClaimValueTypes.String, notification.Options.ClaimsIssuer));
}
var userName = user.TryGetValue("login", out value) ? value.ToString() : null;

var userName = user.Value<string>("login");
if (!string.IsNullOrEmpty(userName))
{
identity.AddClaim(new Claim(ClaimsIdentity.DefaultNameClaimType, userName, ClaimValueTypes.String, context.Options.ClaimsIssuer));
notification.Identity.AddClaim(new Claim(
ClaimsIdentity.DefaultNameClaimType, userName,
ClaimValueTypes.String, notification.Options.ClaimsIssuer));
}
var name = user.TryGetValue("name", out value) ? value.ToString() : null;

var name = user.Value<string>("name");
if (!string.IsNullOrEmpty(name))
{
identity.AddClaim(new Claim("urn:github:name", name, ClaimValueTypes.String, context.Options.ClaimsIssuer));
notification.Identity.AddClaim(new Claim(
"urn:github:name", name,
ClaimValueTypes.String, notification.Options.ClaimsIssuer));
}
var link = user.TryGetValue("url", out value) ? value.ToString() : null;

var link = user.Value<string>("url");
if (!string.IsNullOrEmpty(link))
{
identity.AddClaim(new Claim("urn:github:url", link, ClaimValueTypes.String, context.Options.ClaimsIssuer));
notification.Identity.AddClaim(new Claim(
"urn:github:url", link,
ClaimValueTypes.String, notification.Options.ClaimsIssuer));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

image

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).

Copy link
Member

@HaoK HaoK Jun 26, 2015 via email

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

nm, I should scroll up :)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

context.Principal = new ClaimsPrincipal(identity);
},
};
});
Expand Down Expand Up @@ -207,8 +214,8 @@ dnx . web
{
signoutApp.Run(async context =>
{
await context.Authentication.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);
context.Response.ContentType = "text/html";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HaoK now that Challenge/SignIn/SignOut are not deferred anymore, this kind of code seems really dangerous, specially if the authentication middleware writes to the response stream from SignOut.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah well, as you can tell, the samples code for security is basically a little test app (that I never ever run clearly) I mostly rely on MVC/Identity/MusicStore for functional verification, but I should use this as a canary as well (until we get real functional tests...in the repo sigh)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started porting AspNet.Security.OpenIdConnect.Server to the new authentication APIs... and I almost had a heart attack: the new ChallengeAsync/SignInAsync/SignOutAsync are really fragile.

For instance, returning a simple new HttpStatusCodeResult(200) result from MVC after invoking one of these methods may result in either a no-op thing... or in an "headers already sent" exception thrown by the host 😱

Copy link
Member

Choose a reason for hiding this comment

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

Instead of fragile, I prefer the term powerful and immediate :) But yeah the behavior is very different, I don't think we entirely know all the ramifications yet (i.e. map path issue)

Copy link
Member

Choose a reason for hiding this comment

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

I would argue calling these methods which immediately do something is easier to grok, than the old behavior of stuff happens at some undetermined future time (whenever the response is sent). Perhaps you are just used to the old behavior, and the new will grow on you. I didn't play with the old behavior much (so I'm missing any fond memories of it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not saying that the new behavior is necessarily bad, it's just surprising and... obviously different 😄

What I'm saying is that we can't ship these new methods without changing the way we use them: altering the headers after invoking SignInAsync/SignOutAsync/ChallengeAsync is now very risky, specially if the authentication handler started writing to the response stream.

For instance, this snippet will throw an exception:

await Context.Authentication.SignInAsync(OpenIdConnectDefaults.AuthenticationScheme, new ClaimsPrincipal(identity));

return new HttpStatusCodeResult(200);

... while this one, that uses EmptyResult (which doesn't change the status code), won't:

await Context.Authentication.SignInAsync(OpenIdConnectDefaults.AuthenticationScheme, new ClaimsPrincipal(identity));

return new EmptyResult();

image

Copy link
Member

Choose a reason for hiding this comment

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

Yep totally agree, this is something that definitely needs to go on the Announcements as a heads up/breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved context.Response.ContentType = "text/html"; before the SignOut call.

await context.Authentication.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);
await context.Response.WriteAsync("<html><body>");
await context.Response.WriteAsync("You have been logged out. Goodbye " + context.User.Identity.Name + "<br>");
await context.Response.WriteAsync("<a href=\"/\">Home</a>");
Expand All @@ -219,7 +226,7 @@ dnx . web
// Deny anonymous request beyond this point.
app.Use(async (context, next) =>
{
if (string.IsNullOrEmpty(context.User.Identity.Name))
if (!context.User.Identities.Any(identity => identity.IsAuthenticated))
{
// The cookie middleware will intercept this 401 and redirect to /login
await context.Authentication.ChallengeAsync();
Expand All @@ -233,7 +240,7 @@ dnx . web
{
context.Response.ContentType = "text/html";
await context.Response.WriteAsync("<html><body>");
await context.Response.WriteAsync("Hello " + context.User.Identity.Name + "<br>");
await context.Response.WriteAsync("Hello " + (context.User.Identity.Name ?? "anonymous") + "<br>");
foreach (var claim in context.User.Claims)
{
await context.Response.WriteAsync(claim.Type + ": " + claim.Value + "<br>");
Expand Down
84 changes: 46 additions & 38 deletions src/Microsoft.AspNet.Authentication.Facebook/FacebookAuthenticationHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@

namespace Microsoft.AspNet.Authentication.Facebook
{
internal class FacebookAuthenticationHandler : OAuthAuthenticationHandler<FacebookAuthenticationOptions, IFacebookAuthenticationNotifications>
internal class FacebookAuthenticationHandler : OAuthAuthenticationHandler<FacebookAuthenticationOptions>
{
public FacebookAuthenticationHandler(HttpClient httpClient)
: base(httpClient)
{
}

protected override async Task<TokenResponse> ExchangeCodeAsync(string code, string redirectUri)
protected override async Task<OAuthTokenResponse> ExchangeCodeAsync(string code, string redirectUri)
{
var queryBuilder = new QueryBuilder()
{
Expand All @@ -35,70 +35,78 @@ protected override async Task<TokenResponse> ExchangeCodeAsync(string code, stri
{ "client_secret", Options.AppSecret },
};

var tokenResponse = await Backchannel.GetAsync(Options.TokenEndpoint + queryBuilder.ToString(), Context.RequestAborted);
tokenResponse.EnsureSuccessStatusCode();
var oauthTokenResponse = await tokenResponse.Content.ReadAsStringAsync();
var response = await Backchannel.GetAsync(Options.TokenEndpoint + queryBuilder.ToString(), Context.RequestAborted);
response.EnsureSuccessStatusCode();

var form = new FormCollection(FormReader.ReadForm(oauthTokenResponse));
var response = new JObject();
var form = new FormCollection(FormReader.ReadForm(await response.Content.ReadAsStringAsync()));
var payload = new JObject();
foreach (string key in form.Keys)
{
response.Add(string.Equals(key, "expires", StringComparison.OrdinalIgnoreCase) ? "expires_in" : key, form[key]);
payload.Add(string.Equals(key, "expires", StringComparison.OrdinalIgnoreCase) ? "expires_in" : key, form[key]);
}

// The refresh token is not available.
return new TokenResponse(response);
return new OAuthTokenResponse(payload);
}

protected override async Task<AuthenticationTicket> GetUserInformationAsync(AuthenticationProperties properties, TokenResponse tokens)
protected override async Task<AuthenticationTicket> CreateTicketAsync(ClaimsIdentity identity, AuthenticationProperties properties, OAuthTokenResponse tokens)
{
var graphAddress = Options.UserInformationEndpoint + "?access_token=" + UrlEncoder.UrlEncode(tokens.AccessToken);
var endpoint = Options.UserInformationEndpoint + "?access_token=" + UrlEncoder.UrlEncode(tokens.AccessToken);
if (Options.SendAppSecretProof)
{
graphAddress += "&appsecret_proof=" + GenerateAppSecretProof(tokens.AccessToken);
endpoint += "&appsecret_proof=" + GenerateAppSecretProof(tokens.AccessToken);
}

var graphResponse = await Backchannel.GetAsync(graphAddress, Context.RequestAborted);
graphResponse.EnsureSuccessStatusCode();
var text = await graphResponse.Content.ReadAsStringAsync();
var user = JObject.Parse(text);

var context = new FacebookAuthenticatedContext(Context, Options, user, tokens);
var identity = new ClaimsIdentity(
Options.ClaimsIssuer,
ClaimsIdentity.DefaultNameClaimType,
ClaimsIdentity.DefaultRoleClaimType);
if (!string.IsNullOrEmpty(context.Id))
var response = await Backchannel.GetAsync(endpoint, Context.RequestAborted);
response.EnsureSuccessStatusCode();

var payload = JObject.Parse(await response.Content.ReadAsStringAsync());

var notification = new OAuthAuthenticatedContext(Context, Options, Backchannel, tokens, payload)
{
Properties = properties,
Principal = new ClaimsPrincipal(identity)
};

var identifier = FacebookAuthenticationHelper.GetId(payload);
if (!string.IsNullOrEmpty(identifier))
{
identity.AddClaim(new Claim(ClaimTypes.NameIdentifier, context.Id, ClaimValueTypes.String, Options.ClaimsIssuer));
identity.AddClaim(new Claim(ClaimTypes.NameIdentifier, identifier, ClaimValueTypes.String, Options.ClaimsIssuer));
}
if (!string.IsNullOrEmpty(context.UserName))

var userName = FacebookAuthenticationHelper.GetUserName(payload);
if (!string.IsNullOrEmpty(userName))
{
identity.AddClaim(new Claim(ClaimsIdentity.DefaultNameClaimType, context.UserName, ClaimValueTypes.String, Options.ClaimsIssuer));
identity.AddClaim(new Claim(ClaimsIdentity.DefaultNameClaimType, userName, ClaimValueTypes.String, Options.ClaimsIssuer));
}
if (!string.IsNullOrEmpty(context.Email))

var email = FacebookAuthenticationHelper.GetEmail(payload);
if (!string.IsNullOrEmpty(email))
{
identity.AddClaim(new Claim(ClaimTypes.Email, context.Email, ClaimValueTypes.String, Options.ClaimsIssuer));
identity.AddClaim(new Claim(ClaimTypes.Email, email, ClaimValueTypes.String, Options.ClaimsIssuer));
}
if (!string.IsNullOrEmpty(context.Name))

var name = FacebookAuthenticationHelper.GetName(payload);
if (!string.IsNullOrEmpty(name))
{
identity.AddClaim(new Claim("urn:facebook:name", context.Name, ClaimValueTypes.String, Options.ClaimsIssuer));
identity.AddClaim(new Claim("urn:facebook:name", name, ClaimValueTypes.String, Options.ClaimsIssuer));

// Many Facebook accounts do not set the UserName field. Fall back to the Name field instead.
if (string.IsNullOrEmpty(context.UserName))
if (string.IsNullOrEmpty(userName))
{
identity.AddClaim(new Claim(ClaimsIdentity.DefaultNameClaimType, context.Name, ClaimValueTypes.String, Options.ClaimsIssuer));
identity.AddClaim(new Claim(identity.NameClaimType, name, ClaimValueTypes.String, Options.ClaimsIssuer));
}
}
if (!string.IsNullOrEmpty(context.Link))

var link = FacebookAuthenticationHelper.GetLink(payload);
if (!string.IsNullOrEmpty(link))
{
identity.AddClaim(new Claim("urn:facebook:link", context.Link, ClaimValueTypes.String, Options.ClaimsIssuer));
identity.AddClaim(new Claim("urn:facebook:link", link, ClaimValueTypes.String, Options.ClaimsIssuer));
}
context.Properties = properties;
context.Principal = new ClaimsPrincipal(identity);

await Options.Notifications.Authenticated(context);
await Options.Notifications.Authenticated(notification);

return new AuthenticationTicket(context.Principal, context.Properties, context.Options.AuthenticationScheme);
return new AuthenticationTicket(notification.Principal, notification.Properties, notification.Options.AuthenticationScheme);
}

private string GenerateAppSecretProof(string accessToken)
Expand Down
40 changes: 40 additions & 0 deletions src/Microsoft.AspNet.Authentication.Facebook/FacebookAuthenticationHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using Microsoft.Framework.Internal;
using Newtonsoft.Json.Linq;

namespace Microsoft.AspNet.Authentication.Facebook
{
/// <summary>
/// Contains static methods that allow to extract user's information from a <see cref="JObject"/>
/// instance retrieved from Facebook after a successful authentication process.
/// </summary>
public static class FacebookAuthenticationHelper
{
/// <summary>
/// Gets the Facebook user ID.
/// </summary>
public static string GetId([NotNull] JObject user) => user.Value<string>("id");

/// <summary>
/// Gets the user's name.
/// </summary>
public static string GetName([NotNull] JObject user) => user.Value<string>("name");

/// <summary>
/// Gets the user's link.
/// </summary>
public static string GetLink([NotNull] JObject user) => user.Value<string>("link");

/// <summary>
/// Gets the Facebook username.
/// </summary>
public static string GetUserName([NotNull] JObject user) => user.Value<string>("username");

/// <summary>
/// Gets the Facebook email.
/// </summary>
public static string GetEmail([NotNull] JObject user) => user.Value<string>("email");
}
}
Loading