rmannibucau commented on code in PR #7:
URL: https://github.com/apache/geronimo-jwt-auth/pull/7#discussion_r1559090214
##########
src/main/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/KidMapper.java:
##########
@@ -79,9 +101,48 @@ private void init() {
.collect(Collectors.toSet()))
.orElseGet(HashSet::new);
ofNullable(config.read("issuer.default", config.read(Names.ISSUER,
null))).ifPresent(defaultIssuers::add);
+ jwksUrl = config.read("mp.jwt.verify.publickey.location", null);
+ readerFactory = Json.createReaderFactory(emptyMap());
+ ofNullable(jwksUrl).ifPresent(url -> {
+ HttpClient.Builder builder = HttpClient.newBuilder();
+ if (getJwksRefreshInterval() != null) {
+ long secondsRefresh = getJwksRefreshInterval();
+ backgroundThread =
Executors.newSingleThreadScheduledExecutor();
+ builder.executor(backgroundThread);
+ backgroundThread.scheduleAtFixedRate(this::reloadRemoteKeys,
getJwksRefreshInterval(), secondsRefresh, SECONDS);
+ }
+ httpClient = builder.build();
+ reloadJwksRequest = reloadRemoteKeys();// inital load, otherwise
the background thread is too slow to start and serve
+ });
defaultKey = config.read("public-key.default",
config.read(Names.VERIFIER_PUBLIC_KEY, null));
}
+ private Integer getJwksRefreshInterval() {
+ String interval = config.read("jwks.invalidation.interval",null);
+ if (interval != null) {
+ return Integer.parseInt(interval);
+ } else {
+ return null;
+ }
+ }
+
+ private CompletableFuture<Void> reloadRemoteKeys() {
+ HttpRequest request =
HttpRequest.newBuilder().GET().uri(URI.create(jwksUrl)).header("Accept",
"application/json").build();
+ CompletableFuture<HttpResponse<String>> httpResponseCompletableFuture
= httpClient.sendAsync(request, HttpResponse.BodyHandlers.ofString());
+ CompletableFuture<Void> ongoingRequest =
httpResponseCompletableFuture.thenApply(result -> {
+ List<JWK> jwks = parseKeys(result);
+ ConcurrentHashMap<String, String> newKeys = new
ConcurrentHashMap<>();
+ jwks.forEach(key -> newKeys.put(key.getKid(), key.toPemKey()));
+ keyMapping = newKeys;
+ return null;
+ });
+
+ ongoingRequest.thenRun(() -> {
Review Comment:
ok, let me detail my view to ensure we both get the same picture - think we
both see it from a different side:
* overall I think we should always use the "live" mapping and ignore the
`.get()` -> seems we agree on that
* however there is a case we want the `.get()` to happen and potentially
block, ie when the first call is not finished yet, there the first call in the
constructor must be `this.reloadRemoteKeys = ongoingRequest // from
reloadRemoteKeys()` otherwise this first hit can be missed and the call can
fail whereas it should pass
* we don't want `.get()` to be called in postconstruct but lazily - hoping
it is not called
My proposal would be:
1. in postconstruct set the promise to the actual promise and not post
execution (thenApply) as proposed
2. in `loadKey` dont test `reloadJwksRequest!=null` but `if
(reloadJwksRequest!=null&&!reloadJwksRequest.isDone())`
3. don't refresh `this.reloadJwksRequest` so we always test the
postconstruct value
4. in postconstruct add `.thenRun(() -> this.reloadJwksRequest = null)` to
avoid the `isDone()` call at runtime (optim but optional if you don't like that)
side note: reloadJwksRequest could then be renamed `initialJwksPromise` or
alike to show the intent.
hope it makes sense
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@geronimo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
