New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AddPolicyScheme changes the semantics of IAuthenticationSchemeProvider #1832

Open
brockallen opened this Issue Aug 2, 2018 · 11 comments

Comments

Projects
None yet
4 participants
@brockallen

brockallen commented Aug 2, 2018

I suspect my issue here will be too subtle to do anything about, but I need some guidance on this.

My sense is that the introduction of the new virtual/dynamic policy schemes has in an odd way retroactively changed the semantics of the IAuthenticationSchemeProvider and its various GetDefault... scheme APIs. In doing so, I'd almost call this a breaking change since the semantics are now different (and even deprecated).

As you all know, previously there was only one default authN scheme, sign in scheme, etc. and they all had a fallback to the default scheme. Now with the policy scheme handler, the semantics of what's default (via ForwardDefaultSelector) changes request by request. So now there is no longer the concept of default schemes, since they can change on every request. I'd say the semantics defined by IAuthenticationSchemeProvider is now pointless. Of course now the mechanics are relied upon to discover the virtual scheme, so it feels as if you've pained yourself into a corner and have to keep it.

I know this issue isn't terribly actionable, but it breaks code that previously made assumptions about only one scheme for authentication, sign in, etc.

This makes me think the design was incomplete. I also think there might be agreement on your side of that opinion. Perhaps the IAuthenticationSchemeProvider is what should have been made dynamic, not the schemes themselves. Though, there might be other use cases missed by that approach as well.

So what I'm looking for here is what's the plan going forward for this dynamic policy feature? Is what was added officially suggested and supported as a feature, or is it discouraged until a more complete design can be done? I'd like to know because as of now IdentityServer is not compatible with the virtual scheme feature. If I am to make changes to make it compatible, I'd like to know it's going to fit well into the future work as well. But if this virtual scheme feature is not something that is considered really complete (or may change again in vNext), then perhaps I can postpone trying to rework our code to accommodate it, and I can tell people that we don't officially support it because it's not really a complete feature in ASP.NET Core.

This predicament, as it seems to me, is yet again due to the lack of an isolated/multi-tenant pipeline in the DI system.

Thanks for listening :/

@Eilon Eilon added this to the Discussions milestone Aug 2, 2018

@Eilon Eilon added the discussion label Aug 2, 2018

@Eilon

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Aug 2, 2018

Member

@HaoK - can you share some thoughts on this?

We do agree that this is a tricky scenario...

Member

Eilon commented Aug 2, 2018

@HaoK - can you share some thoughts on this?

We do agree that this is a tricky scenario...

@HaoK

This comment has been minimized.

Show comment
Hide comment
@HaoK

HaoK Aug 3, 2018

Member

So I don't have a clear answer but this is mostly a discussion thread so I can fill in the backstory, @davidfowl can provide his thoughts as well.

The virtual/policy/forwarding scheme feature/changes is closer to what I wish we had time to do in 2.0, its definitely more aligned with our long term vision of where we are going. We basically see schemes as named units of authentication, forwarding allows schemes to compose other schemes by providing an easy way delegate to another scheme.

To me, my main regret was introducing the default stuff late in 2.0 RC/RTM, it was kind of rushed in for feature parity/sugar to make it easier for templates/callers to omit the scheme parameter. But we started 2.0 with the idea that scheme would always be explicit, and that's really how most of the apis operate, the default stuff is pure sugar that comes from the top level apis only.

Now specifically with respect to the GetDefaultXyzScheme methods on IAuthenticationSchemeProvider, those are actually returning scheme names (by default it maps more or less directly to AuthenticationOptions properties with a tiny bit of fallback logic). I don't see how adding forwarding really affects the contract there, you had no guarantees about what those schemes actually did, for example, SignInScheme was already doing forwarding for all of the Remote auth handlers, this just standardized it as base functionality for all handlers. Even if we didn't add forwarding as a feature, this still would have been our recommended pattern for how to do request based scheme selection so IdentityServer would have run into this problem anyways when custom handlers would have implemented it, just https://github.com/aspnet/AuthSamples/tree/master/samples/PathSchemeSelection) would have contained the forwarding/virtual scheme code instead.

