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

logging, added tests, fixed code only flow. #202

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ namespace Microsoft.AspNet.Authentication.OpenIdConnect
{
public interface INonceCache
Copy link
Contributor

Choose a reason for hiding this comment

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

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 haven't. Feel free to open an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done: #212

{
string AddNonce(string nonce);
bool TryAddNonce(string nonce);

bool TryRemoveNonce(string nonce);
bool HasNonce(string nonce);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public static class OpenIdConnectAuthenticationDefaults
/// <summary>
/// The prefix used to for the a nonce in the cookie
/// </summary>
internal const string CookieNoncePrefix = ".AspNet.OpenIdConnect.Nonce.";
public const string CookieNoncePrefix = ".AspNet.OpenIdConnect.Nonce.";

/// <summary>
/// The property for the RedirectUri that was used when asking for a 'authorizationCode'
Expand All @@ -36,6 +36,6 @@ public static class OpenIdConnectAuthenticationDefaults
/// <summary>
/// Constant used to identify state in openIdConnect protocal message
/// </summary>
internal const string AuthenticationPropertiesKey = "OpenIdConnect.AuthenticationProperties";
public const string AuthenticationPropertiesKey = "OpenIdConnect.AuthenticationProperties";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,16 @@ public static IApplicationBuilder UseOpenIdConnectAuthentication(this IApplicati
Name = optionsName
});
}

