Aman-Mittal commented on code in PR #39: URL: https://github.com/apache/fineract-consumer-facing/pull/39#discussion_r3585239974
########## consumer/src/main/java/org/apache/fineract/consumer/infrastructure/access/service/JwtDenylist.java: ########## @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fineract.consumer.infrastructure.access.service; + +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.Expiry; +import com.github.benmanes.caffeine.cache.Ticker; +import java.time.Duration; +import java.time.Instant; +import org.springframework.stereotype.Component; + +@Component +public class JwtDenylist { + + private static final Duration CLOCK_SKEW_PAD = Duration.ofSeconds(60); + + private final Cache<String, Instant> deniedTokenIds; + + public JwtDenylist() { + this(Ticker.systemTicker()); + } + + JwtDenylist(Ticker ticker) { + this.deniedTokenIds = Caffeine.newBuilder() + .ticker(ticker) + .expireAfter(new Expiry<String, Instant>() { + + @Override + public long expireAfterCreate(String tokenId, Instant tokenExpiresAt, long currentTime) { + return remainingLifetimeNanos(tokenExpiresAt); + } + + @Override + public long expireAfterUpdate(String tokenId, Instant tokenExpiresAt, long currentTime, + long currentDuration) { + return remainingLifetimeNanos(tokenExpiresAt); + } + + @Override + public long expireAfterRead(String tokenId, Instant tokenExpiresAt, long currentTime, + long currentDuration) { + return currentDuration; + } + }) + .build(); + } + + public void deny(String tokenId, Instant tokenExpiresAt) { + deniedTokenIds.put(tokenId, tokenExpiresAt); + } + + public boolean isDenied(String tokenId) { + return tokenId != null && deniedTokenIds.getIfPresent(tokenId) != null; + } + + private static long remainingLifetimeNanos(Instant tokenExpiresAt) { + return Math.max(0, Duration.between(Instant.now(), tokenExpiresAt.plus(CLOCK_SKEW_PAD)).toNanos()); + } +} Review Comment: <h2 data-section-id="qj57ka" data-start="50" data-end="129">JwtDenylist should use Valkey instead of Caffeine for distributed revocation</h2> <p data-start="131" data-end="338"><code data-start="131" data-end="144">JwtDenylist</code> currently uses an in-memory Caffeine cache. While Caffeine is a high-performance local cache, it is not suitable as the primary storage mechanism for JWT revocation in a distributed deployment.</p> <p data-start="340" data-end="522">For a real security control, revoked tokens must be visible to <strong data-start="403" data-end="432">all application instances</strong> and survive application restarts. A distributed datastore such as <strong data-start="499" data-end="509">Valkey</strong> is required.</p> <p data-start="524" data-end="668">The project uses <strong data-start="541" data-end="613">Valkey instead of Redis due to licensing considerations around Redis</strong>, while maintaining Redis-compatible behavior and APIs.</p> </div></div> <hr data-start="1271" data-end="1274"> <h2 data-section-id="16cbkga" data-start="1276" data-end="1308">Example failure with Caffeine</h2> <pre class="overflow-visible! px-0!" data-start="1310" data-end="1589"><div class="relative w-full mt-4 mb-1"><div class=""><div class="contents"><div class="relative"><div class="h-full min-h-0 min-w-0"><div class="h-full min-h-0 min-w-0"><div class="border border-token-border-light border-radius-3xl corner-superellipse/1.1 rounded-3xl"><div class="h-full w-full border-radius-3xl bg-(--code-block-surface) corner-superellipse/1.1 overflow-clip rounded-3xl [--code-block-surface:var(--bg-elevated-secondary)] dark:[--code-block-surface:var(--composer-surface-primary)] lxnfua_clipPathFallback"><div class="pointer-events-none absolute end-1.5 top-1 z-2 md:end-2 md:top-1"></div><div class="relative"><div class="pe-11 pt-3"><div class="relative z-0 flex max-w-full"><div id="code-block-viewer" dir="ltr" class="q9tKkq_viewer cm-editor z-10 light:cm-light dark:cm-light flex h-full w-full flex-col items-stretch ͼd ͼr"><div class="cm-scroller"><pre class="cm-content q9tKkq_readonly m-0"><co de><span>Pod A User logs out | v JwtDenylist.deny("token-xyz") | v Stored only in Pod A memory Pod B User sends request with "token-xyz" | v JwtDenylist.isDenied("token-xyz") | v No entry found | v Token accepted</span></code></pre></div></div></div></div></div></div></div></div></div><div class=""><div class=""></div></div></div></div></div></div></pre> <p data-start="1591" data-end="1725">The logout event occurred successfully, but only within Pod A. Any request routed to another pod can continue using the revoked token.</p> <hr data-start="1727" data-end="1730"> <h2 data-section-id="1zft45" data-start="1732" data-end="1759">Recommended architecture</h2> <p data-start="1761" data-end="1795">Use Valkey as the source of truth:</p> <pre class="overflow-visible! px-0!" data-start="1797" data-end="2109"><div class="relative w-full mt-4 mb-1"><div class=""><div class="contents"><div class="relative"><div class="h-full min-h-0 min-w-0"><div class="h-full min-h-0 min-w-0"><div class="border border-token-border-light border-radius-3xl corner-superellipse/1.1 rounded-3xl"><div class="h-full w-full border-radius-3xl bg-(--code-block-surface) corner-superellipse/1.1 overflow-clip rounded-3xl [--code-block-surface:var(--bg-elevated-secondary)] dark:[--code-block-surface:var(--composer-surface-primary)] lxnfua_clipPathFallback"><div class="pointer-events-none absolute end-1.5 top-1 z-2 md:end-2 md:top-1"></div><div class="relative"><div class="pe-11 pt-3"><div class="relative z-0 flex max-w-full"><div id="code-block-viewer" dir="ltr" class="q9tKkq_viewer cm-editor z-10 light:cm-light dark:cm-light flex h-full w-full flex-col items-stretch ͼd ͼr"><div class="cm-scroller"><pre class="cm-content q9tKkq_readonly m-0"><co de><span>Logout request | v Extract JWT identifier (jti) | v Store revocation entry in Valkey Key: jwt:denylist:{jti} Value: revoked metadata TTL: remaining JWT lifetime | v All pods check Valkey during authentication | v Revoked tokens rejected consistently</span></code></pre></div></div></div></div></div></div></div></div></div><div class=""><div class=""></div></div></div></div></div></div></pre> <p data-start="2111" data-end="2195">Caffeine can still be used as an optional <strong data-start="2153" data-end="2181">local optimization layer</strong>, for example:</p> <pre class="overflow-visible! px-0!" data-start="2197" data-end="2456"><div class="relative w-full mt-4 mb-1"><div class=""><div class="contents"><div class="relative"><div class="h-full min-h-0 min-w-0"><div class="h-full min-h-0 min-w-0"><div class="border border-token-border-light border-radius-3xl corner-superellipse/1.1 rounded-3xl"><div class="h-full w-full border-radius-3xl bg-(--code-block-surface) corner-superellipse/1.1 overflow-clip rounded-3xl [--code-block-surface:var(--bg-elevated-secondary)] dark:[--code-block-surface:var(--composer-surface-primary)] lxnfua_clipPathFallback"><div class="pointer-events-none absolute end-1.5 top-1 z-2 md:end-2 md:top-1"></div><div class="relative"><div class="pe-11 pt-3"><div class="relative z-0 flex max-w-full"><div id="code-block-viewer" dir="ltr" class="q9tKkq_viewer cm-editor z-10 light:cm-light dark:cm-light flex h-full w-full flex-col items-stretch ͼd ͼr"><div class="cm-scroller"><pre class="cm-content q9tKkq_readonly m-0"><co de><span>Request | v Check local Caffeine cache | +--> denied → reject immediately | +--> miss → check Valkey | +--> denied → update Caffeine + reject | +--> allowed → continue</span></code></pre></div></div></div></div></div></div></div></div></div><div class=""><div class=""></div></div></div></div></div></div></pre> <p data-start="2458" data-end="2472">In this model:</p> <ul data-start="2474" data-end="2612"> <li data-section-id="1kqe75r" data-start="2474" data-end="2523"> <strong data-start="2476" data-end="2523">Valkey remains the security source of truth</strong> </li> <li data-section-id="imouh8" data-start="2524" data-end="2555"> <strong data-start="2526" data-end="2555">Caffeine improves latency</strong> </li> <li data-section-id="1rts2lr" data-start="2556" data-end="2612"> Revocation remains consistent across pods and restarts </li> </ul> <hr data-start="2614" data-end="2617"> <h2 data-section-id="1xdkkpr" data-start="2619" data-end="2638"></h2> ########## consumer/src/main/java/org/apache/fineract/consumer/beneficiaries/command/exception/BeneficiaryStepUpInvalidException.java: ########## @@ -0,0 +1,31 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.fineract.consumer.beneficiaries.command.exception; + +import org.apache.fineract.consumer.infrastructure.exception.AbstractConsumerException; +import org.springframework.http.HttpStatus; + +public class BeneficiaryStepUpInvalidException extends AbstractConsumerException { + + public static final String CODE = "error.msg.consumer.beneficiaries.command.stepup.invalid"; + + public BeneficiaryStepUpInvalidException() { + super(HttpStatus.UNAUTHORIZED, CODE, "step-up verification failed"); Review Comment: returns HTTP 401 — should be 403 Forbidden. The user is already authenticated; step-up failure is an authorization concern, not authentication -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
