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

Replace configure method on TwitterOptions and OpenIdConnectOptions with CookieBuilder #1284

Merged
merged 1 commit into from
Jul 5, 2017
Merged

Replace configure method on TwitterOptions and OpenIdConnectOptions with CookieBuilder #1284

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
42 changes: 10 additions & 32 deletions src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,7 @@ public async virtual Task SignOutAsync(AuthenticationProperties properties)
/// <returns>A task executing the callback procedure</returns>
protected virtual Task<bool> HandleSignOutCallbackAsync()
{
StringValues protectedState;
if (Request.Query.TryGetValue(OpenIdConnectParameterNames.State, out protectedState))
if (Request.Query.TryGetValue(OpenIdConnectParameterNames.State, out StringValues protectedState))
{
var properties = Options.StateDataFormat.Unprotect(protectedState);
if (!string.IsNullOrEmpty(properties?.RedirectUri))
Expand Down Expand Up @@ -505,8 +504,7 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync
return HandleRequestResult.Fail(Resources.MessageStateIsInvalid);
}

string userstate = null;
properties.Items.TryGetValue(OpenIdConnectDefaults.UserstatePropertiesKey, out userstate);
properties.Items.TryGetValue(OpenIdConnectDefaults.UserstatePropertiesKey, out string userstate);
authorizationResponse.State = userstate;

if (!ValidateCorrelationId(properties))
Expand Down Expand Up @@ -859,8 +857,7 @@ private void SaveTokens(AuthenticationProperties properties, OpenIdConnectMessag

if (!string.IsNullOrEmpty(message.ExpiresIn))
{
int value;
if (int.TryParse(message.ExpiresIn, NumberStyles.Integer, CultureInfo.InvariantCulture, out value))
if (int.TryParse(message.ExpiresIn, NumberStyles.Integer, CultureInfo.InvariantCulture, out int value))
{
var expiresAt = Clock.UtcNow + TimeSpan.FromSeconds(value);
// https://www.w3.org/TR/xmlschema-2/#dateTime
Expand All @@ -885,21 +882,12 @@ private void WriteNonceCookie(string nonce)
throw new ArgumentNullException(nameof(nonce));
}

var options = new CookieOptions
{
HttpOnly = true,
SameSite = Http.SameSiteMode.None,
Path = OriginalPathBase + Options.CallbackPath,
Secure = Request.IsHttps,
Expires = Clock.UtcNow.Add(Options.ProtocolValidator.NonceLifetime)
};

Options.ConfigureNonceCookie?.Invoke(Context, options);
var cookieOptions = Options.NonceCookie.Build(Context, Clock.UtcNow);

Response.Cookies.Append(
OpenIdConnectDefaults.CookieNoncePrefix + Options.StringDataFormat.Protect(nonce),
Options.NonceCookie.Name + Options.StringDataFormat.Protect(nonce),
NonceProperty,
options);
cookieOptions);
}

/// <summary>
Expand All @@ -918,23 +906,14 @@ private string ReadNonceCookie(string nonce)

foreach (var nonceKey in Request.Cookies.Keys)
{
if (nonceKey.StartsWith(OpenIdConnectDefaults.CookieNoncePrefix))
if (nonceKey.StartsWith(Options.NonceCookie.Name))
{
try
{
var nonceDecodedValue = Options.StringDataFormat.Unprotect(nonceKey.Substring(OpenIdConnectDefaults.CookieNoncePrefix.Length, nonceKey.Length - OpenIdConnectDefaults.CookieNoncePrefix.Length));
var nonceDecodedValue = Options.StringDataFormat.Unprotect(nonceKey.Substring(Options.NonceCookie.Name.Length, nonceKey.Length - Options.NonceCookie.Name.Length));
if (nonceDecodedValue == nonce)
{
var cookieOptions = new CookieOptions
{
HttpOnly = true,
Path = OriginalPathBase + Options.CallbackPath,
SameSite = Http.SameSiteMode.None,
Secure = Request.IsHttps
};

Options.ConfigureNonceCookie?.Invoke(Context, cookieOptions);

var cookieOptions = Options.NonceCookie.Build(Context, Clock.UtcNow);
Response.Cookies.Delete(nonceKey, cookieOptions);
return nonce;
}
Expand Down Expand Up @@ -1170,8 +1149,7 @@ private ClaimsPrincipal ValidateToken(string idToken, AuthenticationProperties p
validationParameters.IssuerSigningKeys = validationParameters.IssuerSigningKeys?.Concat(_configuration.SigningKeys) ?? _configuration.SigningKeys;
}

SecurityToken validatedToken = null;
var principal = Options.SecurityTokenValidator.ValidateToken(idToken, validationParameters, out validatedToken);
var principal = Options.SecurityTokenValidator.ValidateToken(idToken, validationParameters, out SecurityToken validatedToken);
jwt = validatedToken as JwtSecurityToken;
if (jwt == null)
{
Expand Down
50 changes: 46 additions & 4 deletions src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.IdentityModel.Tokens.Jwt;
using Microsoft.AspNetCore.Authentication.Internal;
using Microsoft.AspNetCore.Authentication.OAuth.Claims;
using Microsoft.AspNetCore.Http;
using Microsoft.IdentityModel.Protocols;
Expand All @@ -17,6 +18,8 @@ namespace Microsoft.AspNetCore.Authentication.OpenIdConnect
/// </summary>
public class OpenIdConnectOptions : RemoteAuthenticationOptions
{
private CookieBuilder _nonceCookieBuilder;

/// <summary>
/// Initializes a new <see cref="OpenIdConnectOptions"/>
/// </summary>
Expand Down Expand Up @@ -65,6 +68,14 @@ public OpenIdConnectOptions()
ClaimActions.MapUniqueJsonKey("family_name", "family_name");
ClaimActions.MapUniqueJsonKey("profile", "profile");
ClaimActions.MapUniqueJsonKey("email", "email");

_nonceCookieBuilder = new OpenIdConnectNonceCookieBuilder(this)
{
Name = OpenIdConnectDefaults.CookieNoncePrefix,
HttpOnly = true,
SameSite = SameSiteMode.None,
SecurePolicy = CookieSecurePolicy.SameAsRequest,
};
}

/// <summary>
Expand Down Expand Up @@ -145,8 +156,8 @@ public override void Validate()
/// </summary>
public new OpenIdConnectEvents Events
{
get { return (OpenIdConnectEvents)base.Events; }
set { base.Events = value; }
get => (OpenIdConnectEvents)base.Events;
set => base.Events = value;
}

/// <summary>
Expand Down Expand Up @@ -259,9 +270,40 @@ public override void Validate()
public bool DisableTelemetry { get; set; }

/// <summary>
/// Gets or sets an action that can override the nonce cookie options before the
/// Determines the settings used to create the nonce cookie before the
/// cookie gets added to the response.
/// </summary>
public Action<HttpContext, CookieOptions> ConfigureNonceCookie { get; set; }
/// <remarks>
/// The value of <see cref="CookieBuilder.Name"/> is treated as the prefix to the cookie name, and defaults to <seealso cref="OpenIdConnectDefaults.CookieNoncePrefix"/>.
/// </remarks>
public CookieBuilder NonceCookie
{
get => _nonceCookieBuilder;
set => _nonceCookieBuilder = value ?? throw new ArgumentNullException(nameof(value));
}

private class OpenIdConnectNonceCookieBuilder : RequestPathBaseCookieBuilder
{
private readonly OpenIdConnectOptions _options;

public OpenIdConnectNonceCookieBuilder(OpenIdConnectOptions oidcOptions)
{
_options = oidcOptions;
}

protected override string AdditionalPath => _options.CallbackPath;

public override CookieOptions Build(HttpContext context, DateTimeOffset expiresFrom)
{
var cookieOptions = base.Build(context, expiresFrom);

if (!Expiration.HasValue || !cookieOptions.Expires.HasValue)
{
cookieOptions.Expires = expiresFrom.Add(_options.ProtocolValidator.NonceLifetime);
}

return cookieOptions;
}
}
}
}
27 changes: 5 additions & 22 deletions src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
using System.Text;
using System.Text.Encodings.Web;
using System.Threading.Tasks;
using Microsoft.AspNetCore.DataProtection;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.WebUtilities;
using Microsoft.Extensions.Logging;
Expand All @@ -23,7 +22,6 @@ namespace Microsoft.AspNetCore.Authentication.Twitter
internal class TwitterHandler : RemoteAuthenticationHandler<TwitterOptions>
{
private static readonly DateTime Epoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);
private const string StateCookie = "__TwitterState";
private const string RequestTokenEndpoint = "https://api.twitter.com/oauth/request_token";
private const string AuthenticationEndpoint = "https://api.twitter.com/oauth/authenticate?oauth_token=";
private const string AccessTokenEndpoint = "https://api.twitter.com/oauth/access_token";
Expand All @@ -50,7 +48,7 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync
{
AuthenticationProperties properties = null;
var query = Request.Query;
var protectedRequestToken = Request.Cookies[StateCookie];
var protectedRequestToken = Request.Cookies[Options.StateCookie.Name];

var requestToken = Options.StateDataFormat.Unprotect(protectedRequestToken);

Expand Down Expand Up @@ -80,16 +78,9 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync
return HandleRequestResult.Fail("Missing or blank oauth_verifier");
}

var cookieOptions = new CookieOptions
{
HttpOnly = true,
SameSite = SameSiteMode.Lax,
Secure = Request.IsHttps
};

Options.ConfigureStateCookie?.Invoke(Context, cookieOptions);
var cookieOptions = Options.StateCookie.Build(Context, Clock.UtcNow);

Response.Cookies.Delete(StateCookie, cookieOptions);
Response.Cookies.Delete(Options.StateCookie.Name, cookieOptions);
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 trying to think if a user would Build their options any differently if they knew they were building a delete cookie.


var accessToken = await ObtainAccessTokenAsync(requestToken, oauthVerifier);

Expand Down Expand Up @@ -144,17 +135,9 @@ protected override async Task HandleChallengeAsync(AuthenticationProperties prop
var requestToken = await ObtainRequestTokenAsync(BuildRedirectUri(Options.CallbackPath), properties);
var twitterAuthenticationEndpoint = AuthenticationEndpoint + requestToken.Token;

var cookieOptions = new CookieOptions
{
HttpOnly = true,
SameSite = SameSiteMode.Lax,
Secure = Request.IsHttps,
Expires = Clock.UtcNow.Add(Options.RemoteAuthenticationTimeout),
};

Options.ConfigureStateCookie?.Invoke(Context, cookieOptions);
var cookieOptions = Options.StateCookie.Build(Context, Clock.UtcNow);

Response.Cookies.Append(StateCookie, Options.StateDataFormat.Protect(requestToken), cookieOptions);
Response.Cookies.Append(Options.StateCookie.Name, Options.StateDataFormat.Protect(requestToken), cookieOptions);

var redirectContext = new RedirectContext<TwitterOptions>(Context, Scheme, Options, properties, twitterAuthenticationEndpoint);
await Events.RedirectToAuthorizationEndpoint(redirectContext);
Expand Down
53 changes: 43 additions & 10 deletions src/Microsoft.AspNetCore.Authentication.Twitter/TwitterOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@

using System;
using System.Security.Claims;
using Microsoft.AspNetCore.Authentication;
using Microsoft.AspNetCore.Authentication.OAuth.Claims;
using Microsoft.AspNetCore.Authentication.Twitter;
using Microsoft.AspNetCore.DataProtection;
using Microsoft.AspNetCore.Http;

namespace Microsoft.AspNetCore.Authentication.Twitter
Expand All @@ -16,6 +13,10 @@ namespace Microsoft.AspNetCore.Authentication.Twitter
/// </summary>
public class TwitterOptions : RemoteAuthenticationOptions
{
private const string DefaultStateCookieName = "__TwitterState";

private CookieBuilder _stateCookieBuilder;

/// <summary>
/// Initializes a new instance of the <see cref="TwitterOptions"/> class.
/// </summary>
Expand All @@ -26,6 +27,14 @@ public TwitterOptions()
Events = new TwitterEvents();

ClaimActions.MapJsonKey(ClaimTypes.Email, "email", ClaimValueTypes.Email);

_stateCookieBuilder = new TwitterCookieBuilder(this)
{
Name = DefaultStateCookieName,
SecurePolicy = CookieSecurePolicy.SameAsRequest,
HttpOnly = true,
SameSite = SameSiteMode.Lax,
};
}

/// <summary>
Expand Down Expand Up @@ -59,18 +68,42 @@ public TwitterOptions()
public ISecureDataFormat<RequestToken> StateDataFormat { get; set; }

/// <summary>
/// Gets or sets an action that can override the state cookie options before the
/// cookie gets added to the response.
/// Gets or sets the <see cref="TwitterEvents"/> used to handle authentication events.
/// </summary>
public Action<HttpContext, CookieOptions> ConfigureStateCookie { get; set; }
public new TwitterEvents Events
{
get => (TwitterEvents)base.Events;
set => base.Events = value;
}

/// <summary>
/// Gets or sets the <see cref="TwitterEvents"/> used to handle authentication events.
/// Determines the settings used to create the state cookie before the
/// cookie gets added to the response.
/// </summary>
public new TwitterEvents Events
public CookieBuilder StateCookie
{
get => _stateCookieBuilder;
set => _stateCookieBuilder = value ?? throw new ArgumentNullException(nameof(value));
}

private class TwitterCookieBuilder : CookieBuilder
{
get { return (TwitterEvents)base.Events; }
set { base.Events = value; }
private readonly TwitterOptions _twitterOptions;

public TwitterCookieBuilder(TwitterOptions twitterOptions)
{
_twitterOptions = twitterOptions;
}

public override CookieOptions Build(HttpContext context, DateTimeOffset expiresFrom)
{
var options = base.Build(context, expiresFrom);
if (!Expiration.HasValue)
{
options.Expires = expiresFrom.Add(_twitterOptions.RemoteAuthenticationTimeout);
}
return options;
}
}
}
}
38 changes: 38 additions & 0 deletions src/Microsoft.AspNetCore.Authentication/Internal/RequestPathBaseCookieBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// 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 System;
using Microsoft.AspNetCore.Http;

namespace Microsoft.AspNetCore.Authentication.Internal
{
/// <summary>
/// A cookie builder that sets <see cref="CookieOptions.Path"/> to the request path base.
/// </summary>
public class RequestPathBaseCookieBuilder : CookieBuilder
{
/// <summary>
/// Gets an optional value that is appended to the request path base.
/// </summary>
protected virtual string AdditionalPath { get; }

public override CookieOptions Build(HttpContext context, DateTimeOffset expiresFrom)
{
// check if the user has overridden the default value of path. If so, use that instead of our default value.
var path = Path;
if (path == null)
{
var originalPathBase = context.Features.Get<IAuthenticationFeature>()?.OriginalPathBase ?? context.Request.PathBase;
Copy link
Member

Choose a reason for hiding this comment

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

Commenting again in the right version: Should we make this a property on RequestPathBaseCookieBuilder that the handlers set instead of making the builder pull it out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly racy. The builder is shared across multiple contexts.

path = originalPathBase + AdditionalPath;
}

var options = base.Build(context, expiresFrom);

options.Path = !string.IsNullOrEmpty(path)
? path
: "/";

return options;
}
}
}
Loading