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

Fix #140, #167, address a comment in #144 and rework the bearer middleware tests #168

Merged
merged 2 commits into from
Apr 13, 2015
Merged
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 @@ -99,7 +99,10 @@ public string Caption
public PathString CallbackPath { get; set; }

/// <summary>
/// Gets or sets the name of another authentication middleware which will be responsible for actually issuing a user <see cref="System.Security.Claims.ClaimsIdentity"/>.
/// 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.
/// </summary>
public string SignInScheme { get; set; }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,12 +136,13 @@ protected override async Task<AuthenticationTicket> AuthenticateCoreAsync()
AuthenticationTicket = ticket
};

if (securityTokenReceivedNotification.HandledResponse)
await Options.Notifications.SecurityTokenValidated(securityTokenValidatedNotification);
if (securityTokenValidatedNotification.HandledResponse)
{
return securityTokenValidatedNotification.AuthenticationTicket;
}

if (securityTokenReceivedNotification.Skipped)
if (securityTokenValidatedNotification.Skipped)
{
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -585,4 +585,4 @@ private async Task<bool> InvokeReplyPathAsync()
return false;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@
using System.IdentityModel.Tokens;
using System.Net.Http;
using System.Text;
using Microsoft.AspNet.Builder;
using Microsoft.AspNet.DataProtection;
using Microsoft.AspNet.Http;
using Microsoft.AspNet.Authentication.DataHandler;
using Microsoft.AspNet.Authentication.DataHandler.Encoder;
using Microsoft.AspNet.Authentication.DataHandler.Serializer;
using Microsoft.AspNet.Builder;
using Microsoft.AspNet.DataProtection;
using Microsoft.AspNet.Http;
using Microsoft.Framework.Logging;
using Microsoft.Framework.OptionsModel;
using Microsoft.IdentityModel.Protocols;
Expand Down Expand Up @@ -45,9 +45,14 @@ public OpenIdConnectAuthenticationMiddleware(
{
_logger = loggerFactory.CreateLogger<OpenIdConnectAuthenticationMiddleware>();

if (string.IsNullOrEmpty(Options.SignInScheme))
Copy link
Contributor

Choose a reason for hiding this comment

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

What is SignInScheme used for?

{
Options.SignInScheme = externalOptions.Options.SignInScheme;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you throw if SignInScheme is null or empty. I see this check in some middlewares and some lack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO, the persistence delegation should be optional so that the end dev can implement his own logic, just like the CallbackPath is not mandatory.

That said, I agree that this kind of inconsistency is bad and should be fixed. https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.Twitter/TwitterAuthenticationHandler.cs#L206 seems to indicate that we should remove the null check you mentioned and make SignInScheme optional.

Copy link
Member

Choose a reason for hiding this comment

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

So, we are talking about SignInScheme that lives on ExternalAuthenticationOptions correct? This should be optional always, since its only useful when forwarding a "passive" (new terminology is Automatic = false), auth middleware's ticket to something automatic like bearer/cookie for persistence. We probably should rename this to convey the usage. Perhaps something like SignInForwardingOptions && SignInScheme maybe? @lodejard what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this specific SignInScheme lives on OpenIdConnectAuthenticationOptions and is just a wrapper around TokenValidationParameters.AuthenticationType. But of course, it serves the same purpose.

That said, I'm not sure that the passive/active/automatic/manual question is pertinent here, as it can be used in both modes (and anyway, this middleware doesn't inherit from AutomaticAuthenticationHandler).

Copy link
Contributor

Choose a reason for hiding this comment

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

I asked above, so I will ask again here.
What is SignInScheme used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would require two (or three if you have a global configuration) services.Configure<ExternalAuthenticationOptions> calls + the OIDC options themselves and I'm not sure it would be easier from a UX perspective.

Copy link
Member

Choose a reason for hiding this comment

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

So is it more likely to have multiple OIDC middleware in the pipeline, as compared to Google/Facebook/Twitter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More likely? In most cases, I'd say no. But anyway, the same remarks apply to the OAuth2 providers, that also expose a SignInScheme property.

https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OAuth/OAuthAuthenticationOptions.cs#L104

Copy link
Member

Choose a reason for hiding this comment

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

Okay for now lets just leave them where they are, I filed #174 to sort this out later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great 😎


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

if (Options.StateDataFormat == null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -233,13 +233,12 @@ public OpenIdConnectProtocolValidator ProtocolValidator
public string Scope { get; set; }

/// <summary>
/// Gets or sets the AuthenticationScheme used when creating the <see cref="System.Security.Claims.ClaimsIdentity"/>.
/// 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.
/// </summary>
public string SignInScheme
{
get { return TokenValidationParameters.AuthenticationType; }
set { TokenValidationParameters.AuthenticationType = value; }
}
public string SignInScheme { get; set; }

/// <summary>
/// Gets or sets the type used to secure data handled by the middleware.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,10 @@ public string Caption
public PathString CallbackPath { get; set; }

/// <summary>
/// Gets or sets the name of another authentication middleware which will be responsible for actually issuing a user <see cref="System.Security.Claims.ClaimsIdentity"/>.
/// 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.
/// </summary>
public string SignInScheme { get; set; }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ namespace Microsoft.AspNet.Authentication
{
public class ExternalAuthenticationOptions
{
/// <summary>
/// Gets or sets the authentication scheme corresponding to the default middleware
/// responsible of persisting user's identity after a successful authentication.
/// This value typically corresponds to a cookie middleware registered in the Startup class.
/// </summary>
public string SignInScheme { get; set; }
}
}
Loading