Skip to content

Conversation

@purejava
Copy link
Contributor

This replaces GnomeKeyringKeychainAccess and KDEWalletKeychainAccess with a new KeychainAccess, that implements the latest Secret Service API Draft 0.2.

So a single keychain interface can be used on Linux. kwallet implements the Secret Service API since version v5.97.0, which was released in August 2022.

SecretServiceKeychainAccess works ootb with existing GNOME keyring entries, former KDE wallet entries get migrated.

Closes cryptomator/cryptomator#3979.

@coderabbitai
Copy link

coderabbitai bot commented Oct 27, 2025

Note

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

Adds a Secret Service-based Linux keychain backend and registers it as a KeychainAccessProvider. Changes include: new Maven property/dependency for org.purejava:secret-service; new class SecretServiceKeychainAccess implementing KeychainAccessProvider (session/collection lifecycle, CRUD, lock/support checks, KDE migration); module-info.java updated to require the secret-service module and to provide the new provider; service descriptor updated to register the provider; and tests added/updated to exercise the new backend. Existing KDE and GNOME implementations remain unchanged.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Areas needing extra attention:
    • src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java — dense logic (session/collection lifecycle, encryption/decryption, migration, event handlers, error handling).
    • module-info.java — verify requires and provides entries match module names and exported types.
    • src/test/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccessTest.java — environment detection and test semantics.
    • pom.xml and META-INF/services/org.cryptomator.integrations.keychain.KeychainAccessProvider — dependency/version wiring and service registration.