Member

HaoK commented Aug 3, 2018

So I don't have a clear answer but this is mostly a discussion thread so I can fill in the backstory, @davidfowl can provide his thoughts as well.

The virtual/policy/forwarding scheme feature/changes is closer to what I wish we had time to do in 2.0, its definitely more aligned with our long term vision of where we are going. We basically see schemes as named units of authentication, forwarding allows schemes to compose other schemes by providing an easy way delegate to another scheme.

To me, my main regret was introducing the default stuff late in 2.0 RC/RTM, it was kind of rushed in for feature parity/sugar to make it easier for templates/callers to omit the scheme parameter. But we started 2.0 with the idea that scheme would always be explicit, and that's really how most of the apis operate, the default stuff is pure sugar that comes from the top level apis only.

Now specifically with respect to the GetDefaultXyzScheme methods on IAuthenticationSchemeProvider, those are actually returning scheme names (by default it maps more or less directly to AuthenticationOptions properties with a tiny bit of fallback logic). I don't see how adding forwarding really affects the contract there, you had no guarantees about what those schemes actually did, for example, SignInScheme was already doing forwarding for all of the Remote auth handlers, this just standardized it as base functionality for all handlers. Even if we didn't add forwarding as a feature, this still would have been our recommended pattern for how to do request based scheme selection so IdentityServer would have run into this problem anyways when custom handlers would have implemented it, just https://github.com/aspnet/AuthSamples/tree/master/samples/PathSchemeSelection) would have contained the forwarding/virtual scheme code instead.

@HaoK

This comment has been minimized.

Show comment
Hide comment
@HaoK

HaoK Aug 3, 2018

Member

With respect to how complex dynamic auth should be composed in the future (long term vision), we were basically envisioning something like this where you can basically have full control based on request/method and plug in whatever handlers to do the actual work:

   services.AddAuthentication("YourPolicyScheme") // Virtual scheme that has logic to delgate
               .AddJwtBearer()
               .AddGoogle()
               .AddFacebook()
               .AddIdentityCookies() // Or your own cookies
               .AddPolicyScheme("YourPolicyScheme", o => {
                    o.FowardAuthenticate = "AuthenticatePolicy";
                    o.FowardChallenge = "ChallengePolicy";
                    o.FowardSignIn = "SignInPolicy";
               })
               .AddPolicyScheme("AuthenticatePolicy", // Custom authenticate routing 
                       o => o.ForwardDefaultSelector = ctx => CustomAuthenticateLogic(ctx))) 
               .AddPolicyScheme("ChallengePolicy", // Custom challenge routing
                       o => o.ForwardDefaultSelector = ctx => CustomChallengeLogic(ctx))) 
               .AddPolicyScheme("SignInPolicy", // Custom sign in routing
                       o => o.ForwardDefaultSelector = ctx => CustomSignInLogic(ctx))) 

Not every app needs this level of complexity, but this is basically the most flexible/complicated case.

Member

HaoK commented Aug 3, 2018

With respect to how complex dynamic auth should be composed in the future (long term vision), we were basically envisioning something like this where you can basically have full control based on request/method and plug in whatever handlers to do the actual work:

   services.AddAuthentication("YourPolicyScheme") // Virtual scheme that has logic to delgate
               .AddJwtBearer()
               .AddGoogle()
               .AddFacebook()
               .AddIdentityCookies() // Or your own cookies
               .AddPolicyScheme("YourPolicyScheme", o => {
                    o.FowardAuthenticate = "AuthenticatePolicy";
                    o.FowardChallenge = "ChallengePolicy";
                    o.FowardSignIn = "SignInPolicy";
               })
               .AddPolicyScheme("AuthenticatePolicy", // Custom authenticate routing 
                       o => o.ForwardDefaultSelector = ctx => CustomAuthenticateLogic(ctx))) 
               .AddPolicyScheme("ChallengePolicy", // Custom challenge routing
                       o => o.ForwardDefaultSelector = ctx => CustomChallengeLogic(ctx))) 
               .AddPolicyScheme("SignInPolicy", // Custom sign in routing
                       o => o.ForwardDefaultSelector = ctx => CustomSignInLogic(ctx))) 

