Skip to content
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,10 @@

<!-- runtime dependencies -->

<api.version>1.6.0</api.version>
<api.version>1.7.0-SNAPSHOT</api.version>
<secret-service.version>2.0.1-alpha</secret-service.version>
<kdewallet.version>1.4.0</kdewallet.version>
<flatpakupdateportal.version>1.0.0</flatpakupdateportal.version>
<slf4j.version>2.0.17</slf4j.version>
<appindicator.version>1.4.2</appindicator.version>

Expand Down Expand Up @@ -88,6 +89,11 @@
<artifactId>libappindicator-gtk3-java-minimal</artifactId>
<version>${appindicator.version}</version>
</dependency>
<dependency>
<groupId>org.purejava</groupId>
<artifactId>flatpak-update-portal</artifactId>
<version>${flatpakupdateportal.version}</version>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter</artifactId>
Expand Down
7 changes: 6 additions & 1 deletion src/main/java/module-info.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,23 @@
import org.cryptomator.integrations.quickaccess.QuickAccessService;
import org.cryptomator.integrations.revealpath.RevealPathService;
import org.cryptomator.integrations.tray.TrayMenuController;
import org.cryptomator.integrations.update.UpdateMechanism;
import org.cryptomator.linux.autostart.FreedesktopAutoStartService;
import org.cryptomator.linux.keychain.KDEWalletKeychainAccess;
import org.cryptomator.linux.keychain.GnomeKeyringKeychainAccess;
import org.cryptomator.linux.keychain.KDEWalletKeychainAccess;
import org.cryptomator.linux.quickaccess.DolphinPlaces;
import org.cryptomator.linux.quickaccess.NautilusBookmarks;
import org.cryptomator.linux.revealpath.DBusSendRevealPathService;
import org.cryptomator.linux.tray.AppindicatorTrayMenuController;
import org.cryptomator.linux.update.FlatpakUpdater;

module org.cryptomator.integrations.linux {
requires org.cryptomator.integrations.api;
requires org.slf4j;
requires org.freedesktop.dbus;
requires org.purejava.appindicator;
requires org.purejava.kwallet;
requires org.purejava.portal;
requires de.swiesend.secretservice;
requires java.xml;

Expand All @@ -25,8 +28,10 @@
provides RevealPathService with DBusSendRevealPathService;
provides TrayMenuController with AppindicatorTrayMenuController;
provides QuickAccessService with NautilusBookmarks, DolphinPlaces;
provides UpdateMechanism with FlatpakUpdater;

opens org.cryptomator.linux.tray to org.cryptomator.integrations.api;
opens org.cryptomator.linux.quickaccess to org.cryptomator.integrations.api;
opens org.cryptomator.linux.autostart to org.cryptomator.integrations.api;
opens org.cryptomator.linux.update to org.cryptomator.integrations.api;
}
212 changes: 212 additions & 0 deletions src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,212 @@
package org.cryptomator.linux.update;

import org.cryptomator.integrations.common.CheckAvailability;
import org.cryptomator.integrations.common.DisplayName;
import org.cryptomator.integrations.common.OperatingSystem;
import org.cryptomator.integrations.common.Priority;
import org.cryptomator.integrations.update.UpdateFailedException;
import org.cryptomator.integrations.update.UpdateMechanism;
import org.cryptomator.integrations.update.UpdateStep;
import org.freedesktop.dbus.FileDescriptor;
import org.freedesktop.dbus.exceptions.DBusException;
import org.freedesktop.dbus.types.UInt32;
import org.freedesktop.dbus.types.Variant;
import org.purejava.portal.Flatpak;
import org.purejava.portal.FlatpakSpawnFlag;
import org.purejava.portal.UpdatePortal;
import org.purejava.portal.Util;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.IOException;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;

@Priority(1000)
@CheckAvailability
@DisplayName("Update via Flatpak update")
@OperatingSystem(OperatingSystem.Value.LINUX)
public class FlatpakUpdater implements UpdateMechanism {

private static final Logger LOG = LoggerFactory.getLogger(FlatpakUpdater.class);
private static final String APP_NAME = "org.cryptomator.Cryptomator";

private final UpdatePortal portal;

public FlatpakUpdater() {
this.portal = new UpdatePortal();
portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY);
}
Comment on lines +47 to +50
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove unnecessary monitor creation in constructor.

The constructor creates an update monitor but doesn't check or use the result. This monitor should be created lazily when first needed via getUpdateMonitor().

 public FlatpakUpdater() {
 	this.portal = new UpdatePortal();
-	portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public FlatpakUpdater() {
this.portal = new UpdatePortal();
portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY);
}
public FlatpakUpdater() {
this.portal = new UpdatePortal();
}
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java between lines
42 and 45, remove the call to
portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY) from the constructor
since the update monitor is created but never used there. Instead, implement
lazy initialization of the update monitor inside the getUpdateMonitor() method
so that the monitor is only created when first accessed.


@CheckAvailability
public boolean isSupported() {
return portal.isAvailable();
}

@Override
public boolean isUpdateAvailable(String installedVersion) {
var cdl = new CountDownLatch(1);
portal.setUpdateCheckerTaskFor(APP_NAME);
var checkTask = portal.getUpdateCheckerTaskFor(APP_NAME);
var updateAvailable = new AtomicBoolean(false);
checkTask.setOnSucceeded(updateVersion -> {
updateAvailable.set(UpdateMechanism.isUpdateAvailable(updateVersion, installedVersion));
cdl.countDown();
});
checkTask.setOnFailed(error -> {
LOG.warn("Error while checking for updates.", error);
cdl.countDown();
});
try {
cdl.await();
return updateAvailable.get();
} catch (InterruptedException e) {
checkTask.cancel();
Thread.currentThread().interrupt();
return false;
}
}

@Override
public UpdateStep firstStep() throws UpdateFailedException {
var monitorPath = portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY);
if (monitorPath == null) {
throw new UpdateFailedException("Failed to create UpdateMonitor on DBus");
}

return new FlatpakUpdateStep(portal.getUpdateMonitor(monitorPath.toString()));
}

private class FlatpakUpdateStep implements UpdateStep {

private final CountDownLatch latch = new CountDownLatch(1);
private final Flatpak.UpdateMonitor monitor;
private volatile double progress = 0.0;
private volatile UpdateFailedException error;
private AutoCloseable signalHandler;

private FlatpakUpdateStep(Flatpak.UpdateMonitor monitor) {
this.monitor = monitor;
}

@Override
public String description() {
return "Updating via Flatpak... %1.0f%%".formatted(preparationProgress() * 100);
}

@Override
public void start() {
try {
this.signalHandler = portal.getDBusConnection().addSigHandler(Flatpak.UpdateMonitor.Progress.class, this::handleProgressSignal);
} catch (DBusException e) {
LOG.error("DBus error", e);
latch.countDown();
}
portal.updateApp("x11:0", monitor, UpdatePortal.OPTIONS_DUMMY);
}

private void handleProgressSignal(Flatpak.UpdateMonitor.Progress signal) {
int status = ((UInt32) signal.info.get("status").getValue()).intValue();
switch (status) {
case 0 -> { // In progress
Variant<?> progressVariant = signal.info.get("progress");
if (progressVariant != null) {
progress = ((UInt32) progressVariant.getValue()).doubleValue() / 100.0; // progress reported as int in range [0, 100]
}
}
case 1 -> { // No update available
error = new UpdateFailedException("No update available");
latch.countDown();
}
case 2 -> { // Update complete
progress = 1.0;
latch.countDown();
}
case 3 -> { // Update failed
error = new UpdateFailedException("Update preparation failed");
latch.countDown();
}
default -> {
error = new UpdateFailedException("Unknown update status " + status);
latch.countDown();
}
}
}
Comment on lines +138 to +164
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add null and type safety checks for signal data.

Line 113 performs an unsafe cast without checking if signal.info.get("status") is null or if the value is actually a UInt32. This risks NullPointerException or ClassCastException.

Apply defensive checks:

 private void handleProgressSignal(Flatpak.UpdateMonitor.Progress signal) {
-	int status = ((UInt32) signal.info.get("status").getValue()).intValue();
+	Variant<?> statusVariant = signal.info.get("status");
+	if (statusVariant == null || !(statusVariant.getValue() instanceof UInt32)) {
+		LOG.warn("Invalid or missing status in progress signal");
+		return;
+	}
+	int status = ((UInt32) statusVariant.getValue()).intValue();
 	switch (status) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private void handleProgressSignal(Flatpak.UpdateMonitor.Progress signal) {
int status = ((UInt32) signal.info.get("status").getValue()).intValue();
switch (status) {
case 0 -> { // In progress
Variant<?> progressVariant = signal.info.get("progress");
if (progressVariant != null) {
progress = ((UInt32) progressVariant.getValue()).doubleValue() / 100.0; // progress reported as int in range [0, 100]
}
}
case 1 -> { // No update available
error = new UpdateFailedException("No update available");
latch.countDown();
}
case 2 -> { // Update complete
progress = 1.0;
latch.countDown();
}
case 3 -> { // Update failed
error = new UpdateFailedException("Update preparation failed");
latch.countDown();
}
default -> {
error = new UpdateFailedException("Unknown update status " + status);
latch.countDown();
}
}
}
private void handleProgressSignal(Flatpak.UpdateMonitor.Progress signal) {
Variant<?> statusVariant = signal.info.get("status");
if (statusVariant == null || !(statusVariant.getValue() instanceof UInt32)) {
LOG.warn("Invalid or missing status in progress signal");
return;
}
int status = ((UInt32) statusVariant.getValue()).intValue();
switch (status) {
case 0 -> { // In progress
Variant<?> progressVariant = signal.info.get("progress");
if (progressVariant != null) {
progress = ((UInt32) progressVariant.getValue()).doubleValue() / 100.0; // progress reported as int in range [0, 100]
}
}
case 1 -> { // No update available
error = new UpdateFailedException("No update available");
latch.countDown();
}
case 2 -> { // Update complete
progress = 1.0;
latch.countDown();
}
case 3 -> { // Update failed
error = new UpdateFailedException("Update preparation failed");
latch.countDown();
}
default -> {
error = new UpdateFailedException("Unknown update status " + status);
latch.countDown();
}
}
}
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java around lines
112 to 138, the code unsafely casts signal.info.get("status").getValue() to
UInt32 and similarly assumes "progress" exists and is UInt32 — add defensive
null and type checks: retrieve the Variant<?> statusVariant =
signal.info.get("status"), if null set error = new
UpdateFailedException("Missing update status") and latch.countDown(); otherwise
get Object statusVal = statusVariant.getValue(), verify statusVal instanceof
UInt32 before casting, and if not set error with a descriptive message and
latch.countDown(); apply the same pattern for the "progress" variant (check null
and instanceof UInt32 before using), and ensure every early-error path sets
error and counts down the latch so the caller is not left waiting.


private void stopReceivingSignals() {
if (signalHandler != null) {
try {
signalHandler.close();
} catch (Exception e) {
LOG.error("Failed to close signal handler", e);
}
signalHandler = null;
}
}

@Override
public double preparationProgress() {
return progress;
}

@Override
public void cancel() {
portal.cancelUpdateMonitor(monitor);
stopReceivingSignals();
portal.close(); // TODO: is this right? belongs to parent class. update can not be retried afterwards. or should each process have its own portal instance?
error = new UpdateFailedException("Update cancelled by user");
}
Comment on lines +184 to +188
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Portal lifecycle & resource leak

cancel() closes the shared portal, but the normal success path (applyUpdate) never does, leaving the DBus connection open for the lifetime of the JVM. Either:
• Make FlatpakUpdateProcess own a dedicated UpdatePortal and always close it in finally, or
• Let FlatpakUpdater implement AutoCloseable and close the portal when the application shuts down.

🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java around lines
154 to 158, the shared portal is closed on cancel but not on successful update,
causing a resource leak. To fix this, either refactor FlatpakUpdateProcess to
own a dedicated UpdatePortal instance and ensure it is closed in a finally block
after update attempts, or modify FlatpakUpdater to implement AutoCloseable and
close the shared portal when the application shuts down, ensuring the DBus
connection is properly released in all cases.

Comment on lines +183 to +188
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Ensure cancel completes and leaves the portal reusable.
After a user cancels, callers waiting on await()/isDone() block forever because the latch is never counted down. On top of that, closing the shared UpdatePortal here makes every subsequent update attempt with this FlatpakUpdater fail against the closed connection. Drop the close from the per-step cancel path and count the latch down so cancellation is deterministic; manage portal lifecycle at the updater level instead.

-            portal.cancelUpdateMonitor(monitor);
-            stopReceivingSignals();
-            portal.close(); // TODO: is this right? belongs to parent class. update can not be retried afterwards. or should each process have its own portal instance?
-            error = new UpdateFailedException("Update cancelled by user");
+            portal.cancelUpdateMonitor(monitor);
+            stopReceivingSignals();
+            error = new UpdateFailedException("Update cancelled by user");
+            latch.countDown();
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java around lines
183-188, the cancel() method currently closes the shared UpdatePortal and never
signals completion, causing await()/isDone() to block and later updates to fail
on a closed portal; modify cancel() to NOT call portal.close() (remove that
line), ensure the latch or completion signal used by await()/isDone() is counted
down (or set the done flag and notify listeners) so callers unblock
deterministically, keep portal.cancelUpdateMonitor(monitor) and
stopReceivingSignals(), and manage the portal lifecycle at the updater level
instead of per-step so subsequent updates can reuse the portal.


@Override
public void await() throws InterruptedException {
latch.await();
}

@Override
public boolean await(long timeout, TimeUnit unit) throws InterruptedException {
return latch.await(timeout, unit);
}

@Override
public boolean isDone() {
try {
return latch.await(0, TimeUnit.MILLISECONDS);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
return false;
}
}

@Override
public UpdateStep nextStep() throws IllegalStateException, IOException {
return UpdateStep.of("Restarting application", this::applyUpdate);
}

public UpdateStep applyUpdate() throws IllegalStateException, IOException {
if (!isDone()) {
throw new IllegalStateException("Update preparation is not complete");
}
stopReceivingSignals();
if (error != null) {
throw error;
}

// spawn new Cryptomator process:
var cwdPath = Util.stringToByteList(System.getProperty("user.dir"));
List<List<Byte>> argv = List.of(
Util.stringToByteList(APP_NAME));
Map<UInt32, FileDescriptor> fds = Collections.emptyMap();
Map<String, String> envs = Map.of();
UInt32 flags = new UInt32(FlatpakSpawnFlag.LATEST_VERSION.getValue());
Map<String, Variant<?>> options = UpdatePortal.OPTIONS_DUMMY;
var pid = portal.Spawn(cwdPath, argv, fds, envs, flags, options).longValue();
LOG.info("Spawned updated Cryptomator process with PID {}", pid);
return null;
}
Comment on lines 215 to 235
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Complete the implementation: return ProcessHandle instead of null.

Line 208 returns null, which violates the return type contract. The spawned process PID is logged but not returned as a ProcessHandle. Additionally, the portal is never closed in the success path, leaving the DBus connection open.

Consider these improvements:

 public UpdateStep applyUpdate() throws IllegalStateException, IOException {
 	if (!isDone()) {
 		throw new IllegalStateException("Update preparation is not complete");
 	}
 	stopReceivingSignals();
 	if (error != null) {
 		throw error;
 	}

 	// spawn new Cryptomator process:
 	var cwdPath = Util.stringToByteList(System.getProperty("user.dir"));
 	List<List<Byte>> argv = List.of(
 			Util.stringToByteList(APP_NAME));
 	Map<UInt32, FileDescriptor> fds = Collections.emptyMap();
 	Map<String, String> envs = Map.of();
 	UInt32 flags = new UInt32(FlatpakSpawnFlag.LATEST_VERSION.getValue());
 	Map<String, Variant<?>> options = UpdatePortal.OPTIONS_DUMMY;
-	var pid = portal.Spawn(cwdPath, argv, fds, envs, flags, options).longValue();
+	long pid = portal.Spawn(cwdPath, argv, fds, envs, flags, options).longValue();
 	LOG.info("Spawned updated Cryptomator process with PID {}", pid);
-	return null;
+	try {
+		return UpdateStep.of("Update complete", () -> ProcessHandle.of(pid).orElseThrow());
+	} catch (Exception e) {
+		throw new IOException("Failed to obtain ProcessHandle for spawned process", e);
+	}
 }

Note: Also address the portal lifecycle issue raised in the cancel() method review to ensure proper resource cleanup.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java around lines
189 to 209, the method applyUpdate currently logs the spawned process PID but
returns null and never closes the portal; change it to return a ProcessHandle
corresponding to the spawned PID (e.g., ProcessHandle.of(pid).orElseThrow(...)
or similar) instead of null, and ensure the portal is closed in the success path
before returning (mirror the cleanup used in the cancel() method or refactor
portal lifecycle so both success and cancel paths close it reliably), making
sure any thrown exceptions still result in portal closure to avoid leaking the
DBus connection.

}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
org.cryptomator.linux.update.FlatpakUpdater