Possibly related issues

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The title "New secret service" is concise but lacks specificity about the core change. While it does reference the addition of new secret service functionality, it fails to convey the main point—that this PR replaces two existing keychain implementations (GnomeKeyringKeychainAccess and KDEWalletKeychainAccess) with a single unified Secret Service API implementation. The title could be more descriptive (e.g., "Unify Linux keychain backends with Secret Service API") to better signal the primary purpose of the changeset to someone reviewing the commit history.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues Check ✅ Passed The PR successfully implements the core requirements from issue #3979. It adds a new SecretServiceKeychainAccess class that implements the Secret Service API Draft 0.2, providing unified keychain backend support compatible with both KDE kwallet (since v5.97.0) and GNOME keyring. The implementation includes all necessary CRUD operations (storePassphrase, loadPassphrase, deletePassphrase, changePassphrase), state checks (isSupported, isLocked), and migration logic for KDE wallet entries. The addition of the org.purejava:secret-service dependency enables use of a modern, maintained Secret Service library. The new implementation is properly registered in the service provider descriptor and accompanied by comprehensive tests.
Out of Scope Changes Check ✅ Passed The overwhelming majority of changes are directly in-scope with issue #3979: the new SecretServiceKeychainAccess implementation, the service provider registration, module-info updates, dependency additions, and comprehensive test coverage. Minor trailing newline additions to KDEWalletKeychainAccess.java and GnomeKeyringKeychainAccessTest.java are negligible formatting adjustments. Notably, the existing GnomeKeyringKeychainAccess and KDEWalletKeychainAccess implementations are retained in the service provider descriptor alongside the new implementation, which aligns with the PR description's "replaces" language in terms of primary usage while maintaining backward compatibility.
Description Check ✅ Passed The PR description clearly explains what is being replaced, what is being added, and provides meaningful context. It explicitly states that GnomeKeyringKeychainAccess and KDEWalletKeychainAccess are being replaced with a new implementation using Secret Service API Draft 0.2, mentions the benefits (single interface, kwallet support since v5.97.0), describes the migration path for KDE wallet entries, and references the closing issue (#3979). The description is directly related to the changeset and provides sufficient detail to understand the purpose and scope.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1c54f97 and 17a4f69.

📒 Files selected for processing (2)
  • pom.xml (2 hunks)
  • src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • pom.xml
  • src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 34377f1 and 161ea15.

📒 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.secret module is properly declared in requires (module-info.java:18)
  • Service descriptor correctly points to SecretServiceKeychainAccess implementation
  • No stale references to KDEWalletKeychainAccess, GnomeKeyringKeychainAccess, de.swiesend.secretservice, or kwallet exist in the codebase
  • pom.xml dependency correctly updated to org.purejava:secret-service:1.0.0
src/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 declares module org.purejava.secret, which aligns with your requires statement. Service wiring to SecretServiceKeychainAccess is properly configured in the provides clause.

Comment on lines 85 to 88
public static boolean serviceAvailableAndUnlocked() {
var service = new SecretServiceKeychainAccess();
return service.isSupported() && !service.isLocked();
}
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

🧩 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.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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-alpha and org.purejava:kdewallet:1.4.0 to org.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.

@overheadhunter
Copy link
Member

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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 161ea15 and b9c6eb1.

📒 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 SecretServiceKeychainAccess is correctly registered as the first provider, which means it will be discovered first by Java's ServiceLoader mechanism. 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 SecretServiceKeychainAccess import is properly added to support the new provider registration.


provides AutoStartProvider with FreedesktopAutoStartService;
provides KeychainAccessProvider with GnomeKeyringKeychainAccess, KDEWalletKeychainAccess;
provides KeychainAccessProvider with SecretServiceKeychainAccess, GnomeKeyringKeychainAccess, KDEWalletKeychainAccess;
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

🧩 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 java

Length 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.java

Length 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.

@purejava

This comment was marked as outdated.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b9c6eb1 and 1c54f97.

📒 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 SecretServiceKeychainAccess import is properly added to support the unified Linux keychain backend.


26-26: LGTM: Provider registration order is correct.

The provider list correctly places SecretServiceKeychainAccess first, making it the preferred implementation while retaining GnomeKeyringKeychainAccess and KDEWalletKeychainAccess for 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 @Deprecated annotations in their respective class files.

requires org.purejava.appindicator;
requires org.purejava.kwallet;
requires de.swiesend.secretservice;
requires org.purejava.secret;
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

🧩 Analysis chain

Verify the module name for the secret-service dependency.

The module name is declared as org.purejava.secret, but according to the AI summary, the Maven artifact is org.purejava:secret-service. Module names don't always match Maven artifact names, but this should be verified to ensure module resolution succeeds at compile time.

Run the following script to verify the module name exported by the secret-service JAR:


🏁 Script executed:

#!/bin/bash
# Description: Extract and verify the module name from the secret-service JAR

# Find the secret-service JAR in Maven local repository or target
secret_jar=$(fd -e jar -g '*secret-service*.jar' ~/.m2/repository 2>/dev/null | head -1)

if [ -z "$secret_jar" ]; then
  echo "Secret service JAR not found in local Maven repository."
  echo "Attempting to check Maven Central for module info..."
  # Alternative: check Maven Central metadata
  curl -s "https://repo1.maven.org/maven2/org/purejava/secret-service/maven-metadata.xml" | grep -o '<latest>[^<]*</latest>' || echo "Could not retrieve version info"
else
  echo "Found JAR: $secret_jar"
  echo "Extracting module-info..."
  unzip -p "$secret_jar" module-info.class > /tmp/module-info.class 2>/dev/null
  if [ -f /tmp/module-info.class ]; then
    javap /tmp/module-info.class | grep "^module" || echo "Module info not found or not readable"
  else
    echo "Could not extract module-info.class from JAR"
  fi
fi

Length of output: 131


🌐 Web query:

org.purejava secret-service module name Java

💡 Result:

Do you mean the Java module name (module-info / Automatic-Module-Name) for an org.purejava "secret-service" artifact, or the Maven artifactId/groupId to add as a dependency? If you mean one of those, tell me which and I’ll look it up.


🌐 Web query:

org.purejava secret-service GitHub Java library

💡 Result:

I couldn't find a GitHub repository named "org.purejava/secret-service". The purejava (org.purejava) GitHub organization (owner: Ralph Plawetzki) hosts several Java libraries related to secrets/security instead — e.g. kdewallet and keepassxc-proxy-access. [1][2][3]

Sources:

  • purejava GitHub org (repositories list). [1]
  • org.purejava: kdewallet (Maven / project page). [2]
  • org.purejava: keepassxc-proxy-access (Maven / project page). [3]

🌐 Web query:

Maven Central org.purejava secret-service 1.0.0 jar module

💡 Result:

I couldn't find org.purejava:secret-service:1.0.0 on Maven Central. I searched Maven Central/sonatype and public Maven indexes — there is no artifact with groupId org.purejava and artifactId secret-service at version 1.0.0. [1][2]

Related hits (different groupIds) you may want to check:

  • de.swiesend:secret-service (pre‑release/RC versions shown). [3]
  • other "secret"/"secret-core" artifacts under com.github.houbb (unrelated groupId/artifactId). [4]

If you want, tell me whether you mean one of those (de.swiesend or com.github.houbb), or provide a pom.xml/URL and I’ll look up the exact Maven coordinates and the available module/jar files.

Sources:

  1. Maven Central org.purejava namespace. [https://central.sonatype.com/namespace/org.purejava] [1]
  2. Search results for org.purejava artifacts on Central (keepassxc-proxy-access, winsparkle-java). [https://central.sonatype.com/artifact/org.purejava/keepassxc-proxy-access] [2]
  3. mvnrepository / listings showing secret-service under other groups (de.swiesend / similar). [https://mvnrepository.com/artifact/com.github.javakeyring/java-keyring/1.0.0] [3]
  4. JarCasting pages for com.github.houbb:secret and secret-core. [https://jarcasting.com/artifacts/com.github.houbb/secret-core/] [4]

🏁 Script executed:

#!/bin/bash
# Find pom.xml and search for secret-service dependency
fd -name pom.xml -type f | head -5

Length of output: 230


🏁 Script executed:

#!/bin/bash
# Find pom.xml using find command
find . -name pom.xml -type f | head -5

Length of output: 9


🏁 Script executed:

#!/bin/bash
# Display pom.xml to find the dependency declaration
cat pom.xml

Length of output: 8613


🏁 Script executed:

#!/bin/bash
# Display module-info.java to see all requires directives
cat src/main/java/module-info.java

Length of output: 1745


🌐 Web query:

org.purejava secret-service Maven Central repository versions

💡 Result:

I couldn't find any versions of org.purejava:secret-service on Maven Central — the project's GitHub README shows the intended Maven/Gradle coordinates (version 1.0.0) but the repository shows "No packages published" (i.e. not on Maven Central). [1]

Sources:

  • purejava/secret-service GitHub (shows dependency snippet and "No packages published"). [1]

The dependency org.purejava:secret-service:1.0.0 is not published on Maven Central and will cause build failures.

The pom.xml declares org.purejava:secret-service version 1.0.0, but the project's GitHub shows the intended Maven coordinates but repository shows "No packages published". The build will fail during dependency resolution. Either use a different version if available, or ensure this artifact is published to the repository before building.

IllegalBlockSizeException e) {
LOG.error("Migrating entry {} for vault {} failed", i.getPath(), attribs.value().get("user"));
}
item.delete();
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.
Bildschirmfoto_20251029_054433

@purejava purejava marked this pull request as draft October 28, 2025 06:11
Give it a higher priority
Use latest Secret Service library
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Merge keychain backends for KDE and GNOME

2 participants