Add AccessDeniedPath support to the OIDC/OAuth2/Twitter providers #1887

kevinchalet · 2018-10-22T15:27:09Z

Fixes #710 (see also #1582 (comment) for more information).

This PR introduces native - but opt-in, to avoid a behavioral breaking change - support for automatic redirection for access_denied (OIDC/OAuth2) and denied (Twitter) errors.

Open questions:

Should we flow the RedirectUri attached to the AuthenticationProperties (when present) up to the access denied endpoint?
Do we want a new event allowing to control how the redirection is applied?

Note: I didn't add a test for OIDC because there's no existing test for the callback part (which is crazy, but hem...). Looks like I'm also going to contribute to the technical debt 😄

/cc @Eilon @Tratcher

Eilon · 2018-10-22T21:41:35Z

Looks legit to me, assuming that this is more than just a Twitter thing, and actually a proper OIDC thing. @PinpointTownes - can you confirm that?

@Tratcher - can you take a look and we can discuss further on Thursday if needed?

Tratcher

It's a good start but has some of the same gaps we've wrestled with in prior designs.

It loses the AuthProperties context so the app can't flow data across the login attempt.
This is almost the same functionality as the ExceptionHandler middleware except it's tailored to one specific error and a lot less flexible in how it's handled.
Events would be useful. OIDC has MessageReceived where you could already pre-empt this, OAuth and Twitter don't. Implementing the whole thing as a distinct event would be an alternative to the new property and might help with the AuthProperties and flexibility issues.
We should talk about what support for this looks like in the Identity and OIDC templates. You'd want to populate options and land on a page that said "We're sorry, we really need that information to continue, click here to try again?"

Tratcher · 2018-10-22T22:44:23Z

samples/OpenIdConnectSample/Startup.cs

@@ -126,6 +127,17 @@ await context.SignOutAsync(OpenIdConnectDefaults.AuthenticationScheme, new Authe
                    return;
                }

+                if (context.Request.Path.Equals("/access-denied-from-remote"))
+                {
+                    await context.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);


What's the signout for? You never finished signing in.

Bad copy/paste, I'll remove it 😅

Tratcher · 2018-10-22T22:47:06Z

src/Microsoft.AspNetCore.Authentication.OAuth/OAuthHandler.cs

