From 627d1c0bcd3797cca78baf1cdb09eb8fa0b8408d Mon Sep 17 00:00:00 2001 From: Gregory Rushton Date: Wed, 2 Sep 2026 07:00:19 -0400 Subject: [PATCH 1/3] fix: update NA exception --- .../org/broadinstitute/consent/http/util/HttpClientUtil.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/broadinstitute/consent/http/util/HttpClientUtil.java b/src/main/java/org/broadinstitute/consent/http/util/HttpClientUtil.java index e09c917e7..4297b5886 100644 --- a/src/main/java/org/broadinstitute/consent/http/util/HttpClientUtil.java +++ b/src/main/java/org/broadinstitute/consent/http/util/HttpClientUtil.java @@ -23,6 +23,7 @@ import java.io.IOException; import java.net.URI; import java.nio.charset.Charset; +import java.util.Objects; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.ScheduledThreadPoolExecutor; @@ -193,7 +194,8 @@ public HttpResponse handleHttpRequest(HttpRequest request) { } case HttpStatusCodes.STATUS_CODE_UNAUTHORIZED -> { logErrorResponse(request, response); - throw new NotAuthorizedException(response.getStatusMessage()); + throw new NotAuthorizedException( + Objects.requireNonNullElse(response.getStatusMessage(), "Unauthorized"), "Bearer"); } case HttpStatusCodes.STATUS_CODE_FORBIDDEN -> { logErrorResponse(request, response); From dcba4c788e3ec17a440f5a39256536c4a01a21c0 Mon Sep 17 00:00:00 2001 From: Gregory Rushton Date: Wed, 2 Sep 2026 07:39:33 -0400 Subject: [PATCH 2/3] test: cover 401 responses with no reason phrase A downstream 401 whose status line omits the HTTP reason phrase leaves the status message null. The previous single-argument call bound that null to the NotAuthorizedException challenge parameter, which threw NullPointerException("Primary challenge parameter must not be null.") instead. Callers that catch NotAuthorizedException could not handle it, so the NPE reached the resource layer and became a 500. The second test pins the message. A lone String argument binds to challenge, not to message, so the old code also dropped the downstream reason phrase from the exception message. Co-Authored-By: Claude Opus 5 --- .../consent/http/util/HttpClientUtilTest.java | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/test/java/org/broadinstitute/consent/http/util/HttpClientUtilTest.java b/src/test/java/org/broadinstitute/consent/http/util/HttpClientUtilTest.java index 5efde020b..680f917ec 100644 --- a/src/test/java/org/broadinstitute/consent/http/util/HttpClientUtilTest.java +++ b/src/test/java/org/broadinstitute/consent/http/util/HttpClientUtilTest.java @@ -10,7 +10,14 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; +import com.google.api.client.http.GenericUrl; +import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpStatusCodes; +import com.google.api.client.http.HttpTransport; +import com.google.api.client.testing.http.MockHttpTransport; +import com.google.api.client.testing.http.MockLowLevelHttpResponse; +import jakarta.ws.rs.NotAuthorizedException; +import java.io.IOException; import java.util.stream.IntStream; import org.apache.hc.client5.http.classic.methods.HttpGet; import org.apache.hc.client5.http.impl.classic.RequestFailedException; @@ -111,4 +118,37 @@ void testGetHttpResponseOverTimeout() { clientUtil.getHttpResponse(new HttpGet(statusUrl)); }); } + + /** + * A downstream server can answer 401 without an HTTP reason phrase. Tomcat omits the phrase by + * default, and an ingress that forwards the status line unchanged delivers it that way. The + * status message is then null. A null must not reach the NotAuthorizedException challenge + * parameter, because that constructor answers with a NullPointerException, which callers that + * catch NotAuthorizedException cannot handle. + */ + @Test + void testHandleHttpRequestUnauthorizedWithoutReasonPhrase() throws Exception { + HttpRequest request = requestReturning(HttpStatusCodes.STATUS_CODE_UNAUTHORIZED, null); + + assertThrows(NotAuthorizedException.class, () -> clientUtil.handleHttpRequest(request)); + } + + /** A 401 that does carry a reason phrase keeps that phrase in the exception message. */ + @Test + void testHandleHttpRequestUnauthorizedKeepsReasonPhrase() throws Exception { + HttpRequest request = + requestReturning(HttpStatusCodes.STATUS_CODE_UNAUTHORIZED, "Token expired"); + + NotAuthorizedException e = + assertThrows(NotAuthorizedException.class, () -> clientUtil.handleHttpRequest(request)); + assertTrue(e.getMessage().contains("Token expired"), e.getMessage()); + } + + private HttpRequest requestReturning(int statusCode, String reasonPhrase) throws IOException { + MockLowLevelHttpResponse response = + new MockLowLevelHttpResponse().setStatusCode(statusCode).setReasonPhrase(reasonPhrase); + HttpTransport transport = + new MockHttpTransport.Builder().setLowLevelHttpResponse(response).build(); + return transport.createRequestFactory().buildGetRequest(new GenericUrl(statusUrl)); + } } From 6090aee735e05d68541112958986686c63d51e16 Mon Sep 17 00:00:00 2001 From: Gregory Rushton Date: Wed, 2 Sep 2026 07:41:54 -0400 Subject: [PATCH 3/3] Clean up HttpClientUtil test warnings --- .../consent/http/util/HttpClientUtilTest.java | 43 ++++++------------- 1 file changed, 14 insertions(+), 29 deletions(-) diff --git a/src/test/java/org/broadinstitute/consent/http/util/HttpClientUtilTest.java b/src/test/java/org/broadinstitute/consent/http/util/HttpClientUtilTest.java index 680f917ec..62716948e 100644 --- a/src/test/java/org/broadinstitute/consent/http/util/HttpClientUtilTest.java +++ b/src/test/java/org/broadinstitute/consent/http/util/HttpClientUtilTest.java @@ -5,10 +5,10 @@ import static com.github.tomakehurst.wiremock.client.WireMock.anyRequestedFor; import static com.github.tomakehurst.wiremock.client.WireMock.anyUrl; import static com.github.tomakehurst.wiremock.client.WireMock.exactly; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; import com.google.api.client.http.GenericUrl; import com.google.api.client.http.HttpRequest; @@ -18,7 +18,6 @@ import com.google.api.client.testing.http.MockLowLevelHttpResponse; import jakarta.ws.rs.NotAuthorizedException; import java.io.IOException; -import java.util.stream.IntStream; import org.apache.hc.client5.http.classic.methods.HttpGet; import org.apache.hc.client5.http.impl.classic.RequestFailedException; import org.broadinstitute.consent.http.WireMockTestHelper; @@ -47,15 +46,9 @@ void init() { @Test void testGetCachedResponse_case1() { wireMockServer.stubFor(any(anyUrl()).willReturn(aResponse().withStatus(200))); - IntStream.range(3, 10) - .forEach( - i -> { - try { - clientUtil.getCachedResponse(new HttpGet(statusUrl)); - } catch (Exception e) { - fail(e.getMessage()); - } - }); + for (int i = 3; i < 10; i++) { + assertDoesNotThrow(() -> clientUtil.getCachedResponse(new HttpGet(statusUrl))); + } wireMockServer.verify(exactly(1), anyRequestedFor(anyUrl())); } @@ -70,15 +63,9 @@ void testGetCachedResponse_case2() { wireMockServer.stubFor(any(anyUrl()).willReturn(aResponse().withStatus(200))); int count = randomInt(5, 10); - IntStream.range(0, count) - .forEach( - i -> { - try { - clientUtil.getCachedResponse(new HttpGet(statusUrl)); - } catch (Exception e) { - fail(e.getMessage()); - } - }); + for (int i = 0; i < count; i++) { + assertDoesNotThrow(() -> clientUtil.getCachedResponse(new HttpGet(statusUrl))); + } wireMockServer.verify(exactly(count), anyRequestedFor(anyUrl())); } @@ -113,10 +100,7 @@ void testGetHttpResponseOverTimeout() { wireMockServer.stubFor( any(anyUrl()).willReturn(aResponse().withStatus(200).withFixedDelay(3000))); assertThrows( - RequestFailedException.class, - () -> { - clientUtil.getHttpResponse(new HttpGet(statusUrl)); - }); + RequestFailedException.class, () -> clientUtil.getHttpResponse(new HttpGet(statusUrl))); } /** @@ -128,7 +112,7 @@ void testGetHttpResponseOverTimeout() { */ @Test void testHandleHttpRequestUnauthorizedWithoutReasonPhrase() throws Exception { - HttpRequest request = requestReturning(HttpStatusCodes.STATUS_CODE_UNAUTHORIZED, null); + HttpRequest request = requestReturning(null); assertThrows(NotAuthorizedException.class, () -> clientUtil.handleHttpRequest(request)); } @@ -136,17 +120,18 @@ void testHandleHttpRequestUnauthorizedWithoutReasonPhrase() throws Exception { /** A 401 that does carry a reason phrase keeps that phrase in the exception message. */ @Test void testHandleHttpRequestUnauthorizedKeepsReasonPhrase() throws Exception { - HttpRequest request = - requestReturning(HttpStatusCodes.STATUS_CODE_UNAUTHORIZED, "Token expired"); + HttpRequest request = requestReturning("Token expired"); NotAuthorizedException e = assertThrows(NotAuthorizedException.class, () -> clientUtil.handleHttpRequest(request)); assertTrue(e.getMessage().contains("Token expired"), e.getMessage()); } - private HttpRequest requestReturning(int statusCode, String reasonPhrase) throws IOException { + private HttpRequest requestReturning(String reasonPhrase) throws IOException { MockLowLevelHttpResponse response = - new MockLowLevelHttpResponse().setStatusCode(statusCode).setReasonPhrase(reasonPhrase); + new MockLowLevelHttpResponse() + .setStatusCode(HttpStatusCodes.STATUS_CODE_UNAUTHORIZED) + .setReasonPhrase(reasonPhrase); HttpTransport transport = new MockHttpTransport.Builder().setLowLevelHttpResponse(response).build(); return transport.createRequestFactory().buildGetRequest(new GenericUrl(statusUrl));