WW-5382 Fix stale injections in Dispatcher #826

kusalk · 2023-12-30T13:37:10Z

WW-5382

I understand most applications do not reload configuration after startup - but for applications that do, this is a critical bug as it means stale configuration persists after a reload. This extends to the SecurityMemberAccess policy which is now container injected.

kusalk · 2023-12-30T13:41:02Z

Need to test this further in a staging environment and will improve test coverage to prevent similar regressions from reoccurring

kusalk · 2024-01-01T16:49:22Z

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

@@ -51,10 +51,6 @@ public void setUp() throws Exception {
    @After
    public void tearDown() throws Exception {
        XWorkTestCaseHelper.tearDown(configurationManager);
-        configurationManager = null;


Not necessary

kusalk · 2024-01-01T16:52:28Z

core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java

@@ -325,12 +325,8 @@ public Class<? extends Configuration> type() {
    }

    protected ActionContext setContext(Container cont) {
-        ActionContext context = ActionContext.getContext();
-        if (context == null) {


This method is only called twice, and it is always null on the first call, and never null on the second call. I can't see why we wouldn't want to replace the bootstrap context once the final context has been constructed. Although it is also unclear if the ActionContext is even used after this point - i.e. for loading the PackageProviders. The ActionContext is cleared once the PackageProviders are loaded.

To be honest I would love to re-organise the whole proces. I assume the ActionContext is only needed when serving an action, it should be created just before and destroyed just after.

Yeah that would be ideal - if that were the case we'd be able to remove the need for the bootstrap context too it looks like

kusalk · 2024-01-01T16:59:06Z

core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java

@@ -192,6 +194,11 @@ public class Dispatcher {
     * Store ConfigurationManager instance, set on init.
     */
    protected ConfigurationManager configurationManager;
+    private ObjectFactory objectFactory;


We should save some computation by injecting these beans once when the Dispatcher is initialised instead of constantly retrieving them from the container

Maybe they should be moved into constructor?

Since the Dispatcher itself is not container-managed, we can't use constructor injection for it

It doesn't have to be, just using container.inject(Dispatcher.class) will create a new instance and injects all the dependencies. Anyway this would be a bit out of scope for this PR.

Yep would require a major refactor, I've got some ideas but not a high priority right now, as long as everything works

kusalk · 2024-01-01T17:00:52Z

core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java

@@ -988,18 +1031,7 @@ protected boolean isMultipartRequest(HttpServletRequest request) {
     * @return a multi part request object
     */
    protected MultiPartRequest getMultiPartRequest() {
-        MultiPartRequest mpr = null;


Remnants from before the bean aliasing functionality existed I presume?

Hm... good question, no idea :)

kusalk · 2024-01-01T17:02:44Z

core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java

            }
        }
+        if (injectedContainer != ContainerHolder.get()) {


This is the core change of this PR - we compare the retrieved container instance against the one we last injected this with, and if it has changed, reinject

kusalk · 2024-01-01T17:05:25Z

core/src/main/java/org/apache/struts2/util/StrutsTestCaseHelper.java

-    public static void tearDown() throws Exception {
-        Dispatcher.setInstance(null);
+    public static void tearDown(Dispatcher dispatcher) {
+        if (dispatcher != null && dispatcher.getConfigurationManager() != null) {


Moved this here for better reuse

kusalk · 2024-01-01T17:06:43Z

core/src/test/java/org/apache/struts2/dispatcher/DispatcherTest.java

 /**
 * Test case for Dispatcher.
 */
-public class DispatcherTest extends StrutsInternalTestCase {
+public class DispatcherTest extends StrutsJUnit4InternalTestCase {


Upgraded to JUnit4 and replaced older mocks with Mockito

kusalk · 2024-01-01T17:07:10Z

core/src/test/java/org/apache/struts2/dispatcher/DispatcherTest.java

+    }
+
+    @Test
+    public void dispatcherReinjectedAfterReload() {


New test for reinjection functionality

kusalk · 2024-01-01T17:08:10Z

plugins/portlet/src/main/java/org/apache/struts2/portlet/dispatcher/Jsr168Dispatcher.java

(Changed line-endings to LF for this file)

kusalk · 2024-01-01T17:08:41Z

plugins/sitemesh/src/main/java/org/apache/struts2/sitemesh/OldDecorator2NewStrutsDecorator.java

Same deal with line-endings

sonarcloud · 2024-01-02T07:00:02Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

8 New issues
0 Security Hotspots
87.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

kusalk · 2024-01-02T07:27:58Z

I've now tested this fix with the Confluence DC test suite and did some additional manual sanity testing and all seems to be as expected now. Following a ConfigurationManager#reload, all constants, including the exclusion list and allowlist, are now refreshed as per the latest ConfigurationProviders.

lukaszlenart

Looks ok, left a few comments

lukaszlenart · 2024-01-05T06:24:39Z

core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java

@@ -325,12 +325,8 @@ public Class<? extends Configuration> type() {
    }

    protected ActionContext setContext(Container cont) {
-        ActionContext context = ActionContext.getContext();
-        if (context == null) {


To be honest I would love to re-organise the whole proces. I assume the ActionContext is only needed when serving an action, it should be created just before and destroyed just after.

lukaszlenart · 2024-01-05T06:27:09Z

core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java

@@ -192,6 +194,11 @@ public class Dispatcher {
     * Store ConfigurationManager instance, set on init.
     */
    protected ConfigurationManager configurationManager;
+    private ObjectFactory objectFactory;


Maybe they should be moved into constructor?

lukaszlenart · 2024-01-05T06:29:08Z

core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java

@@ -988,18 +1031,7 @@ protected boolean isMultipartRequest(HttpServletRequest request) {
     * @return a multi part request object
     */
    protected MultiPartRequest getMultiPartRequest() {
-        MultiPartRequest mpr = null;


Hm... good question, no idea :)

core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java

kusalk added 3 commits December 31, 2023 00:33

WW-5382 Fix StrutsInternalTestCase

32584c3

WW-5382 Fix stale injections in Dispatcher

15acd72

WW-5382 Fix stale bootstrap context on ActionContext

2024d83

kusalk force-pushed the WW-5382-stale-config branch from bda8f01 to b9e3c5e Compare January 1, 2024 15:28

kusalk added 2 commits January 2, 2024 02:56

WW-5382 Rework existing Dispatcher tests and base test classes

6f1e122

WW-5382 Add test for Dispatcher reinjection

fa5b46c

kusalk force-pushed the WW-5382-stale-config branch from b9e3c5e to fa5b46c Compare January 1, 2024 15:57

WW-5382 Delete redundant code

ae71c46

kusalk force-pushed the WW-5382-stale-config branch from fbd32f3 to 2c5087e Compare January 1, 2024 16:35

WW-5382 Rework Dispatcher injections

946737c

kusalk force-pushed the WW-5382-stale-config branch from 2c5087e to 946737c Compare January 1, 2024 16:57

kusalk commented Jan 1, 2024

View reviewed changes

WW-5382 Update Dispatcher#getContainer JavaDoc

3d25caa

kusalk marked this pull request as ready for review January 2, 2024 07:28

kusalk requested a review from lukaszlenart January 2, 2024 07:34

lukaszlenart reviewed Jan 5, 2024

View reviewed changes

lukaszlenart approved these changes Jan 6, 2024

View reviewed changes

kusalk merged commit 3b07899 into master Jan 6, 2024
9 checks passed

kusalk deleted the WW-5382-stale-config branch January 6, 2024 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WW-5382 Fix stale injections in Dispatcher #826

WW-5382 Fix stale injections in Dispatcher #826

kusalk commented Dec 30, 2023

kusalk commented Dec 30, 2023

kusalk Jan 1, 2024

kusalk Jan 1, 2024 •

edited

Loading

lukaszlenart Jan 5, 2024

kusalk Jan 5, 2024

kusalk Jan 1, 2024

lukaszlenart Jan 5, 2024

kusalk Jan 5, 2024

lukaszlenart Jan 6, 2024

kusalk Jan 6, 2024

kusalk Jan 1, 2024

lukaszlenart Jan 5, 2024

kusalk Jan 1, 2024

kusalk Jan 1, 2024

kusalk Jan 1, 2024

kusalk Jan 1, 2024

kusalk Jan 1, 2024

kusalk Jan 1, 2024

sonarcloud bot commented Jan 2, 2024

kusalk commented Jan 2, 2024

lukaszlenart left a comment

lukaszlenart Jan 5, 2024

lukaszlenart Jan 5, 2024

lukaszlenart Jan 5, 2024

WW-5382 Fix stale injections in Dispatcher #826

WW-5382 Fix stale injections in Dispatcher #826

Conversation

kusalk commented Dec 30, 2023

kusalk commented Dec 30, 2023

Choose a reason for hiding this comment

kusalk Jan 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 2, 2024

Quality Gate passed

kusalk commented Jan 2, 2024

lukaszlenart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kusalk Jan 1, 2024 •

edited

Loading