From caac9f25ae329cfa7f07633588c46be6c73ff0cb Mon Sep 17 00:00:00 2001 From: "David M. Johnson" Date: Sat, 29 Aug 2026 10:21:24 -0400 Subject: [PATCH 1/2] Move frontpage selection into the administrator setup workflow Setup is a read-only bootstrap page. Choosing the initial frontpage weblog is a separate global-administrator action reached by POST (FrontpageSetup); later changes go through global configuration, which resolves the handle through the shared FrontpageSettings service. The service stores the weblog's canonical handle, treats a missing aggregation checkbox as false, writes both properties before a single flush, and clears the site-wide, page and feed caches so the change is visible on this node. save() rejects non-POST requests. Peers pick the change up when their own cache entries expire; Roller has no cross-node invalidation transport and this does not add one. Claude-Session: https://claude.ai/code/session_01A1fhY1E2PCFU6UAPXu2WtV --- .../weblogger/business/FrontpageSettings.java | 139 ++++++++++++++++ .../ui/struts2/admin/GlobalConfig.java | 17 ++ .../ui/struts2/core/FrontpageSetup.java | 122 ++++++++++++++ .../weblogger/ui/struts2/core/Setup.java | 98 ++++++------ .../resources/ApplicationResources.properties | 2 + app/src/main/resources/struts.xml | 9 +- .../main/webapp/WEB-INF/jsps/core/Setup.jsp | 5 +- .../core/FrontpageSetupAccessTest.java | 150 ++++++++++++++++++ 8 files changed, 491 insertions(+), 51 deletions(-) create mode 100644 app/src/main/java/org/apache/roller/weblogger/business/FrontpageSettings.java create mode 100644 app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/FrontpageSetup.java create mode 100644 app/src/test/java/org/apache/roller/weblogger/ui/struts2/core/FrontpageSetupAccessTest.java diff --git a/app/src/main/java/org/apache/roller/weblogger/business/FrontpageSettings.java b/app/src/main/java/org/apache/roller/weblogger/business/FrontpageSettings.java new file mode 100644 index 0000000000..18e5bdf2c6 --- /dev/null +++ b/app/src/main/java/org/apache/roller/weblogger/business/FrontpageSettings.java @@ -0,0 +1,139 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. 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.business; + +import org.apache.commons.lang3.StringUtils; +import org.apache.roller.weblogger.WebloggerException; +import org.apache.roller.weblogger.pojos.RuntimeConfigProperty; +import org.apache.roller.weblogger.pojos.Weblog; +import org.apache.roller.weblogger.ui.rendering.util.cache.SiteWideCache; +import org.apache.roller.weblogger.ui.rendering.util.cache.WeblogFeedCache; +import org.apache.roller.weblogger.ui.rendering.util.cache.WeblogPageCache; + +/** + * Reads and writes the site frontpage weblog settings. + * + *

Two screens change these values: the one-time setup screen used to choose + * a frontpage while the site is being installed, and the global configuration + * screen used afterwards. Both go through here so that the handle is resolved + * and validated the same way, both properties move together, and the rendered + * page and feed caches are invalidated consistently. + */ +public final class FrontpageSettings { + + public static final String HANDLE_PROPERTY = "site.frontpage.weblog.handle"; + public static final String AGGREGATED_PROPERTY = "site.frontpage.weblog.aggregated"; + + private FrontpageSettings() { + } + + /** + * Resolves a submitted handle to a weblog that actually exists and is + * enabled. + * + * @return the weblog, or null when the handle is blank, unknown or refers + * to a disabled weblog + */ + public static Weblog resolveWeblog(String handle) throws WebloggerException { + if (StringUtils.isBlank(handle)) { + return null; + } + return WebloggerFactory.getWeblogger().getWeblogManager() + .getWeblogByHandle(handle.trim(), Boolean.TRUE); + } + + /** @return the configured frontpage handle, or null when none is set. */ + public static String getConfiguredHandle() throws WebloggerException { + RuntimeConfigProperty prop = WebloggerFactory.getWeblogger() + .getPropertiesManager().getProperty(HANDLE_PROPERTY); + if (prop == null || StringUtils.isBlank(prop.getValue())) { + return null; + } + return prop.getValue(); + } + + /** @return true when a frontpage weblog has already been chosen. */ + public static boolean isConfigured() throws WebloggerException { + return getConfiguredHandle() != null; + } + + /** + * Validates and stores the frontpage selection. + * + *

Both properties are written before the single flush so the pair cannot + * be left half-applied, and the canonical handle from the resolved weblog is + * stored rather than the submitted text. A missing aggregation value is + * treated as false, which is what an unchecked checkbox means. + * + * @param handle submitted weblog handle + * @param aggregated submitted aggregation flag; null means false + * @throws InvalidFrontpageWeblogException when the handle does not name an + * existing, enabled weblog + */ + public static void apply(String handle, Boolean aggregated) + throws WebloggerException { + + Weblog weblog = resolveWeblog(handle); + if (weblog == null) { + throw new InvalidFrontpageWeblogException(handle); + } + + PropertiesManager mgr = WebloggerFactory.getWeblogger().getPropertiesManager(); + + RuntimeConfigProperty handleProp = mgr.getProperty(HANDLE_PROPERTY); + handleProp.setValue(weblog.getHandle()); + mgr.saveProperty(handleProp); + + RuntimeConfigProperty aggregatedProp = mgr.getProperty(AGGREGATED_PROPERTY); + aggregatedProp.setValue(Boolean.toString(Boolean.TRUE.equals(aggregated))); + mgr.saveProperty(aggregatedProp); + + WebloggerFactory.getWeblogger().flush(); + + invalidateRenderedContent(); + } + + /** + * Drops the locally cached rendering of the front page. + * + *

The properties themselves are read through the properties manager on + * each request, but rendered pages and feeds are cached separately and would + * otherwise keep serving the previous weblog. Roller has no cross-node + * invalidation transport, so peers pick the change up when their own cache + * entries expire. + */ + private static void invalidateRenderedContent() { + SiteWideCache.getInstance().clear(); + WeblogPageCache.getInstance().clear(); + WeblogFeedCache.getInstance().clear(); + } + + /** Raised when a submitted frontpage handle cannot be used. */ + public static class InvalidFrontpageWeblogException extends WebloggerException { + private final String handle; + + public InvalidFrontpageWeblogException(String handle) { + super("Not an existing, enabled weblog handle: " + handle); + this.handle = handle; + } + + public String getHandle() { + return handle; + } + } +} diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/admin/GlobalConfig.java b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/admin/GlobalConfig.java index 194337886c..b757615330 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/admin/GlobalConfig.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/admin/GlobalConfig.java @@ -25,6 +25,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.roller.weblogger.WebloggerException; +import org.apache.roller.weblogger.business.FrontpageSettings; import org.apache.roller.weblogger.business.PropertiesManager; import org.apache.roller.weblogger.business.WeblogManager; import org.apache.roller.weblogger.business.WebloggerFactory; @@ -209,6 +210,22 @@ public String save() { Arrays.asList(propDesc, propName)); } + } else if ( FrontpageSettings.HANDLE_PROPERTY.equals(propertyDef.getName()) + && incomingProp != null ) { + // Declared as a plain string, but it names a weblog, so it + // is resolved through the same service as the setup path. The + // stored value is always a weblog that exists and is enabled. + try { + if (FrontpageSettings.resolveWeblog(incomingProp) == null) { + addError("frontpageConfig.invalidWeblog"); + } else { + updProp.setValue( incomingProp.trim() ); + } + } catch (WebloggerException ex) { + log.error("Error resolving frontpage weblog", ex); + addError("frontpageConfig.values.error"); + } + } else if ( incomingProp != null ){ updProp.setValue( incomingProp.trim() ); log.debug("Set something " + propName + " = " + incomingProp); diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/FrontpageSetup.java b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/FrontpageSetup.java new file mode 100644 index 0000000000..23a7585ac0 --- /dev/null +++ b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/FrontpageSetup.java @@ -0,0 +1,122 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. 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.ui.struts2.core; + +import java.util.Collections; +import java.util.List; + +import javax.servlet.http.HttpServletRequest; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.roller.weblogger.WebloggerException; +import org.apache.roller.weblogger.business.FrontpageSettings; +import org.apache.roller.weblogger.pojos.GlobalPermission; +import org.apache.roller.weblogger.ui.struts2.util.UIAction; +import org.apache.struts2.ServletActionContext; + +/** + * Chooses the site frontpage weblog for the first time. + * + *

This exists separately from {@link Setup} because the bootstrap page is + * reachable without a login while the site has no users. Here the caller must + * hold the global administrator permission, which the first registered user + * receives by default. + * + *

The action applies only to the initial choice. Once a frontpage weblog is + * set, later changes go through the global configuration screen, which is + * already administrator-only. + */ +public class FrontpageSetup extends UIAction { + + private static final Log LOG = LogFactory.getLog(FrontpageSetup.class); + + private String frontpageBlog; + private Boolean aggregated; + + public FrontpageSetup() { + this.pageTitle = "index.heading"; + } + + @Override + public boolean isWeblogRequired() { + return false; + } + + @Override + public List requiredGlobalPermissionActions() { + return Collections.singletonList(GlobalPermission.ADMIN); + } + + /** + * Stores the initial frontpage selection. + * + *

Reached only by POST, so the CSRF salt filter covers it, and only while + * no frontpage weblog has been chosen. + */ + public String save() { + + HttpServletRequest req = ServletActionContext.getRequest(); + if (!"POST".equalsIgnoreCase(req.getMethod())) { + return DENIED; + } + + try { + // Re-read immediately before writing so that a second submission + // arriving alongside the first cannot replace the winner. This + // narrows the window rather than closing it outright; the two + // submissions would have to interleave within this method, and the + // losing caller is told the choice is already made. + if (FrontpageSettings.isConfigured()) { + addError("frontpageConfig.alreadyConfigured"); + return "home"; + } + + FrontpageSettings.apply(frontpageBlog, aggregated); + addMessage("frontpageConfig.values.saved"); + + } catch (FrontpageSettings.InvalidFrontpageWeblogException ex) { + addError("frontpageConfig.invalidWeblog"); + return INPUT; + + } catch (WebloggerException ex) { + LOG.error("ERROR saving frontpage configuration", ex); + addError("frontpageConfig.values.error"); + return INPUT; + } + + return "home"; + } + + public String getFrontpageBlog() { + return frontpageBlog; + } + + public void setFrontpageBlog(String frontpageBlog) { + this.frontpageBlog = frontpageBlog; + } + + public Boolean getAggregated() { + return aggregated; + } + + public void setAggregated(Boolean aggregated) { + this.aggregated = aggregated; + } +} diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/Setup.java b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/Setup.java index 00ab7e19c7..d224b65b57 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/Setup.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/Setup.java @@ -22,19 +22,25 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.roller.weblogger.WebloggerException; -import org.apache.roller.weblogger.business.PropertiesManager; +import org.apache.roller.weblogger.business.FrontpageSettings; import org.apache.roller.weblogger.business.WeblogManager; import org.apache.roller.weblogger.business.WebloggerFactory; -import org.apache.roller.weblogger.pojos.RuntimeConfigProperty; import org.apache.roller.weblogger.pojos.Weblog; import org.apache.roller.weblogger.ui.struts2.util.UIAction; -import org.apache.struts2.convention.annotation.AllowedMethods; /** * Page used to display Roller install instructions. + * + *

This page is reachable without a login because a brand new site has no + * users yet. While the site is empty it shows bootstrap guidance; once users + * exist it requires a global administrator, and once a frontpage weblog has + * been chosen it redirects home. + * + *

Choosing the initial frontpage weblog is {@link FrontpageSetup}, a + * separate global-administrator action; later changes go through the global + * configuration screen. */ -// TODO: make this work @AllowedMethods({"execute","save"}) public class Setup extends UIAction { private static final Log LOG = LogFactory.getLog(Setup.class); @@ -42,12 +48,12 @@ public class Setup extends UIAction { private long userCount = 0; private long blogCount = 0; - private String frontpageBlog; - private Boolean aggregated; - // weblogs for frontpage blog chooser private Collection weblogs; + // true while the site has no users and only bootstrap guidance is shown + private boolean bootstrap = false; + public Setup() { this.pageTitle = "index.heading"; } @@ -64,14 +70,6 @@ public boolean isWeblogRequired() { @Override public String execute() { - - try { - WeblogManager mgr = WebloggerFactory.getWeblogger().getWeblogManager(); - setWeblogs(mgr.getWeblogs(true, null, null, null, 0, -1)); - } catch (WebloggerException ex) { - LOG.error("Error getting weblogs", ex); - addError("frontpageConfig.weblogs.error"); - } try { setUserCount(WebloggerFactory.getWeblogger().getUserManager().getUserCount()); @@ -79,31 +77,42 @@ public String execute() { } catch (WebloggerException ex) { LOG.error("Error getting user/weblog counts", ex); } - - return SUCCESS; - } - public String save() { - PropertiesManager mgr = WebloggerFactory.getWeblogger().getPropertiesManager(); - try { - RuntimeConfigProperty frontpageBlogProp = mgr.getProperty("site.frontpage.weblog.handle"); - frontpageBlogProp.setValue(frontpageBlog); - mgr.saveProperty(frontpageBlogProp); - - RuntimeConfigProperty aggregatedProp = mgr.getProperty("site.frontpage.weblog.aggregated"); - aggregatedProp.setValue(aggregated.toString()); - mgr.saveProperty(aggregatedProp); + // A site with no users cannot have an administrator yet, so the + // bootstrap instructions are shown to anyone. Nothing about the site's + // contents is exposed here: registering the first user is the only + // thing that can usefully be done. + if (getUserCount() == 0) { + setBootstrap(true); + return SUCCESS; + } - WebloggerFactory.getWeblogger().flush(); + // Beyond that point this is a site configuration screen. + if (!isUserIsAdmin()) { + return DENIED; + } - addMessage("frontpageConfig.values.saved"); + try { + if (FrontpageSettings.isConfigured()) { + // Already chosen; later changes belong in global configuration. + return "home"; + } + } catch (WebloggerException ex) { + LOG.error("Error reading frontpage configuration", ex); + } + try { + WeblogManager mgr = WebloggerFactory.getWeblogger().getWeblogManager(); + setWeblogs(mgr.getWeblogs(true, null, null, null, 0, -1)); } catch (WebloggerException ex) { - LOG.error("ERROR saving frontpage configuration", ex); - addError("frontpageConfig.values.error"); + LOG.error("Error getting weblogs", ex); + addError("frontpageConfig.weblogs.error"); } - return "home"; + + return SUCCESS; } + + public long getUserCount() { return userCount; @@ -121,6 +130,14 @@ public void setBlogCount(long blogCount) { this.blogCount = blogCount; } + public boolean isBootstrap() { + return bootstrap; + } + + public void setBootstrap(boolean bootstrap) { + this.bootstrap = bootstrap; + } + public Collection getWeblogs() { return weblogs; } @@ -129,19 +146,4 @@ public void setWeblogs(Collection weblogs) { this.weblogs = weblogs; } - public String getFrontpageBlog() { - return frontpageBlog; - } - - public void setFrontpageBlog(String frontpageBlog) { - this.frontpageBlog = frontpageBlog; - } - - public Boolean getAggregated() { - return aggregated; - } - - public void setAggregated(Boolean aggregated) { - this.aggregated = aggregated; - } } diff --git a/app/src/main/resources/ApplicationResources.properties b/app/src/main/resources/ApplicationResources.properties index 66072c23f0..42943ffe2e 100644 --- a/app/src/main/resources/ApplicationResources.properties +++ b/app/src/main/resources/ApplicationResources.properties @@ -553,6 +553,8 @@ frontpageConfig.frontpageAggregated=Enable aggregated site-wide frontpage frontpageConfig.values.saved=Properties successfully saved frontpageConfig.values.error=Error saving properties frontpageConfig.weblogs.error=Unexpected error accessing Weblogs +frontpageConfig.invalidWeblog=Choose an existing, enabled weblog for the frontpage +frontpageConfig.alreadyConfigured=A frontpage weblog has already been chosen; change it from the global configuration page # --------------------------------------------------------------- Invite member diff --git a/app/src/main/resources/struts.xml b/app/src/main/resources/struts.xml index cc94ba6588..80a29e2f81 100644 --- a/app/src/main/resources/struts.xml +++ b/app/src/main/resources/struts.xml @@ -112,7 +112,14 @@ class="org.apache.roller.weblogger.ui.struts2.core.Setup"> .Setup home - activate,execute,save + execute + + + + .Setup + home + save

- - + - @@ -93,7 +93,8 @@ - + The setup screen is reachable without a login, because a site with no users + * has nobody who could log in. A page in that position should display bootstrap + * guidance and nothing more, so the frontpage write lives on a separate action + * that requires a global administrator. These tests pin that arrangement in + * place: the display page exposes no write method, the write action requires the + * permission, and both write paths validate through one service. + */ +public class FrontpageSetupAccessTest { + + private static final Path STRUTS_XML = Paths.get("src", "main", "resources", "struts.xml"); + private static final Path SETUP_JSP = + Paths.get("src", "main", "webapp", "WEB-INF", "jsps", "core", "Setup.jsp"); + + private String read(Path path) throws IOException { + assertTrue(Files.isReadable(path), + "cannot read " + path.toAbsolutePath() + " (run from the app module)"); + return new String(Files.readAllBytes(path), StandardCharsets.UTF_8); + } + + /** + * The mutation action requires the global administrator permission. This is + * the single check the whole fix rests on. + */ + @Test + public void frontpageSetupRequiresGlobalAdmin() { + List required = new FrontpageSetup().requiredGlobalPermissionActions(); + assertEquals(1, required.size(), "expected exactly one required permission"); + assertEquals(GlobalPermission.ADMIN, required.get(0), + "the frontpage write must require a global administrator"); + } + + /** + * The public setup page must not require a user, because it has to work on + * an empty site. That is precisely why it must not be able to write. + */ + @Test + public void publicSetupPageStillNeedsNoUserButCannotWrite() throws IOException { + Setup setup = new Setup(); + assertFalse(setup.isUserRequired(), + "the bootstrap page must stay reachable on a site with no users"); + + String struts = read(STRUTS_XML); + int setupIdx = struts.indexOf("name=\"setup\""); + assertTrue(setupIdx > 0, "setup action not found in struts.xml"); + String setupBlock = struts.substring(setupIdx, struts.indexOf("", setupIdx)); + assertFalse(setupBlock.contains("save"), + "the public setup action must expose no save method:\n" + setupBlock); + } + + /** The separate action exists and exposes only its save method. */ + @Test + public void frontpageSetupActionIsWiredAndSaveOnly() throws IOException { + String struts = read(STRUTS_XML); + int idx = struts.indexOf("name=\"frontpageSetup\""); + assertTrue(idx > 0, "frontpageSetup action not wired in struts.xml"); + String block = struts.substring(idx, struts.indexOf("", idx)); + assertTrue(block.contains("FrontpageSetup"), "wrong action class:\n" + block); + assertTrue(block.contains("save"), + "frontpageSetup must expose only save:\n" + block); + } + + /** The form must post to the administrator-only action, over POST. */ + @Test + public void setupFormPostsToTheAdminAction() throws IOException { + String jsp = read(SETUP_JSP); + assertFalse(jsp.contains("setup!save"), + "the form must no longer target the public setup action"); + assertTrue(jsp.contains("frontpageSetup!save"), + "the form must target the administrator-only action"); + assertTrue(jsp.contains("method=\"post\""), + "the form must POST so the CSRF salt filter applies"); + assertTrue(jsp.contains(""), + "the form must carry a CSRF salt"); + } + + /** + * Both write paths must resolve the handle through the shared service, so + * neither can store a weblog that does not exist. + */ + @Test + public void bothWritePathsValidateThroughTheSharedService() throws IOException { + String globalConfig = read(Paths.get("src", "main", "java", "org", "apache", "roller", + "weblogger", "ui", "struts2", "admin", "GlobalConfig.java")); + assertTrue(globalConfig.contains("FrontpageSettings.resolveWeblog"), + "the global configuration screen must validate the frontpage handle"); + + String frontpageSetup = read(Paths.get("src", "main", "java", "org", "apache", "roller", + "weblogger", "ui", "struts2", "core", "FrontpageSetup.java")); + assertTrue(frontpageSetup.contains("FrontpageSettings.apply"), + "the initial write must go through the shared service"); + assertTrue(frontpageSetup.contains("FrontpageSettings.isConfigured"), + "the initial write must apply only while no frontpage is set"); + } + + /** The write action must reject requests that are not HTTP POST. */ + @Test + public void frontpageSetupSaveEnforcesPost() throws IOException { + String frontpageSetup = read(Paths.get("src", "main", "java", "org", "apache", "roller", + "weblogger", "ui", "struts2", "core", "FrontpageSetup.java")); + assertTrue(frontpageSetup.contains("\"POST\".equalsIgnoreCase"), + "save() must reject non-POST requests"); + assertTrue(frontpageSetup.contains("getMethod()"), + "save() must inspect the request method"); + } + + /** A blank handle can never resolve, whatever the database contains. */ + @Test + public void blankHandlesNeverResolve() throws Exception { + assertEquals(null, FrontpageSettings.resolveWeblog(null)); + assertEquals(null, FrontpageSettings.resolveWeblog("")); + assertEquals(null, FrontpageSettings.resolveWeblog(" ")); + } +} From 7decd9ae70973721094ab11cf08d82b0e12c2a4b Mon Sep 17 00:00:00 2001 From: "David M. Johnson" Date: Wed, 2 Sep 2026 08:18:41 -0400 Subject: [PATCH 2/2] Correct frontpage setup flow and cache updates --- .../weblogger/business/FrontpageSettings.java | 41 ++++---- .../weblogger/business/PropertiesManager.java | 9 ++ .../jpa/JPAPropertiesManagerImpl.java | 16 +++ .../ui/struts2/admin/GlobalConfig.java | 28 +++++- .../ui/struts2/core/FrontpageSetup.java | 40 +++++--- .../weblogger/ui/struts2/core/Setup.java | 61 ++++++++---- .../resources/ApplicationResources.properties | 1 + .../main/webapp/WEB-INF/jsps/core/Setup.jsp | 8 +- .../weblogger/business/PropertiesTest.java | 51 ++++++++++ .../core/FrontpageSetupAccessTest.java | 99 ++++--------------- .../roller/selenium/InitialLoginTestIT.java | 2 +- .../roller/selenium/core/SetupPage.java | 13 ++- 12 files changed, 224 insertions(+), 145 deletions(-) diff --git a/app/src/main/java/org/apache/roller/weblogger/business/FrontpageSettings.java b/app/src/main/java/org/apache/roller/weblogger/business/FrontpageSettings.java index 18e5bdf2c6..5f75c8ebe5 100644 --- a/app/src/main/java/org/apache/roller/weblogger/business/FrontpageSettings.java +++ b/app/src/main/java/org/apache/roller/weblogger/business/FrontpageSettings.java @@ -21,9 +21,6 @@ import org.apache.roller.weblogger.WebloggerException; import org.apache.roller.weblogger.pojos.RuntimeConfigProperty; import org.apache.roller.weblogger.pojos.Weblog; -import org.apache.roller.weblogger.ui.rendering.util.cache.SiteWideCache; -import org.apache.roller.weblogger.ui.rendering.util.cache.WeblogFeedCache; -import org.apache.roller.weblogger.ui.rendering.util.cache.WeblogPageCache; /** * Reads and writes the site frontpage weblog settings. @@ -31,8 +28,7 @@ *

Two screens change these values: the one-time setup screen used to choose * a frontpage while the site is being installed, and the global configuration * screen used afterwards. Both go through here so that the handle is resolved - * and validated the same way, both properties move together, and the rendered - * page and feed caches are invalidated consistently. + * and validated the same way and both properties move together. */ public final class FrontpageSettings { @@ -50,7 +46,7 @@ private FrontpageSettings() { * to a disabled weblog */ public static Weblog resolveWeblog(String handle) throws WebloggerException { - if (StringUtils.isBlank(handle)) { + if (StringUtils.isBlank(handle) || !isValidHandle(handle.trim())) { return null; } return WebloggerFactory.getWeblogger().getWeblogManager() @@ -69,7 +65,7 @@ public static String getConfiguredHandle() throws WebloggerException { /** @return true when a frontpage weblog has already been chosen. */ public static boolean isConfigured() throws WebloggerException { - return getConfiguredHandle() != null; + return resolveWeblog(getConfiguredHandle()) != null; } /** @@ -85,7 +81,7 @@ public static boolean isConfigured() throws WebloggerException { * @throws InvalidFrontpageWeblogException when the handle does not name an * existing, enabled weblog */ - public static void apply(String handle, Boolean aggregated) + public static boolean applyInitial(String handle, Boolean aggregated) throws WebloggerException { Weblog weblog = resolveWeblog(handle); @@ -96,8 +92,11 @@ public static void apply(String handle, Boolean aggregated) PropertiesManager mgr = WebloggerFactory.getWeblogger().getPropertiesManager(); RuntimeConfigProperty handleProp = mgr.getProperty(HANDLE_PROPERTY); - handleProp.setValue(weblog.getHandle()); - mgr.saveProperty(handleProp); + String currentValue = handleProp == null ? null : handleProp.getValue(); + if (resolveWeblog(currentValue) != null + || !mgr.compareAndSetProperty(HANDLE_PROPERTY, currentValue, weblog.getHandle())) { + return false; + } RuntimeConfigProperty aggregatedProp = mgr.getProperty(AGGREGATED_PROPERTY); aggregatedProp.setValue(Boolean.toString(Boolean.TRUE.equals(aggregated))); @@ -105,22 +104,16 @@ public static void apply(String handle, Boolean aggregated) WebloggerFactory.getWeblogger().flush(); - invalidateRenderedContent(); + return true; } - /** - * Drops the locally cached rendering of the front page. - * - *

The properties themselves are read through the properties manager on - * each request, but rendered pages and feeds are cached separately and would - * otherwise keep serving the previous weblog. Roller has no cross-node - * invalidation transport, so peers pick the change up when their own cache - * entries expire. - */ - private static void invalidateRenderedContent() { - SiteWideCache.getInstance().clear(); - WeblogPageCache.getInstance().clear(); - WeblogFeedCache.getInstance().clear(); + private static boolean isValidHandle(String handle) { + for (int i = 0; i < handle.length(); i++) { + if (!Character.isLetterOrDigit(handle.charAt(i)) && handle.charAt(i) != '_') { + return false; + } + } + return true; } /** Raised when a submitted frontpage handle cannot be used. */ diff --git a/app/src/main/java/org/apache/roller/weblogger/business/PropertiesManager.java b/app/src/main/java/org/apache/roller/weblogger/business/PropertiesManager.java index 5b0fef784b..d0ed156441 100644 --- a/app/src/main/java/org/apache/roller/weblogger/business/PropertiesManager.java +++ b/app/src/main/java/org/apache/roller/weblogger/business/PropertiesManager.java @@ -52,6 +52,15 @@ public interface PropertiesManager { * Save a list of properties */ void saveProperties(Map properties) throws WebloggerException; + + + /** + * Replace a property's value only when it still has the expected value. + * + * @return true when the property was updated, otherwise false + */ + boolean compareAndSetProperty(String name, String expectedValue, String newValue) + throws WebloggerException; /** diff --git a/app/src/main/java/org/apache/roller/weblogger/business/jpa/JPAPropertiesManagerImpl.java b/app/src/main/java/org/apache/roller/weblogger/business/jpa/JPAPropertiesManagerImpl.java index 255ee047f8..b576d6aa74 100644 --- a/app/src/main/java/org/apache/roller/weblogger/business/jpa/JPAPropertiesManagerImpl.java +++ b/app/src/main/java/org/apache/roller/weblogger/business/jpa/JPAPropertiesManagerImpl.java @@ -21,6 +21,9 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; + +import jakarta.persistence.LockModeType; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -144,6 +147,19 @@ public void saveProperties(Map properties) throws this.strategy.store(prop); } } + + + @Override + public boolean compareAndSetProperty(String name, String expectedValue, String newValue) + throws WebloggerException { + RuntimeConfigProperty property = strategy.getEntityManager(true).find( + RuntimeConfigProperty.class, name, LockModeType.PESSIMISTIC_WRITE); + if (property == null || !Objects.equals(expectedValue, property.getValue())) { + return false; + } + property.setValue(newValue); + return true; + } /** diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/admin/GlobalConfig.java b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/admin/GlobalConfig.java index b757615330..4cd235084d 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/admin/GlobalConfig.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/admin/GlobalConfig.java @@ -38,6 +38,9 @@ import org.apache.roller.weblogger.pojos.GlobalPermission; import org.apache.roller.weblogger.pojos.RuntimeConfigProperty; import org.apache.roller.weblogger.pojos.Weblog; +import org.apache.roller.weblogger.ui.rendering.util.cache.SiteWideCache; +import org.apache.roller.weblogger.ui.rendering.util.cache.WeblogFeedCache; +import org.apache.roller.weblogger.ui.rendering.util.cache.WeblogPageCache; import org.apache.roller.weblogger.ui.struts2.util.UIAction; import org.apache.roller.weblogger.util.Utilities; import org.apache.struts2.dispatcher.HttpParameters; @@ -157,6 +160,9 @@ public String save() { return ERROR; } + String oldFrontpageHandle = propertyValue(FrontpageSettings.HANDLE_PROPERTY); + String oldFrontpageAggregated = propertyValue(FrontpageSettings.AGGREGATED_PROPERTY); + // only set values for properties that are already defined RuntimeConfigProperty updProp; String incomingProp; @@ -216,10 +222,11 @@ public String save() { // is resolved through the same service as the setup path. The // stored value is always a weblog that exists and is enabled. try { - if (FrontpageSettings.resolveWeblog(incomingProp) == null) { + Weblog weblog = FrontpageSettings.resolveWeblog(incomingProp); + if (weblog == null) { addError("frontpageConfig.invalidWeblog"); } else { - updProp.setValue( incomingProp.trim() ); + updProp.setValue(weblog.getHandle()); } } catch (WebloggerException ex) { log.error("Error resolving frontpage weblog", ex); @@ -257,6 +264,12 @@ public String save() { mgr.saveProperties(getProperties()); WebloggerFactory.getWeblogger().flush(); + if (!Objects.equals(oldFrontpageHandle, propertyValue(FrontpageSettings.HANDLE_PROPERTY)) + || !Objects.equals(oldFrontpageAggregated, + propertyValue(FrontpageSettings.AGGREGATED_PROPERTY))) { + invalidateRenderedContent(); + } + // notify user of our success addMessage("generic.changes.saved"); @@ -268,6 +281,17 @@ public String save() { return SUCCESS; } + private String propertyValue(String name) { + RuntimeConfigProperty property = getProperties().get(name); + return property == null ? null : property.getValue(); + } + + private void invalidateRenderedContent() { + SiteWideCache.getInstance().clear(); + WeblogPageCache.getInstance().clear(); + WeblogFeedCache.getInstance().clear(); + } + @Override public void setParameters(HttpParameters parameters) { diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/FrontpageSetup.java b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/FrontpageSetup.java index 23a7585ac0..cac83ba4e1 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/FrontpageSetup.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/FrontpageSetup.java @@ -28,7 +28,9 @@ import org.apache.roller.weblogger.WebloggerException; import org.apache.roller.weblogger.business.FrontpageSettings; import org.apache.roller.weblogger.pojos.GlobalPermission; -import org.apache.roller.weblogger.ui.struts2.util.UIAction; +import org.apache.roller.weblogger.ui.rendering.util.cache.SiteWideCache; +import org.apache.roller.weblogger.ui.rendering.util.cache.WeblogFeedCache; +import org.apache.roller.weblogger.ui.rendering.util.cache.WeblogPageCache; import org.apache.struts2.ServletActionContext; /** @@ -43,7 +45,7 @@ * set, later changes go through the global configuration screen, which is * already administrator-only. */ -public class FrontpageSetup extends UIAction { +public class FrontpageSetup extends Setup { private static final Log LOG = LogFactory.getLog(FrontpageSetup.class); @@ -59,6 +61,11 @@ public boolean isWeblogRequired() { return false; } + @Override + public boolean isUserRequired() { + return true; + } + @Override public List requiredGlobalPermissionActions() { return Collections.singletonList(GlobalPermission.ADMIN); @@ -72,38 +79,47 @@ public List requiredGlobalPermissionActions() { */ public String save() { - HttpServletRequest req = ServletActionContext.getRequest(); - if (!"POST".equalsIgnoreCase(req.getMethod())) { + if (!isPostRequest()) { return DENIED; } try { - // Re-read immediately before writing so that a second submission - // arriving alongside the first cannot replace the winner. This - // narrows the window rather than closing it outright; the two - // submissions would have to interleave within this method, and the - // losing caller is told the choice is already made. - if (FrontpageSettings.isConfigured()) { + if (!FrontpageSettings.applyInitial(frontpageBlog, aggregated)) { addError("frontpageConfig.alreadyConfigured"); - return "home"; + loadSetupModel(); + setFrontpageConfigured(true); + return INPUT; } - FrontpageSettings.apply(frontpageBlog, aggregated); + invalidateRenderedContent(); addMessage("frontpageConfig.values.saved"); } catch (FrontpageSettings.InvalidFrontpageWeblogException ex) { addError("frontpageConfig.invalidWeblog"); + loadSetupModel(); return INPUT; } catch (WebloggerException ex) { LOG.error("ERROR saving frontpage configuration", ex); addError("frontpageConfig.values.error"); + loadSetupModel(); return INPUT; } return "home"; } + protected boolean isPostRequest() { + HttpServletRequest req = ServletActionContext.getRequest(); + return req != null && "POST".equalsIgnoreCase(req.getMethod()); + } + + private void invalidateRenderedContent() { + SiteWideCache.getInstance().clear(); + WeblogPageCache.getInstance().clear(); + WeblogFeedCache.getInstance().clear(); + } + public String getFrontpageBlog() { return frontpageBlog; } diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/Setup.java b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/Setup.java index d224b65b57..77db586404 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/Setup.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/Setup.java @@ -33,9 +33,9 @@ * Page used to display Roller install instructions. * *

This page is reachable without a login because a brand new site has no - * users yet. While the site is empty it shows bootstrap guidance; once users - * exist it requires a global administrator, and once a frontpage weblog has - * been chosen it redirects home. + * users yet. While the site is empty it shows bootstrap guidance. Once users + * exist it remains useful to everyone, but only a global administrator sees + * the frontpage chooser. * *

Choosing the initial frontpage weblog is {@link FrontpageSetup}, a * separate global-administrator action; later changes go through the global @@ -54,6 +54,9 @@ public class Setup extends UIAction { // true while the site has no users and only bootstrap guidance is shown private boolean bootstrap = false; + // true when a valid frontpage weblog has already been selected + private boolean frontpageConfigured = false; + public Setup() { this.pageTitle = "index.heading"; } @@ -71,6 +74,22 @@ public boolean isWeblogRequired() { @Override public String execute() { + loadSetupModel(); + + if (isBootstrap()) { + return SUCCESS; + } + + if (isFrontpageConfigured()) { + return "home"; + } + + return SUCCESS; + } + + /** Loads the model used by both the public page and failed save results. */ + protected void loadSetupModel() { + try { setUserCount(WebloggerFactory.getWeblogger().getUserManager().getUserCount()); setBlogCount(WebloggerFactory.getWeblogger().getWeblogManager().getWeblogCount()); @@ -84,32 +103,24 @@ public String execute() { // thing that can usefully be done. if (getUserCount() == 0) { setBootstrap(true); - return SUCCESS; - } - - // Beyond that point this is a site configuration screen. - if (!isUserIsAdmin()) { - return DENIED; + return; } try { - if (FrontpageSettings.isConfigured()) { - // Already chosen; later changes belong in global configuration. - return "home"; - } + setFrontpageConfigured(FrontpageSettings.isConfigured()); } catch (WebloggerException ex) { LOG.error("Error reading frontpage configuration", ex); } - try { - WeblogManager mgr = WebloggerFactory.getWeblogger().getWeblogManager(); - setWeblogs(mgr.getWeblogs(true, null, null, null, 0, -1)); - } catch (WebloggerException ex) { - LOG.error("Error getting weblogs", ex); - addError("frontpageConfig.weblogs.error"); + if (isUserIsAdmin() && !isFrontpageConfigured()) { + try { + WeblogManager mgr = WebloggerFactory.getWeblogger().getWeblogManager(); + setWeblogs(mgr.getWeblogs(true, null, null, null, 0, -1)); + } catch (WebloggerException ex) { + LOG.error("Error getting weblogs", ex); + addError("frontpageConfig.weblogs.error"); + } } - - return SUCCESS; } @@ -138,6 +149,14 @@ public void setBootstrap(boolean bootstrap) { this.bootstrap = bootstrap; } + public boolean isFrontpageConfigured() { + return frontpageConfigured; + } + + public void setFrontpageConfigured(boolean frontpageConfigured) { + this.frontpageConfigured = frontpageConfigured; + } + public Collection getWeblogs() { return weblogs; } diff --git a/app/src/main/resources/ApplicationResources.properties b/app/src/main/resources/ApplicationResources.properties index 42943ffe2e..1fab65e322 100644 --- a/app/src/main/resources/ApplicationResources.properties +++ b/app/src/main/resources/ApplicationResources.properties @@ -547,6 +547,7 @@ index.setFrontpageHelp=\ You must specify a weblog to serve as the front page weblog, you can do this \ via the Server Admin->Configuration page or the form that will appear \ below once you have created at least one weblog. +index.setFrontpageAdminRequired=A global administrator must designate the frontpage weblog. frontpageConfig.frontpageBlogName=Name of weblog to serve as frontpage blog frontpageConfig.frontpageAggregated=Enable aggregated site-wide frontpage diff --git a/app/src/main/webapp/WEB-INF/jsps/core/Setup.jsp b/app/src/main/webapp/WEB-INF/jsps/core/Setup.jsp index 5d2a3db017..01c7582e08 100644 --- a/app/src/main/webapp/WEB-INF/jsps/core/Setup.jsp +++ b/app/src/main/webapp/WEB-INF/jsps/core/Setup.jsp @@ -53,6 +53,7 @@ <%-- STEP 2: Create a weblog if you don't already have one --%> +

@@ -91,7 +92,7 @@

- + @@ -112,6 +113,9 @@ + +

+

- + diff --git a/app/src/test/java/org/apache/roller/weblogger/business/PropertiesTest.java b/app/src/test/java/org/apache/roller/weblogger/business/PropertiesTest.java index d35a9fc0c2..facccc4c50 100644 --- a/app/src/test/java/org/apache/roller/weblogger/business/PropertiesTest.java +++ b/app/src/test/java/org/apache/roller/weblogger/business/PropertiesTest.java @@ -27,6 +27,10 @@ import org.junit.jupiter.api.Test; import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import static org.junit.jupiter.api.Assertions.*; @@ -91,5 +95,52 @@ public void testProperiesCRUD() throws Exception { assertEquals("foofoo", props.get("site.name").getValue()); assertEquals("blahblah", props.get("site.description").getValue()); } + + @Test + public void compareAndSetAllowsOnlyOneConcurrentWinner() throws Exception { + PropertiesManager mgr = WebloggerFactory.getWeblogger().getPropertiesManager(); + RuntimeConfigProperty prop = mgr.getProperty("site.frontpage.weblog.handle"); + String original = prop.getValue(); + prop.setValue(""); + mgr.saveProperty(prop); + TestUtils.endSession(true); + + CountDownLatch ready = new CountDownLatch(2); + CountDownLatch start = new CountDownLatch(1); + ExecutorService executor = Executors.newFixedThreadPool(2); + try { + Future first = executor.submit(() -> compareAndSetAfterSignal( + "site.frontpage.weblog.handle", "first", ready, start)); + Future second = executor.submit(() -> compareAndSetAfterSignal( + "site.frontpage.weblog.handle", "second", ready, start)); + ready.await(); + start.countDown(); + + assertNotEquals(first.get(), second.get(), "exactly one update must win"); + + RuntimeConfigProperty saved = WebloggerFactory.getWeblogger() + .getPropertiesManager().getProperty("site.frontpage.weblog.handle"); + assertTrue("first".equals(saved.getValue()) || "second".equals(saved.getValue())); + saved.setValue(original); + WebloggerFactory.getWeblogger().getPropertiesManager().saveProperty(saved); + TestUtils.endSession(true); + } finally { + executor.shutdownNow(); + } + } + + private boolean compareAndSetAfterSignal(String name, String value, + CountDownLatch ready, CountDownLatch start) throws Exception { + ready.countDown(); + start.await(); + try { + boolean updated = WebloggerFactory.getWeblogger().getPropertiesManager() + .compareAndSetProperty(name, "", value); + WebloggerFactory.getWeblogger().flush(); + return updated; + } finally { + WebloggerFactory.getWeblogger().release(); + } + } } diff --git a/app/src/test/java/org/apache/roller/weblogger/ui/struts2/core/FrontpageSetupAccessTest.java b/app/src/test/java/org/apache/roller/weblogger/ui/struts2/core/FrontpageSetupAccessTest.java index 5f20e8b2fe..c82c64f727 100644 --- a/app/src/test/java/org/apache/roller/weblogger/ui/struts2/core/FrontpageSetupAccessTest.java +++ b/app/src/test/java/org/apache/roller/weblogger/ui/struts2/core/FrontpageSetupAccessTest.java @@ -17,11 +17,6 @@ */ package org.apache.roller.weblogger.ui.struts2.core; -import java.io.IOException; -import java.nio.charset.StandardCharsets; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.Paths; import java.util.List; import org.apache.roller.weblogger.business.FrontpageSettings; @@ -30,7 +25,7 @@ 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.junit.jupiter.api.Assertions.assertNull; /** * Checks who is allowed to change the site frontpage setting. @@ -40,20 +35,10 @@ * guidance and nothing more, so the frontpage write lives on a separate action * that requires a global administrator. These tests pin that arrangement in * place: the display page exposes no write method, the write action requires the - * permission, and both write paths validate through one service. + * permission and validates requests before attempting a write. */ public class FrontpageSetupAccessTest { - private static final Path STRUTS_XML = Paths.get("src", "main", "resources", "struts.xml"); - private static final Path SETUP_JSP = - Paths.get("src", "main", "webapp", "WEB-INF", "jsps", "core", "Setup.jsp"); - - private String read(Path path) throws IOException { - assertTrue(Files.isReadable(path), - "cannot read " + path.toAbsolutePath() + " (run from the app module)"); - return new String(Files.readAllBytes(path), StandardCharsets.UTF_8); - } - /** * The mutation action requires the global administrator permission. This is * the single check the whole fix rests on. @@ -71,80 +56,36 @@ public void frontpageSetupRequiresGlobalAdmin() { * an empty site. That is precisely why it must not be able to write. */ @Test - public void publicSetupPageStillNeedsNoUserButCannotWrite() throws IOException { + public void publicSetupPageStillNeedsNoUser() { Setup setup = new Setup(); assertFalse(setup.isUserRequired(), "the bootstrap page must stay reachable on a site with no users"); - - String struts = read(STRUTS_XML); - int setupIdx = struts.indexOf("name=\"setup\""); - assertTrue(setupIdx > 0, "setup action not found in struts.xml"); - String setupBlock = struts.substring(setupIdx, struts.indexOf("", setupIdx)); - assertFalse(setupBlock.contains("save"), - "the public setup action must expose no save method:\n" + setupBlock); - } - - /** The separate action exists and exposes only its save method. */ - @Test - public void frontpageSetupActionIsWiredAndSaveOnly() throws IOException { - String struts = read(STRUTS_XML); - int idx = struts.indexOf("name=\"frontpageSetup\""); - assertTrue(idx > 0, "frontpageSetup action not wired in struts.xml"); - String block = struts.substring(idx, struts.indexOf("", idx)); - assertTrue(block.contains("FrontpageSetup"), "wrong action class:\n" + block); - assertTrue(block.contains("save"), - "frontpageSetup must expose only save:\n" + block); } - /** The form must post to the administrator-only action, over POST. */ + /** The separate write action always requires an authenticated user. */ @Test - public void setupFormPostsToTheAdminAction() throws IOException { - String jsp = read(SETUP_JSP); - assertFalse(jsp.contains("setup!save"), - "the form must no longer target the public setup action"); - assertTrue(jsp.contains("frontpageSetup!save"), - "the form must target the administrator-only action"); - assertTrue(jsp.contains("method=\"post\""), - "the form must POST so the CSRF salt filter applies"); - assertTrue(jsp.contains(""), - "the form must carry a CSRF salt"); - } - - /** - * Both write paths must resolve the handle through the shared service, so - * neither can store a weblog that does not exist. - */ - @Test - public void bothWritePathsValidateThroughTheSharedService() throws IOException { - String globalConfig = read(Paths.get("src", "main", "java", "org", "apache", "roller", - "weblogger", "ui", "struts2", "admin", "GlobalConfig.java")); - assertTrue(globalConfig.contains("FrontpageSettings.resolveWeblog"), - "the global configuration screen must validate the frontpage handle"); - - String frontpageSetup = read(Paths.get("src", "main", "java", "org", "apache", "roller", - "weblogger", "ui", "struts2", "core", "FrontpageSetup.java")); - assertTrue(frontpageSetup.contains("FrontpageSettings.apply"), - "the initial write must go through the shared service"); - assertTrue(frontpageSetup.contains("FrontpageSettings.isConfigured"), - "the initial write must apply only while no frontpage is set"); + public void frontpageSetupRequiresAUser() { + assertEquals(true, new FrontpageSetup().isUserRequired()); } /** The write action must reject requests that are not HTTP POST. */ @Test - public void frontpageSetupSaveEnforcesPost() throws IOException { - String frontpageSetup = read(Paths.get("src", "main", "java", "org", "apache", "roller", - "weblogger", "ui", "struts2", "core", "FrontpageSetup.java")); - assertTrue(frontpageSetup.contains("\"POST\".equalsIgnoreCase"), - "save() must reject non-POST requests"); - assertTrue(frontpageSetup.contains("getMethod()"), - "save() must inspect the request method"); + public void frontpageSetupSaveEnforcesPost() { + FrontpageSetup action = new FrontpageSetup() { + @Override + protected boolean isPostRequest() { + return false; + } + }; + assertEquals(FrontpageSetup.DENIED, action.save()); } - /** A blank handle can never resolve, whatever the database contains. */ + /** Blank and malformed handles are rejected before a database lookup. */ @Test - public void blankHandlesNeverResolve() throws Exception { - assertEquals(null, FrontpageSettings.resolveWeblog(null)); - assertEquals(null, FrontpageSettings.resolveWeblog("")); - assertEquals(null, FrontpageSettings.resolveWeblog(" ")); + public void invalidHandlesNeverResolve() throws Exception { + assertNull(FrontpageSettings.resolveWeblog(null)); + assertNull(FrontpageSettings.resolveWeblog("")); + assertNull(FrontpageSettings.resolveWeblog(" ")); + assertNull(FrontpageSettings.resolveWeblog("not/a/handle")); } } diff --git a/it-selenium/src/test/java/org/apache/roller/selenium/InitialLoginTestIT.java b/it-selenium/src/test/java/org/apache/roller/selenium/InitialLoginTestIT.java index c766c15b7e..95b396678e 100644 --- a/it-selenium/src/test/java/org/apache/roller/selenium/InitialLoginTestIT.java +++ b/it-selenium/src/test/java/org/apache/roller/selenium/InitialLoginTestIT.java @@ -86,7 +86,7 @@ public void testInitialLogin() throws Exception { driver.get(baseUrl); sp = new SetupPage(driver); driver.navigate().refresh(); - BlogHomePage bhp = sp.chooseFrontPageBlog(); + BlogHomePage bhp = sp.chooseFrontPageBlog("bobsblog"); // create and read first blog entry String blogEntryTitle = "My First Blog Entry"; diff --git a/it-selenium/src/test/java/org/apache/roller/selenium/core/SetupPage.java b/it-selenium/src/test/java/org/apache/roller/selenium/core/SetupPage.java index 8359dff863..7a8ae4d42b 100644 --- a/it-selenium/src/test/java/org/apache/roller/selenium/core/SetupPage.java +++ b/it-selenium/src/test/java/org/apache/roller/selenium/core/SetupPage.java @@ -19,7 +19,9 @@ import org.apache.roller.selenium.AbstractRollerPage; import org.apache.roller.selenium.view.BlogHomePage; +import org.openqa.selenium.By; import org.openqa.selenium.WebDriver; +import org.openqa.selenium.support.ui.Select; /** * represents core/Setup.jsp @@ -40,9 +42,12 @@ public RegisterPage createNewUser() { return new RegisterPage(driver); } - public BlogHomePage chooseFrontPageBlog() { - verifyPageTitle("setup_0", "Front Page: Welcome to Roller!"); - clickById("setup_0"); + public BlogHomePage chooseFrontPageBlog(String handle) { + verifyPageTitle("Front Page: Welcome to Roller!"); + Select chooser = new Select(driver.findElement(By.name("frontpageBlog"))); + chooser.selectByValue(handle); + driver.findElement(By.cssSelector( + "form[action*='frontpageSetup'] input[type='submit']")).click(); return new BlogHomePage(driver); } -} \ No newline at end of file +}