Updates RestController to reject unauthorized requests and allow requ…

…ests with no headers to pass-through for time being

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

sandbox/libs/authn/src/test/java/org/opensearch/authn/type/BasicAuthenticationTests.java

@@ -13,6 +13,7 @@
 import org.opensearch.test.rest.OpenSearchRestTestCase;
 
 import java.io.IOException;
+import java.util.Map;
 
 import static org.hamcrest.Matchers.equalTo;
 
@@ -52,29 +53,27 @@ public void testClusterHealthWithNoHeader() throws IOException {
         request.setOptions(options);
         Response response = client().performRequest(request);
 
-        // allowed if no authorization header was passed. Should be updated once that is fixed
+        // Should not fail, because current implementation allows a request with missing header to pass
+        // TODO: Update this test to check for missing-header response, once that is implemented
         assertOK(response);
 
         // Standard cluster health response
         MatcherAssert.assertThat(entityAsMap(response).size(), equalTo(17));
         MatcherAssert.assertThat(entityAsMap(response).get("status"), equalTo("green"));
-
     }
 
     public void testClusterHealthWithInvalidAuthenticationHeader() throws IOException {
         Request request = new Request("GET", "/_cluster/health");
         RequestOptions options = RequestOptions.DEFAULT.toBuilder().addHeader("Authorization", "Basic bWFydmluOmdhbGF4eQ==").build(); // marvin:galaxy
         request.setOptions(options);
-        Response response = client().performRequest(request);
-
-        // Should not fail, because current implementation allows a unauthorized request to pass
-        // TODO: Update this to test for UNAUTHORIZED once that flow is implemented
-        assertOK(response);
-
-        // Standard cluster health response
-        MatcherAssert.assertThat(entityAsMap(response).size(), equalTo(17));
-        MatcherAssert.assertThat(entityAsMap(response).get("status"), equalTo("green"));
-
+        try {
+            client().performRequest(request);
+        } catch (ResponseException re) {
+            Map<String, Object> responseMap = entityAsMap(re.getResponse());
+            MatcherAssert.assertThat(responseMap.size(), equalTo(2));
+            MatcherAssert.assertThat(responseMap.get("status"), equalTo(401));
+            MatcherAssert.assertThat(responseMap.get("error"), equalTo("marvin does not exist in internal realm."));
+        }
     }
 
     public void testClusterHealthWithCorruptAuthenticationHeader() throws IOException {

server/src/main/java/org/opensearch/rest/RestController.java

@@ -404,9 +404,7 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel
                     }
                 } else {
                     // Authenticate incoming request
-                    authenticate(request, channel);
-
-                    // TODO: see if we need to break the flow here in future in case authentication fails
+                    if (!authenticate(request, channel)) return;
 
                     dispatchRequest(request, channel, handler);
                     return;
@@ -606,9 +604,10 @@ private static CircuitBreaker inFlightRequestsBreaker(CircuitBreakerService circ
      * Authenticates the subject of the incoming REST request based on the auth header
      * @param request the request whose subject is to be authenticated
      * @param channel the channel to send the response on
+     * @return true if authentication was successful, false otherwise
      * @throws IOException when an exception is raised writing response to channel
      */
-    private void authenticate(RestRequest request, RestChannel channel) throws IOException {
+    private boolean authenticate(RestRequest request, RestChannel channel) throws IOException {
 
         final Optional<String> authHeader = request.getHeaders()
             .getOrDefault(HttpHeaderToken.HEADER_NAME, Collections.emptyList())
@@ -619,29 +618,43 @@ private void authenticate(RestRequest request, RestChannel channel) throws IOExc
 
         AuthenticationToken headerToken = null;
 
-        // TODO: Handle anonymous Auth - Allowed or Disallowed (set by the user of the system) - 401 or Login-redirect ??
-
         if (authHeader.isPresent()) {
             try {
                 headerToken = tokenType(authHeader.get());
                 subject = Identity.getAuthManager().getSubject();
                 subject.login(headerToken);
                 logger.info("Authentication successful");
+                return true;
             } catch (final AuthenticationException ae) {
                 logger.info("Authentication finally failed: {}", ae.getMessage());
 
-                /*
-                 TODO: For now don't send response if authentication fails, but this should be addressed soon
-                 TODO: If we uncomment this and authentication fails then cluster dies
-                 final BytesRestResponse bytesRestResponse = BytesRestResponse.createSimpleErrorResponse(
-                 channel,
-                 RestStatus.UNAUTHORIZED,
-                 ae.getMessage()
-                 );
-                 channel.sendResponse(bytesRestResponse);
-                */
+                final BytesRestResponse bytesRestResponse = BytesRestResponse.createSimpleErrorResponse(
+                    channel,
+                    RestStatus.UNAUTHORIZED,
+                    ae.getMessage()
+                );
+                channel.sendResponse(bytesRestResponse);
+                return false;
             }
         }
+
+        // TODO: Handle anonymous Auth - Allowed or Disallowed (set by the user of the system) - 401 or Login-redirect ??
+
+        /*
+        TODO: Uncomment this once it is decided to proceed with this workflow
+        logger.info("Authentication unsuccessful: Missing Authentication Header");
+        final BytesRestResponse bytesRestResponse = BytesRestResponse.createSimpleErrorResponse(
+            channel,
+            RestStatus.BAD_REQUEST,
+            "Missing Authentication Header"
+        );
+        channel.sendResponse(bytesRestResponse);
+        */
+
+        // This is allowing headers without Auth header to pass through.
+        // At the time of writing this, all rest-tests would fail if this is set to false
+        // TODO: Change this to false once there is a decision on what to do with requests that don't have auth Headers
+        return true;
     }
 
     /**

server/src/test/java/org/opensearch/rest/RestControllerTests.java

@@ -673,7 +673,7 @@ public void testRestRequestAuthenticationFailure() {
 
         // RestStatus is OK even though the authn information is incorrect. This is because we, yet, don't fail the request
         // if it was unauthorized. The status should be changed to UNAUTHORIZED once the flow is updated.
-        final AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.OK);
+        final AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.UNAUTHORIZED);
         restController.dispatchRequest(fakeRestRequest, channel, threadContext);
 
         assertTrue(channel.getSendResponseCalled());