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

Make it harder (or impossible) to have more than one auth provider have "automatic" challenges #1054

Closed
Eilon opened this issue Nov 29, 2016 · 15 comments

Comments

@Eilon
Copy link
Member

Eilon commented Nov 29, 2016

From discussion with @HaoK @Tratcher @blowdart and others, we suggest either:

  • Try to detect this scenario at runtime and alert the developer that this is an unsupported scenario (having more than one “automatic” challenge registered).
  • Or change how automatic challenges are registered so that only one auth provider per app pipeline can be automatic. For example, instead of each provider having its own bool setting, you would instead have one "global" setting (i.e. per-IAppBuilder) where you specify which auth protocol is the automatic one (and all others are not automatic).
@Tratcher
Copy link
Member

Design details:

  • There is a small subset of auth providers that can collaborate in automatic challenge mode because their challenges can stack without conflicting. E.g. Basic, Bearer, and Windows auth.
  • Whatever we do should take into account Windows auth as provided by IISIntegration and WebListener. They don't directly derive from the Auth base classes in this repo, only the raw HttpAbstraction.Auth infrastructure. IISIntegration auth is enabled by default but already has a mitigation for this built in. WebListener auth is not enabled by default.

@HaoK
Copy link
Member

HaoK commented Nov 30, 2016

We could try moving from the AutomaticChallenge bool on each options, to a single AutomaticChallengeScheme[s?] on SharedAuthenticationOptions, which would effectively make it impossible to mave multiple defaults by accident. We could even allow this to be an ordered list by priority to support firing off multiple challenges (Basic/Bearer/Windows) where it would stop once any 'succeeded'

The downside of this would be no longer being able to default any auth to true, so all apps would need to explicitly configure this new setting in Startup.cs (but we could hide this a bit in the templates at least by having Identity configure this inside of AddIdentity()

@Tratcher
Copy link
Member

The most common scenario where people run into this is when their apps have both interactive and API controllers. Right now we tell people they should specify Bearer on all of their API controllers, but what if MVC could allow for two separate default policies, one for each kind of controller? Then you just put Authorize everywhere and not worry about the two crossing.

@HaoK
Copy link
Member

HaoK commented Nov 30, 2016

This is already the guidance I thought, when mixing interactive and API, you have a Bearer policy for the API controllers, and a Cookie policy for the interactive, Authorize("Bearer") on one with Authorize("Cookie") on the other.

@Tratcher
Copy link
Member

Tratcher commented Nov 30, 2016

I meant that we could make [Authorize] smart enough to pick a policy based on the controller type so you didn't have to specify it everywhere, only in one central location.

Edit: Like splitting AuthorizationOptions.DefaultPolicy into DefaultApiPolicy and DefaultInteractivePolicy

@leastprivilege
Copy link
Contributor

Yes please add more "automatic magic" - didn't that bring us here in the first place?

@Tratcher
Copy link
Member

@Eilon does the combine WebApi/MVC still make a clear distinction between API controllers and interactive controllers?

@leastprivilege
Copy link
Contributor

no.

@HaoK
Copy link
Member

HaoK commented Nov 30, 2016

I don't think it makes sense for Authorization to have any notion of interactive/API, once you have something like two 'default' policies, that doesn't seem 'default' at all. Name them if you want specific behavior i.e. [Authorize] + [Authorize("Interactive")]

@Tratcher
Copy link
Member

That's our current guidance and it clearly isn't working for people.

@HaoK
Copy link
Member

HaoK commented Nov 30, 2016

That's because our defaults are 'bad' and make it easy to do the wrong thing. When everything used to be false by default, it was much harder to get into this state. I believe for templates, we flipped JwtBearer's AutomaticChallenge = true, so now its very easy to hit, just by using Identity and JwtBearer together with their defaults.

Hence they wouldn't even look for guidance, it would just 'work'. I think this stuff is complicated enough that we should just require explicit configuration for any scenarios when using bearer + cookies

@blowdart
Copy link
Member

blowdart commented Dec 7, 2016

#1061 (oh look a PM type spec)

@HaoK
Copy link
Member

HaoK commented Dec 7, 2016

Maybe its because I am bleary eyed with 4 hours of sleep after failing to stack a donkey at a poker table until 6 am...but looks like you just filed a dupe to me :)

@blowdart
Copy link
Member

blowdart commented Dec 7, 2016

Because @davidfowl was so determined a PM should do it :p

@blowdart blowdart modified the milestones: 2.0.0, 1.2.0 Dec 13, 2016
@Eilon
Copy link
Member Author

Eilon commented Feb 2, 2017

Closing as dup of #1061 even though I OPENED THIS BUG FIRST!!!!!!!!!!!!!!!

@Eilon Eilon closed this as completed Feb 2, 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