diff --git a/CHANGES.md b/CHANGES.md index d6988febd2..64db62dbbe 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,6 +4,13 @@ ### Behaviour changes worth reading before upgrading +- **Planet is off by default.** `planet.aggregator.enabled` now defaults to + `false`. While it is off, the Planet admin pages, `/planetrss` and the + Planet background tasks (`RefreshRollerPlanetTask`, `SyncWebsitesTask`) do + nothing. A site that uses Planet must set `planet.aggregator.enabled=true` + in `roller-custom.properties` before upgrading. +- **Planet admin changes require POST.** Saving or deleting Planet groups and + subscriptions is refused unless the request is a POST from the admin form. - **XML parsing uses Apache Commons Secure XML.** Roller now bundles `commons-secure-xml` 1.0.0 and builds all of its XML parsers through it. - **Startup fails if the XML-RPC parser cannot be configured.** Roller used to diff --git a/app/src/main/java/org/apache/roller/weblogger/planet/tasks/RefreshRollerPlanetTask.java b/app/src/main/java/org/apache/roller/weblogger/planet/tasks/RefreshRollerPlanetTask.java index 367a8958b2..9babf25c6e 100644 --- a/app/src/main/java/org/apache/roller/weblogger/planet/tasks/RefreshRollerPlanetTask.java +++ b/app/src/main/java/org/apache/roller/weblogger/planet/tasks/RefreshRollerPlanetTask.java @@ -131,6 +131,10 @@ public void init(String name) throws WebloggerException { @Override public void runTask() { + if (!WebloggerConfig.getBooleanProperty("planet.aggregator.enabled")) { + log.debug("Planet is disabled; not running " + getName()); + return; + } try { log.info("Refreshing Planet subscriptions"); diff --git a/app/src/main/java/org/apache/roller/weblogger/planet/tasks/SyncWebsitesTask.java b/app/src/main/java/org/apache/roller/weblogger/planet/tasks/SyncWebsitesTask.java index 95675b2095..80f4b23789 100644 --- a/app/src/main/java/org/apache/roller/weblogger/planet/tasks/SyncWebsitesTask.java +++ b/app/src/main/java/org/apache/roller/weblogger/planet/tasks/SyncWebsitesTask.java @@ -141,6 +141,10 @@ public void init(String name) throws WebloggerException { */ @Override public void runTask() { + if (!WebloggerConfig.getBooleanProperty("planet.aggregator.enabled")) { + log.debug("Planet is disabled; not running " + getName()); + return; + } log.info("Syncing local weblogs with planet subscriptions list"); diff --git a/app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetConfig.java b/app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetConfig.java index 8d24508f5e..ed331ed3c4 100644 --- a/app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetConfig.java +++ b/app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetConfig.java @@ -108,6 +108,9 @@ public String execute() { public String save() { + if (!isPostRequest()) { + return DENIED; + } try { String incomingProp = null; diff --git a/app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetGroupSubs.java b/app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetGroupSubs.java index 7d6ed549e3..b4959f5471 100644 --- a/app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetGroupSubs.java +++ b/app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetGroupSubs.java @@ -114,6 +114,9 @@ public String execute() { * Save group. */ public String saveGroup() { + if (!isPostRequest()) { + return DENIED; + } validateGroup(); @@ -171,6 +174,9 @@ private void validateGroup() { * Save subscription, add to current group */ public String saveSubscription() { + if (!isPostRequest()) { + return DENIED; + } valudateNewSub(); @@ -223,6 +229,9 @@ public String saveSubscription() { * Delete subscription, reset form */ public String deleteSubscription() { + if (!isPostRequest()) { + return DENIED; + } if (getSubUrl() != null) { try { diff --git a/app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetGroups.java b/app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetGroups.java index cea9025766..45ce5e58b4 100644 --- a/app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetGroups.java +++ b/app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetGroups.java @@ -67,6 +67,9 @@ public String execute() { * Delete group */ public String delete() { + if (!isPostRequest()) { + return DENIED; + } if (getGroup() != null) { try { diff --git a/app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetUIAction.java b/app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetUIAction.java index 2fe3232b7b..c8976e8214 100644 --- a/app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetUIAction.java +++ b/app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetUIAction.java @@ -20,8 +20,11 @@ import org.apache.commons.logging.LogFactory; import org.apache.roller.planet.business.PlanetManager; import org.apache.roller.planet.pojos.Planet; +import javax.servlet.http.HttpServletRequest; import org.apache.roller.weblogger.business.WebloggerFactory; +import org.apache.roller.weblogger.config.WebloggerConfig; import org.apache.roller.weblogger.ui.struts2.util.UIAction; +import org.apache.struts2.ServletActionContext; /** @@ -37,6 +40,23 @@ public abstract class PlanetUIAction extends UIAction { private Planet planet = null; + /** + * Planet actions are only available while the Planet aggregator is enabled. + */ + @Override + public boolean isFeatureEnabled() { + return WebloggerConfig.getBooleanProperty("planet.aggregator.enabled"); + } + + /** + * Planet changes are made only by POST, which the CSRF salt filter + * checks. Methods that change Planet state return DENIED otherwise. + */ + protected boolean isPostRequest() { + HttpServletRequest req = ServletActionContext.getRequest(); + return req != null && "POST".equalsIgnoreCase(req.getMethod()); + } + public Planet getPlanet() { if(planet == null) { try { diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/rendering/servlets/PlanetFeedServlet.java b/app/src/main/java/org/apache/roller/weblogger/ui/rendering/servlets/PlanetFeedServlet.java index 432a0c33a1..af0558e5f5 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/rendering/servlets/PlanetFeedServlet.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/rendering/servlets/PlanetFeedServlet.java @@ -32,6 +32,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.roller.util.RollerConstants; +import org.apache.roller.weblogger.config.WebloggerConfig; import org.apache.roller.weblogger.config.WebloggerRuntimeConfig; import org.apache.roller.planet.business.PlanetManager; import org.apache.roller.planet.config.PlanetRuntimeConfig; @@ -79,6 +80,11 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) log.debug("Entering"); + if (!WebloggerConfig.getBooleanProperty("planet.aggregator.enabled")) { + response.sendError(HttpServletResponse.SC_NOT_FOUND); + return; + } + PlanetManager planet = WebloggerFactory.getWeblogger() .getPlanetManager(); diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/UISecurityEnforced.java b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/UISecurityEnforced.java index f40f45b5ec..b51fdf5694 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/UISecurityEnforced.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/UISecurityEnforced.java @@ -62,4 +62,13 @@ public interface UISecurityEnforced { * List of weblog permissions required to access action if applicable. */ List requiredGlobalPermissionActions(); + + /** + * Whether the feature this action belongs to is turned on. Actions for + * optional features override this; while it returns false the action is + * refused. + */ + default boolean isFeatureEnabled() { + return true; + } } diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/UISecurityInterceptor.java b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/UISecurityInterceptor.java index 8e97899b82..2e2ce00a70 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/UISecurityInterceptor.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/UISecurityInterceptor.java @@ -56,6 +56,14 @@ public String doIntercept(ActionInvocation invocation) throws Exception { final UISecurityEnforced theAction = (UISecurityEnforced) action; + // is the feature this action belongs to turned on? + if (!theAction.isFeatureEnabled()) { + if (log.isDebugEnabled()) { + log.debug("DENIED: feature is disabled"); + } + return UIAction.DENIED; + } + // are we requiring an authenticated user? if (theAction.isUserRequired()) { diff --git a/app/src/main/resources/org/apache/roller/weblogger/config/roller.properties b/app/src/main/resources/org/apache/roller/weblogger/config/roller.properties index cdb7d5252a..6a2baebc3e 100644 --- a/app/src/main/resources/org/apache/roller/weblogger/config/roller.properties +++ b/app/src/main/resources/org/apache/roller/weblogger/config/roller.properties @@ -198,9 +198,11 @@ site.bannedwordslist.enable.referrers=false #---------------------------------- # Planet Aggregator settings -# Set to true to enable the Planet aggregator. You also need to enable the -# RefreshRollerPlanetTask task below to get the feed fetcher running. -planet.aggregator.enabled=true +# Set to true to enable the Planet aggregator. It is off by default. While it +# is off, the Planet admin pages and the /planetrss feed are unavailable. You +# also need to enable the RefreshRollerPlanetTask task below to get the feed +# fetcher running. +planet.aggregator.enabled=false # Planet backend guice module, customized for use with Weblogger planet.aggregator.guice.module=\ diff --git a/app/src/test/java/org/apache/roller/weblogger/business/PlanetManagerLocalTest.java b/app/src/test/java/org/apache/roller/weblogger/business/PlanetManagerLocalTest.java index 846849bb7e..1a46e326d4 100644 --- a/app/src/test/java/org/apache/roller/weblogger/business/PlanetManagerLocalTest.java +++ b/app/src/test/java/org/apache/roller/weblogger/business/PlanetManagerLocalTest.java @@ -23,6 +23,7 @@ import org.apache.roller.planet.pojos.Subscription; import org.apache.roller.planet.pojos.SubscriptionEntry; import org.apache.roller.weblogger.TestUtils; +import org.apache.roller.weblogger.config.WebloggerConfig; import org.apache.roller.weblogger.planet.tasks.RefreshRollerPlanetTask; import org.apache.roller.weblogger.planet.tasks.SyncWebsitesTask; import org.apache.roller.weblogger.pojos.User; @@ -32,6 +33,7 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; import java.sql.Timestamp; import java.util.Date; @@ -40,6 +42,8 @@ import static org.junit.jupiter.api.Assertions.fail; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.mockito.Mockito.CALLS_REAL_METHODS; +import static org.mockito.Mockito.mockStatic; /** @@ -138,7 +142,11 @@ public void tearDown() throws Exception { @Test public void testRefreshEntries() { - try { + // Planet is off by default, and its tasks do nothing while it is off. + try (MockedStatic config = + mockStatic(WebloggerConfig.class, CALLS_REAL_METHODS)) { + config.when(() -> WebloggerConfig.getBooleanProperty("planet.aggregator.enabled")) + .thenReturn(true); PlanetManager planet = WebloggerFactory.getWeblogger().getPlanetManager(); // run sync task to fill aggregator with websites created by super diff --git a/app/src/test/java/org/apache/roller/weblogger/planet/ui/PlanetAvailabilityTest.java b/app/src/test/java/org/apache/roller/weblogger/planet/ui/PlanetAvailabilityTest.java new file mode 100644 index 0000000000..5604b7462b --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/planet/ui/PlanetAvailabilityTest.java @@ -0,0 +1,156 @@ +/* + * 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. For additional information regarding + * copyright in this work, please see the NOTICE file in the top level + * directory of this distribution. + */ + +package org.apache.roller.weblogger.planet.ui; + +import java.io.InputStream; +import java.util.HashMap; +import java.util.Map; +import java.util.Properties; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import com.opensymphony.xwork2.ActionContext; +import com.opensymphony.xwork2.ActionInvocation; +import org.apache.roller.weblogger.business.WebloggerFactory; +import org.apache.roller.weblogger.config.WebloggerConfig; +import org.apache.roller.weblogger.planet.tasks.RefreshRollerPlanetTask; +import org.apache.roller.weblogger.planet.tasks.SyncWebsitesTask; +import org.apache.roller.weblogger.ui.rendering.servlets.PlanetFeedServlet; +import org.apache.roller.weblogger.ui.struts2.util.UIAction; +import org.apache.roller.weblogger.ui.struts2.util.UISecurityInterceptor; +import org.apache.struts2.StrutsStatics; +import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +class PlanetAvailabilityTest { + + private static final String ENABLED = "planet.aggregator.enabled"; + + @Test + void planetIsOffByDefault() throws Exception { + Properties defaults = new Properties(); + try (InputStream in = WebloggerConfig.class.getResourceAsStream( + "/org/apache/roller/weblogger/config/roller.properties")) { + defaults.load(in); + } + assertEquals("false", defaults.getProperty(ENABLED)); + } + + @Test + void planetActionsFollowTheSetting() { + PlanetUIAction action = new PlanetGroups(); + try (MockedStatic config = mockStatic(WebloggerConfig.class)) { + config.when(() -> WebloggerConfig.getBooleanProperty(ENABLED)).thenReturn(false); + assertFalse(action.isFeatureEnabled()); + + config.when(() -> WebloggerConfig.getBooleanProperty(ENABLED)).thenReturn(true); + assertTrue(action.isFeatureEnabled()); + } + } + + @Test + void disabledFeatureIsRefusedBeforeAnythingElse() throws Exception { + UIAction action = new UIAction() { + @Override + public boolean isFeatureEnabled() { + return false; + } + }; + ActionInvocation invocation = mock(ActionInvocation.class); + when(invocation.getAction()).thenReturn(action); + + assertEquals(UIAction.DENIED, new UISecurityInterceptor().doIntercept(invocation)); + verify(invocation, never()).invoke(); + } + + @Test + void planetChangesAreRefusedUnlessPosted() { + HttpServletRequest get = mock(HttpServletRequest.class); + when(get.getMethod()).thenReturn("GET"); + Map context = new HashMap<>(); + context.put(StrutsStatics.HTTP_REQUEST, get); + ActionContext.setContext(new ActionContext(context)); + try (MockedStatic factory = mockStatic(WebloggerFactory.class)) { + assertEquals(UIAction.DENIED, new PlanetConfig().save()); + assertEquals(UIAction.DENIED, new PlanetGroups().delete()); + assertEquals(UIAction.DENIED, new PlanetGroupSubs().saveGroup()); + assertEquals(UIAction.DENIED, new PlanetGroupSubs().saveSubscription()); + assertEquals(UIAction.DENIED, new PlanetGroupSubs().deleteSubscription()); + factory.verifyNoInteractions(); + } finally { + ActionContext.setContext(null); + } + } + + @Test + void onlyAPostCountsAsAPostRequest() { + PlanetUIAction action = new PlanetGroups(); + try { + ActionContext.setContext(new ActionContext(new HashMap<>())); + assertFalse(action.isPostRequest()); + + HttpServletRequest post = mock(HttpServletRequest.class); + when(post.getMethod()).thenReturn("post"); + Map context = new HashMap<>(); + context.put(StrutsStatics.HTTP_REQUEST, post); + ActionContext.setContext(new ActionContext(context)); + assertTrue(action.isPostRequest()); + } finally { + ActionContext.setContext(null); + } + } + + @Test + void planetFeedIsNotServedWhileOff() throws Exception { + HttpServletRequest request = mock(HttpServletRequest.class); + HttpServletResponse response = mock(HttpServletResponse.class); + try (MockedStatic config = mockStatic(WebloggerConfig.class); + MockedStatic factory = mockStatic(WebloggerFactory.class)) { + config.when(() -> WebloggerConfig.getBooleanProperty(ENABLED)).thenReturn(false); + + new PlanetFeedServlet().doGet(request, response); + + verify(response).sendError(HttpServletResponse.SC_NOT_FOUND); + factory.verifyNoInteractions(); + } + } + + @Test + void planetTasksDoNothingWhileOff() { + try (MockedStatic config = mockStatic(WebloggerConfig.class); + MockedStatic factory = mockStatic(WebloggerFactory.class)) { + config.when(() -> WebloggerConfig.getBooleanProperty(ENABLED)).thenReturn(false); + + new RefreshRollerPlanetTask().runTask(); + new SyncWebsitesTask().runTask(); + + factory.verifyNoInteractions(); + } + } +}