This is an automated email from the ASF dual-hosted git repository.
ashishvijaywargiya pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/trunk by this push:
new 5d00c0cefb Improved: Route the layout-image events through the
permission-checked content services and tighten data resource template
rendering (#1846)
5d00c0cefb is described below
commit 5d00c0cefb533b9c4982145e710a4141ba172634
Author: Krishna Uprit <[email protected]>
AuthorDate: Fri Sep 4 16:10:17 2026 +0530
Improved: Route the layout-image events through the permission-checked
content services and tighten data resource template rendering (#1846)
- Aligns updateLayoutImage and cloneLayout with the rest of the content
component: their DataResource / Content writes now go through the
permission-checked services (updateDataResource, updateContent) instead
of storing the entities directly, and updateLayoutImage can no longer
change a resource's type.
- Also removes the unused updateLayoutImageOnly mapping, requires
CONTENTMGR_SUPER on createDataTemplateType / updateDataTemplateType /
deleteDataTemplateType, and restricts DataResourceWorker template
rendering — no *_OBJECT source, FTL allow-list instead of deny-list,
SCREEN_COMBINED only from a component:// location.
- Adds UpdateLayoutPermissionTests for the low-privilege (OFBTOOLS_VIEW
/ CONTENTMGR_VIEW) case.
Thank you Krishna Uprit for the contribution.
---
applications/content/servicedef/services_data.xml | 12 +++
.../ofbiz/content/data/DataResourceWorker.java | 53 ++++++++-----
.../apache/ofbiz/content/layout/LayoutEvents.java | 87 +++++++++++++++++-----
.../permission/UpdateLayoutPermissionTests.groovy | 36 +++++++++
.../content/webapp/content/WEB-INF/controller.xml | 6 --
5 files changed, 150 insertions(+), 44 deletions(-)
diff --git a/applications/content/servicedef/services_data.xml
b/applications/content/servicedef/services_data.xml
index d9432eeaf5..8cf83d4db6 100644
--- a/applications/content/servicedef/services_data.xml
+++ b/applications/content/servicedef/services_data.xml
@@ -315,18 +315,30 @@
<auto-attributes include="pk" mode="IN" />
</service>
+ <!-- A data template type decides which rendering engine a data resource
may drive
+ (FTL / SCREEN_COMBINED / FORM_COMBINED / XSLT), so maintaining these
rows is gated on
+ CONTENTMGR_SUPER, consistent with the other template-related content
services. -->
<service name="createDataTemplateType" engine="entity-auto"
invoke="create" default-entity-name="DataTemplateType" auth="true">
<description>Create a Data Template Type</description>
+ <required-permissions join-type="AND">
+ <check-permission permission="CONTENTMGR_SUPER" action="_CREATE"/>
+ </required-permissions>
<auto-attributes include="pk" mode="INOUT" optional="true"/>
<auto-attributes include="nonpk" mode="IN" optional="true"/>
</service>
<service name="updateDataTemplateType" engine="entity-auto"
invoke="update" default-entity-name="DataTemplateType" auth="true">
<description>Update a Data Template Type</description>
+ <required-permissions join-type="AND">
+ <check-permission permission="CONTENTMGR_SUPER" action="_UPDATE"/>
+ </required-permissions>
<auto-attributes include="pk" mode="IN"/>
<auto-attributes include="nonpk" mode="IN" optional="true"/>
</service>
<service name="deleteDataTemplateType" engine="entity-auto"
invoke="delete" default-entity-name="DataTemplateType" auth="true">
<description>Delete a Data Template Type</description>
+ <required-permissions join-type="AND">
+ <check-permission permission="CONTENTMGR_SUPER" action="_DELETE"/>
+ </required-permissions>
<auto-attributes include="pk" mode="IN"/>
</service>
diff --git
a/applications/content/src/main/java/org/apache/ofbiz/content/data/DataResourceWorker.java
b/applications/content/src/main/java/org/apache/ofbiz/content/data/DataResourceWorker.java
index 10ea5aa822..0398e3447b 100644
---
a/applications/content/src/main/java/org/apache/ofbiz/content/data/DataResourceWorker.java
+++
b/applications/content/src/main/java/org/apache/ofbiz/content/data/DataResourceWorker.java
@@ -45,6 +45,7 @@ import java.util.LinkedList;
import java.util.List;
import java.util.Locale;
import java.util.Map;
+import java.util.Set;
import java.util.TreeMap;
import jakarta.servlet.http.HttpServletRequest;
@@ -118,6 +119,15 @@ public class DataResourceWorker implements
org.apache.ofbiz.widget.content.DataR
private static final String ERR_RESOURCE = "ContentErrorUiLabels";
private static final String PROPERTY_RESOURCE = "content";
+ // Data resource source types whose stored text may be rendered as a
FreeMarker template
+ // (dataTemplateTypeId=FTL). Limited to the file- and URL-backed types,
each of which is
+ // independently path- or protocol-restricted elsewhere
(OFBIZ/LOCAL/CONTEXT file path
+ // allow-lists in SecurityUtil; URL_RESOURCE http(s) and host checks in
checkUrlResourceAllowed).
+ // Free-text types (ELECTRONIC_TEXT/SHORT_TEXT/LINK) are not supported as
FreeMarker templates.
+ // Kept as an allow-list so a data resource type added later is not
renderable by default.
+ private static final Set<String> FTL_ALLOWED_SOURCE_TYPES =
+ UtilMisc.toSet("OFBIZ_FILE", "LOCAL_FILE", "CONTEXT_FILE",
"URL_RESOURCE");
+
/**
* Traverses the DataCategory parent/child structure and put it in
categoryNode. Returns non-null error string if there is an error.
* @param depth The place on the categoryTypesIds to start collecting.
@@ -883,6 +893,16 @@ public class DataResourceWorker implements
org.apache.ofbiz.widget.content.DataR
// a template is defined; render the template first
templateContext.put("mimeTypeId", targetMimeTypeId);
+ // A template engine (FTL, XSLT, SCREEN_COMBINED, FORM_COMBINED)
is only given content that
+ // is authored template text. For an *_OBJECT data resource
getDataResourceText() returns
+ // the data resource's own primary key (see
writeDataResourceText), not template text, so
+ // an *_OBJECT is never a valid template source for any template
type.
+ String drSourceTypeId =
dataResource.getString("dataResourceTypeId");
+ if (drSourceTypeId != null && drSourceTypeId.endsWith("_OBJECT")) {
+ throw new GeneralException("Data resource [" + dataResourceId
+ "] of type [" + drSourceTypeId
+ + "] cannot be rendered as a [" + dataTemplateTypeId +
"] template.");
+ }
+
// FTL template
if ("FTL".equals(dataTemplateTypeId)) {
try {
@@ -904,11 +924,12 @@ public class DataResourceWorker implements
org.apache.ofbiz.widget.content.DataR
// render the FTL template
boolean useTemplateCache = cache &&
!UtilProperties.getPropertyAsBoolean("content", "disable.ftl.template.cache",
false);
- if
("ELECTRONIC_TEXT".equals(dataResource.getString("dataResourceTypeId"))
- ||
"SHORT_TEXT".equalsIgnoreCase(dataResource.getString("dataResourceTypeId"))
- ||
"LINK".equalsIgnoreCase(dataResource.getString("dataResourceTypeId"))) {
- throw new GeneralException("Error rendering template:
FreeMarker templates are no longer supported for "
- + dataResource.getString("dataResourceTypeId")
+ " data resources.");
+ // Only file- and URL-backed source types may be rendered
as FreeMarker templates
+ // (see FTL_ALLOWED_SOURCE_TYPES); free-text data
resources (ELECTRONIC_TEXT /
+ // SHORT_TEXT / LINK) are not supported.
+ if (!FTL_ALLOWED_SOURCE_TYPES.contains(drSourceTypeId)) {
+ throw new GeneralException("Error rendering template:
FreeMarker templates are not supported for "
+ + drSourceTypeId + " data resources.");
}
FreeMarkerWorker.renderTemplateFromString("delegator:" +
delegator.getDelegatorName() + ":DataResource:"
@@ -966,20 +987,16 @@ public class DataResourceWorker implements
org.apache.ofbiz.widget.content.DataR
ModelScreen modelScreen = null;
ScreenStringRenderer renderer =
screens.getScreenStringRenderer();
String combinedName = dataResource.getString("objectInfo");
- if
("URL_RESOURCE".equals(dataResource.getString("dataResourceTypeId")) &&
UtilValidate.isNotEmpty(combinedName)
- && combinedName.startsWith("component://")) {
- modelScreen =
ScreenFactory.getScreenFromLocation(combinedName);
- } else { // stored in a single file, long or short text
- Document screenXml =
UtilXml.readXmlDocument(getDataResourceText(dataResource, targetMimeTypeId,
locale, templateContext,
- delegator, cache), true, true);
- Map<String, ModelScreen> modelScreenMap =
ScreenFactory.readScreenDocument(screenXml, "DataResourceId: "
- + dataResource.getString("dataResourceId"));
- if (UtilValidate.isNotEmpty(modelScreenMap)) {
- Map.Entry<String, ModelScreen> entry =
modelScreenMap.entrySet().iterator().next();
- // get first entry, only one screen allowed per
file
- modelScreen = entry.getValue();
- }
+ // A SCREEN_COMBINED data resource is rendered only when
it references a deployed
+ // screen definition (URL_RESOURCE with a component://
objectInfo location).
+ // Building a ModelScreen from screen XML stored in the
data resource text is not
+ // supported: that text can carry widget actions and
scripts.
+ if
(!"URL_RESOURCE".equals(dataResource.getString("dataResourceTypeId")) ||
UtilValidate.isEmpty(combinedName)
+ || !combinedName.startsWith("component://")) {
+ throw new GeneralException("SCREEN_COMBINED data
resource [" + dataResource.getString("dataResourceId")
+ + "] must reference a component:// screen
location via objectInfo.");
}
+ modelScreen =
ScreenFactory.getScreenFromLocation(combinedName);
if (UtilValidate.isNotEmpty(modelScreen)) {
modelScreen.renderScreenString(out, context, renderer);
} else {
diff --git
a/applications/content/src/main/java/org/apache/ofbiz/content/layout/LayoutEvents.java
b/applications/content/src/main/java/org/apache/ofbiz/content/layout/LayoutEvents.java
index cba2143a7e..260c970e21 100644
---
a/applications/content/src/main/java/org/apache/ofbiz/content/layout/LayoutEvents.java
+++
b/applications/content/src/main/java/org/apache/ofbiz/content/layout/LayoutEvents.java
@@ -46,6 +46,7 @@ import org.apache.ofbiz.entity.GenericValue;
import org.apache.ofbiz.entity.util.EntityQuery;
import org.apache.ofbiz.minilang.MiniLangException;
import org.apache.ofbiz.minilang.SimpleMapProcessor;
+import org.apache.ofbiz.security.Security;
import org.apache.ofbiz.service.GenericServiceException;
import org.apache.ofbiz.service.LocalDispatcher;
import org.apache.ofbiz.service.ServiceUtil;
@@ -185,7 +186,7 @@ public class LayoutEvents {
}
String imageFileName = (String) uploadResults.get("imageFileName");
if (Debug.verboseOn()) {
- Debug.logVerbose("in createLayoutImage(java), context:" +
context, "");
+ Debug.logVerbose("in updateLayoutImage(java), context:" +
context, "");
}
context.put("userLogin", session.getAttribute("userLogin"));
context.put("dataResourceTypeId", "IMAGE_OBJECT");
@@ -199,32 +200,62 @@ public class LayoutEvents {
String dataResourceId = (String) context.get("drDataResourceId");
if (Debug.verboseOn()) {
- Debug.logVerbose("in createLayoutImage(java), dataResourceId:"
+ dataResourceId, "");
+ Debug.logVerbose("in updateLayoutImage(java), dataResourceId:"
+ dataResourceId, "");
}
GenericValue dataResource =
EntityQuery.use(delegator).from("DataResource").where("dataResourceId",
dataResourceId).queryOne();
if (Debug.verboseOn()) {
- Debug.logVerbose("in createLayoutImage(java), dataResource:" +
dataResource, "");
+ Debug.logVerbose("in updateLayoutImage(java), dataResource:" +
dataResource + ", imageFileName:" + imageFileName, "");
}
- // Use objectInfo field to store the name of the file, since there
is no
- // place in ImageDataResource for it.
- if (Debug.verboseOn()) {
- Debug.logVerbose("in createLayoutImage(java), imageFileName:"
+ imageFileName, "");
+ if (dataResource == null) {
+ request.setAttribute("_ERROR_MESSAGE_",
+ UtilProperties.getMessage(ERR_RESOURCE,
"layoutEvents.data_ressource_id_null", locale));
+ return "error";
}
- if (dataResource != null) {
- dataResource.setNonPKFields(context);
- dataResource.store();
+ // This event only maintains an existing plain IMAGE_OBJECT layout
resource: it updates the
+ // editable image metadata and the image bytes through the
permission-checked services
+ // (updateDataResource, then create/updateImageDataResource - all
gated by
+ // genericDataResourcePermission, main-action UPDATE / CREATE),
and never writes the
+ // DataResource or ImageDataResource entities directly. A resource
that is template-bearing,
+ // or that is not an IMAGE_OBJECT, is rejected rather than
converted, so this event can no
+ // longer reclassify a resource (the alignment updateLayout
received in OFBIZ #1745).
+ String existingTemplateType =
dataResource.getString("dataTemplateTypeId");
+ if ((UtilValidate.isNotEmpty(existingTemplateType) &&
!"NONE".equals(existingTemplateType))
+ ||
!"IMAGE_OBJECT".equals(dataResource.getString("dataResourceTypeId"))) {
+ request.setAttribute("_ERROR_MESSAGE_",
+ UtilProperties.getMessage("ContentUiLabels",
"ContentPermissionNotGranted", locale));
+ return "error";
}
-
- // See if this needs to be a create or an update procedure
- GenericValue imageDataResource =
EntityQuery.use(delegator).from("ImageDataResource").where("dataResourceId",
dataResourceId).queryOne();
- if (imageDataResource == null) {
- imageDataResource = delegator.makeValue("ImageDataResource",
UtilMisc.toMap("dataResourceId", dataResourceId));
- imageDataResource.set("imageData", byteWrap.array());
- imageDataResource.create();
- } else {
- imageDataResource.set("imageData", byteWrap.array());
- imageDataResource.store();
+ LocalDispatcher dispatcher = (LocalDispatcher)
request.getAttribute("dispatcher");
+ GenericValue userLogin = (GenericValue)
session.getAttribute("userLogin");
+ Map<String, Object> updateDrCtx = new HashMap<>();
+ updateDrCtx.put("dataResourceId", dataResourceId);
+ updateDrCtx.put("userLogin", userLogin);
+ for (String editableField : UtilMisc.toList("dataResourceName",
"statusId", "mimeTypeId",
+ "localeString", "isPublic", "objectInfo")) {
+ Object value = context.get(editableField);
+ if (value != null) {
+ updateDrCtx.put(editableField, value);
+ }
+ }
+ try {
+ Map<String, Object> updateDrResult =
dispatcher.runSync("updateDataResource", updateDrCtx);
+ if (ServiceUtil.isError(updateDrResult)) {
+ request.setAttribute("_ERROR_MESSAGE_",
ServiceUtil.getErrorMessage(updateDrResult));
+ return "error";
+ }
+ GenericValue imageDataResource =
EntityQuery.use(delegator).from("ImageDataResource")
+ .where("dataResourceId", dataResourceId).queryOne();
+ String imageService = (imageDataResource == null) ?
"createImageDataResource" : "updateImageDataResource";
+ Map<String, Object> imageResult =
dispatcher.runSync(imageService, UtilMisc.toMap("dataResourceId",
+ dataResourceId, "imageData", byteWrap.array(),
"userLogin", userLogin));
+ if (ServiceUtil.isError(imageResult)) {
+ request.setAttribute("_ERROR_MESSAGE_",
ServiceUtil.getErrorMessage(imageResult));
+ return "error";
+ }
+ } catch (GenericServiceException e) {
+ request.setAttribute("_ERROR_MESSAGE_", e.getMessage());
+ return "error";
}
} catch (GenericEntityException e3) {
request.setAttribute("_ERROR_MESSAGE_", e3.getMessage());
@@ -338,6 +369,22 @@ public class LayoutEvents {
request.setAttribute("_ERROR_MESSAGE_", errMsg);
return "error";
}
+ // The clone-master step below writes new Content, DataResource
and ContentAssoc rows
+ // directly (no service call), so gate it here: CONTENTMGR create,
plus CONTENTMGR_SUPER
+ // when the source data resource is template-bearing (the clone
would be template-bearing
+ // too) - the same permissions createContent / createDataResource
would require.
+ // Converting these direct writes to service calls is left as
separate follow-up.
+ Security security = (Security) request.getAttribute("security");
+ GenericValue srcDataResource =
EntityQuery.use(delegator).from("DataResource")
+ .where("dataResourceId",
content.getString("dataResourceId")).queryOne();
+ String srcTemplateType = srcDataResource != null ?
srcDataResource.getString("dataTemplateTypeId") : null;
+ boolean srcIsTemplate = UtilValidate.isNotEmpty(srcTemplateType)
&& !"NONE".equals(srcTemplateType);
+ if (security == null ||
!security.hasEntityPermission("CONTENTMGR", "_CREATE", userLogin)
+ || (srcIsTemplate &&
!security.hasEntityPermission("CONTENTMGR_SUPER", "_CREATE", userLogin))) {
+ request.setAttribute("_ERROR_MESSAGE_",
+ UtilProperties.getMessage("ContentUiLabels",
"ContentPermissionNotGranted", locale));
+ return "error";
+ }
newContent = delegator.makeValue("Content", content);
if (Debug.verboseOn()) {
Debug.logVerbose("in cloneLayout, newContent:" + newContent,
"");
diff --git
a/applications/content/src/test/groovy/org/apache/ofbiz/content/permission/UpdateLayoutPermissionTests.groovy
b/applications/content/src/test/groovy/org/apache/ofbiz/content/permission/UpdateLayoutPermissionTests.groovy
index 2f3aa882f5..ac26aa9f94 100644
---
a/applications/content/src/test/groovy/org/apache/ofbiz/content/permission/UpdateLayoutPermissionTests.groovy
+++
b/applications/content/src/test/groovy/org/apache/ofbiz/content/permission/UpdateLayoutPermissionTests.groovy
@@ -72,6 +72,42 @@ class UpdateLayoutPermissionTests implements
JupiterTestHelper {
}
}
+ /**
+ * updateLayoutImage now writes both the DataResource metadata
(updateDataResource) and the image
+ * bytes (create/updateImageDataResource) only through the
permission-checked services, and rejects
+ * a target that is not a plain IMAGE_OBJECT. cloneLayout still copies the
entities directly but is
+ * gated by an explicit CONTENTMGR / CONTENTMGR_SUPER check. These
assertions exercise the service
+ * boundaries those paths rely on for the view-only profile - each must be
denied.
+ */
+ @Test
+ void testViewOnlyUserCannotWriteImageBytesOrCloneTemplateResource() {
+ GenericValue lowPrivUserLogin = createViewOnlyUserLogin()
+ GenericValue dataResource = createTestDataResource()
+
+ // updateLayoutImage's image-bytes write goes through
create/updateImageDataResource,
+ // gated by genericDataResourcePermission (CREATE / UPDATE) on the
target record
+ assertThrows(ServiceAuthException) {
+ dispatcher.runSync('createImageDataResource', [dataResourceId:
dataResource.dataResourceId,
+ imageData: 'x'.bytes, userLogin: lowPrivUserLogin])
+ }
+
+ // updateLayoutImage's metadata write goes through updateDataResource
(UPDATE) on the
+ // real target - denied for a user with no CONTENTMGR_UPDATE and no
role standing
+ assertThrows(ServiceAuthException) {
+ dispatcher.runSync('updateDataResource', [dataResourceId:
dataResource.dataResourceId,
+ dataResourceName: 'renamed by view-only user',
dataResourceTypeId: 'IMAGE_OBJECT',
+ dataTemplateTypeId: 'NONE', userLogin: lowPrivUserLogin])
+ }
+
+ // the boundary cloneLayout's inline CONTENTMGR_SUPER check mirrors:
creating a
+ // template-bearing DataResource is denied for a view-only user
+ assertThrows(ServiceAuthException) {
+ dispatcher.runSync('createDataResource', [dataResourceTypeId:
'ELECTRONIC_TEXT',
+ dataTemplateTypeId: 'FTL', dataResourceName: 'clone',
statusId: 'CTNT_PUBLISHED',
+ userLogin: lowPrivUserLogin])
+ }
+ }
+
private GenericValue createViewOnlyUserLogin() {
GenericValue existing = from('UserLogin').where(userLoginId:
USER_LOGIN_ID).queryOne()
if (existing) {
diff --git a/applications/content/webapp/content/WEB-INF/controller.xml
b/applications/content/webapp/content/WEB-INF/controller.xml
index c809c8b1b6..2dd673184f 100644
--- a/applications/content/webapp/content/WEB-INF/controller.xml
+++ b/applications/content/webapp/content/WEB-INF/controller.xml
@@ -1528,12 +1528,6 @@ under the License.
<response name="success" type="view" value="EditLayoutImage"/>
<response name="error" type="view" value="EditLayoutImage"/>
</request-map>
- <request-map uri="updateLayoutImageOnly">
- <security auth="true" https="true"/>
- <event type="java" invoke="updateLayoutImageOnly"
path="org.apache.ofbiz.content.layout.LayoutEvents"/>
- <response name="success" type="view" value="EditLayoutImage"/>
- <response name="error" type="view" value="EditLayoutImage"/>
- </request-map>
<request-map uri="cloneLayout">
<security auth="true" https="true"/>
<event type="java" invoke="cloneLayout"
path="org.apache.ofbiz.content.layout.LayoutEvents"/>
