Support fetching the JWKS #260
Conversation
There was a problem hiding this comment.
In addition to my questions and suggestions, can you provide info on how the added fetchJsonWebKeys() will be used by clients or the library? That helps understand the context of this PR. Also consider making sure you have test coverage for the swallowed exceptions when trying to generate the public key, to make sure the expected result is still returned.
| @Override | ||
| public Map<String, PublicKey> deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) throws JsonParseException { | ||
| if (!json.isJsonObject() || json.isJsonNull() || json.getAsJsonObject().entrySet().isEmpty()) { | ||
| throw new JsonParseException("jwks json is not a valid json object"); |
There was a problem hiding this comment.
Is this the appropriate exception type to throw here? Since the issue is the json is either not a JsonObject, or is null, or is an empty object - not an error parsing the JSON, it just doesn't meet other requirements. Also perhaps worth including in the exception message information that the json needs to be a non-empty valid JSON object
There was a problem hiding this comment.
Yes, this is the same 3 line block used on the rest of the parsers. Parsers will only kick in if we explicitly tell the library that we want to parse a given response as that type. So, for this case that would only be for the endpoint that I added.
If the JSON present on that response is other than a non-empty object, we fail with that exception that is part of GSON, symbolizing in a way that the JSON input was unexpected.
There was a problem hiding this comment.
Ok, then it makes sense to use the JsonParseException. I do think it may be worthwhile to include info in the message about it needing to be a non-empty json object, otherwise the message can be misleading (an empty object is a valid json object)
| JsonArray keys = object.getAsJsonArray("keys"); | ||
| for (JsonElement k : keys) { | ||
| JsonObject currentKey = k.getAsJsonObject(); | ||
| String keyAlg = context.deserialize(currentKey.remove("alg"), String.class); |
There was a problem hiding this comment.
Why use remove which mutates the object, instead of get? This question also applies to other usages of remove below.
There was a problem hiding this comment.
Good catch. On other scenarios we would end up keeping the remaining claims as an "extra props" param. We don't do this here so this can be changed to a regular get(key).
lbalmaceda
force-pushed
the
support-jwks
branch
from
6a6941b
to
91b2fe0
Compare
Changes
This PR adds a method to the
AuthenticationAPIClientto obtain the JSON Web Keys associated to the base domain. It only supports RSA keys used for verifying signatures (Public Keys). Other type of keys would not fail but will not be parsed either, resulting in an empty map.The reason why I put it on the existing class is because it already "knows" how to make requests and has the HTTP client instance already configured with the different options. In the future, this HTTP client should be provided on demand by a singleton like method.
The name of the method is up to discussion.
References
This feature will be used to improve the OIDC compliance on a later PR.
Devs would normally not need to call this method on their own. It's going to be used to fetch the Auth0 tenant Public keys in order to verify RSA signed tokens. This keys information is available publicly on any tenant's well-known path, e.g. mine
Testing
This change adds unit test coverage
This change adds integration test coverage
This change has been tested on the latest version of the platform/language or why not
Checklist
I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors