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

Improve extensibility of middleware by introducing interfaces for AuthenticationOptions #44

Closed
wants to merge 2 commits into from
Closed

Improve extensibility of middleware by introducing interfaces for AuthenticationOptions #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
667906f
Introduced IAuthenticationOptions and IFacebookAuthenticationOptions …
kingdango Aug 21, 2014
5d7837a
Updated UseFacebookAuthentication extension method to use IFacebookAu…
kingdango Aug 21, 2014
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
2 changes: 1 addition & 1 deletion src/Microsoft.AspNet.Security.Facebook/FacebookAuthenticationExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public static IBuilder UseFacebookAuthentication([NotNull] this IBuilder app, [N
/// <param name="app">The <see cref="IBuilder"/> passed to the configure method</param>
/// <param name="options">Middleware configuration options</param>
/// <returns>The updated <see cref="IBuilder"/></returns>
public static IBuilder UseFacebookAuthentication([NotNull] this IBuilder app, [NotNull] FacebookAuthenticationOptions options)
public static IBuilder UseFacebookAuthentication([NotNull] this IBuilder app, [NotNull] IFacebookAuthenticationOptions options)
{
if (string.IsNullOrEmpty(options.SignInAsAuthenticationType))
{
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.AspNet.Security.Facebook/FacebookAuthenticationHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

namespace Microsoft.AspNet.Security.Facebook
{
internal class FacebookAuthenticationHandler : AuthenticationHandler<FacebookAuthenticationOptions>
internal class FacebookAuthenticationHandler : AuthenticationHandler<IFacebookAuthenticationOptions>
{
private const string XmlSchemaString = "http://www.w3.org/2001/XMLSchema#string";
private const string TokenEndpoint = "https://graph.facebook.com/oauth/access_token";
Expand Down
10 changes: 5 additions & 5 deletions src/Microsoft.AspNet.Security.Facebook/FacebookAuthenticationMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace Microsoft.AspNet.Security.Facebook
/// ASP.NET middleware for authenticating users using Facebook
/// </summary>
[SuppressMessage("Microsoft.Design", "CA1001:TypesThatOwnDisposableFieldsShouldBeDisposable", Justification = "Middleware is not disposable.")]
public class FacebookAuthenticationMiddleware : AuthenticationMiddleware<FacebookAuthenticationOptions>
public class FacebookAuthenticationMiddleware : AuthenticationMiddleware<IFacebookAuthenticationOptions>
{
private readonly ILogger _logger;
private readonly HttpClient _httpClient;
Expand All @@ -32,7 +32,7 @@ public FacebookAuthenticationMiddleware(
IDataProtectionProvider dataProtectionProvider,
ILoggerFactory loggerFactory,
IServiceProvider services,
FacebookAuthenticationOptions options)
IFacebookAuthenticationOptions options)
: base(next, options)
{
if (string.IsNullOrWhiteSpace(Options.AppId))
Expand Down Expand Up @@ -65,14 +65,14 @@ public FacebookAuthenticationMiddleware(
/// <summary>
/// Provides the <see cref="AuthenticationHandler"/> object for processing authentication-related requests.
/// </summary>
/// <returns>An <see cref="AuthenticationHandler"/> configured with the <see cref="FacebookAuthenticationOptions"/> supplied to the constructor.</returns>
protected override AuthenticationHandler<FacebookAuthenticationOptions> CreateHandler()
/// <returns>An <see cref="AuthenticationHandler"/> configured with the <see cref="IFacebookAuthenticationOptions"/> supplied to the constructor.</returns>
protected override AuthenticationHandler<IFacebookAuthenticationOptions> CreateHandler()
{
return new FacebookAuthenticationHandler(_httpClient, _logger);
}

[SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Managed by caller")]
private static HttpMessageHandler ResolveHttpMessageHandler(FacebookAuthenticationOptions options)
private static HttpMessageHandler ResolveHttpMessageHandler(IFacebookAuthenticationOptions options)
{
HttpMessageHandler handler = options.BackchannelHttpHandler ??
#if NET45
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.AspNet.Security.Facebook/FacebookAuthenticationOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ namespace Microsoft.AspNet.Security.Facebook
/// <summary>
/// Configuration options for <see cref="FacebookAuthenticationMiddleware"/>
/// </summary>
public class FacebookAuthenticationOptions : AuthenticationOptions
public class FacebookAuthenticationOptions : AuthenticationOptions, IFacebookAuthenticationOptions
{
/// <summary>
/// Initializes a new <see cref="FacebookAuthenticationOptions"/>
Expand Down
88 changes: 88 additions & 0 deletions src/Microsoft.AspNet.Security.Facebook/IFacebookAuthenticationOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
using System;
using System.Net.Http;
using Microsoft.AspNet.Http;
using Microsoft.AspNet.Http.Security;
using System.Collections.Generic;

namespace Microsoft.AspNet.Security.Facebook
{
/// <summary>
/// Summary description for IFacebookAuthenticationOptions
/// </summary>
public interface IFacebookAuthenticationOptions : IAuthenticationOptions
{
/// <summary>
/// Gets or sets the Facebook-assigned appId
/// </summary>
string AppId { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

In the multi-tenant scenario how does the options implementer get access to the current request in order to decide which AppId to return? HttpContext.Current? That's not a pattern we want to encourage. They'd also have to re-execute that discovery logic for every property call (AppId, AppSecret, Scope, etc.).

Would you do anything differently if you weren't concerned about back-compat?

Copy link
Author

Choose a reason for hiding this comment

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

Good question! In my working example (the one where I duplicated the Facebook middleware) I use HttpContext.Current to get the current Owin context in which I've already built up a TenantContext with other middleware.

HttpContext.Current.Request.GetOwinContext().Get<TenantContext>();

Is there a better way to get access to the context?

If not concerned about backward compatibility I might add a factory method to IAuthenticationOptions, perhaps BuildOptionsForRequest(HttpRequest request). We might call that method from the AuthenticationHandler as part of the per-request work performed. Honestly I haven't thought that through completely but that's probably the direction I'd head in. Do you think that's a better design? Personally I think HttpContext.Current is a fine way for auth middleware to get the context but perhaps I'm not seeing the whole picture.

Side note, I think there is a slight bias towards these AuthenticationOptions being static in nature (set once and forget), which probably makes sense for a lot of developers in simple scenarios, but add in even modestly complex requirements and that design breaks down quickly. I think the bias should be towards per-request configuration -- after all, we're looking at every request already -- and then the default AuthenticationOptions implementation can just support a static configuration for the majority of "basic" users to roll out of the box.

Thanks for your thoughts on this, cheers!

Copy link
Author

Choose a reason for hiding this comment

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

Some more info:
In my current hacked version (on top of Owin.Security) I initialize FacebookAuthenticationOptions with a "settings provider" which uses a cache of tenant contexts and the current request to resolve facebook api settings per tenant.

So the FacebookAuthenticationOptions class looks like this (abbreviated):

public string AppId { get { return _settingsProvider.AppId; } }
public string AppSecret { get { return _settingsProvider.AppSecret; } }
public PathString CallbackPath { get { return new PathString(_settingsProvider.CallbackPath); } }

And the provider:

public class FacebookAuthenticationSettingsProvider : IFacebookAuthenticationApiSettingsProvider
{
    public string AppId
    {
        get
        {
            var community = DeriveCommunity();
            if (community == null || community.FacebookSettings == null) return null;

            return community.FacebookSettings.AppId;
        }
    }

    public string AppSecret
    {
        get
        {
            var community = DeriveCommunity();
            if (community == null || community.FacebookSettings == null) return null;

            return community.FacebookSettings.AppSecret;
        }
    }

    public string CallbackPath
    {
        get
        {
            var community = DeriveCommunity();

            return community != null ? string.Format("/{0}/signin-facebook", community.Namespace) : "/signin-facebook";

        }
    }

    private Community DeriveCommunity()
    {
        var context = HttpContext.Current.Request.GetOwinContext().Get<CommunityContext>();

        if (context == null) return null;

        return context.Community;
    }
}


/// <summary>
/// Gets or sets the Facebook-assigned app secret
/// </summary>
string AppSecret { get; set; }
#if NET45
/// <summary>
/// Gets or sets the a pinned certificate validator to use to validate the endpoints used
/// in back channel communications belong to Facebook.
/// </summary>
/// <value>
/// The pinned certificate validator.
/// </value>
/// <remarks>If this property is null then the default certificate checks are performed,
/// validating the subject name and if the signing chain is a trusted party.</remarks>
ICertificateValidator BackchannelCertificateValidator { get; set; }
#endif
/// <summary>
/// Gets or sets timeout value in milliseconds for back channel communications with Facebook.
/// </summary>
/// <value>
/// The back channel timeout in milliseconds.
/// </value>
TimeSpan BackchannelTimeout { get; set; }

/// <summary>
/// The HttpMessageHandler used to communicate with Facebook.
/// This cannot be set at the same time as BackchannelCertificateValidator unless the value
/// can be downcast to a WebRequestHandler.
/// </summary>
HttpMessageHandler BackchannelHttpHandler { get; set; }

/// <summary>
/// Get or sets the text that the user can display on a sign in user interface.
/// </summary>
string Caption { get; set; }

/// <summary>
/// The request path within the application's base path where the user-agent will be returned.
/// The middleware will process this request when it arrives.
/// Default value is "/signin-facebook".
/// </summary>
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"/>.
/// </summary>
string SignInAsAuthenticationType { get; set; }

/// <summary>
/// Gets or sets the <see cref="IFacebookAuthenticationNotifications"/> used to handle authentication events.
/// </summary>
IFacebookAuthenticationNotifications Notifications { get; set; }

/// <summary>
/// Gets or sets the type used to secure data handled by the middleware.
/// </summary>
ISecureDataFormat<AuthenticationProperties> StateDataFormat { get; set; }

/// <summary>
/// A list of permissions to request.
/// </summary>
IList<string> Scope { get; }

/// <summary>
/// Gets or sets if the appsecret_proof should be generated and sent with Facebook API calls.
/// This is enabled by default.
/// </summary>
bool SendAppSecretProof { get; set; }
}
}
4 changes: 2 additions & 2 deletions src/Microsoft.AspNet.Security.Facebook/Notifications/FacebookApplyRedirectContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Microsoft.AspNet.Security.Facebook
/// <summary>
/// Context passed when a Challenge causes a redirect to authorize endpoint in the Facebook middleware
/// </summary>
public class FacebookApplyRedirectContext : BaseContext<FacebookAuthenticationOptions>
public class FacebookApplyRedirectContext : BaseContext<IFacebookAuthenticationOptions>
{
/// <summary>
/// Creates a new context object.
Expand All @@ -21,7 +21,7 @@ public class FacebookApplyRedirectContext : BaseContext<FacebookAuthenticationOp
/// <param name="redirectUri">The initial redirect URI</param>
[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1054:UriParametersShouldNotBeStrings", MessageId = "3#",
Justification = "Represents header value")]
public FacebookApplyRedirectContext(HttpContext context, FacebookAuthenticationOptions options,
public FacebookApplyRedirectContext(HttpContext context, IFacebookAuthenticationOptions options,
AuthenticationProperties properties, string redirectUri)
: base(context, options)
{
Expand Down
5 changes: 1 addition & 4 deletions src/Microsoft.AspNet.Security/AuthenticationOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,14 @@


using System;
using System.Collections.Generic;
using Microsoft.AspNet.Http.Security;
using Microsoft.AspNet.HttpFeature.Security;
using Microsoft.AspNet.PipelineCore.Security;

namespace Microsoft.AspNet.Security
{
/// <summary>
/// Base Options for all authentication middleware
/// </summary>
public abstract class AuthenticationOptions
public abstract class AuthenticationOptions : IAuthenticationOptions
{
private string _authenticationType;

Expand Down
28 changes: 28 additions & 0 deletions src/Microsoft.AspNet.Security/IAuthenticationOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
using Microsoft.AspNet.Http.Security;

namespace Microsoft.AspNet.Security
{
/// <summary>
/// Interface for Base Options for all authentication middleware
/// </summary>
public interface IAuthenticationOptions
{
/// <summary>
/// The AuthenticationType in the options corresponds to the IIdentity AuthenticationType property. A different
/// value may be assigned in order to use the same authentication middleware type more than once in a pipeline.
/// </summary>
string AuthenticationType { get;set; }

/// <summary>
/// If Active the authentication middleware alter the request user coming in and
/// alter 401 Unauthorized responses going out. If Passive the authentication middleware will only provide
/// identity and alter responses when explicitly indicated by the AuthenticationType.
/// </summary>
AuthenticationMode AuthenticationMode { get; set; }

/// <summary>
/// Additional information about the authentication type which is made available to the application.
/// </summary>
AuthenticationDescription Description { get; set; }
}
}
6 changes: 3 additions & 3 deletions src/Microsoft.AspNet.Security/Infrastructure/AuthenticationHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public abstract class AuthenticationHandler : IAuthenticationHandler
private bool _applyResponseInitialized;
private object _applyResponseSyncLock;

private AuthenticationOptions _baseOptions;
private IAuthenticationOptions _baseOptions;

protected IChallengeContext ChallengeContext { get; set; }
protected SignInIdentityContext SignInIdentityContext { get; set; }
Expand All @@ -53,14 +53,14 @@ protected HttpResponse Response

protected PathString RequestPathBase { get; private set; }

internal AuthenticationOptions BaseOptions
internal IAuthenticationOptions BaseOptions
{
get { return _baseOptions; }
}

public IAuthenticationHandler PriorHandler { get; set; }

protected async Task BaseInitializeAsync(AuthenticationOptions options, HttpContext context)
protected async Task BaseInitializeAsync(IAuthenticationOptions options, HttpContext context)
{
_baseOptions = options;
Context = context;
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.AspNet.Security/Infrastructure/AuthenticationHandler`1.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace Microsoft.AspNet.Security.Infrastructure
/// Base class for the per-request work performed by most authentication middleware.
/// </summary>
/// <typeparam name="TOptions">Specifies which type for of AuthenticationOptions property</typeparam>
public abstract class AuthenticationHandler<TOptions> : AuthenticationHandler where TOptions : AuthenticationOptions
public abstract class AuthenticationHandler<TOptions> : AuthenticationHandler where TOptions : IAuthenticationOptions
{
protected TOptions Options { get; private set; }

Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.AspNet.Security/Infrastructure/AuthenticationMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

namespace Microsoft.AspNet.Security.Infrastructure
{
public abstract class AuthenticationMiddleware<TOptions> where TOptions : AuthenticationOptions
public abstract class AuthenticationMiddleware<TOptions> where TOptions : IAuthenticationOptions
{
private readonly RequestDelegate _next;

Expand Down