Closed kevinchalet closed 5 years ago
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?
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 :smile:
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).
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...
denied
is already handled by the Twitter handler.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.
@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.
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?
I took a look at how the different Security components handle URIs and woooo, things are insanely inconsistent:
AuthenticationProperties.RedirectUri
as a relative URL.AuthenticationProperties.RedirectUri
as an absolute URL.Response.Redirect()
are absolute using the BuildRedirectUri
helper.AuthenticationProperties.RedirectUri
.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 /
).Any guess what the WsFed variant of this would look like for https://github.com/aspnet/Security/issues/1891?
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.
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
).
@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.
The design looks good. Go ahead and finish the tests.
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.
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.
@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.
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.
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? :trollface:
@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!
@PinpointTownes - thank you!
My pleasure :smile:
Fixes https://github.com/aspnet/Security/issues/710 (see also https://github.com/aspnet/Security/pull/1582#issuecomment-355060722 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) anddenied
(Twitter) errors.Open questions:
RedirectUri
attached to theAuthenticationProperties
(when present) up to the access denied endpoint?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 :smile:
/cc @Eilon @Tratcher