/// <summary>
/// Adds the <see cref="OpenIdConnectAuthenticationMiddleware"/> into the ASP.NET runtime.
/// </summary>
/// <param name="app">The application builder</param>
/// <param name="options">Options which control the processing of the OpenIdConnect protocol and token validation.</param>
/// <returns>The application builder</returns>
public static IApplicationBuilder UseOpenIdConnectAuthentication(this IApplicationBuilder app, IOptions<OpenIdConnectAuthenticationOptions> options)
{
return app.UseMiddleware<OpenIdConnectAuthenticationMiddleware>(options);
}
}
}
}

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -30,31 +30,29 @@ public class OpenIdConnectAuthenticationMiddleware : AuthenticationMiddleware<Op
/// <summary>
/// Initializes a <see cref="OpenIdConnectAuthenticationMiddleware"/>
/// </summary>
/// <param name="next">The next middleware in the ASP.NET pipeline to invoke</param>
/// <param name="app">The ASP.NET application</param>
/// <param name="options">Configuration options for the middleware</param>
/// <param name="next">The next middleware in the ASP.NET pipeline to invoke.</param>
/// <param name="dataProtectionProvider"> provider for creating a data protector.</param>
/// <param name="loggerFactory">factory for creating a <see cref="ILogger"/>.</param>
/// <param name="options">a <see cref="IOptions{OpenIdConnectAuthenticationOptions}"/> instance that will supply <see cref="OpenIdConnectAuthenticationOptions"/>
/// if configureOptions is null.</param>
/// <param name="configureOptions">a <see cref="ConfigureOptions{OpenIdConnectAuthenticationOptions}"/> instance that will be passed to an instance of <see cref="OpenIdConnectAuthenticationOptions"/>
/// that is retrieved by calling <see cref="IOptions{OpenIdConnectAuthenticationOptions}.GetNamedOptions(string)"/> where string == <see cref="ConfigureOptions{OpenIdConnectAuthenticationOptions}.Name"/> provides runtime configuration.</param>
[SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Managed by caller")]
public OpenIdConnectAuthenticationMiddleware(
[NotNull] RequestDelegate next,
[NotNull] IDataProtectionProvider dataProtectionProvider,
[NotNull] ILoggerFactory loggerFactory,
[NotNull] IOptions<ExternalAuthenticationOptions> externalOptions,
[NotNull] IOptions<OpenIdConnectAuthenticationOptions> options,
ConfigureOptions<OpenIdConnectAuthenticationOptions> configureOptions)
ConfigureOptions<OpenIdConnectAuthenticationOptions> configureOptions = null)
: base(next, options, configureOptions)
{
_logger = loggerFactory.CreateLogger<OpenIdConnectAuthenticationMiddleware>();

if (string.IsNullOrEmpty(Options.SignInScheme))
if (string.IsNullOrEmpty(Options.SignInScheme) && !string.IsNullOrEmpty(externalOptions.Options.SignInScheme))
{
Options.SignInScheme = externalOptions.Options.SignInScheme;
}

if (string.IsNullOrWhiteSpace(Options.TokenValidationParameters.AuthenticationType))
{
Options.TokenValidationParameters.AuthenticationType = Options.AuthenticationScheme;
}

if (Options.StateDataFormat == null)
{
var dataProtector = dataProtectionProvider.CreateProtector(
Expand Down Expand Up @@ -152,7 +150,7 @@ private static HttpMessageHandler ResolveHttpMessageHandler(OpenIdConnectAuthent
var webRequestHandler = handler as WebRequestHandler;
if (webRequestHandler == null)
{
throw new InvalidOperationException(Resources.Exception_ValidatorHandlerMismatch);
throw new InvalidOperationException(Resources.OIDCH_0102_ExceptionValidatorHandlerMismatch);
}
webRequestHandler.ServerCertificateValidationCallback = options.BackchannelCertificateValidator.Validate;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,31 +52,30 @@ public OpenIdConnectAuthenticationOptions()
[SuppressMessage("Microsoft.Globalization", "CA1303:Do not pass literals as localized parameters", MessageId = "Microsoft.Owin.Security.OpenIdConnect.OpenIdConnectAuthenticationOptions.set_Caption(System.String)", Justification = "Not a LOC field")]
public OpenIdConnectAuthenticationOptions(string authenticationScheme)
{
// REVIEW: why was this active by default??
//AuthenticationMode = AuthenticationMode.Active;
AuthenticationScheme = authenticationScheme;
BackchannelTimeout = TimeSpan.FromMinutes(1);
Caption = OpenIdConnectAuthenticationDefaults.Caption;
ProtocolValidator = new OpenIdConnectProtocolValidator();
RefreshOnIssuerKeyNotFound = true;
ResponseMode = OpenIdConnectResponseModes.FormPost;
ResponseType = OpenIdConnectResponseTypes.CodeIdToken;
Scope = OpenIdConnectScopes.OpenIdProfile;
TokenValidationParameters = new TokenValidationParameters();
UseTokenLifetime = true;
}

/// <summary>
/// Gets or sets the Authority to use when making OpenIdConnect calls.
/// Gets or sets the expected audience for any received JWT token.
/// </summary>
public string Authority { get; set; }
/// <value>
/// The expected audience for any received JWT token.
/// </value>
public string Audience { get; set; }

/// <summary>
/// An optional constrained path on which to process the authentication callback.
/// If not provided and RedirectUri is available, this value will be generated from RedirectUri.
/// Gets or sets the Authority to use when making OpenIdConnect calls.
/// </summary>
/// <remarks>If you set this value, then the <see cref="OpenIdConnectAuthenticationHandler"/> will only listen for posts at this address.
/// If the IdentityProvider does not post to this address, you may end up in a 401 -> IdentityProvider -> Client -> 401 -> ...</remarks>
public PathString CallbackPath { get; set; }
public string Authority { get; set; }

#if DNX451
/// <summary>
Expand Down Expand Up @@ -112,7 +111,7 @@ public TimeSpan BackchannelTimeout
{
if (value <= TimeSpan.Zero)
{
throw new ArgumentOutOfRangeException("BackchannelTimeout", value, Resources.ArgsException_BackchallelLessThanZero);
throw new ArgumentOutOfRangeException("BackchannelTimeout", value, Resources.OIDCH_0101_BackChallnelLessThanZero);
}

_backchannelTimeout = value;
Expand All @@ -128,6 +127,14 @@ public string Caption
set { Description.Caption = value; }
}

/// <summary>
/// An optional constrained path on which to process the authentication callback.
/// If not provided and RedirectUri is available, this value will be generated from RedirectUri.
/// </summary>
/// <remarks>If you set this value, then the <see cref="OpenIdConnectAuthenticationHandler"/> will only listen for posts at this address.
/// If the IdentityProvider does not post to this address, you may end up in a 401 -> IdentityProvider -> Client -> 401 -> ...</remarks>
Copy link
Contributor

Choose a reason for hiding this comment

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

Heh? 😄

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 am not sure what you are asking?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just saying that this remark is not totally clear 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then make a suggestion.

public PathString CallbackPath { get; set; }

/// <summary>
/// Gets or sets the 'client_id'.
/// </summary>
Expand All @@ -145,36 +152,28 @@ public string Caption
public OpenIdConnectConfiguration Configuration { get; set; }

/// <summary>
/// The OpenIdConnect protocol http://openid.net/specs/openid-connect-core-1_0.html
/// recommends adding a nonce to a request as a mitigation against replay attacks when requesting id_tokens.
/// By default the runtime uses cookies with unique names generated from a hash of the nonce.
/// </summary>
public INonceCache NonceCache { get; set; }

/// <summary>
/// Gets or sets the discovery endpoint for obtaining metadata
/// Responsible for retrieving, caching, and refreshing the configuration from metadata.
/// If not provided, then one will be created using the MetadataAddress and Backchannel properties.
/// </summary>
public string MetadataAddress { get; set; }
public IConfigurationManager<OpenIdConnectConfiguration> ConfigurationManager { get; set; }

/// <summary>
/// Gets or sets the expected audience for any received JWT token.
/// Gets or sets a value controlling if the 'CurrentUri' should be used as the 'local redirect' post authentication
/// if AuthenticationProperties.RedirectUri is null or empty.
/// </summary>
/// <value>
/// The expected audience for any received JWT token.
/// </value>
public string Audience { get; set; }
public bool DefaultToCurrentUriOnRedirect { get; set; }

/// <summary>
/// Responsible for retrieving, caching, and refreshing the configuration from metadata.
/// If not provided, then one will be created using the MetadataAddress and Backchannel properties.
/// Gets or sets the discovery endpoint for obtaining metadata
/// </summary>
public IConfigurationManager<OpenIdConnectConfiguration> ConfigurationManager { get; set; }
public string MetadataAddress { get; set; }

/// <summary>
/// Gets or sets if a metadata refresh should be attempted after a SecurityTokenSignatureKeyNotFoundException. This allows for automatic
/// recovery in the event of a signature key rollover. This is enabled by default.
/// The OpenIdConnect protocol http://openid.net/specs/openid-connect-core-1_0.html
/// recommends adding a nonce to a request as a mitigation against replay attacks when requesting id_tokens.
/// By default the runtime uses cookies with unique names generated from a hash of the nonce.
/// </summary>
public bool RefreshOnIssuerKeyNotFound { get; set; }
public INonceCache NonceCache { get; set; }

/// <summary>
/// Gets or sets the <see cref="OpenIdConnectAuthenticationNotifications"/> to notify when processing OpenIdConnect messages.
Expand Down Expand Up @@ -217,11 +216,22 @@ public OpenIdConnectProtocolValidator ProtocolValidator
[SuppressMessage("Microsoft.Design", "CA1056:UriPropertiesShouldNotBeStrings", Justification = "By Design")]
public string RedirectUri { get; set; }

/// <summary>
/// Gets or sets if a metadata refresh should be attempted after a SecurityTokenSignatureKeyNotFoundException. This allows for automatic
/// recovery in the event of a signature key rollover. This is enabled by default.
/// </summary>
public bool RefreshOnIssuerKeyNotFound { get; set; }

/// <summary>
/// Gets or sets the 'resource'.
/// </summary>
public string Resource { get; set; }

/// <summary>
/// Gets or sets the 'response_mode'.
/// </summary>
public string ResponseMode { get; private set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm definitely not sure that exposing ResponseMode is really useful, specially since you can't change it (and BTW, the comment is wrong, you can't set 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.

No, but you can get it. If you want it changed, open an issue.


/// <summary>
/// Gets or sets the 'response_type'.
/// </summary>
Expand All @@ -233,10 +243,7 @@ public OpenIdConnectProtocolValidator ProtocolValidator
public string Scope { get; set; }

/// <summary>
/// Gets or sets the authentication scheme corresponding to the middleware
/// responsible of persisting user's identity after a successful authentication.
/// This value typically corresponds to a cookie middleware registered in the Startup class.
/// When omitted, <see cref="ExternalAuthenticationOptions.SignInScheme"/> is used as a fallback value.
/// Gets or sets the SignInScheme which will be used to set the <see cref="System.Security.Claims.ClaimsIdentity.AuthenticationType"/>.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is not clear and should mention that SignInScheme actually corresponds to the final authentication middleware that will persist the identity (usually a cookie middleware).

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 want the comment short and not link it to another middleware that may or may not be present.

Copy link
Contributor

Choose a reason for hiding this comment

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

In most cases, it will be present, because the OIDC (just like the other social middleware) doesn't issue any cookie: persistence is always delegated to another middleware.

And BTW, you SHOULDN'T set TokenValidationParameters.AuthenticationType using SignInScheme but AuthenticationScheme. See my PR: https://github.com/aspnet/Security/pull/168/files#diff-960390b7edd1b3a479fbd7fcfd07fbe8R55

Copy link
Member

Choose a reason for hiding this comment

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

You don't have to mention specifically cookie middleware, but it would be good to have more detail given that this is one of the more critical properties which will break everything if misconfigured

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we figure out what to say, we will make a change.

/// </summary>
public string SignInScheme { get; set; }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ public static class OpenIdConnectServiceCollectionExtensions
{
public static IServiceCollection ConfigureOpenIdConnectAuthentication([NotNull] this IServiceCollection services, [NotNull] Action<OpenIdConnectAuthenticationOptions> configure)
{
return services.ConfigureOpenIdConnectAuthentication(configure, optionsName: "");
return ConfigureOpenIdConnectAuthentication(services, configure, null);
}

public static IServiceCollection ConfigureOpenIdConnectAuthentication([NotNull] this IServiceCollection services, [NotNull] Action<OpenIdConnectAuthenticationOptions> configure, string optionsName)
Expand All @@ -25,7 +25,7 @@ public static IServiceCollection ConfigureOpenIdConnectAuthentication([NotNull]

public static IServiceCollection ConfigureOpenIdConnectAuthentication([NotNull] this IServiceCollection services, [NotNull] IConfiguration config)
{
return services.ConfigureOpenIdConnectAuthentication(config, optionsName: "");
return ConfigureOpenIdConnectAuthentication(services, config, null);
}

public static IServiceCollection ConfigureOpenIdConnectAuthentication([NotNull] this IServiceCollection services, [NotNull] IConfiguration config, string optionsName)
Expand Down
Loading