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); 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..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,13 +5,19 @@ 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; import com.google.api.client.http.HttpStatusCodes; -import java.util.stream.IntStream; +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 org.apache.hc.client5.http.classic.methods.HttpGet; import org.apache.hc.client5.http.impl.classic.RequestFailedException; import org.broadinstitute.consent.http.WireMockTestHelper; @@ -40,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())); } @@ -63,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())); } @@ -106,9 +100,40 @@ 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))); + } + + /** + * 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(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("Token expired"); + + NotAuthorizedException e = + assertThrows(NotAuthorizedException.class, () -> clientUtil.handleHttpRequest(request)); + assertTrue(e.getMessage().contains("Token expired"), e.getMessage()); + } + + private HttpRequest requestReturning(String reasonPhrase) throws IOException { + MockLowLevelHttpResponse response = + new MockLowLevelHttpResponse() + .setStatusCode(HttpStatusCodes.STATUS_CODE_UNAUTHORIZED) + .setReasonPhrase(reasonPhrase); + HttpTransport transport = + new MockHttpTransport.Builder().setLowLevelHttpResponse(response).build(); + return transport.createRequestFactory().buildGetRequest(new GenericUrl(statusUrl)); } }