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
Hash jwks keys by kid #349
Conversation
return find_key(kid) | ||
end | ||
jwk = jwks[kid] | ||
jwk ||= reload && jwks[kid] |
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.
Useless assignment to variable - jwk
. Use ||
instead of ||=
.
key_n = decode_open_ssl_bn(jwk_n) | ||
key_e = decode_open_ssl_bn(jwk_e) | ||
|
||
if key.respond_to?(:set_key) |
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.
JWT::JWK::RSA#self.rsa_pkey manually dispatches method call
SourceLevel has finished reviewing this Pull Request and has found:
|
This fixes an issue where JSON.parse results of an actual .jwks file fail to import into the key finder because they have string keys. Errors also improved in a way that helps debugging issues with JWKS loading.
Assuming here that lookup by kid happens much more often than reloading, so there is a slight benefit to not searching the array every time. Note that I only did this because code climate was complaining. This could be analyzed separately from my big fix.
As discussed in #348, I realize this keyloader is short-lived and so the optimization here does nothing. Closing. |
Depends on #348.
I found this improvement when trying to make the code better for fetching keys by kid. You can see it's a little messy in my other branch and cleaner here.
I separated it into another PR since this refactor changes things unrelated to parsing string keys.