[DT-4058] Update NA exception signature

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);

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));
   }
 }