Introduce events support #18
Conversation
MonkeyJamboree
force-pushed
the
Features/Events
branch
from
6e9a9fe to
3d7d436
Compare
|
Well, I owe you one then. |
| using Newtonsoft.Json.Linq; | ||
|
|
||
| namespace AspNet.Security.OAuth.Introspection { | ||
| public class IntrospectTokenContext : BaseIntrospectionContext { |
There was a problem hiding this comment.
IntrospectTokenContext -> IntrospectionTokenContext?
There was a problem hiding this comment.
My bad, just noticed that this is meant for an action, as if please introspect the token 👍
There was a problem hiding this comment.
Yeah. Do you think the naming should be clearer? Something like InitiateIntrospectionRequestContext or similar?
There was a problem hiding this comment.
BeSoKindAndIntrospectThisTokenContext.cs would be a polite and clear way of saying what this has to do.. 😄
Hmm... IntrospectIncomingToken, sth like that?..
When you understand the intention of this event, the name looks clear. But when you see it for the first time - I got confused, so can other people.
|
I'll work on the build issues and push some fixed code for that shortly. @DovydasNavickas thanks for all the feedback and spell-checking! 👍 |
MonkeyJamboree
force-pushed
the
Features/Events
branch
from
3ff2c22 to
7a7fbab
Compare
|
Should be able to pull this in now unless there are more complaints about things 😉 |
|
Nice: |
| using Microsoft.AspNetCore.Http; | ||
|
|
||
| namespace AspNet.Security.OAuth.Introspection { | ||
| public class AccessTokenReceivedContext : BaseIntrospectionContext { |
There was a problem hiding this comment.
Please consider adding some XML documentation love for all the public stuff 😄
There was a problem hiding this comment.
Will do when I have another spare minute or three.
|
Minor a few remarks, it looks nice (and it builds!) 👏 |
| protected override async Task<AuthenticateResult> HandleAuthenticateAsync() | ||
| { | ||
| // Give the application an opportunity to parse the token from a different location, adjust, or reject token | ||
| var accessTokenReceivedContext = new AccessTokenReceivedContext(Context, Options); |
There was a problem hiding this comment.
Should we try to parse the Authorization header before invoking the event to let the dev' adjust it or are fine with the current order?
There was a problem hiding this comment.
If so, we can't return the multiple failure messages while doing the parsing, and if they're not using the header at all it makes sense (at least to me) to skip that processing entirely.
| /// <summary> | ||
| /// Invoked when audiences are to be validated for a message. | ||
| /// </summary> | ||
| Task ValidateAudience(ValidateAudienceContext context); |
There was a problem hiding this comment.
Don't forget to kill this event 👏
There was a problem hiding this comment.
Yeah, I was focusing on one middleware at a time and ran into the issue I had. It'll be gone 👍
MonkeyJamboree
force-pushed
the
Features/Events
branch
2 times, most recently
from
181b7af to
7fb1214
Compare
MonkeyJamboree
force-pushed
the
Features/Events
branch
5 times, most recently
from
041aaf1 to
b8e1c47
Compare
|
Merged: a1951c7 @MonkeyJamboree @DovydasNavickas thanks for your contribution guys! 👏 |
I needed the functionality so I went ahead and finished up the work on #2. There are a couple missing unit tests for the introspection events, but should work knock on wood.
Thanks for the initial work @DovydasNavickas! I cherry-picked what you'd already done and finished the work up for all of the projects in this repo.