Improve extensibility of middleware by introducing interfaces for AuthenticationOptions

[kingdango]

Resolve #35

[Tratcher]

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?

[kingdango]

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!

[kingdango]

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;
    }
}

[davidfowl]

I'l let @Tratcher and @loudej deal with the specifics of this PR but there's some other things that need to be done:

• You need to sign a CLA - https://github.com/aspnet/Home/blob/master/CONTRIBUTING.md#contributing-code-and-content
• I thought I saw tabs in the PR, if so, please change them to spaces, if not ignore this 😄

[Tratcher]

I think the base design needs more work before continuing with a PR.

Audience: The vast majority of our users are single tenant applications, which is why we targeted them for the initial design. Multi-tenant is an advanced scenario for a small subset of users. Whatever we come up with for multi-tenant needs to preserve the simplicity of the common scenario.

To that end, I think we'd need two different components; simple and advanced, possibly implementing the simple one on top of the other.

I think the first question for multi-tenant is when and how does the application determine which tenant to use for a given request. For Facebook the tenant data (e.g. client id) is used on both the challenge and response paths, so you'd have to select the tenant before or during the initial challenge and preserve that information across requests to the authenticated response. This means you have to select the tenant either in the application and flow it down when the challenge is issued (e.g. the user selected from a list), or in a factory when the challenge is processed, based on something in the request context (or both).

Think about that and we'll pick this up again in the morning.

[Eilon]

@kingdango do you have any thoughts on this given @Tratcher's comments? BTW I don't yet see a CLA either, and that can take a few days for us to confirm and process, so it's often a good idea to get that started ASAP.

Thanks,
Eilon

[brentschmaltz]

A solution may lie in providing delegates for obtaining the 'appId' etc. or the entire Options for that matter, the default would return the Options set on the Middleware. I think we can provide a solution without another interface.

Tratcher's observation that the majority of users are developing apps that function as a single 'client' seems correct.
I think the scenario that this discussion brings up, is one that will help some users.

[Eilon]

@kingdango we're closing out this PR for now because we would like to take a different approach to multi-tenancy for the auth middleware. As we make progress on any future design ideas, we'll keep #35 updated with notes.