From a65a9951b0eb0730a6079928db40ce6f8e540136 Mon Sep 17 00:00:00 2001 From: Francois Prunayre Date: Thu, 18 Jan 2024 14:07:34 +0100 Subject: [PATCH] Formatter / Withheld element not always hidden. 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. --- .../org/fao/geonet/kernel/XmlSerializer.java | 36 +++---------------- .../kernel/XmlSerializerIntegrationTest.java | 11 ------ .../api/records/formatters/FormatterApi.java | 25 ++++--------- .../org/fao/geonet/guiservices/util/Env.java | 3 -- .../formatters/AbstractFormatterTest.java | 2 +- .../FormatterApiIntegrationTest.java | 6 ++-- .../main/java/org/fao/geonet/Geonetwork.java | 2 +- 7 files changed, 17 insertions(+), 68 deletions(-) diff --git a/core/src/main/java/org/fao/geonet/kernel/XmlSerializer.java b/core/src/main/java/org/fao/geonet/kernel/XmlSerializer.java index 762afc9b551..fd22881c373 100644 --- a/core/src/main/java/org/fao/geonet/kernel/XmlSerializer.java +++ b/core/src/main/java/org/fao/geonet/kernel/XmlSerializer.java @@ -54,22 +54,6 @@ * (id, data, lastChangeDate). */ public abstract class XmlSerializer { - private static InheritableThreadLocal configThreadLocal = new InheritableThreadLocal(); - - 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 namespaces) throws JDOMException { @@ -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; @@ -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."); @@ -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); } } @@ -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) @@ -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; - } - } } diff --git a/core/src/test/java/org/fao/geonet/kernel/XmlSerializerIntegrationTest.java b/core/src/test/java/org/fao/geonet/kernel/XmlSerializerIntegrationTest.java index b16e898db5a..e30b06d96ca 100644 --- a/core/src/test/java/org/fao/geonet/kernel/XmlSerializerIntegrationTest.java +++ b/core/src/test/java/org/fao/geonet/kernel/XmlSerializerIntegrationTest.java @@ -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 threadLocalInstance = (InheritableThreadLocal) field.get(null); - threadLocalInstance.set(null); - assertHiddenElements(true); - } - @Test public void testInternalSelectHidingWithheldAdministrator() throws Exception { try (Closeable ignored = configureXmlSerializerAndServiceContext(true, false, false)) { diff --git a/services/src/main/java/org/fao/geonet/api/records/formatters/FormatterApi.java b/services/src/main/java/org/fao/geonet/api/records/formatters/FormatterApi.java index a548af626bd..4a09f7162c7 100644 --- a/services/src/main/java/org/fao/geonet/api/records/formatters/FormatterApi.java +++ b/services/src/main/java/org/fao/geonet/api/records/formatters/FormatterApi.java @@ -262,9 +262,12 @@ 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; @@ -272,11 +275,6 @@ public void getRecordFormattedBy( 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; @@ -414,8 +412,6 @@ public HttpEntity 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} @@ -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()); @@ -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"); @@ -610,11 +604,6 @@ public Pair 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); } diff --git a/services/src/main/java/org/fao/geonet/guiservices/util/Env.java b/services/src/main/java/org/fao/geonet/guiservices/util/Env.java index 05c14986b57..b6971709704 100644 --- a/services/src/main/java/org/fao/geonet/guiservices/util/Env.java +++ b/services/src/main/java/org/fao/geonet/guiservices/util/Env.java @@ -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); diff --git a/services/src/test/java/org/fao/geonet/api/records/formatters/AbstractFormatterTest.java b/services/src/test/java/org/fao/geonet/api/records/formatters/AbstractFormatterTest.java index 7c201ebd10d..285b4e29f05 100644 --- a/services/src/test/java/org/fao/geonet/api/records/formatters/AbstractFormatterTest.java +++ b/services/src/test/java/org/fao/geonet/api/records/formatters/AbstractFormatterTest.java @@ -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); } }; diff --git a/services/src/test/java/org/fao/geonet/api/records/formatters/FormatterApiIntegrationTest.java b/services/src/test/java/org/fao/geonet/api/records/formatters/FormatterApiIntegrationTest.java index b019a489d36..60bf0c6a49f 100644 --- a/services/src/test/java/org/fao/geonet/api/records/formatters/FormatterApiIntegrationTest.java +++ b/services/src/test/java/org/fao/geonet/api/records/formatters/FormatterApiIntegrationTest.java @@ -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 { @@ -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); @@ -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"); diff --git a/web/src/main/java/org/fao/geonet/Geonetwork.java b/web/src/main/java/org/fao/geonet/Geonetwork.java index 5af4127b245..f6c1e2f4f49 100644 --- a/web/src/main/java/org/fao/geonet/Geonetwork.java +++ b/web/src/main/java/org/fao/geonet/Geonetwork.java @@ -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); }