[Tratcher]

#1165 The remote auth components like Facebook or OIDC have never handled remote failures gracefully. E.g. if a user declines permissions to your app then an exception is thrown and the client gets a 500 error.

OAuth and OIDC define some standard error fields and we currently use these to generate a detailed exception message. This takes that one step further and generates a redirect to /home/error with those error fields as query parameters.

Sending this initial PR to see if this is a change we're willing to take for 2.1. If so then I'll add a few more tests and clean it up.

Here are some notes on the behavior of individual providers:

Google
- Doesn't even ask for consent? It only ask you to select an account.

Facebook
- https://www.facebook.com/settings?tab=applications
- GET /signin-facebook?
- error=access_denied&
- error_code=200&
- error_description=Permissions%20error&
- error_reason=user_denied&
- state=CfDJ8JQ7RNKCadBHk4_sI5UNfETy2xdPTigkm_jCBShuc9asO__-yHcEa_3dizjzX2PVb2IAPdODytGjkySQsU8HxmWRI3i3PO-9tXRQEocO8mkCz16pxAqdcdOdWpsfx2i9EMqMdpnCeTwmHjHvhlRqZ5IBqgTbbFQWhBHkhbILKzECS8lZ6DYJneLziBS-Fn10PrLe4w_EGLO1_dr5a5qp2MI HTTP/1.1

Twitter
- https://twitter.com/settings/applications
- GET /signin-twitter?denied=vBzGPQAAAAAAu4omAAABYKRNrBY HTTP/1.1

Microsoft
- https://account.live.com/consent/Manage
- GET /signin-microsoft?
- error=access_denied&
- error_description=The%20user%20has%20denied%20access%20to%20the%20scope%20requested%20by%20the%20client%20application.&
- state=CfDJ8JQ7RNKCadBHk4_sI5UNfESFOFwEQvoQNcB5iKIYymQ9rjkvRwSlJQIwMbFv59pAQEXo8Nrs1KSLKQUBaSQ7d1rPMgTZUnr8RGap2Ej5npXBDTHQtEgeBMJyzjOT2YTHvJqQGu0KLT0yHZFsI912UDqY8tsWEyXIp_9pdzxPj9mKWF1fXvI-5fG3vvElWSslNI_jInsFUIAtRPYspbGkTTc HTTP/1.1

Github
- No cancel button
- https://github.com/settings/applications

@PinpointTownes how well do the other providers you've seen map to this?

[HaoK]

Yeah this would look a lot nicer if we added first class properties to AuthenticationProperties like the other ones there

[HaoK]

Should there be a way to disable this redirect? Or what if there is no redirect url?

[Tratcher]

You can handle the event above to avoid the redirect.

[HaoK]

Fair enough, we still need to validate or ignore a null RemoteFailureRedirect though

[kevinchalet]

@PinpointTownes how well do the other providers you've seen map to this?

In theory, both the OAuth2/OIDC specs encourage redirecting the user agent back to the client application's redirect_uri when an error occurs as long as it's safe to do so (i.e the client_id/redirect_uri couple was valid).

In practice, there are 3 main schools of thought:

1. There are providers that redirect the UA back to the RP for all errors (except for invalid client_id/redirect_uri).
2. There are providers that redirect the UA only for a few errors like access_denied and directly render an error page for everything else (e.g invalid scope or unsupported response type).
3. There are providers that refuse to redirect the UA for security reasons and always render an error page. In this case, pressing the back button is the only way for the UA to get back to the RP.

It's hard to give precise numbers as every provider does its own sauce, but 2) and 3) are probably the most common ones.

There's nothing you can do if the provider decides to adopt the 3rd approach, but the other cases should be handled by what you've implemented as part of this PR.

[kevinchalet]

Putting on my @blowdart hat: "fix this damn XSS!" 😅

[kevinchalet]

The only thing I'm not super comfortable with is adding the error/description to the "remote error endpoint path" URL as it may encourage folks to create an error page displaying the error/description as-is. And we all know what happens when we do that... 😅

[Tratcher]

@HaoK Changed to use HandleRequestResult rather than AuthenticationProperties to flow data. Any better?

[HaoK]

Nit, does this need some kind of Uri/Path suffix to indicate that its the RedirectPath/Uri?

[Tratcher]

RemoteFailurePath?

[HaoK]

Yeah that works, matches the cookie naming

[kevinchalet]

Not sure why, but I think I personally preferred the "authentication properties" approach 😅

[Tratcher]

@PinpointTownes you may need to be more specific 😆

[Tratcher]

Hmm, this makes the dev scenario more difficult. UseDeveloperExceptionPage will no longer show you configuration errors, you'll be redirected to /Home/Error with no details instead.

[HaoK]

You can always put the info back in the exception message (when running in debug mode)? So that will show up in the error page?

[kevinchalet]

@PinpointTownes you may need to be more specific 😆

I dunno, maybe that's because we now have too many properties to represent errors in the results primitives (Failure/ErrorTitle/ErrorDescription). Maybe that would be worth introducing a new abstraction (e.g AuthenticationError) at some point.

[HaoK]

Yeah we were just using an Exception to encapsulate the Failure/Error, but yeah a custom AuthenticationError is where we are heading now

[Tratcher]

On further evaluation this feature is causing more harm than good. It's hampering both dev and test capabilities. Nor is it adding anything the users can't already do (except the Twitter check for denied).

The normal template code handles the basic scenarios better than this PR with both UseDeveloperExceptionPage and UseExceptionHandler:

            if (env.IsDevelopment())
            {
                app.UseDeveloperExceptionPage();
                app.UseBrowserLink();
                app.UseDatabaseErrorPage();
            }
            else
            {
                app.UseExceptionHandler("/Home/Error");
            }

            app.UseStaticFiles();

            app.UseAuthentication();

Users that want to customize auth errors can do so with the current event system, the same as they'd have to override the behavior added with this PR.

I'm going to cull this PR down to just fix the Twitter handler to check for remote denials.

[Tratcher]

Closing in favor of #1585

[kevinchalet]

Meh, relying on the exception handler gives a poor experience. In practice, you almost always want to redirect the user back to the login page you receive an access_denied error. I really think we should at least handle this case and offer an opt-in way to redirect the user.

Related topic: #710

[Tratcher]

I know the exception handler isn't a great experience, but it's a reasonably foolproof default. Anything smarter requires A) per-handler customization to detect the correct denial conditions, B) a well known route to redirect to which doesn't exist today in across the various templates, not to mention already created apps, and C) some way to flow meaningful content to that page.

We're going to back up and build something for the docs & samples and see where we can go from there.

[kevinchalet]

Honestly I don't care much about OIDC/OAuth2 errors caused by invalid configuration (e.g when an invalid scope is used) as it's essentially a server error, but access_denied is a truly special error and deserves a simpler option to redirect the user to a neutral page that doesn't imply he did something wrong, which is what the exception handler and the status page thingies are for.

An opt-in AccessDeniedPath seems reasonable to me.

[kevinchalet]

@Tratcher looks like a few guys are interested by my suggestion. Time to revive this PR?

[Tratcher]

Not yet, we'll revisit for 2.2.

[kevinchalet]

Alright (please move #710 to the 2.2 milestone so you don't forget 😄)