Skip to content
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

Consider adding more base methods to TokenHandler #1259

Open
kevinchalet opened this issue Sep 30, 2019 · 5 comments · May be fixed by #2630
Open

Consider adding more base methods to TokenHandler #1259

kevinchalet opened this issue Sep 30, 2019 · 5 comments · May be fixed by #2630
Assignees
Labels
Customer reported Indicates issue was opened by customer Enhancement The issue is a new feature IdentityModel8x Future breaking issues/features for IdentityModel 8x

Comments

@kevinchalet
Copy link
Contributor

TokenHandler seems pretty useless as a base class, as it doesn't expose any useful method for token generation or validation.

Please consider adding abstract or virtual properties/methods for at least the following APIs:

  • Type TokenType { get; }
  • bool CanReadToken(string token)
  • string CreateToken(SecurityTokenDescriptor tokenDescriptor)
  • TokenValidationResult ValidateToken(string token, TokenValidationParameters validationParameters)
@brentschmaltz
Copy link
Member

@PinpointTownes we agree and want to do this work in our 6.x so we can easily have asp.net 3.x take a dependency.

@brentschmaltz brentschmaltz added Enhancement The issue is a new feature P1 More important, prioritize highly Customer reported Indicates issue was opened by customer labels Oct 1, 2019
@brentschmaltz brentschmaltz added this to the 6.x milestone Oct 1, 2019
@brentschmaltz brentschmaltz modified the milestones: v6 Backlog, 6.7.2 Jul 13, 2020
@brentschmaltz brentschmaltz self-assigned this Aug 13, 2020
@brentschmaltz
Copy link
Member

@kevinchalet CreateToken(SecurityTokenDescriptor ...) is defined to return by SecurityTokenHandler to return SecurityToken. I agree this method would be helpful to return a string.
Perhaps a different name.

@mafurman mafurman modified the milestones: 6.8.1, 6.8.2 Jan 13, 2021
@jennyf19 jennyf19 removed this from the 6.9.1 milestone Sep 19, 2023
@jennyf19 jennyf19 added IdentityModel8x Future breaking issues/features for IdentityModel 8x and removed P1 More important, prioritize highly labels Sep 19, 2023
@jennyf19
Copy link
Collaborator

@kevinchalet sorry we missed this one for Wilson7. Will keep it on the radar for Wilson8 (no ETA)

@brentschmaltz
Copy link
Member

Type TokenType { get; } - Should this be on JsonWebToken?

bool CanReadToken(string token) - OK
string CreateToken(SecurityTokenDescriptor tokenDescriptor) - OK

TokenValidationResult ValidateToken(string token, TokenValidationParameters validationParameters) - we already have ValidateTokenAsync, so I don't think we need this.

@westin-m westin-m linked a pull request Jun 12, 2024 that will close this issue
@kevinchalet
Copy link
Contributor Author

Type TokenType { get; } - Should this be on JsonWebToken?

The most logical approach is to have a virtual (or abstract, but it would be breaking) property in the base class and override it in each implementation so that the correct type is returned.

bool CanReadToken(string token) - OK
string CreateToken(SecurityTokenDescriptor tokenDescriptor) - OK

Since there are now async overloads for the validation part, should these new methods be async/return ValueTask<T>? Could be overkill for the first one, but quite useful for the second one if you ever add things like async encryption or signature providers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Customer reported Indicates issue was opened by customer Enhancement The issue is a new feature IdentityModel8x Future breaking issues/features for IdentityModel 8x
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants