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

Add a help extensions for setting up OIDC with Cookie sign in #1212

Closed
danroth27 opened this issue May 11, 2017 · 17 comments
Closed

Add a help extensions for setting up OIDC with Cookie sign in #1212

danroth27 opened this issue May 11, 2017 · 17 comments

Comments

@danroth27
Copy link
Member

danroth27 commented May 11, 2017

To setup OIDC with cookie sign in you have to do three things: 1. add OIDC, 2. add Cookies, 3. configure default auth schemes. We should add an help extension that does these three things for you.

Currently we have this code in the template that we should replace with an API in the framework:

        public static IServiceCollection AddWebApplicationAuthentication(this IServiceCollection services)
        {
            services.AddAuthentication(sharedOptions =>
            {
                sharedOptions.DefaultSignInScheme = CookieAuthenticationDefaults.AuthenticationScheme;
                sharedOptions.DefaultAuthenticateScheme = CookieAuthenticationDefaults.AuthenticationScheme;
                sharedOptions.DefaultChallengeScheme = OpenIdConnectDefaults.AuthenticationScheme;
            });

            services.AddOpenIdConnectAuthentication();
            services.AddCookieAuthentication();
            return services;
        }

Alternative names: AddOpenIdConnectWithCookies, AddOpenIdConnectWithCookieSignIn, AddDefaultOpenIdConnectAuthentication, ...

@danroth27 danroth27 added this to the 2.0.0-preview2 milestone May 11, 2017
@danroth27 danroth27 changed the title Add a help extensions for setting OIDC with Cookie sign in Add a help extensions for setting up OIDC with Cookie sign in May 11, 2017
@kevinchalet
Copy link
Contributor

Doesn't sound like a good idea to me.

If you do that for OIDC, people will want the same thing for all the OAuth1/OAuth2 providers (Facebook, Google, Twitter, MS Account, etc.) and you'll end up with multiple extensions that all update the shared authentication options and replace the schemes set by the previous calls, which won't have the result you expect.

@HaoK
Copy link
Member

HaoK commented May 11, 2017

Well the better way to think of this is probably more like AddIdentity which does a lot of opinionated auth setup as well that you have to override if you don't like. i.e. Identity sets the DefaultSIgnInScheme to its External cookies scheme. Its an extension for the templates to use, where we don't have that with the other Oauth providers.

That said, Identity is being refactored a bit into Microsoft.Extensions + Microsoft.AspNet, so there will likely end up with AddIdentityCore() [non auth], with AddIdentity calling AddIdentityCore + configuring auth similar to what this method is doing.

@HaoK
Copy link
Member

HaoK commented May 11, 2017

In terms of naming, I think something like AddDefault[Something]Authentication is good since the default will help signal that its opinionated.

@kevinchalet
Copy link
Contributor

kevinchalet commented May 11, 2017

Well the better way to think of this is probably more like AddIdentity which does a lot of opinionated auth setup as well that you have to override if you don't like. i.e. Identity sets the DefaultSIgnInScheme to its External cookies scheme.

I'm not sure adding more magical stuff is the right answer. The fact Identity does this in a totally non-obvious way is already problematic. Since you can't combine 2 automatic handlers in 2.0, people will have to find how they can combine cookies + bearer authentication in their applications. If all these extensions override the default schemes in multiple places, it will be a total mess.

@HaoK
Copy link
Member

HaoK commented May 11, 2017

Sure, this isn't solving that problem (shared options being used), this is just the equivalent/replacement for AddIdentity's auth logic for the new templates that aren't using AddIdentity to setup auth.

We can certainly revisit things if you have any specific ideas for improvement to include in Auth 2.0 for this, but there's only a single default authenticate (automatic) scheme in 2.0 now, last one wins in terms of setting that.

@HaoK
Copy link
Member

HaoK commented May 11, 2017

Also specifically for "combine cookies + bearer authentication". shared options and defaults can pick either cookies or bearer, but in reality those apps should fully specify the schemes in their challenge/authorize/sign-ins so there's no magic or confusion.

@kevinchalet
Copy link
Contributor

kevinchalet commented May 11, 2017

Sure, this isn't solving that problem (shared options being used)

True, it just makes the situation worse 😄

We can certainly revisit things if you have any specific ideas for improvement to include in Auth 2.0 for this

A possible approach would be to stop relying on automatic authentication in the templates and make that something devs would have to explicitly enable by registering themselves the right schemes in the shared options. At least, it would make the "am I using cookies or bearer authentication when I register both?" answer way more obvious.

@Tratcher
Copy link
Member

How much of this is an attempt to squash template/sample code? How often do developers actually write this vs have it handed to them?

@danroth27
Copy link
Member Author

I think Cookies, JWT Bearer and OIDC are special in that they are the three most common active authentication schemes. We wouldn't provide extensions that set the default schemes for things like social logins.

@brockallen
Copy link

How often do developers actually write this vs have it handed to them?

In all the projects I consult on, I'm configuring all this stuff manually so they can see what's what. Perhaps I'm in the minority, and they might not know what all the little settings do, but they see that there are all these settings so they know it's all in front of them and they see what properties they might need to go read up on.

@Eilon
Copy link
Member

Eilon commented May 19, 2017

We should add a new AddOpenIdConnectCookieAuthentication method, but we need to figure out where it can go.

The purpose of this API it to have a scenario-focused helper API. It's higher-level than the low-level "atom" methods that are needed by more complex scenarios.

@kevinchalet
Copy link
Contributor

We should add a new AddOpenIdConnectCookieAuthentication method, but we need to figure out where it can go.

Not in the core packages, please.

The purpose of this API it to have a scenario-focused helper API. It's higher-level than the low-level "atom" methods that are needed by more complex scenarios.

It's so "scenario-focused" that it can't be used to configure an OIDC provider as an external provider in an app using ASP.NET Core Identity (it will spawn an additional cookies handler instead of using the external cookies handler configured by Identity). Yet, it's an extremely frequent scenario, and not low-level at all.

@Eilon
Copy link
Member

Eilon commented May 22, 2017

@PinpointTownes do you have some thoughts on what would be a useful scenario-focused helper, but wouldn't be too restrictive? It's always a challenge to balance out the two. I think for a lot of developers having a nicely-named method that "does exactly what I want" is very useful. And, I also agree that we want to make sure it isn't a "pit of failure" where the moment you want to do something even slightly more advanced, you have to go down a completely different path.

@kevinchalet
Copy link
Contributor

@Eilon I don't believe adding generic helpers is the right answer to this problem, because there's really no "unique way" to use OIDC in ASP.NET Core. Adding new extensions would also make the situation more confusing than it is already (most people have no idea if/when/how they should use app.UseJwtBearerAuthentication() or app.UseOpenIdConnectAuthentication()).

IMHO, it would be much better to concentrate the effort on documentation and explain how things work in the different scenarios (do you want to treat your OIDC as the single source of truth or just like any other social provider? Do you use Identity or the cookies middleware as-is?).

@HaoK
Copy link
Member

HaoK commented May 25, 2017

per discussing @danroth27 we will keep this in the template code

@HaoK HaoK closed this as completed May 25, 2017
@danroth27 danroth27 reopened this May 25, 2017
@danroth27
Copy link
Member Author

Let's discuss with the B2C and Org auth stuff next week.

@danroth27
Copy link
Member Author

We decided not to do this.

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

6 participants