This repository has been archived by the owner on Dec 13, 2018. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add AccessDeniedPath support to the OIDC/OAuth2/Twitter providers #1887
Add AccessDeniedPath support to the OIDC/OAuth2/Twitter providers #1887
Changes from 4 commits
c1b19d6
3db120d
e03fc3e
0afe7f1
f2a9f0b
fd5bc2c
b5bf034
8c6a456
5fcbb1f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 likeIUrlHelper.IsLocalUrl
andRedirectToLocal()
work correctly (they don't deal with absolute URLs, even if the domain matches the current one).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be OriginalPathBase + OriginalPath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. FYI: #1730
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather invoke the AccessDenied event here than create a new AccessDeniedException type that's never actually thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Exception
s in 2.0. I'd also have preferred a properAuthentiationError
, but I guess that's too late for that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't like having a new exception for that, I guess we could use
Exception
and store a boolean marker inException.Data
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.