Conversation
…ServiceKeychainAccess
Add handlers
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new SecretServiceKeychainAccess implementation and registers it as a KeychainAccessProvider; updates module-info to require org.purejava.secret and imports the new class; adds org.purejava:secret-service to Maven properties/dependencies; registers the new provider in META-INF/services; introduces JUnit tests for the Secret Service backend (environment-gated, functional CRUD tests); marks GnomeKeyringKeychainAccess and KDEWalletKeychainAccess as deprecated. No other behavior outside the new Secret Service integration and deprecation metadata was changed. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java (6)
35-49: Avoid side effects in constructor; defer alias setup and migration.Constructor sets alias and migrates immediately, which can trigger unlock prompts at startup. Defer to first safe point (e.g., after isSupported() succeeds and store/load/delete is invoked) and gate with a once-only flag.
@@ - public SecretServiceKeychainAccess() { + private static final java.util.concurrent.atomic.AtomicBoolean MIGRATED = new java.util.concurrent.atomic.AtomicBoolean(false); + + public SecretServiceKeychainAccess() { session.getService().addCollectionChangedHandler(collection -> LOG.debug("Collection {} changed", collection.getPath())); @@ - var getAlias = session.getService().readAlias("default"); - if (getAlias.isSuccess() && "/".equals(getAlias.value().getPath())) { - // default alias is not set; set it to the login keyring - session.getService().setAlias("default", new DBusPath(Static.DBusPath.LOGIN_COLLECTION)); - } + // Defer alias setup until session is established @@ - migrateKDEWalletEntries(); + // Defer migration; run once after session is ready (see isSupported()) } + + private void ensurePostInit() { + // Safe to call often + var getAlias = session.getService().readAlias("default"); + if (getAlias.isSuccess() && "/".equals(getAlias.value().getPath())) { + session.getService().setAlias("default", new DBusPath(Static.DBusPath.LOGIN_COLLECTION)); + } + if (MIGRATED.compareAndSet(false, true)) { + migrateKDEWalletEntries(); + } + }
77-96: Make load resilient; ensure session and post-init.Prevent dependency on external initialization.
@@ public char[] loadPassphrase(String key) throws KeychainAccessException { try { + if (!session.setupEncryptedSession()) { + throw new KeychainAccessException("Loading password failed: unable to establish encrypted session."); + } + ensurePostInit(); var call = collection.searchItems(createAttributes(key));
98-121: Same initialization concerns in deletePassphrase.Mirror the session/post-init checks.
@@ public void deletePassphrase(String key) throws KeychainAccessException { try { + if (!session.setupEncryptedSession()) { + throw new KeychainAccessException("Deleting password failed: unable to establish encrypted session."); + } + ensurePostInit(); var call = collection.searchItems(createAttributes(key));
123-146: Same initialization concerns in changePassphrase.Add session handshake and post-init.
@@ public void changePassphrase(String key, String displayName, CharSequence passphrase) throws KeychainAccessException { try { + if (!session.setupEncryptedSession()) { + throw new KeychainAccessException("Updating password failed: unable to establish encrypted session."); + } + ensurePostInit(); var call = collection.searchItems(createAttributes(key));
154-158: isLocked should tolerate uninitialized session.Optional, but avoids false negatives.
@@ public boolean isLocked() { - var call = collection.isLocked(); + session.setupEncryptedSession(); + ensurePostInit(); + var call = collection.isLocked(); return call.isSuccess() && call.value(); }
164-191: Migration hygiene: avoid String for secrets, wipe char[], and check delete result.Reduce exposure of secrets and handle errors explicitly.
@@ - var getItems = collection.getItems(); + var getItems = collection.getItems(); if (getItems.isSuccess() && !getItems.value().isEmpty()) { for (DBusPath i : getItems.value()) { session.getService().ensureUnlocked(i); var attribs = new Item(i).getAttributes(); if (attribs.isSuccess() && attribs.value().containsKey("server") && attribs.value().containsKey("user") && attribs.value().get("server").equals("Cryptomator")) { @@ - var item = new Item(i); - var secret = item.getSecret(session.getSession()); - try { - storePassphrase(attribs.value().get("user"), "Cryptomator", new String(session.decrypt(secret))); - } catch (KeychainAccessException | NoSuchPaddingException | NoSuchAlgorithmException | - InvalidAlgorithmParameterException | InvalidKeyException | BadPaddingException | - IllegalBlockSizeException e) { - LOG.error("Migrating entry {} for vault {} failed", i.getPath(), attribs.value().get("user")); - } - item.delete(); - LOG.info("Successfully migrated password for vault {}", attribs.value().get("user")); + var item = new Item(i); + var secret = item.getSecret(session.getSession()); + char[] decrypted = null; + try { + decrypted = session.decrypt(secret); + storePassphrase(attribs.value().get("user"), "Cryptomator", java.nio.CharBuffer.wrap(decrypted)); + var deleted = item.delete(); + if (!deleted.isSuccess()) { + LOG.warn("Migrated but failed to delete legacy entry {} for vault {}", i.getPath(), attribs.value().get("user")); + } else { + LOG.info("Successfully migrated password for vault {}", attribs.value().get("user")); + } + } catch (KeychainAccessException | NoSuchPaddingException | NoSuchAlgorithmException | + InvalidAlgorithmParameterException | InvalidKeyException | BadPaddingException | + IllegalBlockSizeException e) { + LOG.error("Migrating entry {} for vault {} failed", i.getPath(), attribs.value().get("user"), e); + } finally { + if (decrypted != null) { + java.util.Arrays.fill(decrypted, '\0'); + } + } } } }src/test/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccessTest.java (5)
22-24: Avoid DISPLAY gate; rely on actual D‑Bus availability.DISPLAY may not be set on Wayland/headless although org.freedesktop.secrets is available. Prefer enabling based on the service probe.
-@EnabledIfEnvironmentVariable(named = "DISPLAY", matches = ".*") -public class SecretServiceKeychainAccessTest { +public class SecretServiceKeychainAccessTest {
27-41: Do not throw from @BeforeAll; catch and mark unavailable.Throwing IOException from @BeforeAll can fail the whole class on environments without dbus-send/grep.
- @BeforeAll - public static void checkSystemAndSetup() throws IOException { + @BeforeAll + public static void checkSystemAndSetup() { ProcessBuilder dbusSend = new ProcessBuilder("dbus-send", "--print-reply", "--dest=org.freedesktop.DBus", "/org/freedesktop/DBus", "org.freedesktop.DBus.ListNames"); ProcessBuilder grep = new ProcessBuilder("grep", "-q", "org.freedesktop.secrets"); try { Process end = ProcessBuilder.startPipeline(List.of(dbusSend, grep)).get(1); if (end.waitFor(1000, TimeUnit.MILLISECONDS)) { isInstalled = end.exitValue() == 0; } else { isInstalled = false; } - } catch (InterruptedException e) { + } catch (InterruptedException e) { Thread.currentThread().interrupt(); + isInstalled = false; + } catch (IOException e) { + isInstalled = false; } }
45-47: Assert supported state to surface misconfig early.If the service is probed as installed, assert that isSupported() is true to catch alias/session issues.
- var service = new SecretServiceKeychainAccess(); - Assertions.assertEquals(isInstalled, service.isSupported()); + var service = new SecretServiceKeychainAccess(); + Assertions.assertEquals(isInstalled, service.isSupported(), "Secret Service installed state should match isSupported()");
51-56: KEYRING self‑init shouldn’t require manual warm‑up.After provider changes to self‑initialize, this warm‑up call can be removed or asserted.
- final static SecretServiceKeychainAccess KEYRING = new SecretServiceKeychainAccess(); + final static SecretServiceKeychainAccess KEYRING = new SecretServiceKeychainAccess();(Leave as-is for now; consider removing KEYRING.isSupported() below once provider self-inits.)
60-62: Turn warm‑up into an assertion (or remove after provider self‑inits).- KEYRING.isSupported(); // ensure encrypted session + Assertions.assertTrue(KEYRING.isSupported(), "Secret Service is not supported"); KEYRING.storePassphrase(KEY_ID, "cryptomator-test", "p0ssw0rd");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pom.xml(2 hunks)src/main/java/module-info.java(2 hunks)src/main/java/org/cryptomator/linux/keychain/GnomeKeyringKeychainAccess.java(0 hunks)src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java(0 hunks)src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java(1 hunks)src/main/resources/META-INF/services/org.cryptomator.integrations.keychain.KeychainAccessProvider(1 hunks)src/test/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccessTest.java(0 hunks)src/test/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccessTest.java(2 hunks)
💤 Files with no reviewable changes (3)
- src/main/java/org/cryptomator/linux/keychain/GnomeKeyringKeychainAccess.java
- src/test/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccessTest.java
- src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java
🔇 Additional comments (3)
src/main/resources/META-INF/services/org.cryptomator.integrations.keychain.KeychainAccessProvider (1)
1-1: Service registration aligned with the unified provider.Matches module-info; good for classpath consumers. No further action.
pom.xml (1)
44-44: All verification checks passed—no issues found.JPMS module coordination is correct:
org.purejava.secretmodule is properly declared inrequires(module-info.java:18)- Service descriptor correctly points to
SecretServiceKeychainAccessimplementation- No stale references to
KDEWalletKeychainAccess,GnomeKeyringKeychainAccess,de.swiesend.secretservice, orkwalletexist in the codebase- pom.xml dependency correctly updated to
org.purejava:secret-service:1.0.0src/main/java/module-info.java (1)
7-8: No module name mismatch found; code is correct as-is.Verification confirms that
requires org.purejava.secret;correctly matches the actual module name declared in the org.purejava:secret-service library. The library's module-info.java declaresmodule org.purejava.secret, which aligns with your requires statement. Service wiring to SecretServiceKeychainAccess is properly configured in the provides clause.
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
Show resolved
Hide resolved
src/test/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccessTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This pull request replaces the existing GNOME Keyring and KDE Wallet keychain implementations with a unified Secret Service implementation that adheres to the latest Secret Service API Draft 0.2. This allows for a single keychain interface on Linux, as KWallet has supported the Secret Service API since version v5.97.0 (August 2022). The new implementation maintains compatibility with existing GNOME keyring entries and includes automatic migration for former KDE wallet entries.
Key Changes:
- Unified Secret Service implementation replacing separate GNOME Keyring and KDE Wallet providers
- Automatic migration of KDE Wallet entries to the new Secret Service format
- Updated dependencies from
de.swiesend:secret-service:2.0.1-alphaandorg.purejava:kdewallet:1.4.0toorg.purejava:secret-service:1.0.0
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| SecretServiceKeychainAccess.java | New unified keychain implementation using Secret Service API with KDE Wallet entry migration |
| GnomeKeyringKeychainAccess.java | Removed old GNOME Keyring implementation |
| KDEWalletKeychainAccess.java | Removed old KDE Wallet implementation |
| SecretServiceKeychainAccessTest.java | Updated tests to use new Secret Service implementation |
| KDEWalletKeychainAccessTest.java | Removed KDE Wallet-specific tests |
| module-info.java | Updated module to reference new Secret Service provider and dependency |
| pom.xml | Updated to use single org.purejava:secret-service:1.0.0 dependency |
| META-INF/services/org.cryptomator.integrations.keychain.KeychainAccessProvider | Updated service provider configuration to reference unified implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
Outdated
Show resolved
Hide resolved
|
Haven't looked into the details yet, but I would like to keep the old implementations (mark them as deprecated) and remove them in a separate PR. This way we can release a version where in case of a bug the user can switch back to the old impl. If everything went smooth, the next Cryptomator release will then drop these. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pom.xml(2 hunks)src/main/java/module-info.java(2 hunks)src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java(1 hunks)src/main/resources/META-INF/services/org.cryptomator.integrations.keychain.KeychainAccessProvider(1 hunks)src/test/java/org/cryptomator/linux/keychain/GnomeKeyringKeychainAccessTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/test/java/org/cryptomator/linux/keychain/GnomeKeyringKeychainAccessTest.java
- src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java
🚧 Files skipped from review as they are similar to previous changes (1)
- pom.xml
🔇 Additional comments (2)
src/main/resources/META-INF/services/org.cryptomator.integrations.keychain.KeychainAccessProvider (1)
1-3: LGTM! Provider ordering and backwards compatibility look good.The new
SecretServiceKeychainAccessis correctly registered as the first provider, which means it will be discovered first by Java'sServiceLoadermechanism. Keeping the existing KDE Wallet and GNOME Keyring providers ensures backwards compatibility, aligning with the maintainer's suggestion to retain old implementations.src/main/java/module-info.java (1)
8-9: LGTM! Import additions are correct.The new
SecretServiceKeychainAccessimport is properly added to support the new provider registration.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/module-info.java(2 hunks)
🔇 Additional comments (2)
src/main/java/module-info.java (2)
8-9: LGTM: Import statements updated correctly.The new
SecretServiceKeychainAccessimport is properly added to support the unified Linux keychain backend.
26-26: LGTM: Provider registration order is correct.The provider list correctly places
SecretServiceKeychainAccessfirst, making it the preferred implementation while retainingGnomeKeyringKeychainAccessandKDEWalletKeychainAccessfor backward compatibility. This aligns with the maintainer's guidance to keep old implementations available initially.Note: As flagged in past review comments, the old implementations should be marked with
@Deprecatedannotations in their respective class files.
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
Outdated
Show resolved
Hide resolved
Give it a higher priority Use latest Secret Service library
purejava
force-pushed
the
new-secret-service
branch
from
2366a34 to
857c812
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/java/module-info.java (1)
31-31: Verify that old keychain implementations are marked@Deprecated.The provider list correctly prioritizes
SecretServiceKeychainAccessfirst, with the old implementations (GnomeKeyringKeychainAccess,KDEWalletKeychainAccess) retained for backwards compatibility. Per the maintainer's request in the PR comments, these old implementations should be marked with@Deprecatedannotations before removal in a future release.#!/bin/bash # Check if old keychain implementations have @Deprecated annotations echo "=== Checking GnomeKeyringKeychainAccess ===" if rg -q "@Deprecated" src/main/java/org/cryptomator/linux/keychain/GnomeKeyringKeychainAccess.java; then echo "✓ GnomeKeyringKeychainAccess is marked @Deprecated" else echo "✗ GnomeKeyringKeychainAccess is NOT marked @Deprecated" fi echo "" echo "=== Checking KDEWalletKeychainAccess ===" if rg -q "@Deprecated" src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java; then echo "✓ KDEWalletKeychainAccess is marked @Deprecated" else echo "✗ KDEWalletKeychainAccess is NOT marked @Deprecated" fisrc/test/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccessTest.java (1)
22-22: Class-levelDISPLAYgate is too restrictive; rely on nested class gate instead.The
@EnabledIfEnvironmentVariable(named = "DISPLAY", matches = ".*")annotation at the class level preventstestIsSupported()from running even though it usesdbus-sendCLI and doesn't require a display. The nestedFunctionalTestsclass already has@EnabledIf("serviceAvailableAndUnlocked"), which comprehensively checks both service availability and lock state.Remove the class-level
DISPLAYgate to allowtestIsSupported()to run in headless environments while functional tests remain properly gated.🔎 Proposed fix
/** * Unit tests for Secret Service access via Dbus. */ -@EnabledIfEnvironmentVariable(named = "DISPLAY", matches = ".*") public class SecretServiceKeychainAccessTest {src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java (1)
177-194: Risk of data loss if migration fails after deletion.Line 186 deletes the original KDE Wallet item before line 188 confirms that
storePassphrasesucceeded. If storing the migrated passphrase fails (network issue, permissions, disk full, etc.), the original data is permanently lost.Per the maintainer's earlier comment, deletion should only occur after successful migration. The current code structure has improved but still deletes before the final store completes.
🔎 Recommended fix: Move deletion after successful migration
var itemProps = Item.createProperties(label, newAttribs); var replace = collection.createItem(itemProps, secret, true); if (!replace.isSuccess()) { LOG.error("Replacing migrated item {} failed: {}", i.getPath(), replace.error()); continue; } - item.delete(); try { storePassphrase(attribs.value().get("user"), "Cryptomator", new String(session.decrypt(secret))); + item.delete(); LOG.info("Successfully migrated password for vault {}", attribs.value().get("user")); } catch (KeychainAccessException | NoSuchPaddingException | NoSuchAlgorithmException | InvalidAlgorithmParameterException | InvalidKeyException | BadPaddingException | IllegalBlockSizeException e) { LOG.error("Migrating entry {} for vault {} failed", i.getPath(), attribs.value().get("user")); }
🧹 Nitpick comments (1)
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java (1)
36-50: Consider deferring migration to first use rather than constructor.Running
migrateKDEWalletEntries()in the constructor can cause the constructor to fail or block for an extended time if migration encounters issues. This impacts application startup and makes error handling more difficult.Consider deferring migration to the first keychain operation (e.g., lazy initialization pattern) or providing a separate
initialize()method that callers can invoke explicitly with appropriate error handling.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pom.xml(2 hunks)src/main/java/module-info.java(2 hunks)src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java(1 hunks)src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java(1 hunks)src/main/resources/META-INF/services/org.cryptomator.integrations.keychain.KeychainAccessProvider(1 hunks)src/test/java/org/cryptomator/linux/keychain/GnomeKeyringKeychainAccessTest.java(1 hunks)src/test/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccessTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/org/cryptomator/linux/keychain/GnomeKeyringKeychainAccessTest.java
- src/main/resources/META-INF/services/org.cryptomator.integrations.keychain.KeychainAccessProvider
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-07-16T22:36:32.769Z
Learnt from: infeo
Repo: cryptomator/integrations-linux PR: 80
File: src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java:56-62
Timestamp: 2024-07-16T22:36:32.769Z
Learning: For the `FreedesktopAutoStartService` in the Cryptomator project, exceptions are preferred to contain all necessary debugging information without additional logging before throwing them.
Applied to files:
src/main/java/module-info.java
🔇 Additional comments (1)
pom.xml (1)
107-111: Verify thatorg.purejava:secret-service:1.0.1is published and accessible.This PR adds
org.purejava:secret-serviceversion1.0.1alongsidede.swiesend:secret-serviceversion2.0.1-alpha. Prior reviews indicated version 1.0.0 was unavailable on Maven Central. Confirm version 1.0.1 is published before merging to prevent build failures.
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
Outdated
Show resolved
Hide resolved
|
I did another test from top to toe and noticed a corner case, where an Item on the DBus provided by gnome-keyring suddenly disappears, after all Items from the collection have been retrieved by This in the end kept the 16:23:08.290 [JavaFX Application Thread] INFO o.f.d.c.transports.TransportBuilder - Using transport dbus-java-transport-native-unixsocket for address unix:path=/run/user/1000/bus
16:23:08.357 [JavaFX Application Thread] WARN org.purejava.secret.api.Item - DBus error on calling Get(Locked) for /org/freedesktop/secrets/collection/login/7: Das Objekt existiert nicht am Pfad »/org/freedesktop/secrets/collection/login/7«
16:23:08.358 [JavaFX Application Thread] WARN org.purejava.secret.api.Item - DBus error on calling Get(Attributes) for /org/freedesktop/secrets/collection/login/7: Das Objekt existiert nicht am Pfad »/org/freedesktop/secrets/collection/login/7«
16:23:08.374 [JavaFX Application Thread] INFO o.f.d.c.transports.TransportBuilder - Using transport dbus-java-transport-native-unixsocket for address unix:path=/run/user/1000/bus |
infeo
left a comment
infeo
left a comment
There was a problem hiding this comment.
I finally had the time to review. Will test it in the next days and hopefully this will be integrated into the next Cryptomator release.
src/test/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccessTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/test/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccessTest.java`:
- Around line 29-37: The pipeline started via
ProcessBuilder.startPipeline(List.of(dbusSend, grep)) can leave child processes
running when end.waitFor(1000, TimeUnit.MILLISECONDS) times out; change the code
to capture the full Process[] returned by startPipeline, call waitFor on the
final process as before, and if it times out iterate over all processes (from
the Process[]), call destroy() then destroyForcibly() if needed and waitFor
their termination to ensure no stray dbus-send/grep remain; reference the
variables ProcessBuilder dbusSend, grep, the Process[] from
ProcessBuilder.startPipeline and the Process end when implementing the cleanup
on timeout.
🧹 Nitpick comments (1)
src/test/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccessTest.java (1)
22-23: Add an OS gate to prevent non-Linux runs.This test suite is Linux-specific; add
@EnabledOnOs(OS.LINUX)to avoid accidental execution on other platforms where DBus behavior is undefined.🔧 Suggested change
import org.junit.jupiter.api.condition.EnabledIf; import org.junit.jupiter.api.condition.EnabledIfEnvironmentVariable; +import org.junit.jupiter.api.condition.EnabledOnOs; +import org.junit.jupiter.api.condition.OS; @@ `@EnabledIfEnvironmentVariable`(named = "DBUS_SESSION_BUS_ADDRESS", matches = ".*") +@EnabledOnOs(OS.LINUX) public class SecretServiceKeychainAccessTest {
src/test/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccessTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java`:
- Around line 154-158: The isLocked() method in SecretServiceKeychainAccess uses
collection.isLocked() and currently returns call.isSuccess() && call.value(),
which yields false on errors; change it to fail-safe and return true when
call.isSuccess() is false (i.e., treat unknown/error as locked) so it matches
GnomeKeyringKeychainAccess/KDEWalletKeychainAccess behavior — update the logic
in SecretServiceKeychainAccess.isLocked() to return call.value() when successful
and true when not.
- Around line 111-114: SecretServiceKeychainAccess currently throws a
KeychainAccessException when deletePassphrase cannot find the item ("Vault {key}
not found, deleting failed"), which is inconsistent with
GnomeKeyringKeychainAccess and KDEWalletKeychainAccess that treat missing items
as a no-op; make delete idempotent by converting the "not found" branch in
SecretServiceKeychainAccess (the block that currently builds msg and throws
KeychainAccessException) to silently succeed (optionally log at debug/trace
level) and return normally so all implementations consistently no-op on missing
keys, or alternatively document and centralize behavior if you prefer strict
semantics across all KeychainAccess implementations.
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
Show resolved
Hide resolved
src/test/java/org/cryptomator/linux/keychain/GnomeKeyringKeychainAccessTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/main/java/org/cryptomator/linux/keychain/GnomeKeyringKeychainAccess.java`:
- Around line 16-22: The Javadoc version and the `@Deprecated` annotation in
GnomeKeyringKeychainAccess are inconsistent (Javadoc "since version 1.19.0" vs
`@Deprecated`(since = "1.7.0")); update one so both match—e.g., change the
`@Deprecated`(since = "...") value to "1.19.0" (or adjust the Javadoc to "1.7.0"
if that is the intended baseline) so the class GnomeKeyringKeychainAccess and
its deprecation metadata are consistent with KDEWalletKeychainAccess.
In `@src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java`:
- Around line 22-28: The Javadoc deprecation tag and the `@Deprecated`(since =
"...") annotation on KDEWalletKeychainAccess disagree (Javadoc says 1.19.0,
annotation says 1.7.0); update them to be consistent by either changing the
Javadoc to reference the same version scheme as the annotation or update the
`@Deprecated`(since = "...") value to match the Javadoc, or alternatively add a
brief clarifying note in the Javadoc explaining the two different versioning
schemes (integrations-linux vs. Cryptomator) and then align the
`@Deprecated`(since) value accordingly so both the Javadoc and the `@Deprecated`
annotation convey the same, unambiguous deprecation version for
KDEWalletKeychainAccess.
🧹 Nitpick comments (1)
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java (1)
72-74: Exception double-wrapping:KeychainAccessExceptiongets re-wrapped.The
catch (Exception e)block catchesKeychainAccessExceptioninstances thrown at lines 64 and 70, wrapping them in anotherKeychainAccessException. This creates nested exceptions with similar messages, making debugging harder.♻️ Proposed fix to rethrow already-wrapped exceptions
- } catch (Exception e) { + } catch (KeychainAccessException e) { + throw e; + } catch (Exception e) { throw new KeychainAccessException("Storing password failed.", e); }This same pattern appears in
loadPassphrase,deletePassphrase, andchangePassphrase—consider applying the fix consistently across all methods.
src/main/java/org/cryptomator/linux/keychain/GnomeKeyringKeychainAccess.java
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java
Show resolved
Hide resolved
|
@purejava which version of kWallet or GnomeKeyring is needed in order to test the new keychain backend? |
infeo
left a comment
infeo
left a comment
There was a problem hiding this comment.
I tested it on Ubuntu 24.0.4.3.
I noticed the different naming scheme:
With the GnomeKeychainAccess, the entry was displayed as "Cryptomator" with "vault=[some id]" as details. The description should stay "Cryptomator", to easily identify what the entry is for (the vault name can change, but the app stays the same). You can add another detail containing the vault name ("vault name" or something), if you wan to.
Adding more details is no problem. The "Name" of the vault can be added as an additional attribute, so that it gets displayed on opening an entry. Adding the further attribute here does the job. However: someone having many vaults will have to look through many entries all stating "Cryptomator" on searching for the password of a certain vault. A displayName like "Cryptomator - Vault: %name" would be an alternative. I'll change it as you wished and test, that |
Have the name of a vault as an attribute of the keychain entry
@infeo Changes done. The last commit / I also tested migrating the keyrings from one to the other (GNOME Keyring / KDE Wallet / Secret Service). |
|
@infeo Nonetheless what keychain backend was selected, it gets set to "GNOME Keyring" on application start, where "GNOME Keyring" is available and to a blank selection box on a KDE DE. This is due to: private void migrateLegacySettings(SettingsJson json) {
// migrate renamed keychainAccess
if(this.keychainProvider.getValueSafe().equals("org.cryptomator.linux.keychain.SecretServiceKeychainAccess")) {
this.keychainProvider.setValue("org.cryptomator.linux.keychain.GnomeKeyringKeychainAccess");
}
...
}in I am confused. A |
"Cryptomator" is the most general term where users are still be able to roughly identify a keychain entry. Also, this follows the same pattern as "Chromium Safe Storage". Searching will take a longer time, but from my point of view this is acceptable, since i believe users have most of the times less than 5 vaults and from within the app a password could be deleted or changed.
It is not needed anymore. In 826e29d the keychain service was renamed, thus the main app required a migration strategy. But since GnomeKeyringKeychainAccess (formerly known as SecretService) and SecretServiceKeychainAccess (formerly not existing) are both effectively using the secret-service API and addtionally, this API also works properly with KWallet, the migration is not required anymore. |
add to easy to read "with-er" methods
There was a problem hiding this comment.
@purejava i made a small cleanup.
Additionally, i changed the handling when deleting a not-existing item: Just log the event, do not throw an error: Deleting a password should be idempotent (several deletions still still have the outcome of "item does not exist"), additionally, it reduces API friction (aka handling deletion cases is easier)
Not needed anymore, see cryptomator/integrations-linux#125 (comment)
3 participants
This replaces
GnomeKeyringKeychainAccessandKDEWalletKeychainAccesswith a newKeychainAccess, that implements the latest Secret Service API Draft 0.2.So a single keychain interface can be used on Linux.
kwalletimplements the Secret Service API since versionv5.97.0, which was released in August 2022.SecretServiceKeychainAccessworks ootb with existing GNOME keyring entries, former KDE wallet entries get migrated.Refers to cryptomator/cryptomator#3979.