-
Notifications
You must be signed in to change notification settings - Fork 464
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
Switch to Microsoft.IdentityModel.JsonWebTokens package #7269
Conversation
return handler.CreateToken(new SecurityTokenDescriptor | ||
{ | ||
IssuedAt = _clock.UtcNow.UtcDateTime, | ||
SigningCredentials = new(new SymmetricSecurityKey(_secret), SecurityAlgorithms.HmacSha256) |
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.
Should we cache SymmetricSecurityKey
instance?
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 tried not to make any other changes but replace the API as I don't know much about Kute. But the suggested change is small enough, so applied in the commit below.
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 should be no need for caching since we're already caching the JWT generation. By default, this code will be executed once per minute.
} | ||
|
||
private string GenerateAuthToken() | ||
{ | ||
var signingKey = new SymmetricSecurityKey(_secret); | ||
var credentials = new SigningCredentials(signingKey, SecurityAlgorithms.HmacSha256); | ||
var claims = new[] { new Claim(JwtRegisteredClaimNames.Iat, _clock.UtcNow.ToUnixTimeSeconds().ToString(), ClaimValueTypes.Integer64) }; |
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.
It's failing to compile:
MSBuild version 17.8.5+b5265ef37 for .NET
Determining projects to restore...
Restored /root/nethermind/tools/Nethermind.Tools.Kute/Nethermind.Tools.Kute.csproj (in 3.61 sec).
/root/nethermind/tools/Nethermind.Tools.Kute/JsonRpcMethodFilter/ComposedJsonRpcMethodFilter.cs(18,18): error CS1061: 'IEnumerable<IJsonRpcMethodFilter>' does not contain a definition for 'IsNullOrEmpty' and no accessible extension method 'IsNullOrEmpty' accepting a first argument of type 'IEnumerable<IJsonRpcMethodFilter>' could be found (are you missing a using directive or an assembly reference?) [/root/nethermind/tools/Nethermind.Tools.Kute/Nethermind.Tools.Kute.csproj]
Build FAILED.
/root/nethermind/tools/Nethermind.Tools.Kute/JsonRpcMethodFilter/ComposedJsonRpcMethodFilter.cs(18,18): error CS1061: 'IEnumerable<IJsonRpcMethodFilter>' does not contain a definition for 'IsNullOrEmpty' and no accessible extension method 'IsNullOrEmpty' accepting a first argument of type 'IEnumerable<IJsonRpcMethodFilter>' could be found (are you missing a using directive or an assembly reference?) [/root/nethermind/tools/Nethermind.Tools.Kute/Nethermind.Tools.Kute.csproj]
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.
@emlautarom1 The updated code has been tested to ensure it generates the same token as it was with the replaced package.
Changes
Replaced the
System.IdentityModel.Tokens.Jwt
package withMicrosoft.IdentityModel.JsonWebTokens
as recommended by Microsoft:For the Nethermind client, the replaced package is no longer needed after #7177.
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?