AzureAD / azure-activedirectory-identitymodel-extensions-for-dotnet

IdentityModel extensions for .Net
MIT License
1.04k stars 390 forks source link

Consider adding more base methods to TokenHandler #1259

Open kevinchalet opened 4 years ago

kevinchalet commented 4 years ago

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:

brentschmaltz commented 4 years ago

@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 commented 4 years ago

@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.

jennyf19 commented 11 months ago

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

brentschmaltz commented 2 months ago

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.

kevinchalet commented 2 months ago

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.