Skip to content

Commit

Permalink
Formatter / Withheld element not always hidden.
Browse files Browse the repository at this point in the history 
Affected API calls:
* http://localhost:8080/geonetwork/srv/api/records/720ceda7-a9ea-4bbe-896a-04925c44a7f0/formatters/xyz
* http://localhost:8080/geonetwork/srv/api/records/720ceda7-a9ea-4bbe-896a-04925c44a7f0/related?type=thumbnails&type=onlines

For testing, add a record with 2 online resources. Add withheld attribute to one of them, call the related API.
Sometimes 1 or 2 links are returned (it does not happen always, but usually if accessing the same API call with a browser and Curl using the same session the problem can be observed). A variable was set using ThreadLocal which looks to be the cause of this. It was noticed while opening the editor not always listing all distribution links.

The Formatter API has a `hide_withheld` parameter to hide with held elements even if the user is allowed to see them. The application is not using this parameter, and opening an incognito window can be used to check if with held elements are properly removed.

This change propose to remove the ThreadLocal variable and only rely on user session and edits rights of the current user to hide or not with held elements.
  • Loading branch information
@fxprunayre
fxprunayre committed Jan 18, 2024
1 parent d9c7da5 commit fecd128
Show file tree
Hide file tree
Showing 7 changed files with 17 additions and 68 deletions.
36 changes: 5 additions & 31 deletions core/src/main/java/org/fao/geonet/kernel/XmlSerializer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,22 +54,6 @@
* (id, data, lastChangeDate).
*/
public abstract class XmlSerializer {
private static InheritableThreadLocal<ThreadLocalConfiguration> configThreadLocal = new InheritableThreadLocal<XmlSerializer.ThreadLocalConfiguration>();

public static ThreadLocalConfiguration getThreadLocal(boolean setIfNotPresent) {
ThreadLocalConfiguration config = configThreadLocal.get();
if (config == null && setIfNotPresent) {
config = new ThreadLocalConfiguration();
configThreadLocal.set(config);
}

return config;
}

public static void clearThreadLocal() {
configThreadLocal.set(null);
}

public static void removeFilteredElement(Element metadata,
final MetadataSchemaOperationFilter filter,
List<Namespace> namespaces) throws JDOMException {
Expand All @@ -82,6 +66,7 @@ public static void removeFilteredElement(Element metadata,
List<?> nodes = Xml.selectNodes(metadata,
xpath,
namespaces);

for (Object object : nodes) {
if (object instanceof Element) {
Element element = (Element) object;
Expand Down Expand Up @@ -125,7 +110,7 @@ public boolean resolveXLinks() {
return false;
}

String xlR = _settingManager.getValue(Settings.SYSTEM_XLINKRESOLVER_ENABLE);
String xlR = _settingManager.getValue(Settings.SYSTEM_XLINKRESOLVER_ENABLE);
if (xlR != null) {
boolean isEnabled = xlR.equals("true");
if (isEnabled) Log.debug(Geonet.DATA_MANAGER, "XLink Resolver enabled.");
Expand Down Expand Up @@ -217,7 +202,8 @@ public Element removeHiddenElements(boolean isIndexingTask, AbstractMetadata met
}
}
}
if (filterEditOperationElements || (getThreadLocal(false) != null && getThreadLocal(false).forceFilterEditOperation)) {

if (filterEditOperationElements) {
removeFilteredElement(metadataXml, editFilter, namespaces);
}
}
Expand Down Expand Up @@ -292,7 +278,7 @@ protected void deleteDb(String id) throws Exception {
public abstract void delete(String id, ServiceContext context)
throws Exception;

/* API to be overridden by extensions */
/* API to be overridden by extensions */

public abstract void update(String id, Element xml,
String changeDate, boolean updateDateStamp, String uuid, ServiceContext context)
Expand All @@ -310,16 +296,4 @@ public abstract AbstractMetadata insert(AbstractMetadata metadata, Element dataX

public abstract Element selectNoXLinkResolver(String id, boolean isIndexingTask, boolean applyOperationsFilters)
throws Exception;

public static class ThreadLocalConfiguration {
private boolean forceFilterEditOperation = false;

public boolean isForceFilterEditOperation() {
return forceFilterEditOperation;
}

public void setForceFilterEditOperation(boolean forceFilterEditOperation) {
this.forceFilterEditOperation = forceFilterEditOperation;
}
}
}
11 changes: 0 additions & 11 deletions core/src/test/java/org/fao/geonet/kernel/XmlSerializerIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,17 +143,6 @@ public void testInternalSelectHidingWithheldSettingsDisabled() throws Exception
assertHiddenElements(false, false);
}

@SuppressWarnings("unchecked")
@Test
public void testInternalSelectHidingWithheldNullServiceContext() throws Exception {
setSchemaFilters(true, true);
Field field = ServiceContext.class.getDeclaredField("THREAD_LOCAL_INSTANCE");
field.setAccessible(true);
InheritableThreadLocal<ServiceContext> threadLocalInstance = (InheritableThreadLocal<ServiceContext>) field.get(null);
threadLocalInstance.set(null);
assertHiddenElements(true);
}

@Test
public void testInternalSelectHidingWithheldAdministrator() throws Exception {
try (Closeable ignored = configureXmlSerializerAndServiceContext(true, false, false)) {
Expand Down
25 changes: 7 additions & 18 deletions services/src/main/java/org/fao/geonet/api/records/formatters/FormatterApi.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,21 +262,19 @@ public void getRecordFormattedBy(
}


Boolean hideWithheld = true;
// final boolean hideWithheld = Boolean.TRUE.equals(hide_withheld) ||
// !context.getBean(AccessManager.class).canEdit(context, resolvedId);
final ServiceContext context = createServiceContext(
language,
formatType,
request.getNativeRequest(HttpServletRequest.class));

Boolean hideWithheld = !context.getBean(AccessManager.class).canEdit(context, String.valueOf(metadata.getId()));
Key key = new Key(metadata.getId(), language, formatType, formatterId, hideWithheld, width);
final boolean skipPopularityBool = false;

ISODate changeDate = metadata.getDataInfo().getChangeDate();

Validator validator;

final ServiceContext context = createServiceContext(
language,
formatType,
request.getNativeRequest(HttpServletRequest.class));

if (changeDate != null) {
final long changeDateAsTime = changeDate.toDate().getTime();
long roundedChangeDate = changeDateAsTime / 1000 * 1000;
Expand Down Expand Up @@ -414,8 +412,6 @@ public HttpEntity<byte[]> getCachedPublicMetadata(
* @param id the id, uuid or fileIdentifier of the metadata
* @param xslid the id of the formatter
* @param skipPopularity if true then don't increment popularity
* @param hide_withheld if true hideWithheld (private) elements even if the current user would
* normally have access to them.
* @param width the approximate size of the element that the formatter output will be
* embedded in compared to the full device width. Allowed options are the
* enum values: {@link org.fao.geonet.api.records.formatters.FormatterWidth}
Expand All @@ -431,7 +427,6 @@ public void exec(
@RequestParam(value = "uuid", required = false) final String uuid,
@RequestParam(value = "xsl", required = false) final String xslid,
@RequestParam(defaultValue = "n") final String skipPopularity,
@RequestParam(value = "hide_withheld", required = false) final Boolean hide_withheld,
@RequestParam(value = "width", defaultValue = "_100") final FormatterWidth width,
final NativeWebRequest request) throws Exception {
final FormatType formatType = FormatType.valueOf(type.toLowerCase());
Expand All @@ -440,8 +435,7 @@ public void exec(
ServiceContext context = createServiceContext(lang, formatType, request.getNativeRequest(HttpServletRequest.class));
Lib.resource.checkPrivilege(context, resolvedId, ReservedOperation.view);

final boolean hideWithheld = Boolean.TRUE.equals(hide_withheld) ||
!context.getBean(AccessManager.class).canEdit(context, resolvedId);
final boolean hideWithheld = !context.getBean(AccessManager.class).canEdit(context, resolvedId);
Key key = new Key(Integer.parseInt(resolvedId), lang, formatType, xslid, hideWithheld, width);
final boolean skipPopularityBool = skipPopularity.equals("y");

Expand Down Expand Up @@ -610,11 +604,6 @@ public Pair<Element, AbstractMetadata> getMetadata(ServiceContext context, int i
Element metadata = serializer.removeHiddenElements(false, md, true);
if (doXLinks) Processor.processXLink(metadata, context);

boolean withholdWithheldElements = hide_withheld != null && hide_withheld;
if (XmlSerializer.getThreadLocal(false) != null || withholdWithheldElements) {
XmlSerializer.getThreadLocal(true).setForceFilterEditOperation(withholdWithheldElements);
}

return Pair.read(metadata, md);

}
Expand Down
3 changes: 0 additions & 3 deletions services/src/main/java/org/fao/geonet/guiservices/util/Env.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,6 @@ public void init(Path appPath, ServiceConfig params) throws Exception {
//--------------------------------------------------------------------------

public Element exec(Element params, ServiceContext context) throws Exception {
// reset the thread local
XmlSerializer.clearThreadLocal();

GeonetContext gc = (GeonetContext) context.getHandlerContext(Geonet.CONTEXT_NAME);

Element response = gc.getBean(SettingManager.class).getAllAsXML(true);
Expand Down
2 changes: 1 addition & 1 deletion services/src/test/java/org/fao/geonet/api/records/formatters/AbstractFormatterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ protected void measureFormatterPerformance(final MockHttpServletRequest request,
TestFunction testFunction = new TestFunction() {
@Override
public void exec() throws Exception {
formatService.exec(getUILang(), getOutputType().name(), "" + id, null, formatterId, "true", false, FormatterWidth._100,
formatService.exec(getUILang(), getOutputType().name(), "" + id, null, formatterId, "true", FormatterWidth._100,
webRequest);
}
};
Expand Down
6 changes: 3 additions & 3 deletions ...ices/src/test/java/org/fao/geonet/api/records/formatters/FormatterApiIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public void testExec() throws Exception {
JeevesDelegatingFilterProxy.setApplicationContextAttributeKey(srvAppContext);
RequestContextHolder.setRequestAttributes(new ServletRequestAttributes(request));

formatService.exec("eng", "html", "" + id, null, formatter.getId(), "true", false, _100, new ServletWebRequest(request, response));
formatService.exec("eng", "html", "" + id, null, formatter.getId(), "true", _100, new ServletWebRequest(request, response));

final String view = response.getContentAsString();
try {
Expand All @@ -105,7 +105,7 @@ public void testExec() throws Exception {
}
try {
response = new MockHttpServletResponse();
formatService.exec("eng", "testpdf", "" + id, null, formatter.getId(), "true", false, _100,
formatService.exec("eng", "testpdf", "" + id, null, formatter.getId(), "true", _100,
new ServletWebRequest(request, response));
// Files.write(Paths.get("e:/tmp/view.pdf"), response.getContentAsByteArray());
// System.exit(0);
Expand Down Expand Up @@ -138,7 +138,7 @@ public void testExecXslt() throws Exception {
JeevesDelegatingFilterProxy.setApplicationContextAttributeKey(applicationContextAttributeKey);

final MockHttpServletResponse response = new MockHttpServletResponse();
formatService.exec("eng", "html", "" + id, null, formatterName, "true", false, _100, new ServletWebRequest(request, response));
formatService.exec("eng", "html", "" + id, null, formatterName, "true", _100, new ServletWebRequest(request, response));
final String viewXml = response.getContentAsString();
final Element view = Xml.loadString(viewXml, false);
assertEqualsText("fromFunction", view, "*//p");
Expand Down
2 changes: 1 addition & 1 deletion web/src/main/java/org/fao/geonet/Geonetwork.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ public void run() {
final MockHttpServletResponse response = new MockHttpServletResponse();
try {
formatService.exec("eng", FormatType.html.toString(), mdId.toString(), null, formatterName,
Boolean.TRUE.toString(), false, FormatterWidth._100, new ServletWebRequest(servletRequest, response));
Boolean.TRUE.toString(), FormatterWidth._100, new ServletWebRequest(servletRequest, response));
} catch (Throwable t) {
Log.info(Geonet.GEONETWORK, "Error while initializing the Formatter with id: " + formatterName, t);
}
Expand Down

0 comments on commit fecd128

Please sign in to comment.