Refactors Dolphin quick access integration for robustness

* Refactors Dolphin quick access integration for robustness

Refactor the Dolphin quick access integration to use DOM manipulation for adding and removing bookmarks in the XBEL file. This change aims to improve the reliability and correctness of bookmark management by handling stale entries.

Changes

- Replaces string-based manipulation of the XBEL file with DOM manipulation using org.w3c.dom.
- Introduces methods for loading, validating, and writing the XBEL document.
- Implements bookmark creation and removal using XPath queries and DOM operations.
- Adds more test cases to DolphinPlacesTest to cover adding and removing bookmarks under different scenarios.

Author: jrauber

src/main/java/module-info.java

@@ -18,6 +18,7 @@
 	requires org.purejava.appindicator;
 	requires org.purejava.kwallet;
 	requires de.swiesend.secretservice;
+	requires java.xml;
 
 	provides AutoStartProvider with FreedesktopAutoStartService;
 	provides KeychainAccessProvider with GnomeKeyringKeychainAccess, KDEWalletKeychainAccess;

src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java

@@ -6,18 +6,40 @@
 import org.cryptomator.integrations.common.Priority;
 import org.cryptomator.integrations.quickaccess.QuickAccessService;
 import org.cryptomator.integrations.quickaccess.QuickAccessServiceException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.w3c.dom.DOMException;
+import org.w3c.dom.Document;
+import org.w3c.dom.Node;
+import org.w3c.dom.NodeList;
+import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
 
 import javax.xml.XMLConstants;
+import javax.xml.namespace.QName;
+import javax.xml.parsers.DocumentBuilder;
+import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.ParserConfigurationException;
+import javax.xml.transform.OutputKeys;
 import javax.xml.transform.Source;
+import javax.xml.transform.Transformer;
+import javax.xml.transform.TransformerException;
+import javax.xml.transform.TransformerFactory;
+import javax.xml.transform.dom.DOMSource;
+import javax.xml.transform.stream.StreamResult;
 import javax.xml.transform.stream.StreamSource;
 import javax.xml.validation.SchemaFactory;
 import javax.xml.validation.Validator;