Not every app needs this level of complexity, but this is basically the most flexible/complicated case.

@brockallen

This comment has been minimized.

Show comment
Hide comment
@brockallen

brockallen Aug 3, 2018

I don't see how adding forwarding really affects the contract there

In IdentityServer we need to know which scheme is your main cookie scheme, and we relied upon the semantics of the application wide default authN scheme for that. Now the default authN scheme can be virtual, and could map to potentially many diff schemes (say cookies and bearer). So our assumption that there's a default authN scheme is violated.

I'm not fighting for this particular API to stay, mind you. I just am ready for the authentication system to settle down.

brockallen commented Aug 3, 2018

I don't see how adding forwarding really affects the contract there

In IdentityServer we need to know which scheme is your main cookie scheme, and we relied upon the semantics of the application wide default authN scheme for that. Now the default authN scheme can be virtual, and could map to potentially many diff schemes (say cookies and bearer). So our assumption that there's a default authN scheme is violated.

I'm not fighting for this particular API to stay, mind you. I just am ready for the authentication system to settle down.

@HaoK

This comment has been minimized.

Show comment
Hide comment
@HaoK

HaoK Aug 3, 2018

Member

Is it safe to even have that assumption that there is a main cookie scheme? That's sort of the question I guess, perhaps the defaults led you to that false assumption in the first place.

Member

HaoK commented Aug 3, 2018

Is it safe to even have that assumption that there is a main cookie scheme? That's sort of the question I guess, perhaps the defaults led you to that false assumption in the first place.

@HaoK

This comment has been minimized.

Show comment
Hide comment
@HaoK

HaoK Aug 3, 2018

Member

Why do you need to know what the main application cookie is? perhaps there's a better way to accomplish what identity server is doing

Member

HaoK commented Aug 3, 2018

Why do you need to know what the main application cookie is? perhaps there's a better way to accomplish what identity server is doing

@brockallen

This comment has been minimized.

Show comment
Hide comment
@brockallen

brockallen Aug 4, 2018

Actually, since we have your ear... you might really be interested in all the stuff we need to to the authentication system. For example, we add decorators in several places to achieve what we need to. I'm happy to get on a call to show you. Perhaps you can finally convince fowler that the DI system needs modern features that all the other ones offer :)

This might be of interest to @blowdart and @javiercn too.

brockallen commented Aug 4, 2018

Actually, since we have your ear... you might really be interested in all the stuff we need to to the authentication system. For example, we add decorators in several places to achieve what we need to. I'm happy to get on a call to show you. Perhaps you can finally convince fowler that the DI system needs modern features that all the other ones offer :)

This might be of interest to @blowdart and @javiercn too.

@HaoK

This comment has been minimized.

Show comment
Hide comment
@HaoK

HaoK Aug 7, 2018

Member

@blowdart do you want to setup a call for us to see what they are doing?

Member

HaoK commented Aug 7, 2018

@blowdart do you want to setup a call for us to see what they are doing?

@blowdart

This comment has been minimized.

Show comment
Hide comment
@blowdart

blowdart Aug 7, 2018

Member

Horrible things probably, but sure, when I get back from hacker summer camp next week.

Member

blowdart commented Aug 7, 2018

Horrible things probably, but sure, when I get back from hacker summer camp next week.

@Eilon

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Aug 20, 2018

Member

@blowdart it's next next week now 😄

Member

Eilon commented Aug 20, 2018

@blowdart it's next next week now 😄

@blowdart

This comment has been minimized.

Show comment
Hide comment
@blowdart

blowdart Aug 20, 2018

Member

And they're both on vacation :p

Member

blowdart commented Aug 20, 2018

And they're both on vacation :p

@HaoK HaoK assigned blowdart and unassigned HaoK Aug 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment