Correct frontpage setup flow and cache updates
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 18e5bdf..5f75c8e 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 @@ * <p>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 @@ * 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 @@ /** @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 @@ * @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 @@ 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 @@ WebloggerFactory.getWeblogger().flush(); - invalidateRenderedContent(); + return true; } - /** - * Drops the locally cached rendering of the front page. - * - * <p>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 5b0fef7..d0ed156 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 @@ * Save a list of properties */ void saveProperties(Map<String, RuntimeConfigProperty> 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 255ee04..b576d6a 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 @@ 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 b757615..4cd2350 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 @@ 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 @@ // 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 @@ 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 @@ 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 23a7585..cac83ba 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); @@ -60,6 +62,11 @@ } @Override + public boolean isUserRequired() { + return true; + } + + @Override public List<String> requiredGlobalPermissionActions() { return Collections.singletonList(GlobalPermission.ADMIN); } @@ -72,38 +79,47 @@ */ 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 d224b65..77db586 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. * * <p>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. * * <p>Choosing the initial frontpage weblog is {@link FrontpageSetup}, a * separate global-administrator action; later changes go through the global @@ -54,6 +54,9 @@ // 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 @@ @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 @@ // 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 @@ this.bootstrap = bootstrap; } + public boolean isFrontpageConfigured() { + return frontpageConfigured; + } + + public void setFrontpageConfigured(boolean frontpageConfigured) { + this.frontpageConfigured = frontpageConfigured; + } + public Collection<Weblog> getWeblogs() { return weblogs; }
diff --git a/app/src/main/resources/ApplicationResources.properties b/app/src/main/resources/ApplicationResources.properties index 42943ff..1fab65e 100644 --- a/app/src/main/resources/ApplicationResources.properties +++ b/app/src/main/resources/ApplicationResources.properties
@@ -547,6 +547,7 @@ You must specify a weblog to serve as the front page weblog, you can do this \ via the <b>Server Admin->Configuration</b> 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 5d2a3db..01c7582 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 --%> +<s:if test="!bootstrap"> <div class="panel panel-default"> <div class="panel-heading"> <h3 class="panel-title"> @@ -91,7 +92,7 @@ <p><s:text name="index.setFrontpageHelp"/></p> - <s:if test="blogCount > 0"> + <s:if test="blogCount > 0 && userIsAdmin && !frontpageConfigured"> <s:form action="frontpageSetup!save" method="post" theme="bootstrap" cssClass="form-horizontal"> @@ -112,6 +113,9 @@ </s:form> </s:if> + <s:elseif test="blogCount > 0 && !userIsAdmin && !frontpageConfigured"> + <p><s:text name="index.setFrontpageAdminRequired"/></p> + </s:elseif> </div> </div> - +</s:if>
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 d35a9fc..facccc4 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 @@ 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<Boolean> first = executor.submit(() -> compareAndSetAfterSignal( + "site.frontpage.weblog.handle", "first", ready, start)); + Future<Boolean> 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 5f20e8b..c82c64f 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 @@ * 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("</action>", 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. */ + /** The separate write action always requires an authenticated user. */ @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("</action>", idx)); - assertTrue(block.contains("FrontpageSetup"), "wrong action class:\n" + block); - assertTrue(block.contains("<allowed-methods>save</allowed-methods>"), - "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("<s:hidden name=\"salt\"/>"), - "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 c766c15..95b3966 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 @@ 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 8359dff..7a8ae4d 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 @@ 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 +}