+import javax.xml.xpath.XPathConstants;
+import javax.xml.xpath.XPathExpressionException;
+import javax.xml.xpath.XPathFactory;
+import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.StringReader;
+import java.io.StringWriter;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.util.List;
 import java.util.UUID;
 
 /**
@@ -29,20 +51,11 @@
 @Priority(90)
 public class DolphinPlaces extends FileConfiguredQuickAccess implements QuickAccessService {
 
+	private static final Logger LOG = LoggerFactory.getLogger(DolphinPlaces.class);
+
+	private static final String XBEL_NAMESPACE = "http://www.freedesktop.org/standards/desktop-bookmarks";
 	private static final int MAX_FILE_SIZE = 1 << 20; //1MiB, xml is quite verbose
 	private static final Path PLACES_FILE = Path.of(System.getProperty("user.home"), ".local/share/user-places.xbel");
-	private static final String ENTRY_TEMPLATE = """
-			<bookmark href=\"%s\">
-			 <title>%s</title>
-			 <info>
-			  <metadata owner=\"http://freedesktop.org\">
-			   <bookmark:icon name="drive-harddisk-encrypted"/>
-			  </metadata>
-			  <metadata owner=\"https://cryptomator.org\">
-			  	<id>%s</id>
-			  </metadata>
-			 </info>
-			</bookmark>""";
 
 	private static final Validator XML_VALIDATOR;
 
@@ -61,29 +74,157 @@ public DolphinPlaces() {
 		super(PLACES_FILE, MAX_FILE_SIZE);
 	}
 
+	public DolphinPlaces(Path configFilePath) {
+		super(configFilePath, MAX_FILE_SIZE);
+	}
+
 	@Override
 	EntryAndConfig addEntryToConfig(String config, Path target, String displayName) throws QuickAccessServiceException {
 		try {
-			String id = UUID.randomUUID().toString();
-			//validate
+			var id = UUID.randomUUID().toString();
+			LOG.trace("Adding bookmark for target: '{}', displayName: '{}', id: '{}'", target, displayName, id);
 			XML_VALIDATOR.validate(new StreamSource(new StringReader(config)));
-			// modify
-			int insertIndex = config.lastIndexOf("</xbel"); //cannot be -1 due to validation; we do not match the whole end tag, since between tag name and closing bracket can be whitespaces
-			var adjustedConfig = config.substring(0, insertIndex) //
-					+ "\n" //
-					+ ENTRY_TEMPLATE.formatted(target.toUri(), escapeXML(displayName), id).indent(1) //
-					+ "\n" //
-					+ config.substring(insertIndex);
-			return new EntryAndConfig(new DolphinPlacesEntry(id), adjustedConfig);
-		} catch (SAXException | IOException e) {
-			throw new QuickAccessServiceException("Adding entry to KDE places file failed.", e);
+			var xmlDocument = loadXmlDocument(config);
+			var nodeList = extractBookmarksByPath(target, xmlDocument);
+			removeStaleBookmarks(nodeList);
+			createBookmark(target, displayName, id, xmlDocument);
+			var changedConfig = documentToString(xmlDocument);
+			XML_VALIDATOR.validate(new StreamSource(new StringReader(changedConfig)));
+			return new EntryAndConfig(new DolphinPlacesEntry(id), changedConfig);
+		} catch (SAXException e) {
+			throw new QuickAccessServiceException("Invalid structure in xbel bookmark file", e);
+		} catch (IOException e) {
+			throw new QuickAccessServiceException("Failed reading/writing the xbel bookmark file", e);
 		}
 	}
 
-	private String escapeXML(String s) {
-		return s.replace("&","&amp;") //
-				.replace("<","&lt;") //
-				.replace(">","&gt;");
+	private void removeStaleBookmarks(NodeList nodeList) {
+		for (int i = nodeList.getLength() - 1; i >= 0; i--) {
+			Node node = nodeList.item(i);
+			node.getParentNode().removeChild(node);
+		}
+	}
+
+	private NodeList extractBookmarksByPath(Path target, Document xmlDocument) throws QuickAccessServiceException {
+		try {
+			var xpathFactory = XPathFactory.newInstance();
+			var xpath = xpathFactory.newXPath();
+			xpath.setXPathVariableResolver(v -> {
+				if (v.equals(new QName("uri"))) {
+					return target.toUri().toString();
+				}
+				throw new IllegalArgumentException();
+			});
+			var expression = "/xbel/bookmark[info/metadata[@owner='https://cryptomator.org']][@href=$uri]";
+			return (NodeList) xpath.compile(expression).evaluate(xmlDocument, XPathConstants.NODESET);
+		} catch (XPathExpressionException xee) {
+			throw new QuickAccessServiceException("Invalid XPath expression", xee);
+		}
+	}
+
+	private NodeList extractBookmarksById(String id, Document xmlDocument) throws QuickAccessServiceException {
+		try {
+			var xpathFactory = XPathFactory.newInstance();
+			var xpath = xpathFactory.newXPath();
+			xpath.setXPathVariableResolver(v -> {
+				if (v.equals(new QName("id"))) {
+					return id;
+				}
+				throw new IllegalArgumentException();
+			});
+			var expression = "/xbel/bookmark[info/metadata[@owner='https://cryptomator.org']][info/metadata/id[text()=$id]]";
+			return (NodeList) xpath.compile(expression).evaluate(xmlDocument, XPathConstants.NODESET);
+		} catch (XPathExpressionException xee) {
+			throw new QuickAccessServiceException("Invalid XPath expression", xee);
+		}
+	}
+
+	private Document loadXmlDocument(String config) throws QuickAccessServiceException {
+		try {
+			var builderFactory = DocumentBuilderFactory.newInstance();
+			builderFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+			builderFactory.setXIncludeAware(false);
+			builderFactory.setExpandEntityReferences(false);
+			builderFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
+			builderFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
+			builderFactory.setNamespaceAware(true);
+			DocumentBuilder builder = builderFactory.newDocumentBuilder();
+			// Prevent external entities from being resolved
+			builder.setEntityResolver((publicId, systemId) -> new InputSource(new StringReader("")));
+			return builder.parse(new ByteArrayInputStream(config.getBytes(StandardCharsets.UTF_8)));
+		} catch (IOException | SAXException | ParserConfigurationException e) {
+			throw new QuickAccessServiceException("Error while loading xml file", e);
+		}
+	}
+
+	private String documentToString(Document xmlDocument) throws QuickAccessServiceException {
+		try {
+			var buf = new StringWriter();
+			Transformer transformer = TransformerFactory.newInstance().newTransformer();
+			transformer.setOutputProperty(OutputKeys.DOCTYPE_PUBLIC, "");
+			transformer.setOutputProperty(OutputKeys.DOCTYPE_SYSTEM, "");
+			transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "no");
+			transformer.setOutputProperty(OutputKeys.INDENT, "yes");
+			transformer.setOutputProperty(OutputKeys.ENCODING, StandardCharsets.UTF_8.name());
+			transformer.transform(new DOMSource(xmlDocument), new StreamResult(buf));
+			var content = buf.toString();
+			content = content.replaceFirst("\\s*standalone=\"(yes|no)\"", "");
+			content = content.replaceFirst("<!DOCTYPE xbel PUBLIC \"\" \"\">","<!DOCTYPE xbel>");
+			return content;
+		} catch (TransformerException e) {
+			throw new QuickAccessServiceException("Error while serializing document to string", e);
+		}
+	}
+
+	/**
+	 *
+	 * Adds a xml bookmark element to the specified xml document
+	 *
+	 * <pre>{@code
+	 * <bookmark href="file:///home/someuser/folder1/">
+	 *   <title>integrations-linux</title>
+	 *   <info>
+	 *     <metadata owner="http://freedesktop.org">
+	 *       <bookmark:icon name="drive-harddisk-encrypted"/>
+	 *     </metadata>
+	 *     <metadata owner="https://cryptomator.org">
+	 *       <id>sldkf-sadf-sadf-sadf</id>
+	 *     </metadata>
+	 *   </info>
+	 * </bookmark>
+	 * }</pre>
+	 *
+	 * @param target The mount point of the vault
+	 * @param displayName Caption of the vault link in dolphin
+	 * @param xmlDocument The xbel document to which the bookmark should be added
+	 *
+	 * @throws QuickAccessServiceException if the bookmark could not be created
+	 */
+	private void createBookmark(Path target, String displayName, String id, Document xmlDocument) throws QuickAccessServiceException {
+		try {
+			var bookmark = xmlDocument.createElement("bookmark");
+			var title = xmlDocument.createElement("title");
+			var info = xmlDocument.createElement("info");
+			var metadataBookmark = xmlDocument.createElement("metadata");
+			var metadataOwner = xmlDocument.createElement("metadata");
+			var bookmarkIcon = xmlDocument.createElementNS(XBEL_NAMESPACE, "bookmark:icon");
+			var idElem = xmlDocument.createElement("id");
+			bookmark.setAttribute("href", target.toUri().toString());
+			title.setTextContent(displayName);
+			bookmark.appendChild(title);
+			bookmark.appendChild(info);
+			info.appendChild(metadataBookmark);
+			info.appendChild(metadataOwner);
+			metadataBookmark.appendChild(bookmarkIcon);
+			metadataOwner.appendChild(idElem);
+			metadataBookmark.setAttribute("owner", "http://freedesktop.org");
+			bookmarkIcon.setAttribute("name","drive-harddisk-encrypted");
+			metadataOwner.setAttribute("owner", "https://cryptomator.org");
+			idElem.setTextContent(id);
+			xmlDocument.getDocumentElement().appendChild(bookmark);
+		} catch (DOMException | IllegalArgumentException e) {
+			throw new QuickAccessServiceException("Error while creating bookmark for target: " + target, e);
+		}
 	}
 
 	private class DolphinPlacesEntry extends FileConfiguredQuickAccessEntry implements QuickAccessEntry {
@@ -97,46 +238,20 @@ private class DolphinPlacesEntry extends FileConfiguredQuickAccessEntry implemen
 		@Override
 		public String removeEntryFromConfig(String config) throws QuickAccessServiceException {
 			try {
-				int idIndex = config.lastIndexOf(id);
-				if (idIndex == -1) {
-					return config; //assume someone has removed our entry, nothing to do
-				}
-				//validate
-				XML_VALIDATOR.validate(new StreamSource(new StringReader(config)));
-				//modify
-				int openingTagIndex = indexOfEntryOpeningTag(config, idIndex);
-				var contentToWrite1 = config.substring(0, openingTagIndex).stripTrailing();
-
-				int closingTagEndIndex = config.indexOf('>', config.indexOf("</bookmark", idIndex));
-				var part2Tmp = config.substring(closingTagEndIndex + 1).split("\\A\\v+", 2); //removing leading vertical whitespaces, but no indentation
-				var contentToWrite2 = part2Tmp[part2Tmp.length - 1];
-
-				return contentToWrite1 + "\n" + contentToWrite2;
+				var xmlDocument = loadXmlDocument(config);
+				var nodeList = extractBookmarksById(id, xmlDocument);
+				removeStaleBookmarks(nodeList);
+				var changedConfig = documentToString(xmlDocument);
+				XML_VALIDATOR.validate(new StreamSource(new StringReader(changedConfig)));
+				return changedConfig;
 			} catch (IOException | SAXException | IllegalStateException e) {
 				throw new QuickAccessServiceException("Removing entry from KDE places file failed.", e);
 			}
 		}
-
-		/**
-		 * Returns the start index (inclusive)  of the {@link DolphinPlaces#ENTRY_TEMPLATE} entry
-		 * @param placesContent the content of the XBEL places file
-		 * @param idIndex start index (inclusive) of the entrys id tag value
-		 * @return start index of the first bookmark tag, searching backwards from idIndex
-		 */
-		private int indexOfEntryOpeningTag(String placesContent, int idIndex) {
-			var xmlWhitespaceChars = List.of(' ', '\t', '\n');
-			for (char c : xmlWhitespaceChars) {
-				int idx = placesContent.lastIndexOf("<bookmark" + c, idIndex); //with the whitespace we ensure, that no tags starting with "bookmark" (e.g. bookmarkz) are selected
-				if (idx != -1) {
-					return idx;
-				}
-			}
-			throw new IllegalStateException("Found entry id " + id + " in " + PLACES_FILE + ", but it is not a child of <bookmark> tag.");
-		}
 	}
 
 	@CheckAvailability
 	public static boolean isSupported() {
 		return Files.exists(PLACES_FILE);
 	}
-}
+}

src/main/java/org/cryptomator/linux/quickaccess/FileConfiguredQuickAccess.java

@@ -31,10 +31,20 @@ abstract class FileConfiguredQuickAccess implements QuickAccessService {
 		Runtime.getRuntime().addShutdownHook(new Thread(this::cleanup));
 	}
 
+	/**
+	 *
+	 * Adds the vault path to the quick-access config file
+	 *
+	 * @param target The mount point of the vault
+	 * @param displayName Caption of the vault link
+	 * @return A cleanup reference for vault link removal
+	 * @throws QuickAccessServiceException If the entry could not be added to the quick-access config file
+	 */
 	@Override
 	public QuickAccessEntry add(Path target, String displayName) throws QuickAccessServiceException {
 		try {
 			modifyLock.lock();
+			checkFileSize();
 			var entryAndConfig = addEntryToConfig(readConfig(), target, displayName);
 			persistConfig(entryAndConfig.config());
 			return entryAndConfig.entry();