Merge pull request #635 from apache/WW-4173-optional

[WW-4173] Introduces a dedicated interface to allow conditionally disabling a given interceptor

core/src/main/java/com/opensymphony/xwork2/DefaultActionInvocation.java

@@ -24,6 +24,7 @@ import com.opensymphony.xwork2.config.entities.InterceptorMapping;
 import com.opensymphony.xwork2.config.entities.ResultConfig;
 import com.opensymphony.xwork2.inject.Container;
 import com.opensymphony.xwork2.inject.Inject;
+import com.opensymphony.xwork2.interceptor.ConditionalInterceptor;
 import com.opensymphony.xwork2.interceptor.Interceptor;
 import com.opensymphony.xwork2.interceptor.PreResultListener;
 import com.opensymphony.xwork2.interceptor.WithLazyParams;
@@ -248,10 +249,10 @@ public class DefaultActionInvocation implements ActionInvocation {
                 if (interceptor instanceof WithLazyParams) {
                     interceptor = lazyParamInjector.injectParams(interceptor, interceptorMapping.getParams(), invocationContext);
                 }
-                if (interceptor.isDisabled(this)) {
-                    LOG.debug("Interceptor: {} is disabled, skipping to next", interceptor.getClass().getSimpleName());
-                    resultCode = this.invoke();
+                if (interceptor instanceof ConditionalInterceptor) {
+                    resultCode = executeConditional((ConditionalInterceptor) interceptor);
                 } else {
+                    LOG.debug("Executing normal interceptor: {}", interceptorMapping.getName());
                     resultCode = interceptor.intercept(this);
                 }
             } else {
@@ -292,6 +293,16 @@ public class DefaultActionInvocation implements ActionInvocation {
         return resultCode;
     }
 
+    protected String executeConditional(ConditionalInterceptor conditionalInterceptor) throws Exception {
+        if (conditionalInterceptor.shouldIntercept(this)) {
+            LOG.debug("Executing conditional interceptor: {}", conditionalInterceptor.getClass().getSimpleName());
+            return conditionalInterceptor.intercept(this);
+        } else {
+            LOG.debug("Interceptor: {} is disabled, skipping to next", conditionalInterceptor.getClass().getSimpleName());
+            return this.invoke();
+        }
+    }
+
     public String invokeActionOnly() throws Exception {
         return invokeAction(getAction(), proxy.getConfig());
     }

core/src/main/java/com/opensymphony/xwork2/interceptor/AbstractInterceptor.java

@@ -23,7 +23,7 @@ import com.opensymphony.xwork2.ActionInvocation;
 /**
  * Provides default implementations of optional lifecycle methods
  */
-public abstract class AbstractInterceptor implements Interceptor {
+public abstract class AbstractInterceptor implements ConditionalInterceptor {
 
     private boolean disabled;
 
@@ -44,12 +44,18 @@ public abstract class AbstractInterceptor implements Interceptor {
      */
     public abstract String intercept(ActionInvocation invocation) throws Exception;
 
+    /**
+     * Allows to skip executing a given interceptor, just define {@code <param name="disabled">true</param>}
+     * or use other way to override interceptor's parameters, see
+     * <a href="https://struts.apache.org/core-developers/interceptors#interceptor-parameter-overriding">docs</a>.
+     * @param disable if set to true, execution of a given interceptor will be skipped.
+     */
     public void setDisabled(String disable) {
         this.disabled = Boolean.parseBoolean(disable);
     }
 
     @Override
-    public boolean isDisabled(ActionInvocation invocation) {
-        return this.disabled;
+    public boolean shouldIntercept(ActionInvocation invocation) {
+        return !this.disabled;
     }
 }

core/src/main/java/com/opensymphony/xwork2/interceptor/ConditionalInterceptor.java

@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package com.opensymphony.xwork2.interceptor;
+
+import com.opensymphony.xwork2.ActionInvocation;
+
+/**
+ * A marking interface, when implemented allows to conditionally execute a given interceptor
+ * within the current action invocation.
+ *
+ * @since Struts 6.1.1
+ */
+public interface ConditionalInterceptor extends Interceptor {
+
+    /**
+     * Determines if a given interceptor should be executed in the current processing of action invocation.
+     *
+     * @param invocation current {@link ActionInvocation} to determine if the interceptor should be executed
+     * @return true if the given interceptor should be included in the current action invocation
+     * @since 6.1.1
+     */
+    boolean shouldIntercept(ActionInvocation invocation);
+}

core/src/main/java/com/opensymphony/xwork2/interceptor/Interceptor.java

@@ -219,12 +219,4 @@ public interface Interceptor extends Serializable {
      */
     String intercept(ActionInvocation invocation) throws Exception;
 
-    /**
-     * Allows to disable processing a given interceptor
-     *
-     * @param invocation current {@link ActionInvocation} to determine if the interceptor should be executed
-     * @return true if the given interceptor should be skipped
-     * @since 6.1.0
-     */
-    boolean isDisabled(ActionInvocation invocation);
 }

core/src/main/java/org/apache/struts2/interceptor/CoepInterceptor.java

@@ -51,20 +51,12 @@ public class CoepInterceptor extends AbstractInterceptor implements PreResultLis
 
     @Override
     public String intercept(ActionInvocation invocation) throws Exception {
-        if (this.isDisabled(invocation)) {
-            LOG.trace("COEP interceptor has been disabled");
-        } else {
-            invocation.addPreResultListener(this);
-        }
+        invocation.addPreResultListener(this);
         return invocation.invoke();
     }
 
     @Override
     public void beforeResult(ActionInvocation invocation, String resultCode) {
-        if (this.isDisabled(invocation)) {
-            return;
-        }
-
         HttpServletRequest req = invocation.getInvocationContext().getServletRequest();
         final String path = req.getContextPath();
 

core/src/main/java/org/apache/struts2/interceptor/CoopInterceptor.java

@@ -53,19 +53,12 @@ public class CoopInterceptor extends AbstractInterceptor implements PreResultLis
 
     @Override
     public String intercept(ActionInvocation invocation) throws Exception {
-        if (this.isDisabled(invocation)) {
-            LOG.trace("COOP interceptor has been disabled");
-        } else {
-            invocation.addPreResultListener(this);
-        }
+        invocation.addPreResultListener(this);
         return invocation.invoke();
     }
 
     @Override
     public void beforeResult(ActionInvocation invocation, String resultCode) {
-        if (this.isDisabled(invocation)) {
-            return;
-        }
         HttpServletRequest request = invocation.getInvocationContext().getServletRequest();
         String path = request.getContextPath();
 

core/src/main/java/org/apache/struts2/interceptor/FetchMetadataInterceptor.java

@@ -62,10 +62,6 @@ public class FetchMetadataInterceptor extends AbstractInterceptor {
 
     @Override
     public String intercept(ActionInvocation invocation) throws Exception {
-        if (this.isDisabled(invocation)) {
-            LOG.trace("Fetch Metadata interceptor has been disabled");
-            return invocation.invoke();
-        }
         ActionContext context = invocation.getInvocationContext();
         HttpServletRequest request = context.getServletRequest();
 

core/src/main/java/org/apache/struts2/interceptor/csp/CspInterceptor.java

@@ -21,8 +21,6 @@ package org.apache.struts2.interceptor.csp;
 import com.opensymphony.xwork2.ActionInvocation;
 import com.opensymphony.xwork2.interceptor.AbstractInterceptor;
 import com.opensymphony.xwork2.interceptor.PreResultListener;
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
@@ -41,24 +39,15 @@ import java.util.Optional;
  **/
 public final class CspInterceptor extends AbstractInterceptor implements PreResultListener {
 
-    private static final Logger LOG = LogManager.getLogger(CspInterceptor.class);
-
     private final CspSettings settings = new DefaultCspSettings();
 
     @Override
     public String intercept(ActionInvocation invocation) throws Exception {
-        if (this.isDisabled(invocation)) {
-            LOG.trace("CSP interceptor has been disabled");
-        } else {
-            invocation.addPreResultListener(this);
-        }
+        invocation.addPreResultListener(this);
         return invocation.invoke();
     }
 
     public void beforeResult(ActionInvocation invocation, String resultCode) {
-        if (this.isDisabled(invocation)) {
-            return;
-        }
         HttpServletRequest request = invocation.getInvocationContext().getServletRequest();
         HttpServletResponse response = invocation.getInvocationContext().getServletResponse();
         settings.addCspHeaders(request, response);

core/src/test/java/com/opensymphony/xwork2/DefaultActionInvocationTest.java

@@ -22,7 +22,7 @@ import com.opensymphony.xwork2.config.entities.ActionConfig;
 import com.opensymphony.xwork2.config.entities.InterceptorMapping;
 import com.opensymphony.xwork2.config.entities.ResultConfig;
 import com.opensymphony.xwork2.config.providers.XmlConfigurationProvider;
-import com.opensymphony.xwork2.interceptor.PreResultListener;
+import com.opensymphony.xwork2.interceptor.Interceptor;
 import com.opensymphony.xwork2.mock.MockActionProxy;
 import com.opensymphony.xwork2.mock.MockInterceptor;
 import com.opensymphony.xwork2.mock.MockResult;
@@ -65,10 +65,18 @@ public class DefaultActionInvocationTest extends XWorkTestCase {
         interceptorMappings.add(new InterceptorMapping("test2", mockInterceptor2));
         mockInterceptor2.setFoo("test2");
         mockInterceptor2.setExpectedFoo("test2");
-        MockInterceptor mockInterceptor3 = new MockInterceptor();
+        MockInterceptor mockInterceptor3 = new MockInterceptor() {
+            @Override
+            public boolean shouldIntercept(ActionInvocation invocation) {
+                return false;
+            }
+        };
         interceptorMappings.add(new InterceptorMapping("test3", mockInterceptor3));
-        mockInterceptor3.setFoo("test3");
-        mockInterceptor3.setExpectedFoo("test3");
+
+        MockInterceptor mockInterceptor4 = new MockInterceptor();
+        interceptorMappings.add(new InterceptorMapping("test4", mockInterceptor4));
+        mockInterceptor4.setFoo("test4");
+        mockInterceptor4.setExpectedFoo("test4");
 
         DefaultActionInvocation defaultActionInvocation = new DefaultActionInvocationTester(interceptorMappings);
         container.inject(defaultActionInvocation);
@@ -78,7 +86,8 @@ public class DefaultActionInvocationTest extends XWorkTestCase {
         defaultActionInvocation.invoke();
         assertTrue(mockInterceptor1.isExecuted());
         assertTrue(mockInterceptor2.isExecuted());
-        assertTrue(mockInterceptor3.isExecuted());
+        assertFalse(mockInterceptor3.isExecuted());
+        assertTrue(mockInterceptor4.isExecuted());
         assertTrue(defaultActionInvocation.isExecuted());
         try {
             defaultActionInvocation.setResultCode("");
@@ -92,6 +101,33 @@ public class DefaultActionInvocationTest extends XWorkTestCase {
         }
     }
 
+    public void testInvokeSimpleInterceptor() throws Exception {
+        List<InterceptorMapping> interceptorMappings = new ArrayList<>();
+        Interceptor interceptor1 = new Interceptor() {
+            @Override
+            public void destroy() {
+            }
+
+            @Override
+            public void init() {
+            }
+
+            @Override
+            public String intercept(ActionInvocation invocation) throws Exception {
+                return "done";
+            }
+        };
+        interceptorMappings.add(new InterceptorMapping("test1", interceptor1));
+
+        DefaultActionInvocation defaultActionInvocation = new DefaultActionInvocationTester(interceptorMappings);
+        container.inject(defaultActionInvocation);
+        defaultActionInvocation.stack = container.getInstance(ValueStackFactory.class).createValueStack();
+
+        String result = defaultActionInvocation.invoke();
+        assertTrue(defaultActionInvocation.isExecuted());
+        assertEquals("done", result);
+    }
+
     public void testInvokeWithDisabledInterceptors() throws Exception {
         // given
         List<InterceptorMapping> interceptorMappings = new ArrayList<>();
@@ -147,7 +183,7 @@ public class DefaultActionInvocationTest extends XWorkTestCase {
         assertEquals("success", result);
     }
 
-    public void testInvokingMissingMethod() throws Exception {
+    public void testInvokingMissingMethod() {
         // given
         DefaultActionInvocation dai = new DefaultActionInvocation(ActionContext.getContext().getContextMap(), false);
         container.inject(dai);
@@ -348,7 +384,7 @@ public class DefaultActionInvocationTest extends XWorkTestCase {
     }
 
     public void testInvokeWithAsyncManager() throws Exception {
-        DefaultActionInvocation dai = new DefaultActionInvocation(new HashMap<String, Object>(), false);
+        DefaultActionInvocation dai = new DefaultActionInvocation(new HashMap<>(), false);
         dai.stack = container.getInstance(ValueStackFactory.class).createValueStack();
 
         final Semaphore lock = new Semaphore(1);
@@ -377,29 +413,14 @@ public class DefaultActionInvocationTest extends XWorkTestCase {
             }
         });
 
-        dai.action = new Callable<Callable<String>>() {
-            @Override
-            public Callable<String> call() throws Exception {
-                return new Callable<String>() {
-                    @Override
-                    public String call() throws Exception {
-                        return "success";
-                    }
-                };
-            }
-        };
+        dai.action = (Callable<Callable<String>>) () -> (Callable<String>) () -> "success";
 
         MockActionProxy actionProxy = new MockActionProxy();
         actionProxy.setMethod("call");
         dai.proxy = actionProxy;
 
         final boolean[] preResultExecuted = new boolean[1];
-        dai.addPreResultListener(new PreResultListener() {
-            @Override
-            public void beforeResult(ActionInvocation invocation, String resultCode) {
-                preResultExecuted[0] = true;
-            }
-        });
+        dai.addPreResultListener((invocation, resultCode) -> preResultExecuted[0] = true);
 
         List<InterceptorMapping> interceptorMappings = new ArrayList<>();
         MockInterceptor mockInterceptor1 = new MockInterceptor();
@@ -436,7 +457,7 @@ public class DefaultActionInvocationTest extends XWorkTestCase {
 
     public void testActionEventListener() throws Exception {
         ActionProxy actionProxy = actionProxyFactory.createActionProxy("",
-            "ExceptionFoo", "exceptionMethod", new HashMap<String, Object>());
+            "ExceptionFoo", "exceptionMethod", new HashMap<>());
         DefaultActionInvocation defaultActionInvocation = (DefaultActionInvocation) actionProxy.getInvocation();
 
         SimpleActionEventListener actionEventListener = new SimpleActionEventListener("prepared", "exceptionHandled");
@@ -461,8 +482,7 @@ public class DefaultActionInvocationTest extends XWorkTestCase {
     }
 
     public void testActionChainResult() throws Exception {
-        ActionProxy actionProxy = actionProxyFactory.createActionProxy("", "Foo", null,
-            new HashMap<String, Object>());
+        ActionProxy actionProxy = actionProxyFactory.createActionProxy("", "Foo", null, new HashMap<>());
         DefaultActionInvocation defaultActionInvocation = (DefaultActionInvocation) actionProxy.getInvocation();
         defaultActionInvocation.init(actionProxy);
 
@@ -478,9 +498,8 @@ public class DefaultActionInvocationTest extends XWorkTestCase {
         assertTrue(result instanceof MockResult);
     }
 
-    public void testNoResultDefined() throws Exception {
-        ActionProxy actionProxy = actionProxyFactory.createActionProxy("", "Foo", null,
-            new HashMap<String, Object>());
+    public void testNoResultDefined() {
+        ActionProxy actionProxy = actionProxyFactory.createActionProxy("", "Foo", null, new HashMap<>());
         DefaultActionInvocation defaultActionInvocation = (DefaultActionInvocation) actionProxy.getInvocation();
         defaultActionInvocation.init(actionProxy);
 
@@ -492,8 +511,7 @@ public class DefaultActionInvocationTest extends XWorkTestCase {
     }
 
     public void testNullResultPossible() throws Exception {
-        ActionProxy actionProxy = actionProxyFactory.createActionProxy("",
-            "NullFoo", "nullMethod", new HashMap<String, Object>());
+        ActionProxy actionProxy = actionProxyFactory.createActionProxy("", "NullFoo", "nullMethod", new HashMap<>());
         DefaultActionInvocation defaultActionInvocation = (DefaultActionInvocation) actionProxy.getInvocation();
         defaultActionInvocation.init(actionProxy);
 

core/src/test/java/org/apache/struts2/interceptor/CoepInterceptorTest.java

@@ -41,15 +41,6 @@ public class CoepInterceptorTest extends StrutsInternalTestCase {
     private final String HEADER_CONTENT = "require-corp";
 
 
-    public void testDisabled() throws Exception {
-        interceptor.setDisabled("true");
-
-        interceptor.intercept(mai);
-
-        String header = response.getHeader(COEP_ENFORCING_HEADER);
-        assertTrue("COEP is not disabled", Strings.isEmpty(header));
-    }
-
     public void testEnforcingHeader() throws Exception {
         interceptor.setEnforcingMode("true");
 

core/src/test/java/org/apache/struts2/interceptor/CoopInterceptorTest.java

@@ -75,15 +75,6 @@ public class CoopInterceptorTest extends StrutsInternalTestCase {
         }
     }
 
-    public void testDisabled() throws Exception {
-        interceptor.setDisabled("true");
-
-        interceptor.intercept(mai);
-
-        String header = response.getHeader(COOP_HEADER);
-        assertTrue("COOP is not disabled", Strings.isEmpty(header));
-    }
-
     @Override
     protected void setUp() throws Exception {
         super.setUp();

core/src/test/java/org/apache/struts2/interceptor/CspInterceptorTest.java

@@ -145,17 +145,6 @@ public class CspInterceptorTest extends StrutsInternalTestCase {
         }
     }
 
-    public void testDisabled() throws Exception {
-        interceptor.setDisabled("true");
-
-        interceptor.intercept(mai);
-
-        String header = response.getHeader(CSP_ENFORCE_HEADER);
-        assertTrue("CSP is not disabled", Strings.isEmpty(header));
-        header = response.getHeader(CSP_REPORT_HEADER);
-        assertTrue("CSP is not disabled", Strings.isEmpty(header));
-    }
-
     public void checkHeader(String reportUri, String enforcingMode) {
         String expectedCspHeader;
         if (Strings.isEmpty(reportUri)) {

core/src/test/java/org/apache/struts2/interceptor/FetchMetadataInterceptorTest.java

@@ -261,12 +261,4 @@ public class FetchMetadataInterceptorTest extends XWorkTestCase {
         assertNotEquals("Expected interceptor to accept this request [" + "/" + fetchMetadataExemptedGlobalActionConfig.getName() + "]", SC_FORBIDDEN, configuredFetchMetadataInterceptor.intercept(mai));
     }
 
-    public void testDisabled() throws Exception {
-        interceptor.setDisabled("true");
-
-        interceptor.intercept(mai);
-
-        String header = response.getHeader(VARY_HEADER);
-        assertTrue("Fetch Metadata is not disabled", Strings.isEmpty(header));
-    }
 }