Introduce FilterRestHandler (#98922)

RestHandler has a number of methods that affect the behaviour of request
processing. If the handler is wrapped (e.g. SecurityRestFilter or
DeprecationRestHandler) then these methods must be delegated to the
underlying handler.

This commit introduces a new abstract base class `FilterRestHandler`
that correctly delegates these methods so that wrappers (subclasses) do
not need to implement the behaviour on a case-by-case basis

Backport of: #98861

Author: tvernum

server/src/main/java/org/elasticsearch/rest/DeprecationRestHandler.java

@@ -20,10 +20,10 @@
  * {@code DeprecationRestHandler} provides a proxy for any existing {@link RestHandler} so that usage of the handler can be
  * logged using the {@link DeprecationLogger}.
  */
-public class DeprecationRestHandler implements RestHandler {
+public class DeprecationRestHandler extends FilterRestHandler implements RestHandler {
 
     public static final String DEPRECATED_ROUTE_KEY = "deprecated_route";
-    private final RestHandler handler;
+
     private final String deprecationMessage;
     private final DeprecationLogger deprecationLogger;
     private final String deprecationKey;
@@ -50,7 +50,7 @@ public DeprecationRestHandler(
         String deprecationMessage,
         DeprecationLogger deprecationLogger
     ) {
-        this.handler = Objects.requireNonNull(handler);
+        super(handler);
         this.deprecationMessage = requireValidHeader(deprecationMessage);
         this.deprecationLogger = Objects.requireNonNull(deprecationLogger);
         this.deprecationKey = DEPRECATED_ROUTE_KEY + "_" + method + "_" + path;
@@ -76,12 +76,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c
             deprecationLogger.warn(DeprecationCategory.API, deprecationKey, deprecationMessage);
         }
 
-        handler.handleRequest(request, channel, client);
-    }
-
-    @Override
-    public boolean supportsContentStream() {
-        return handler.supportsContentStream();
+        getDelegate().handleRequest(request, channel, client);
     }
 
     /**

server/src/main/java/org/elasticsearch/rest/FilterRestHandler.java

@@ -0,0 +1,54 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.rest;
+
+import java.util.List;
+import java.util.Objects;
+
+public abstract class FilterRestHandler implements RestHandler {
+    private final RestHandler delegate;
+
+    protected FilterRestHandler(RestHandler delegate) {
+        this.delegate = Objects.requireNonNull(delegate);
+    }
+
+    protected RestHandler getDelegate() {
+        return delegate;
+    }
+
+    @Override
+    public RestHandler getConcreteRestHandler() {
+        return delegate.getConcreteRestHandler();
+    }
+
+    @Override
+    public List<RestHandler.Route> routes() {
+        return delegate.routes();
+    }
+
+    @Override
+    public boolean allowSystemIndexAccessByDefault() {
+        return delegate.allowSystemIndexAccessByDefault();
+    }
+
+    @Override
+    public boolean canTripCircuitBreaker() {
+        return delegate.canTripCircuitBreaker();
+    }
+
+    @Override
+    public boolean allowsUnsafeBuffers() {
+        return delegate.allowsUnsafeBuffers();
+    }
+
+    @Override
+    public boolean supportsContentStream() {
+        return delegate.supportsContentStream();
+    }
+}

server/src/main/java/org/elasticsearch/rest/RestHandler.java

@@ -46,6 +46,15 @@ default boolean supportsContentStream() {
         return false;
     }
 
+    /**
+     * Returns the concrete RestHandler for this RestHandler. That is, if this is a delegating RestHandler it returns the delegate.
+     * Otherwise it returns itself.
+     * @return The underlying RestHandler
+     */
+    default RestHandler getConcreteRestHandler() {
+        return this;
+    }
+
     /**
      * Indicates if the RestHandler supports working with pooled buffers. If the request handler will not escape the return
      * {@link RestRequest#content()} or any buffers extracted from it then there is no need to make a copies of any pooled buffers in the

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java

@@ -21,6 +21,7 @@
 import org.elasticsearch.http.nio.NioHttpRequest;
 import org.elasticsearch.license.XPackLicenseState;
 import org.elasticsearch.rest.BytesRestResponse;
+import org.elasticsearch.rest.FilterRestHandler;
 import org.elasticsearch.rest.RestChannel;
 import org.elasticsearch.rest.RestHandler;
 import org.elasticsearch.rest.RestRequest;
@@ -31,14 +32,13 @@
 import org.elasticsearch.xpack.security.authc.support.SecondaryAuthenticator;
 
 import java.io.IOException;
-import java.util.List;
 
-public class SecurityRestFilter implements RestHandler {
+public class SecurityRestFilter extends FilterRestHandler implements RestHandler {
 
     private static final Logger logger = LogManager.getLogger(SecurityRestFilter.class);
 
-    private final RestHandler restHandler;
     private final AuthenticationService authenticationService;
+
     private final SecondaryAuthenticator secondaryAuthenticator;
     private final XPackLicenseState licenseState;
     private final AuditTrailService auditTrailService;
@@ -50,16 +50,11 @@ public SecurityRestFilter(
         AuditTrailService auditTrailService,
         RestHandler restHandler
     ) {
+        super(restHandler);
         this.licenseState = licenseState;
         this.authenticationService = authenticationService;
         this.secondaryAuthenticator = secondaryAuthenticator;
         this.auditTrailService = auditTrailService;
-        this.restHandler = restHandler;
-    }
-
-    @Override
-    public boolean allowSystemIndexAccessByDefault() {
-        return restHandler.allowSystemIndexAccessByDefault();
     }
 
     @Override
@@ -75,7 +70,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c
                     if (secondaryAuthentication != null) {
                         logger.trace("Found secondary authentication {} in REST request [{}]", secondaryAuthentication, requestUri);
                     }
-                    restHandler.handleRequest(request, channel, client);
+                    getDelegate().handleRequest(request, channel, client);
                 }, e -> handleException("Secondary authentication", request, channel, e)));
             }, e -> handleException("Authentication", request, channel, e)));
         } else {
@@ -101,7 +96,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c
                         + "/security-minimal-setup.html to enable security."
                 );
             }
-            restHandler.handleRequest(request, channel, client);
+            getDelegate().handleRequest(request, channel, client);
         }
     }
 
@@ -134,29 +129,10 @@ private void handleException(String actionType, RestRequest request, RestChannel
         }
     }
 
-    @Override
-    public boolean canTripCircuitBreaker() {
-        return restHandler.canTripCircuitBreaker();
-    }
-
-    @Override
-    public boolean supportsContentStream() {
-        return restHandler.supportsContentStream();
-    }
-
-    @Override
-    public boolean allowsUnsafeBuffers() {
-        return restHandler.allowsUnsafeBuffers();
-    }
-
-    @Override
-    public List<Route> routes() {
-        return restHandler.routes();
-    }
-
     private RestRequest maybeWrapRestRequest(RestRequest restRequest) throws IOException {
-        if (restHandler instanceof RestRequestFilter) {
-            return ((RestRequestFilter) restHandler).getFilteredRequest(restRequest);
+        final RestHandler handler = getConcreteRestHandler();
+        if (handler instanceof RestRequestFilter) {
+            return ((RestRequestFilter) handler).getFilteredRequest(restRequest);
         }
         return restRequest;
     }

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/rest/SecurityRestFilterTests.java

@@ -64,6 +64,7 @@
 import static org.hamcrest.Matchers.sameInstance;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
@@ -279,6 +280,7 @@ private void testProcessAuthenticationFailed(
         } else {
             assertThat(restResponse.content().utf8ToString(), not(containsString(ElasticsearchException.STACK_TRACE)));
         }
+        verify(restHandler, atLeastOnce()).getConcreteRestHandler();
         verifyNoMoreInteractions(restHandler);
     }