New secret service #125
New secret service #125
Conversation
…ServiceKeychainAccess
Add handlers
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new SecretServiceKeychainAccess implementation for Linux using Secret Service, registers it as a KeychainAccessProvider, and adds the corresponding service entry. Updates module declaration to require org.purejava.secret and adds the dependency in pom.xml. Introduces tests (SecretServiceKeychainAccessTest) and minor formatting changes. Also includes KDEWallet migration logic inside the new class and provider wiring updates in module-info and META-INF service file. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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
| public static boolean serviceAvailableAndUnlocked() { | ||
| var service = new SecretServiceKeychainAccess(); | ||
| return service.isSupported() && !service.isLocked(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Single source of truth for gating.
The custom gate already checks isSupported() and lock state; good. Consider removing class-level gates so only this applies.
If you keep the DISPLAY gate, verify on Wayland that tests still run:
🏁 Script executed:
#!/bin/bash
echo "DISPLAY=$DISPLAY XDG_SESSION_TYPE=$XDG_SESSION_TYPE"Length of output: 26
I need to examine the test class to identify class-level gates and understand the DISPLAY gate implementation.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine the test class structure
find . -type f -name "SecretServiceKeychainAccessTest.java" | head -1 | xargs cat
Length of output: 2841
Let me verify if there are any other DISPLAY-related gates or dependencies in the codebase:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for other DISPLAY-related gates or conditions
rg -n "DISPLAY|EnabledIfEnvironmentVariable" --type java src/test/java/org/cryptomator/linux/keychain/
Length of output: 303
Remove class-level DISPLAY gate; rely on nested class's serviceAvailableAndUnlocked() gate instead.
The @EnabledIfEnvironmentVariable(named = "DISPLAY", matches = ".*") annotation at the class level blocks all tests, including testIsSupported(), which doesn't require DISPLAY since it uses dbus-send CLI. The nested FunctionalTests class already gates functional operations via @EnabledIf("serviceAvailableAndUnlocked"), which comprehensively checks both isSupported() and lock state. Removing the class-level gate allows unit tests to run appropriately while preserving the functional tests' guard.
🤖 Prompt for AI Agents
In
src/test/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccessTest.java
around lines 85 to 88, remove the class-level
@EnabledIfEnvironmentVariable(named = "DISPLAY", matches = ".*") annotation so
it no longer blocks all tests; keep the nested FunctionalTests class annotation
@EnabledIf("serviceAvailableAndUnlocked") as the gate for functional tests and
rely on the existing public static boolean serviceAvailableAndUnlocked() method
to check service availability and lock state.
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.
|
|
||
| provides AutoStartProvider with FreedesktopAutoStartService; | ||
| provides KeychainAccessProvider with GnomeKeyringKeychainAccess, KDEWalletKeychainAccess; | ||
| provides KeychainAccessProvider with SecretServiceKeychainAccess, GnomeKeyringKeychainAccess, KDEWalletKeychainAccess; |
There was a problem hiding this comment.
🧩 Analysis chain
Provider registration looks correct. Verify deprecation of old implementations.
The provider list is correctly updated with SecretServiceKeychainAccess first, ensuring the new unified implementation is preferred. Retaining the old providers supports backwards compatibility as suggested by the maintainer.
However, please verify that GnomeKeyringKeychainAccess and KDEWalletKeychainAccess are marked with @Deprecated annotations in their class definitions, as the maintainer recommended keeping them deprecated before eventual removal in a future release.
Run the following script to check if the old implementations are marked as deprecated:
🏁 Script executed:
#!/bin/bash
# Description: Check if old keychain implementations are marked with @Deprecated
echo "=== Checking GnomeKeyringKeychainAccess ==="
rg -A 2 -B 2 "class GnomeKeyringKeychainAccess" --type java
echo -e "\n=== Checking KDEWalletKeychainAccess ==="
rg -A 2 -B 2 "class KDEWalletKeychainAccess" --type javaLength of output: 2448
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Description: Explicitly check for @Deprecated annotation in old keychain implementations
echo "=== Searching for @Deprecated in GnomeKeyringKeychainAccess.java ==="
rg "@Deprecated" src/main/java/org/cryptomator/linux/keychain/GnomeKeyringKeychainAccess.java
echo -e "\n=== Searching for @Deprecated in KDEWalletKeychainAccess.java ==="
rg "@Deprecated" src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java
echo -e "\n=== Full class declaration for GnomeKeyringKeychainAccess ==="
rg -B 5 "public class GnomeKeyringKeychainAccess" src/main/java/org/cryptomator/linux/keychain/GnomeKeyringKeychainAccess.java
echo -e "\n=== Full class declaration for KDEWalletKeychainAccess ==="
rg -B 5 "public class KDEWalletKeychainAccess" src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.javaLength of output: 637
Add @Deprecated annotations to old keychain implementations.
The old GnomeKeyringKeychainAccess and KDEWalletKeychainAccess implementations are not marked with @Deprecated annotations. Both classes require @Deprecated to signal that they are superseded by SecretServiceKeychainAccess and will be removed in a future release.
Add @Deprecated annotation to:
src/main/java/org/cryptomator/linux/keychain/GnomeKeyringKeychainAccess.java(before the class declaration)src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java(before the class declaration)
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/keychain/GnomeKeyringKeychainAccess.java
(at the class declaration) and
src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java (at
the class declaration), add the @Deprecated annotation immediately above each
class declaration to mark them as superseded by SecretServiceKeychainAccess and
slated for removal; ensure the annotation is imported if necessary and place it
on the class-level (before any javadoc or modifiers) so the compiler and IDEs
surface deprecation warnings.
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
| IllegalBlockSizeException e) { | ||
| LOG.error("Migrating entry {} for vault {} failed", i.getPath(), attribs.value().get("user")); | ||
| } | ||
| item.delete(); |
There was a problem hiding this comment.
I don't think migration of such data should never delete the original. Instead, is there an option to mark it as migrated, so it won't migrate twice?
There was a problem hiding this comment.
Do the passwords stored and used by KDEWalletKeychainAccess have a need, when we remove KDEWalletKeychainAccess with a future PR? Nevertheless, I changed the behavior as suggested with 17a4f69.
There was a problem hiding this comment.
This is, how it looks like after migration. The label "Cryptomator" will get changed on a password update/change to the vault's display name, but this information is not yet available on migrating items.
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.
| var label = item.getLabel().value(); | ||
| var itemProps = Item.createProperties(label, newAttribs); | ||
| var replace = collection.createItem(itemProps, secret, true); | ||
| assert replace.isSuccess() : "Replacing migrated item failed"; |
There was a problem hiding this comment.
Replace assertion with proper error handling.
Using assert for error checking in production code is an anti-pattern because assertions can be disabled via the -da JVM flag. If the replace operation fails and assertions are disabled, execution will continue silently past a critical error.
🔎 Proposed fix
var itemProps = Item.createProperties(label, newAttribs);
var replace = collection.createItem(itemProps, secret, true);
- assert replace.isSuccess() : "Replacing migrated item failed";
+ if (!replace.isSuccess()) {
+ LOG.error("Replacing migrated item {} failed: {}", i.getPath(), replace.error());
+ continue;
+ }
item.delete();📝 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.
| assert replace.isSuccess() : "Replacing migrated item failed"; | |
| 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(); |
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
around line 185, replace the assertion "assert replace.isSuccess() : ..." with
explicit error handling: check if replace.isSuccess() is false, log an error
with context and any available details from the replace result (e.g.
replace.toString() or a getError/getMessage accessor), and then fail-fast by
throwing an appropriate unchecked exception (IllegalStateException or a specific
custom exception) so failure cannot be skipped when assertions are disabled;
ensure any necessary cleanup or rollback is performed before throwing.
2 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.Closes cryptomator/cryptomator#3979.