Simplify SecurityRestFilter (#91283)

No need to actually resolve the security-enabled setting for
every request, this actually shows up in profiling since settings are a
little slow.
Also: reduced indentation a little, removed pointless extra reference to
`requestUri` and made `handleException` non-static since it was always
passed the `threadContext` field from the instance anyway.

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

@@ -1583,7 +1583,7 @@ public class Security extends Plugin
             extractClientCertificate = false;
         }
         return handler -> new SecurityRestFilter(
-            settings,
+            enabled,
             threadContext,
             authcService.get(),
             secondayAuthc.get(),

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

@@ -12,7 +12,6 @@ import org.apache.logging.log4j.util.Supplier;
 import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.client.internal.node.NodeClient;
-import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.Maps;
 import org.elasticsearch.common.util.concurrent.ThreadContext;
 import org.elasticsearch.http.HttpChannel;
@@ -23,7 +22,6 @@ import org.elasticsearch.rest.RestRequest.Method;
 import org.elasticsearch.rest.RestRequestFilter;
 import org.elasticsearch.rest.RestResponse;
 import org.elasticsearch.rest.RestStatus;
-import org.elasticsearch.xpack.core.XPackSettings;
 import org.elasticsearch.xpack.security.authc.AuthenticationService;
 import org.elasticsearch.xpack.security.authc.support.SecondaryAuthenticator;
 import org.elasticsearch.xpack.security.transport.SSLEngineUtils;
@@ -40,7 +38,7 @@ public class SecurityRestFilter implements RestHandler {
     private final RestHandler restHandler;
     private final AuthenticationService authenticationService;
     private final SecondaryAuthenticator secondaryAuthenticator;
-    private final Settings settings;
+    private final boolean enabled;
     private final ThreadContext threadContext;
     private final boolean extractClientCertificate;
 
@@ -62,14 +60,14 @@ public class SecurityRestFilter implements RestHandler {
     }
 
     public SecurityRestFilter(
-        Settings settings,
+        boolean enabled,
         ThreadContext threadContext,
         AuthenticationService authenticationService,
         SecondaryAuthenticator secondaryAuthenticator,
         RestHandler restHandler,
         boolean extractClientCertificate
     ) {
-        this.settings = settings;
+        this.enabled = enabled;
         this.threadContext = threadContext;
         this.authenticationService = authenticationService;
         this.secondaryAuthenticator = secondaryAuthenticator;
@@ -90,42 +88,44 @@ public class SecurityRestFilter implements RestHandler {
             return;
         }
 
-        if (XPackSettings.SECURITY_ENABLED.get(settings)) {
-            if (extractClientCertificate) {
-                HttpChannel httpChannel = request.getHttpChannel();
-                SSLEngineUtils.extractClientCertificates(logger, threadContext, httpChannel);
-            }
+        if (enabled == false) {
+            doHandleRequest(request, channel, client);
+            return;
+        }
 
-            final String requestUri = request.uri();
-            authenticationService.authenticate(maybeWrapRestRequest(request), ActionListener.wrap(authentication -> {
-                if (authentication == null) {
-                    logger.trace("No authentication available for REST request [{}]", requestUri);
-                } else {
-                    logger.trace("Authenticated REST request [{}] as {}", requestUri, authentication);
-                }
-                secondaryAuthenticator.authenticateAndAttachToContext(request, ActionListener.wrap(secondaryAuthentication -> {
-                    if (secondaryAuthentication != null) {
-                        logger.trace("Found secondary authentication {} in REST request [{}]", secondaryAuthentication, requestUri);
-                    }
-                    RemoteHostHeader.process(request, threadContext);
-                    try {
-                        threadContext.sanitizeHeaders();
-                        restHandler.handleRequest(request, channel, client);
-                    } catch (Exception e) {
-                        handleException(ActionType.RequestHandling, request, channel, e, threadContext);
-                    }
-                }, e -> handleException(ActionType.SecondaryAuthentication, request, channel, e, threadContext)));
-            }, e -> handleException(ActionType.Authentication, request, channel, e, threadContext)));
-        } else {
-            threadContext.sanitizeHeaders();
-            restHandler.handleRequest(request, channel, client);
+        if (extractClientCertificate) {
+            HttpChannel httpChannel = request.getHttpChannel();
+            SSLEngineUtils.extractClientCertificates(logger, threadContext, httpChannel);
         }
 
+        authenticationService.authenticate(maybeWrapRestRequest(request), ActionListener.wrap(authentication -> {
+            if (authentication == null) {
+                logger.trace("No authentication available for REST request [{}]", request.uri());
+            } else {
+                logger.trace("Authenticated REST request [{}] as {}", request.uri(), authentication);
+            }
+            secondaryAuthenticator.authenticateAndAttachToContext(request, ActionListener.wrap(secondaryAuthentication -> {
+                if (secondaryAuthentication != null) {
+                    logger.trace("Found secondary authentication {} in REST request [{}]", secondaryAuthentication, request.uri());
+                }
+                RemoteHostHeader.process(request, threadContext);
+                try {
+                    doHandleRequest(request, channel, client);
+                } catch (Exception e) {
+                    handleException(ActionType.RequestHandling, request, channel, e);
+                }
+            }, e -> handleException(ActionType.SecondaryAuthentication, request, channel, e)));
+        }, e -> handleException(ActionType.Authentication, request, channel, e)));
+    }
+
+    private void doHandleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
+        threadContext.sanitizeHeaders();
+        restHandler.handleRequest(request, channel, client);
     }
 
-    protected static void handleException(ActionType actionType, RestRequest request, RestChannel channel, Exception e, ThreadContext tc) {
+    protected void handleException(ActionType actionType, RestRequest request, RestChannel channel, Exception e) {
         logger.debug(() -> format("%s failed for REST request [%s]", actionType, request.uri()), e);
-        tc.sanitizeHeaders();
+        threadContext.sanitizeHeaders();
         final RestStatus restStatus = ExceptionsHelper.status(e);
         try {
             channel.sendResponse(new RestResponse(channel, restStatus, e) {

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

@@ -29,7 +29,6 @@ import org.elasticsearch.xcontent.DeprecationHandler;
 import org.elasticsearch.xcontent.NamedXContentRegistry;
 import org.elasticsearch.xcontent.XContentType;
 import org.elasticsearch.xcontent.json.JsonXContent;
-import org.elasticsearch.xpack.core.XPackSettings;
 import org.elasticsearch.xpack.core.security.SecurityContext;
 import org.elasticsearch.xpack.core.security.authc.Authentication;
 import org.elasticsearch.xpack.core.security.authc.Authentication.RealmRef;
@@ -80,7 +79,7 @@ public class SecurityRestFilterTests extends ESTestCase {
         restHandler = mock(RestHandler.class);
         threadContext = new ThreadContext(Settings.EMPTY);
         secondaryAuthenticator = new SecondaryAuthenticator(Settings.EMPTY, threadContext, authcService);
-        filter = new SecurityRestFilter(Settings.EMPTY, threadContext, authcService, secondaryAuthenticator, restHandler, false);
+        filter = new SecurityRestFilter(true, threadContext, authcService, secondaryAuthenticator, restHandler, false);
     }
 
     public void testProcess() throws Exception {
@@ -143,8 +142,7 @@ public class SecurityRestFilterTests extends ESTestCase {
     }
 
     public void testProcessWithSecurityDisabled() throws Exception {
-        Settings settings = Settings.builder().put(XPackSettings.SECURITY_ENABLED.getKey(), false).build();
-        filter = new SecurityRestFilter(settings, threadContext, authcService, secondaryAuthenticator, restHandler, false);
+        filter = new SecurityRestFilter(false, threadContext, authcService, secondaryAuthenticator, restHandler, false);
         RestRequest request = mock(RestRequest.class);
         filter.handleRequest(request, channel, null);
         verify(restHandler).handleRequest(request, channel, null);
@@ -152,7 +150,7 @@ public class SecurityRestFilterTests extends ESTestCase {
     }
 
     public void testProcessAuthenticationFailedNoTrace() throws Exception {
-        filter = new SecurityRestFilter(Settings.EMPTY, threadContext, authcService, secondaryAuthenticator, restHandler, false);
+        filter = new SecurityRestFilter(true, threadContext, authcService, secondaryAuthenticator, restHandler, false);
         testProcessAuthenticationFailed(
             randomBoolean()
                 ? authenticationError("failed authn")
@@ -268,7 +266,7 @@ public class SecurityRestFilterTests extends ESTestCase {
             callback.onResponse(AuthenticationTestHelper.builder().realmRef(new RealmRef("test", "test", "t")).build(false));
             return Void.TYPE;
         }).when(authcService).authenticate(any(RestRequest.class), anyActionListener());
-        filter = new SecurityRestFilter(Settings.EMPTY, threadContext, authcService, secondaryAuthenticator, restHandler, false);
+        filter = new SecurityRestFilter(true, threadContext, authcService, secondaryAuthenticator, restHandler, false);
 
         filter.handleRequest(restRequest, channel, null);
 

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

@@ -89,14 +89,7 @@ public class SecurityRestFilterWarningHeadersTests extends ESTestCase {
     private Map<String, List<String>> testProcessRestHandlingFailed(RestStatus restStatus, MapBuilder<String, List<String>> headers)
         throws Exception {
         RestChannel channel = mock(RestChannel.class);
-        SecurityRestFilter filter = new SecurityRestFilter(
-            Settings.EMPTY,
-            threadContext,
-            authcService,
-            secondaryAuthenticator,
-            restHandler,
-            false
-        );
+        SecurityRestFilter filter = new SecurityRestFilter(true, threadContext, authcService, secondaryAuthenticator, restHandler, false);
         RestRequest request = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).build();
         Authentication primaryAuthentication = AuthenticationTestHelper.builder().build();
         doAnswer(i -> {
@@ -128,14 +121,7 @@ public class SecurityRestFilterWarningHeadersTests extends ESTestCase {
     private Map<String, List<String>> testProcessAuthenticationFailed(RestStatus restStatus, MapBuilder<String, List<String>> headers)
         throws Exception {
         RestChannel channel = mock(RestChannel.class);
-        SecurityRestFilter filter = new SecurityRestFilter(
-            Settings.EMPTY,
-            threadContext,
-            authcService,
-            secondaryAuthenticator,
-            restHandler,
-            false
-        );
+        SecurityRestFilter filter = new SecurityRestFilter(true, threadContext, authcService, secondaryAuthenticator, restHandler, false);
         RestRequest request = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).build();
         doAnswer((i) -> {
             ActionListener<?> callback = (ActionListener<?>) i.getArguments()[1];