Consider reverting https://github.com/aspnet/Security/pull/982

[kevinchalet]

#930 (comment)

[JeanCollas]

Please consider, as it breaks some 3rd party security libraries.
It breaks the openiddict's workflow, by falling into error even before reaching the controller that should handle the authentication.

[kevinchalet]

/cc @Tratcher

[kevinchalet]

@sebastienros FYI, this "by design" issue will likely impact Orchard 2's OpenID module when moving the oauth2 branch to ASP.NET Core 1.1.0, as the JWT bearer middleware will now collide with the cookies middleware set up by Identity.

[HaoK]

@PinpointTownes your mileage might be better if you call it a breaking change, I had a bad feeling about that one :(

[Tratcher]

We've known for a long time now that having multiple active challenge middleware will cause conflicts, especially incompatible ones. The multiple auth types ambiguity can only be properly resolved by naming the desired type when issuing the challenge. I don't see how else the conflict can be deterministicly resolved in the short or long term.

[kevinchalet]

I don't see how else the conflict can be deterministicly resolved in the short or long term.

I've always thought that the whole point of returning a boolean from HandleUnauthorizedAsync was to allow a middleware to prevent the other middleware higher in the pipeline from applying their own challenge when unwinding the pipeline (that's why the bearer middleware returns false - because multiple schemes are allowed - while the cookies or the OIDC middleware return true).

Obviously, this doesn't work properly (specially since this design change), but I'm not sure to understand what's the difficult point here.

[Tratcher]

That bool prevented the downstream handlers from being called during that specific call to Challenge, not for the whole response. As your original bug reported (#930) this didn't affect automatic unwind challenges. They never observed the original Challenge so they didn't know that someone else handled the response already. We "fixed" the automatic unwind challenge issue by enabling all the middleware to observe the initial Challenge, which just highlighted more conflicts between multiple automatic auth challenges.

[kevinchalet]

That bool prevented the downstream handlers from being called during that specific call to Challenge, not for the whole response.

But technically, what prevents us from extending that concept to automatic challenges (applied via the OSH callback)?

IMHO, here's how it should ideally work:

• When Challenge is manually called:

  • Invoke the corresponding middleware when schemes are specified.
  • Invoke the automatic challenge middleware when no scheme is specified (= AutomaticScheme) until one of the middleware returns true.
  • Don't let the OSH callback invoke anything when unwinding the request pipeline.

• When Challenge is not manually called (i.e a simple 401 response is returned):

  • Invoke the automatic challenge middleware when no scheme is specified (= AutomaticScheme) until one of the middleware returns true to stop the processing.

[Eilon]

Hi @PinpointTownes , we discussed this at length with @Tratcher @blowdart @HaoK and some others and we have plans on how to mitigate this in the future, but we're not planning on changing this behavior in 1.1.

The mitigation plans are to do one of these:

• Try to detect this scenario at runtime and alert the developer that this is an unsupported scenario (having more than one “automatic” challenge registered).
• 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).

[Eilon]

Tracking the vNext proposals here: #1054