Optional kid on single item JWK sets#32
Conversation
lbalmaceda
changed the title
kid is optional. This PR allows to obtain a key from a single item array without passing the key id value.
joshcanhelp
left a comment
joshcanhelp
left a comment
There was a problem hiding this comment.
Mostly straight forward and a lot of this is formatting but a few questions on the meat of the implementation.
| } | ||
| } | ||
| } | ||
| throw new SigningKeyNotFoundException("No key found in " + url.toString() + " with kid " + keyId, null); |
There was a problem hiding this comment.
Will this message still be clear if keyId is null?
There was a problem hiding this comment.
Yes,null gets converted to "null" (string). Still clear that the value is missing, that's what null means anyway for us in Java.
"No key found in https://joshcanhelp.auth0.com/.well-known/jwks.json with kid null"
| if (keyId.equals(jwk.getId())) { | ||
| return jwk; | ||
| if (keyId == null && jwks.size() == 1) { | ||
| return jwks.get(0); |
There was a problem hiding this comment.
Is there a work-around if jwks.size() > 1?
There was a problem hiding this comment.
you mean keyid==null and jwks.size()>1 ?? no. Since there are many keys to choose from and the user hasn't passed a key id to filter them. This was the previous behavior: throw if key id not found in the set.
| } | ||
| if (keyId != null) { | ||
| for (Jwk jwk : jwks) { | ||
| if (keyId.equals(jwk.getId())) { |
There was a problem hiding this comment.
Seems odd that this portion of the method takes into account multiple entries in the JWKs but the previous one does not.
There was a problem hiding this comment.
it's only filtering the entries in a list for those which match a given key id. see:
- if key is specified, match by key even if set size is 1, since the user wants that key.
- if key is not specified and size is 1 the solution is trivial -> return that item
- in any other case, throw key with id "{id}" not found.
joshcanhelp
left a comment
joshcanhelp
left a comment
There was a problem hiding this comment.
@lbalmaceda - I was mis-reading what that JWKS check was supposed to do ... read it as looking through multiple JWKSes.
LGTM 👍
2 participants
As per https://tools.ietf.org/html/rfc7517#section-4.4 the
kidis optional. This PR allows to obtain a key from a single item array without passing the key id value.This doesn't break cached provider implementation as a valid key string is still used to store the jwk obtained when passing
nullas parameter to theget(kid)call.Closes #23