+                // access_denied errors are handled differently if AccessDeniedPath was populated.
+                if (Options.AccessDeniedPath.HasValue && StringValues.Equals(error, "access_denied"))
+                {
+                    Response.Redirect(BuildRedirectUri(Options.AccessDeniedPath));


This drops the AuthProperties so the app can't correlate which login attempt this was.

Does this need an event?

Yep, these were the 2 open questions:

Should we flow the RedirectUri attached to the AuthenticationProperties (when present) up to the access denied endpoint?

Do we want a new event allowing to control how the redirection is applied?

src/Microsoft.AspNetCore.Authentication.OAuth/OAuthHandler.cs

Tratcher · 2018-10-22T22:51:37Z

src/Microsoft.AspNetCore.Authentication.OAuth/OAuthHandler.cs

+                // approve the authorization demand requested by the remote authorization server.
+                // Since it's a frequent scenario (that is not caused by incorrect configuration),
+                // access_denied errors are handled differently if AccessDeniedPath was populated.
+                if (Options.AccessDeniedPath.HasValue && StringValues.Equals(error, "access_denied"))


Elsewhere (CookieAuth) we've avoided changing behavior based on the presence or absence of a PathString property. However we never did provide an alternate pattern.

I'm fine changing that if you have something better in mind.

Tratcher · 2018-10-22T22:53:26Z

src/Microsoft.AspNetCore.Authentication.OAuth/OAuthOptions.cs

+        /// Gets or sets the optional path the user agent is redirected to if the user
+        /// doesn't approve the authorization demand requested by the remote server.
+        /// If this property is not populated, an exception is thrown if an access_denied
+        /// response is returned by the remote authorization server to the callback endpoint.


The property is not set by default.

src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs

Tratcher · 2018-10-22T22:56:07Z

src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs

+                    // Since it's a frequent scenario (that is not caused by incorrect configuration),
+                    // access_denied errors are handled differently if AccessDeniedPath was populated.
+                    if (Options.AccessDeniedPath.HasValue &&
+                        string.Equals(authorizationResponse.Error, "access_denied", StringComparison.Ordinal))


Why ordinal for OIDC and not OAuth?

Because OIDC uses string.Equals while OAuth uses StringValues.Equals, that doesn't offer an overload accepting an explicit comparison type. I'm fine changing that if you don't like it.

kevinchalet · 2018-10-23T00:12:44Z

It loses the AuthProperties context so the app can't flow data across the login attempt.

We could certainly flow the state, but in practice, the access denied path would likely point either to the home page or to the login page (so that the user can start a new authentication flow), not really an endpoint controlled by the OIDC/social handlers. So if we flowed the state, developers would have to instantiate the properties reader + the data protection stuff to deserialize it and get back the AuthenticationProperties. Not exactly a trivial thing to do.

The most interesting data is probably the return URL, that we could flow like what the cookies handler does.

This is almost the same functionality as the ExceptionHandler middleware except it's tailored to one specific error and a lot less flexible in how it's handled.

You've once told me that exceptions were not for flow control: it seems gross to throw (and log) an exception every time a user doesn't consent to the authorization demand 😄

The fact the handlers throws an Exception and not something super-specific indicating it's an access denied "error" makes using the exception handler a PITA if you want simply want to redirect the user agent to the login page.

Events would be useful. OIDC has MessageReceived where you could already pre-empt this, OAuth and Twitter don't. Implementing the whole thing as a distinct event would be an alternative to the new property and might help with the AuthProperties and flexibility issues.

The existing alternative is to use the RemoteFailure event to override the default error handling logic. I've shared a snippet doing that multiple times here and elsewhere: the fact you have to repeat that in every project for every provider is a real PITA (hence this PR).

If you only want something generic, we already have that in place. My question was more about something very specific (typically an AccessDenied/OnAccessDenied event).

We should talk about what support for this looks like in the Identity and OIDC templates. You'd want to populate options and land on a page that said "We're sorry, we really need that information to continue, click here to try again?"

Yep, pretty much what I had in mind. Flowing the returnUrl in the query string would be a good start to make restarting the authentication process easier. We could also flow the provider scheme, but I'm not so sure it's a safe approach (except if we only use it as a hint, maybe).

…utAsync()

kevinchalet · 2018-10-23T00:25:16Z

Looks legit to me, assuming that this is more than just a Twitter thing, and actually a proper OIDC thing. @PinpointTownes - can you confirm that?

access_denied is a completely standard OAuth 2.0/OIDC thing and supported by most servers (at least, the ones that don't redirect users to their own site after rejecting the authorization demand).

denied is Twitter-specific and AFAIK, there's no equivalent in the OAuth 1.0 specification. It's not really an issue because...

It's only used in the Twitter package, which is... specific to Twitter.
denied is already handled by the Twitter handler.

Tratcher · 2018-10-23T23:11:47Z

Events would be useful. OIDC has MessageReceived where you could already pre-empt this, OAuth and Twitter don't. Implementing the whole thing as a distinct event would be an alternative to the new property and might help with the AuthProperties and flexibility issues.

The existing alternative is to use the RemoteFailure event to override the default error handling logic. I've shared a snippet doing that multiple times here and elsewhere: the fact you have to repeat that in every project for every provider is a real PITA (hence this PR).

If you only want something generic, we already have that in place. My question was more about something very specific (typically an AccessDenied/OnAccessDenied event).

I was proposing a new dedicated event for the access denied scenario and the ability to intercept the response prior to an exception being thrown.

… handling logic to RemoteAuthenticationHandler

kevinchalet · 2018-10-24T14:25:58Z

@Tratcher I added the AccessDenied event and moved the corresponding logic to RemoteAuthenticationHandler. The OAuth2/OIDC/Twitter provider now simply call AuthenticateResult.Fail() with a special AccessDeniedException error that is detected by the RAT, which first calls the AccessDenied event.

If the event is not marked as skipped or handled, then the RAT determines whether AccessDeniedPath was specified. If it was, then the user agent is redirected. Otherwise, the RemoteFailure event is invoked, which ensures we don't introduce a behavioral breaking change.

Let me know if you like this approach. If you like it, I'll add tests for the new events.

kevinchalet · 2018-10-24T14:26:54Z

Now, the remaining question is: what context information do we want to flow up to the access denied endpoint? Just the returnUrl (if specified)? The whole state?

kevinchalet · 2018-10-24T15:38:31Z

I took a look at how the different Security components handle URIs and woooo, things are insanely inconsistent:

The cookie handler stores the current URL (when the challenge is triggered) in AuthenticationProperties.RedirectUri as a relative URL.
The OIDC/OAuth2/Twitter/WS-Fed store the current URL in AuthenticationProperties.RedirectUri as an absolute URL.
The cookie handler ensures the login/logout URLs eventually passed to Response.Redirect() are absolute using the BuildRedirectUri helper.
The cookie handler doesn't do that conversion when redirecting the user agent back to the AuthenticationProperties.RedirectUri.
The base remote authentication handler doesn't have a preferred format and Response.Redirect() may be called with either an absolute URL (when AuthenticationProperties.RedirectUri is itself an absolute URL) or with a relative one (e.g when AuthenticationProperties.RedirectUri is null, as it defaults to /).

kevinchalet · 2018-10-24T15:42:29Z

src/Microsoft.AspNetCore.Authentication.OAuth/OAuthHandler.cs

@@ -194,7 +205,7 @@ protected override async Task HandleChallengeAsync(AuthenticationProperties prop
        {
            if (string.IsNullOrEmpty(properties.RedirectUri))
            {
-                properties.RedirectUri = CurrentUri;
+                properties.RedirectUri = OriginalPathBase + Request.Path + Request.QueryString;


@Tratcher stricto sensu, these changes are not required, but if we end up flowing the ReturnUrl up to the access denied endpoint, we'll likely want it to be captured as a relative URL so helpers like IUrlHelper.IsLocalUrl and RedirectToLocal() work correctly (they don't deal with absolute URLs, even if the domain matches the current one).

This should be OriginalPathBase + OriginalPath.

I copied it from the cookie handler: https://github.com/aspnet/Security/blob/master/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationHandler.cs#L425

Should I also update the cookie handler?

Sure. FYI: #1730

Tratcher · 2018-10-24T18:43:52Z

Any guess what the WsFed variant of this would look like for https://github.com/aspnet/Security/issues/1891?

Tratcher · 2018-10-24T18:45:57Z

src/Microsoft.AspNetCore.Authentication.OAuth/OAuthHandler.cs

@@ -194,7 +205,7 @@ protected override async Task HandleChallengeAsync(AuthenticationProperties prop
        {
            if (string.IsNullOrEmpty(properties.RedirectUri))
            {
-                properties.RedirectUri = CurrentUri;
+                properties.RedirectUri = OriginalPathBase + Request.Path + Request.QueryString;


This should be OriginalPathBase + OriginalPath.

Tratcher · 2018-10-24T18:50:25Z

src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs

+                    // Visit https://tools.ietf.org/html/rfc6749#section-4.1.2.1 for more information.
+                    if (string.Equals(authorizationResponse.Error, "access_denied", StringComparison.Ordinal))
+                    {
+                        return HandleRequestResult.Fail(new AccessDeniedException(


I'd rather invoke the AccessDenied event here than create a new AccessDeniedException type that's never actually thrown.

That's possible, but it means we'll have the same (duplicated) logic in all the handlers. It will also be a little weird, as RemoteAuthenticationEvents/RemoteAuthenticationOptions will have events/options for something that is actually implemented in subclasses.

It's unfortunate, but errors are represented by Exceptions in 2.0. I'd also have preferred a proper AuthentiationError, but I guess that's too late for that.

If you don't like having a new exception for that, I guess we could use Exception and store a boolean marker in Exception.Data.

Calling the event in a subclass is fine, OAuth CreatingTicket is a lot like that. You could share the code via a protected method if you like.

kevinchalet · 2018-10-24T19:05:05Z

Any guess what the WsFed variant of this would look like for #1891?

I'll have to re-read the WS-Fed specification as I don't remember if there's anything specific for access denied callbacks.

kevinchalet · 2018-10-29T14:54:20Z

I'll have to re-read the WS-Fed specification as I don't remember if there's anything specific for access denied callbacks.

I read it during the weekend (it's always a terrible experience!) and I didn't find any trace of such a thing (AFAICT, there's even no error callback mechanism).

Just to highlight the fact it's not specific to the built-in OIDC/OAuth2/Twitter handlers, I'd likely use this feature in the OpenID 2.0 aspnet-contrib handler (where access denied errors are signaled using openid.mode=cancel).

…r.HandleAccessDeniedErrorAsync()

kevinchalet · 2018-10-29T18:03:00Z

@Tratcher based on your suggestion, I added a protected virtual HandleAccessDeniedErrorAsync() helper in RemoteAuthenticationHandler.

Once the final design is approved, I'll add some tests for the new event.

kevinchalet · 2018-10-29T18:14:35Z

src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs

+                    // Visit https://tools.ietf.org/html/rfc6749#section-4.1.2.1 for more information.
+                    if (string.Equals(authorizationResponse.Error, "access_denied", StringComparison.Ordinal))
+                    {
+                        return await HandleAccessDeniedErrorAsync(properties);


@Tratcher pretty much like RemoteFailure, the fact AccessDenied is protocol-agnostic prevents us from flowing things like the OIDC authorization response, which may limit extensibility. Not sure it's really a problem, but let me know if you'd prefer having multiple (package-specific) AccessDenied events instead of a global one.

There's no other parameters in the message we'd be missing out on, right?

The most awkward bit seems to be having to downcast the Options if you wanted anything specific. Or could you flow the generic TOptions? I remember trying that and having lots of issues before.

There's no other parameters in the message we'd be missing out on, right?

We could update HandleAccessDeniedErrorAsync() to take a message string containing the error/error_description/error_uri if you think it could be useful. Other than these parameters, nope, nothing else.

I can see those being useful if properly populated. It seems like you'd want to flow a generic property bag to the event rather than hardcoded parameters. I guess you could pass them through AuthProperties or some HttpContext field if you really needed to.

The most awkward bit seems to be having to downcast the Options if you wanted anything specific. Or could you flow the generic TOptions? I remember trying that and having lots of issues before.

Yep. RemoteAuthenticationEvents is not generic so we can't easily flow TOptions. We could have a generic AccessDeniedContext<TOptions> class derived from AccessDeniedContext but it wouldn't help much as it would still be exposed as AccessDeniedContext by the events class.

It seems like you'd want to flow a generic property bag to the event rather than hardcoded parameters. I guess you could pass them through AuthProperties or some HttpContext field if you really needed to.

We can certainly do that, but I'm a bit reluctant as it would be fairly inconsistent (we don't do that anywhere else).

Tratcher · 2018-10-29T18:29:35Z

The design looks good. Go ahead and finish the tests.

…ter from the AccessDenied event

kevinchalet · 2018-10-29T19:27:16Z

I just pushed a commit allowing to customize the access denied path/return URL/return URL parameter from the AccessDenied event without having to manually do the Response.Redirect dance yourself.

kevinchalet · 2018-10-29T19:28:52Z

This is the same as setting Properties.RedirectUri, right?

Yup. It just has a better name and you'll be able to set manually it if even if properties is null.

kevinchalet · 2018-10-29T19:59:50Z

@Tratcher assuming this PR won't be part of 2.2 (it's too late for that, right?), we should consider setting RemoteAuthenticationOptions.AccessDeniedPath to /Account/Login by default to match the default login page path. This way, users would be automatically redirected to the login page OOTB without having to configure anything.

Tratcher · 2018-10-29T21:38:20Z

Yeah, this is coming in too late for 2.2. If there were to be a default I'd rather it go to the error page, or better yet a new login error page. That's something we'll need to work out with @blowdart and the templates.

kevinchalet · 2018-11-01T21:16:47Z

Yeah, this is coming in too late for 2.2. If there were to be a default I'd rather it go to the error page, or better yet a new login error page.

Not a huge fan of the default error page option as it gives absolutely no indication of what's wrong with the request, which is precisely what this PR tries to avoid. A separate "try again" page is probably a reasonable tradeoff.

That's something we'll need to work out with @blowdart and the templates.

@blowdart care to chime in or you're too busy posting random stuff on Twitter?

Eilon · 2018-11-15T23:02:55Z

@Tratcher - can you go ahead and merge this?

Also please log a bug in the Docs repo to suggest adding/updating docs regarding this new feature.

@PinpointTownes - thank you!

kevinchalet · 2018-11-16T00:04:12Z

@PinpointTownes - thank you!

My pleasure 😄

Add AccessDeniedPath support to the OIDC/OAuth2/Twitter providers

c1b19d6

Eilon requested a review from Tratcher October 22, 2018 21:40

Tratcher reviewed Oct 22, 2018

View reviewed changes

Update the code documentation and remove an unnecessary call to SignO…

3db120d

…utAsync()

Introduce a new AccessDenied event and move most of the access denied…

e03fc3e

… handling logic to RemoteAuthenticationHandler

Add ReturnUrlParameter support to RemoteAuthenticationHandler

0afe7f1

kevinchalet commented Oct 24, 2018

View reviewed changes

kevinchalet mentioned this pull request Oct 24, 2018

Proposal: Add AccessDeniedPath for OpenIdConnect middleware #336

Closed

Tratcher reviewed Oct 24, 2018

View reviewed changes

kevinchalet added 2 commits October 29, 2018 18:56

Remove AccessDeniedException and introduce RemoteAuthenticationHandle…

f2a9f0b

…r.HandleAccessDeniedErrorAsync()

Use OriginalPath instead of Request.Path

fd5bc2c

Update obsolete code comments

b5bf034

kevinchalet commented Oct 29, 2018

View reviewed changes

kevinchalet added 2 commits October 29, 2018 20:07

Add unit tests for the new AccessDenied event

8c6a456

Allow customizing the access denied path/return URL/return URL parame…

5fcbb1f

…ter from the AccessDenied event

Tratcher mentioned this pull request Nov 2, 2018

Update static files sample to use AuthorizationMiddleware aspnet/AuthSamples#65

Closed

Eilon assigned Tratcher Nov 15, 2018

Tratcher merged commit 7e14b05 into aspnet:master Nov 15, 2018

Tratcher added this to the 3.0.0-preview1 milestone Nov 15, 2018

Tratcher mentioned this pull request Nov 15, 2018

Docs for new AccessDeniedPath feature dotnet/AspNetCore.Docs#9603

Closed

kevinchalet deleted the access_denied branch November 16, 2018 00:03

HaoK mentioned this pull request Dec 4, 2018

3.0 ForgotPassword regression in AzureB2C templates dotnet/aspnetcore#4384

Closed

Tratcher mentioned this pull request Dec 7, 2018

Fix up how OIDC errors flow dotnet/aspnetcore#4520

Merged

kevinchalet mentioned this pull request Oct 26, 2023

Question: How to handle OpenIdConnectProtocolException when user denies resource access openiddict/openiddict-core#334

Closed

Add AccessDeniedPath support to the OIDC/OAuth2/Twitter providers #1887

Add AccessDeniedPath support to the OIDC/OAuth2/Twitter providers #1887

Conversation

kevinchalet commented Oct 22, 2018 • edited Loading

Eilon commented Oct 22, 2018

Tratcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinchalet commented Oct 23, 2018 • edited Loading

kevinchalet commented Oct 23, 2018 • edited Loading

Tratcher commented Oct 23, 2018

kevinchalet commented Oct 24, 2018

kevinchalet commented Oct 24, 2018

kevinchalet commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Oct 24, 2018

Choose a reason for hiding this comment

Tratcher Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinchalet commented Oct 24, 2018 • edited Loading

kevinchalet commented Oct 29, 2018 • edited Loading

kevinchalet commented Oct 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Oct 29, 2018

kevinchalet commented Oct 29, 2018

kevinchalet commented Oct 29, 2018 • edited Loading

kevinchalet commented Oct 29, 2018

Tratcher commented Oct 29, 2018

kevinchalet commented Nov 1, 2018

Eilon commented Nov 15, 2018

kevinchalet commented Nov 16, 2018

kevinchalet commented Oct 22, 2018 •

edited

Loading

kevinchalet commented Oct 23, 2018 •

edited

Loading

kevinchalet commented Oct 23, 2018 •

edited

Loading

Tratcher Oct 24, 2018 •

edited

Loading

kevinchalet commented Oct 24, 2018 •

edited

Loading

kevinchalet commented Oct 29, 2018 •

edited

Loading

kevinchalet commented Oct 29, 2018 •

edited

Loading