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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update JwtBearer and WsFederation to use updated IdentityModel #48966
Update JwtBearer and WsFederation to use updated IdentityModel #48966
Changes from 8 commits
507d241
47ffe32
4153b06
7fa2f96
aca45dd
0402406
df64e72
e63b5ce
4501b82
583728e
65595cb
f22eba9
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.
@kevinchalet i am thinking that creating the interface ITokenValidator that has a couple of methods instead of the abstract class TokenHandler, this would make it easier for users who currently have an implementation of ISecurityTokenValidator.
Thoughts?
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.
IM already has
TokenHandler
,SecurityTokenHandler
andISecurityTokenValidator
, which makes using these abstractions already quite painful. I'm not sure adding a 4th one will help 😄WIF only had
SecurityTokenHandler
and it was much clearer: unifying everything in the next Wilson major version would be more than welcome 😄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.
@kevinchalet OK let's stick with TokenHandler then as this was meant to be the replacement for SecurityTokenHandler.
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.
@kevinchalet we could implement ISecurityTokenValidator on JsonWebTokenHandler, but just because we can cast it to TokenHandler doesn't mean that users will not have a runtime break as the SecurityToken will change from JwtSecurityToken to JsonWebToken.
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, but that would mean an instance of
JsonWebTokenHandler
was added toSecurityTokenValidators
, which, if you keep making using the new JWT stack opt-in, requires a deliberate action from the developer. And in this case, getting aJsonWebToken
instead of aJwtSecurityToken
is a logical result.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'm hoping to make it opt-out.
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.
Addressed: 507d241
@Tratcher default is true, use TokenHandlers.
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 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 wonder if we should rethink these doc comments as they might not make full sense now that we've inverted the boolean to be
UseSecurityTokenValidators
.Maybe we should add why someone would want to use SecurityTokenValidators as well.
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.
+1 for making this the default.
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.
Addressed: 507d241
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.
/azp run
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 appears to have been reverted. Was that intentional?
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.
@eerhardt yes, upon reflection, this would be breaking for some users.
@Tratcher would like to see this 'true' by default, but we have time to change this decision if we choose.
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.
@brentschmaltz was the default set to true?