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

Standardize services.AddXyz() behavior with default config #1191

Closed
HaoK opened this issue Apr 27, 2017 · 13 comments
Closed

Standardize services.AddXyz() behavior with default config #1191

HaoK opened this issue Apr 27, 2017 · 13 comments

Comments

@HaoK
Copy link
Member

HaoK commented Apr 27, 2017

Should AddXyz(o => o = { }) also register the default configuration binding? The complication is each auth can be called multiple times with the name overload, so one possible soln is to have an AddXyz that takes a name, and binds to the named section.

Examples:

services.AddCookies(); // binds to section: Microsoft.AspNetCore.AspNetCore.Authentication:Cookies
services.AddCookies("Application"); // binds to Microsoft.AspNetCore.AspNetCore.Authentication:Application
services.AddOAuth("Github"); // binds to Microsoft.AspNetCore.AspNetCore.Authentication:Github

// Auth generalizes to 
services.AddXyzAuthentication() // binds to "Microsoft.AspNetCore.AspNetCore.Authentication:<Xyz>"
services.AddXyzAuthentication(schemeName) // binds to "Microsoft.AspNetCore.AspNetCore.Authentication:<schemeName>"

// Frameworks:
services.AddIdentity(); => binds to config.GetSection("Microsoft.AspNetCore.Identity")

// Generalize to:
services.AddXyz(); => binds XyzOptions to config.GetSection("Microsoft.AspNetCore.Xyz")

We also should add more support in the binder for things like PathString which cannot be bound today.

@glennc @ajcvickers @divega @DamianEdwards @davidfowl @Eilon @Tratcher

@HaoK HaoK added this to the 2.0.0-preview2 milestone Apr 27, 2017
@HaoK HaoK self-assigned this Apr 27, 2017
@Eilon
Copy link
Contributor

Eilon commented Apr 27, 2017

Interesting problem. How do I add stuff without using any config? Presumably that's also important.

@HaoK
Copy link
Member Author

HaoK commented Apr 27, 2017

Good question, when you say 'without using any config'. There's two ways to interpret that question.

  1. If you mean when there's no config section specified, the binding would be a no-op. But there is still a change that all AddXyz's require a IConfiguration to be in DI.
  2. If you are asking how to explicitly ignore a default section, that becomes problematic if we unify all AddXyz() overloads to implicitly register the config binding.

@Eilon
Copy link
Contributor

Eilon commented Apr 27, 2017

Yeah I think (2) is what I'm saying: I happen to have some config sections in config, but I don't want them to be used.

But maybe that's not a problem?

@HaoK
Copy link
Member Author

HaoK commented Apr 27, 2017

Yeah I'm not sure how big a problem this would be, I guess it depends on how much we end up putting in the config for templates by default, but I'm guessing it's mostly going to be strings, which you could just override in the action.

@Eilon
Copy link
Contributor

Eilon commented Apr 27, 2017

Yeah maybe it's fine.

@DamianEdwards - thoughts on having all the various AddXyz() ServiceCollection extension methods dig up config? I'm not crazy about that, but I'm not sure when it's problematic...

@DamianEdwards
Copy link
Member

I think there's some wider questions to ask here about how we want "convention based configuration" to work, but in the scope of those services that support named options for the purposes of (faux) multi-tenancy/instances, I think @HaoK proposal looks like a fair place to start (we'll likely tweak as we go of course, e.g. I wouldn't use the full type name but rather a nice short name).

@Eilon
Copy link
Contributor

Eilon commented Apr 27, 2017

So we should probably get together and discuss this along with @davidfowl and whoever else ought to be there.

@divega
Copy link

divega commented Apr 28, 2017

Agreed this is a good starting point and that shorter names could be nicer. Should we group the keys in sections per area rather than putting dot separated namespaces in the keys? The convention also needs to be predictable when there are not named and named things. E.g. Named could always bind to sub keys nested under the key of the not named one:

services.AddCookies(); // binds to AspNetCore:Authentication:Cookies (append :Default?)
services.AddCookies("Application"); // binds to AspNetCore:Authentication:Cookies:Application

@HaoK
Copy link
Member Author

HaoK commented Apr 28, 2017

Well right now auth is the only thing with names, and luckily there's always a name for auth, even in the default case(its the name of the auth scheme), i.e. Cookies, Google, Facebook, so we don't have to do anything special for the default section from any other named section.

An example for identity + auth

ConfigureServices code:

    services.AddIdentity<ApplicationUser, ApplicationRole>().AddEntityFrameworkStores();
    services.AddGoogleAuthentication();
    services.AddFacebookAuthentication();

Config

{
    'AspNetCore': {
        'Authentication': { // AuthenticationOptions bind to this section (and ignores Schemes)
            'DefaultAuthenticateScheme' : 'Application'
            'DefaultSignInScheme' : 'External',
            'DefaultChallengeScheme' : 'Application'

            'Schemes' : { // Schemes bind to their name in the scheme subection

               'Application': { // CookieAuthenticationOptions("Application")
                   'CookieName': '.AspNetCore.Application'
                },

               'External': { // CookieAuthenticationOptions("External")
                   'CookieName': '.AspNetCore.External',
                   'Expires': '0:05:00'
                },

               'Google': { // GoogleOptions("Google")
                   'ClientId': 'abcdf', // These would be in user secrets?
                   'ClientSecret': 'ghijz'
                },

               'Facebook': { // FacebookOptions("Facebook")
                   'ClientId': 'abcdf', // These would be in user secrets?
                   'ClientSecret': 'ghijz'
                },
        },

        'Identity' : { // IdentityOptions
            'SecurityStampValidationInterval' : '0:30:00'
            'Password' : {
                'RequiredLength' : 6,
                'RequiredUniqueChars': 3,
            },
        }
}

@Tratcher
Copy link
Member

So you'd have that compose with AddFacebook(options => ...)? E.g. I could define the events in code and the secrets in config?

@HaoK
Copy link
Member Author

HaoK commented Apr 28, 2017

Yeah the idea is that all the AddXyz's will compose

@Eilon
Copy link
Contributor

Eilon commented May 11, 2017

@HaoK - can you set up a mtg for this?

@HaoK
Copy link
Member Author

HaoK commented May 17, 2017

New tracking issue with current POR: aspnet/MetaPackages#117

@HaoK HaoK closed this as completed May 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants