mraible commented on code in PR #170:
URL: https://github.com/apache/roller/pull/170#discussion_r3891439308
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/Setup.java:
##########
@@ -64,46 +70,49 @@ 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());
setBlogCount(WebloggerFactory.getWeblogger().getWeblogManager().getWeblogCount());
} 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;
Review Comment:
`index.jsp` (lines 29-31) forwards `/` here whenever the frontpage handle is
blank, so returning `DENIED` for non-admins makes the home page an
access-denied tile for every visitor of a site that has users but no frontpage
yet (`checkPermission` with a null user throws and `isUserIsAdmin()` swallows
it to false). The page only showed counts and links before; I'd keep
`execute()` open and gate the form in the JSP on `isUserIsAdmin()`, leaving the
POST admin-only.
##########
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.
+ *
+ * <p>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.
+ *
+ * <p>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<String> requiredGlobalPermissionActions() {
+ return Collections.singletonList(GlobalPermission.ADMIN);
+ }
+
+ /**
+ * Stores the initial frontpage selection.
+ *
+ * <p>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;
Review Comment:
`struts.xml` maps `input` for this action to the `.Setup` tile, but
`Setup.jsp` reads `userCount`, `blogCount`, `weblogs` and `bootstrap`, none of
which exist on `FrontpageSetup`, so every `<s:if>` is false and the admin sees
the error above three empty panels with no chooser. Either redirect back to
`setup` carrying the message, or give this action the same properties.
##########
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.
+ *
+ * <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.
+ */
+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);
Review Comment:
`getWeblogByHandle` throws `WebloggerException("Invalid handle")` for
anything outside `[A-Za-z0-9_]`, so a POST with `frontpageBlog=my-blog` takes
the generic `WebloggerException` branch (stack trace at ERROR, "Error saving
properties") instead of the invalid-weblog message this method documents.
Pre-check the handle or catch that case here.
##########
app/src/main/webapp/WEB-INF/jsps/core/Setup.jsp:
##########
@@ -93,7 +93,8 @@
<s:if test="blogCount > 0">
Review Comment:
The heading was changed to `blogCount > 0 && !bootstrap` but this guard
wasn't, so in bootstrap mode (weblogs exist, zero users, `weblogs` is null)
anonymous visitors still get the form with an empty select and a Save that
POSTs to an admin-only action.
##########
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.
+ *
+ * <p>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.
+ *
+ * <p>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<String> requiredGlobalPermissionActions() {
+ return Collections.singletonList(GlobalPermission.ADMIN);
+ }
+
+ /**
+ * Stores the initial frontpage selection.
+ *
+ * <p>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";
Review Comment:
`rollerStack` has no `MessageStore` interceptor, so this `addError` is
discarded by the `redirectAction`; the second admin just lands on the winner's
frontpage. The comment above says the loser is told, but they aren't.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/Setup.java:
##########
@@ -64,46 +70,49 @@ 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());
setBlogCount(WebloggerFactory.getWeblogger().getWeblogManager().getWeblogCount());
} 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()) {
Review Comment:
With `users.firstUserAdmin=false` (documented in `roller.properties`) no
account ever passes `isUserIsAdmin()`, and `frontpageSetup!save` and
`globalConfig` are admin-only too, so the frontpage can never be chosen and `/`
stays broken. The removed `setup!save` was the only non-admin path; if it goes,
the description should say how such installs are expected to finish setup.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/Setup.java:
##########
@@ -64,46 +70,49 @@ 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());
setBlogCount(WebloggerFactory.getWeblogger().getWeblogManager().getWeblogCount());
} 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()) {
Review Comment:
`JPAWeblogManagerImpl.removeWeblog` doesn't clear
`site.frontpage.weblog.handle`, so after the frontpage weblog is deleted
`isConfigured()` is still true and this redirects to `/`, which forwards to the
missing weblog. On master the admin could come back here and pick another one.
Either treat a handle that doesn't resolve as unconfigured, or clear the
property in `removeWeblog`.
##########
app/src/test/java/org/apache/roller/weblogger/ui/struts2/core/FrontpageSetupAccessTest.java:
##########
@@ -0,0 +1,150 @@
+/*
+ * 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.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;
+import org.apache.roller.weblogger.pojos.GlobalPermission;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Checks who is allowed to change the site frontpage setting.
+ *
+ * <p>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<String> 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("</action>", setupIdx));
+ assertFalse(setupBlock.contains("save"),
Review Comment:
These assert on source text (`contains("save")`, `method=\"post\"`), so an
XML comment containing `save` in the setup block fails the build while a
reachable-but-unguarded action passes. `requiredGlobalPermissionActions` is the
only behavioural check here; I'd drop the text ones.
##########
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;
Review Comment:
First `business` → `ui.rendering` dependency in the tree; `SiteWideCache` /
`WeblogPageCache` / `WeblogFeedCache` are rendering singletons that bootstrap
caches on `getInstance()`. The invalidation belongs in the actions
(`FrontpageSetup` / `GlobalConfig`), with `FrontpageSettings` just resolving
and storing.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/admin/GlobalConfig.java:
##########
@@ -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() );
Review Comment:
This validates via `resolveWeblog` but then stores the raw value itself
instead of calling `FrontpageSettings.apply()`, so changing the frontpage from
Global Config skips the cache invalidation the class javadoc promises for "both
screens" and keeps serving the old weblog's cached pages and feeds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
