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

Provide a way to prevent AuthenticationSchemeProvider from picking a specific scheme handler #1287

Closed
kevinchalet opened this issue Jul 2, 2017 · 13 comments
Assignees
Milestone

Comments

@kevinchalet
Copy link
Contributor

When there's only one authentication handler registered, AuthenticationSchemeProvider uses it as the default scheme handler for basically everything (authentication, challenge, forbidden, sign-in and sign-out).

Sadly, this logic makes no sense for handlers like ASOS, that throws an InvalidOperationException if you try to call one of the IAuthenticationService methods outside an OpenID Connect request (for obvious security reasons).

Yet, AuthenticationMiddleware uses GetDefaultAuthenticateSchemeAsync() and automatically calls [ASOS].AuthenticateAsync() for each request, which is detected as an invalid operation and causes an exception when ASOS is the only handler registered in the DI container.

Please consider adding a flag to disable this extremely annoying behavior, either at the global level, or per-handler.

/cc @HaoK

@kevinchalet
Copy link
Contributor Author

A possible (and good) approach would consist in adding new "capabilities" properties to AuthenticationScheme/Builder that would be used as hints by AuthenticationSchemeProvider to determine whether a given scheme handler would be a good candidate for being a default scheme.

E.g:

public class AuthenticationScheme(Builder)
{
    public bool UseAsDefaultAuthenticateScheme { get; set; }
    public bool UseAsDefaultChallengeScheme { get; set; }
    public bool UseAsDefaultForbiddenScheme { get; set; }
    public bool UseAsDefaultSignInScheme { get; set; }
    public bool UseAsDefaultSignOutScheme { get; set; }
}

Using this approach, we could even reunify IAuthenticationHandler and IAuthenticationSignInHandler/IAuthenticationSignOutHandler (and revert all the logging madness it caused...)

@kevinchalet
Copy link
Contributor Author

Adding more people so we can discuss it before it's too late.

/cc @Tratcher @Eilon

@HaoK
Copy link
Member

HaoK commented Jul 3, 2017

You should be able to just plug in your own IAuthenticationSchemeProvider implementation that always returns null for DefaultAuthenticateScheme, that's the mechanism to control the defaults

@kevinchalet
Copy link
Contributor Author

No. I don't want to force my users to use my own IAuthenticationSchemeProvider or register their own one. This scenario should be supported OTB.

@HaoK
Copy link
Member

HaoK commented Jul 3, 2017

Can you describe exactly what your ASOS is doing (why does Authenticate throw?)

@kevinchalet
Copy link
Contributor Author

why does Authenticate throw?

Well, it's not just for AuthenticateAsync(): ASOS throws an InvalidOperationException if you try to use AuthenticateAsync()/ChallengeAsync()/ForbidAsync()/SignInAsync()/SignOutAsync() outside an OpenID Connect request, which helps prevent users from doing illegal things like returning a authorization/token response from an arbitrary/non-OpenID Connect endpoint (if we didn't do that, it would represent a security risk, as the request wouldn't be validated).

The InvalidOperationException is here to tell the users that they are doing something totally wrong. See https://stackoverflow.com/questions/42048770/asp-net-core-openiddict-throws-an-openid-connect-response-cannot-be-returned-fr for a concrete case where an InvalidOperationException can be seen (in this case, it was caused by an inconsistency in how ASP.NET Core compares paths).

Basically, ASOS is a specialized handler that shouldn't be used as an active/automatic handler.

In both ASOS for OWIN and ASP.NET Core 1.0, we have checks to prevent that (Options.AuthenticationMode == AuthenticationMode.Active/Options.AutomaticAuthenticate || Options.AutomaticChallenge).

I'll add something similar in 2.0, but I also need a way to tell AuthenticationSchemeProvider not to use ASOS' handler for the default schemes. The same approach could be used by the built-in cookies/JWT/OIDC middleware to define when they can be safely used as default handlers.

@HaoK
Copy link
Member

HaoK commented Jul 3, 2017

I'm fine with a new global option that is AuthenticationOptions.UseSingleHandlerAsDefault which defaults to true, then you could just turn this off and if you don't specify any defaults, they will always be null and have full control. We could also add another one to disable all the fallbacks as well, UseHandlerDefaultFallbacks

@kevinchalet
Copy link
Contributor Author

kevinchalet commented Jul 3, 2017

It would work. But the downside is that users would have to manually set this flag to false. The per handler approach wouldn't require any particular configuration to make this scenario work, as the flags would be set by the handler writer.

Any particular reason you don't like this approach?

@HaoK
Copy link
Member

HaoK commented Jul 3, 2017

Its not up to the handler writer to disable this or not, these are mostly meant to configure the parameterless overloads. Its up to the app to decide what's the default handler.

The only situation where the default is required, is with the AuthenticationMiddleware, so maybe that needs a new explicit option as well which fallsback to DefaultAuthenticateScheme, something like our old favorite word AutomaticAuthenticateScheme

@kevinchalet
Copy link
Contributor Author

Its not up to the handler writer to disable this or not, these are mostly meant to configure the parameterless overloads. Its up to the app to decide what's the default handler.

That's not what the flags would do with my approach. Maybe I wasn't clear enough, but the flags would only tell the scheme provider whether the handler CAN be used as a default scheme, so that handlers that declare they can't be used as default schemes wouldn't be automatically picked by the scheme provider (e.g the JWT middleware can't be used for sign-in/sign-out). The most accurate name for these flags should be something like CanBeUsedAsDefaultXYZScheme.

@HaoK
Copy link
Member

HaoK commented Jul 3, 2017

The IAuthenticationSchemeProvider is responsible for resolving the defaults, if this becomes a very common ask, we can consider adding support for it in the default implementation, but it doesn't seem worth spreading this concept into the schemes themselves.

Your users wouldn't have to set this flag, you would do it for them via AddASOS() or the equivalent, you are always adding your handler so there's no situation where it would be useful for them anyways right?

@kevinchalet
Copy link
Contributor Author

Your users wouldn't have to set this flag, you would do it for them via AddASOS() or the equivalent, you are always adding your handler so there's no situation where it would be useful for them anyways right?

I'm never super comfortable with changing global settings in my own libs, but if that's the only option... 😅

One advantage with the other option is that the selection logic would be a bit smarter if one registered ASOS + another middleware (like the JWT bearer middleware). In this case, AuthenticationSchemeProvider would simply ignore ASOS - thanks to the flag(s) - and would use JWT as the default handler (even if there are 2 handlers registered).

@kevinchalet
Copy link
Contributor Author

For those interested, this work item is no longer necessary as the fallback logic was removed in aspnet/HttpAbstractions#891.

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

3 participants