This repository was 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 all 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 pretty much like
RemoteFailure
, the factAccessDenied
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 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.
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 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.
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.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 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.
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.
Yep.
RemoteAuthenticationEvents
is not generic so we can't easily flowTOptions
. We could have a genericAccessDeniedContext<TOptions>
class derived fromAccessDeniedContext
but it wouldn't help much as it would still be exposed asAccessDeniedContext
by the events class.We can certainly do that, but I'm a bit reluctant as it would be fairly inconsistent (we don't do that anywhere else).