Bump secret-service to 1.2.0 - #159
Conversation
Walkthrough
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 (4)
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java (4)
81-95: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUnlock failure only logs, then item creation proceeds.
If
unlockreturnsFailure, the code logs a warning and continues tosession.encryptandcollection.createItem. On a locked collection,createItemthen fails with a less specific error, and the original unlock error appears only in the log. Consider aborting with aKeychainAccessExceptionthat carriesunlockFailure.error().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java` around lines 81 - 95, Update the unlock handling around the Failure<Pair<List<DBusPath>, DBusPath>> branch to abort immediately instead of continuing to item creation. Throw a KeychainAccessException carrying unlockFailure.error(), while preserving the existing warning log and successful unlock flow through session.encrypt and collection.createItem.
43-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging the
setAliasresult.The code discards the return value of
setAlias. If the alias assignment fails, no diagnostic is produced, and later collection operations target an unexpected collection. Handle the returnedDBusResultand log the failure, as done forreadAlias.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java` around lines 43 - 51, Handle the DBusResult returned by setAlias in the default-alias branch of SecretServiceKeychainAccess, instead of discarding it. Add failure handling that logs the error details consistently with the existing readAlias/getAlias handling, while preserving the current alias assignment behavior on success.
196-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog-and-throw duplicates the same error.
The
Failurebranch logs the error and then throws aKeychainAccessExceptionwith the same cause. The caller reports the exception, so the error appears twice. The same pattern exists at lines 263-276 inchangePassphrase. Consider removing theLOG.warncall and keeping only the exception.Note: the PMD
InvalidLogMessageFormathints on theseLOG.warncalls are false positives. SLF4J accepts a trailingThrowablein addition to the placeholder arguments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java` around lines 196 - 209, Remove the redundant LOG.warn call from the Failure branch handling deletion in SecretServiceKeychainAccess, retaining the KeychainAccessException with failure.error() as the sole error propagation path. Apply the same change to the corresponding Failure branch in changePassphrase, while preserving the existing exception messages and causes.Source: Linters/SAST tools
64-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the static imports consistently.
Lines 65 and 116 use the fully qualified
DBusMessageHandler.DBusResult.Successand...Failure, while every other branch in this file uses the static-importedSuccess/Failure. Use the short forms here for consistency.♻️ Proposed change
- case DBusMessageHandler.DBusResult.Success<List<DBusPath>> success -> { + case Success<List<DBusPath>> success -> {- case DBusMessageHandler.DBusResult.Failure<List<DBusPath>> failure -> + case Failure<List<DBusPath>> failure ->Also applies to: 116-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java` around lines 64 - 66, Update the switch branches in SecretServiceKeychainAccess to use the statically imported Success and Failure types instead of DBusMessageHandler.DBusResult.Success and DBusMessageHandler.DBusResult.Failure, including the branches around lines 65 and 116; leave all other behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java`:
- Around line 105-128: Prevent double-wrapping by adding a
KeychainAccessException rethrow before the generic catch in storePassphrase
(SecretServiceKeychainAccess.java:105-128), changePassphrase
(SecretServiceKeychainAccess.java:238-241), loadPassphrase
(SecretServiceKeychainAccess.java:150-163), and deletePassphrase
(SecretServiceKeychainAccess.java:204-226). In storePassphrase, also include
collection context in the failure messages at lines 107 and 118 while preserving
existing exception causes.
- Around line 136-156: Align the PR documentation with the declared
org.purejava:secret-service dependency version 1.1.1, or update the dependency
property secret-service-02.version to the intended 1.2.0 API if that version is
required. Ensure the documented API and dependency version remain consistent.
---
Nitpick comments:
In
`@src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java`:
- Around line 81-95: Update the unlock handling around the
Failure<Pair<List<DBusPath>, DBusPath>> branch to abort immediately instead of
continuing to item creation. Throw a KeychainAccessException carrying
unlockFailure.error(), while preserving the existing warning log and successful
unlock flow through session.encrypt and collection.createItem.
- Around line 43-51: Handle the DBusResult returned by setAlias in the
default-alias branch of SecretServiceKeychainAccess, instead of discarding it.
Add failure handling that logs the error details consistently with the existing
readAlias/getAlias handling, while preserving the current alias assignment
behavior on success.
- Around line 196-209: Remove the redundant LOG.warn call from the Failure
branch handling deletion in SecretServiceKeychainAccess, retaining the
KeychainAccessException with failure.error() as the sole error propagation path.
Apply the same change to the corresponding Failure branch in changePassphrase,
while preserving the existing exception messages and causes.
- Around line 64-66: Update the switch branches in SecretServiceKeychainAccess
to use the statically imported Success and Failure types instead of
DBusMessageHandler.DBusResult.Success and DBusMessageHandler.DBusResult.Failure,
including the branches around lines 65 and 116; leave all other behavior
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a1cd3bca-c31d-4d38-9c37-27cbd3d6d4eb
📒 Files selected for processing (1)
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
| case DBusMessageHandler.DBusResult.Success<List<DBusPath>> success -> { | ||
| if (success.value().isEmpty()) { |
There was a problem hiding this comment.
Can be combined into a case ... when ... case, moving the if/else up to the switch.
| List<DBusPath> lockable = new ArrayList<>(); | ||
| lockable.add(new DBusPath(collection.getDBusPath())); |
There was a problem hiding this comment.
Can this be an immutable List.of(...)?
| case Success<List<DBusPath>> success -> { | ||
| if (success.value().isEmpty()) { |
There was a problem hiding this comment.
Again case ... when ...isEmpty()
| case Success<List<DBusPath>> success -> { | ||
| if (success.value().isEmpty()) { |
| case Success<List<DBusPath>> success -> { | ||
| if (success.value().isEmpty()) { |
| return null; | ||
| } | ||
|
|
||
| var path = success.value().getFirst(); |
There was a problem hiding this comment.
Instead of silently returning getFirst, shouldn't it be three different cases:
- empty → null
- exactly one item → success
- more than one items → ambiguous exception? Or at least log a warning?
There was a problem hiding this comment.
The test for an empty search response that returns null is already there:
if (success.value().isEmpty()) {
return null;
}
var path = success.value().getFirst();If the search would return more than one items, there would be two or more vaults with the same ID or the search would be broken. That's unlikely. But, I think, a test for only one returned item and a log message in case the result differs from that expectation does improve the code.
| return; | ||
| } | ||
|
|
||
| var path = success.value().getFirst(); |
| throw new KeychainAccessException(message); | ||
| } | ||
|
|
||
| var path = success.value().getFirst(); |
| case Success<Pair<List<DBusPath>, DBusPath>> unlockSuccess -> { | ||
| var prompt = unlockSuccess.value().b; | ||
| if (!"/".equals(prompt.getPath())) { | ||
| Util.promptAndGetResultAsArrayList(prompt); |
There was a problem hiding this comment.
Result gets discarded? What does this do? Method name doesn't explain the purpose to me, maybe add a comment inside this block?
There was a problem hiding this comment.
Yes, the result gets discarded. That's DBus: you call unlock for the collection in order to be able to store the secret within the collection and, depending on whether a prompt is needed to unlock the collection or not, a prompt is displayed or not, see Javadoc here. That's what this piece of code handles.
I'll add an explaning comment.
|
@overheadhunter Thanks for the fast review! 😃 All your suggestions were implemented. And I tested the PR once again. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java (3)
44-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle the
setAliasresult as well.Line 50 logs a failed
readAlias. Line 46 discards thesetAliasresult. If setting the alias fails, the constructor stays silent and later operations target a collection that was never aliased. Log the failure branch for consistency with the new result handling.♻️ Proposed change
if ("/".equals(success.value().getPath())) { // default alias is not set; set it to the login keyring - session.getService().setAlias("default", new DBusPath(Static.DBusPath.LOGIN_COLLECTION)); + var setAlias = session.getService() + .setAlias("default", new DBusPath(Static.DBusPath.LOGIN_COLLECTION)); + if (setAlias instanceof Failure<?> setAliasFailure) { + LOG.warn("Setting the \"default\" alias failed with: {}", + setAliasFailure.error().getMessage()); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java` around lines 44 - 47, Handle the result returned by setAlias in the alias-initialization flow around SecretServiceKeychainAccess, and log its failure branch consistently with the existing readAlias failure handling. Ensure setAlias failures are reported before the constructor continues.
63-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the imported type names consistently in this switch.
Lines 63, 104, and 107 use
DBusMessageHandler.DBusResult.Success/...Failure, while lines 69, 78, 92, and 96 use the importedSuccess/Failure. Pick one form in this switch to keep the case labels readable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java` around lines 63 - 113, Use the imported Success and Failure type names consistently across the switch handling the collection lookup result, replacing the fully qualified DBusMessageHandler.DBusResult.Success/Failure labels while leaving the existing unlock and item-creation cases unchanged.
284-290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the searched key to the ambiguity message.
assertOnlyOneItemreports only the count. The three call sites search by different keys, so the message cannot identify the affected vault. Pass the key and include it.♻️ Proposed change
- private static void assertOnlyOneItem(Success<List<DBusPath>> success) throws KeychainAccessException { + private static void assertOnlyOneItem(Success<List<DBusPath>> success, String key) throws KeychainAccessException { if (success.value().size() != 1) { throw new KeychainAccessException( - "Expected exactly one item, but found " + success.value().size() + "Expected exactly one item for " + ID_KEY + "=" + key + + ", but found " + success.value().size() ); } }Update the three call sites at lines 137, 177, and 235 to
assertOnlyOneItem(success, key);.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java` around lines 284 - 290, Update assertOnlyOneItem to accept the searched key, include that key in the ambiguity exception message, and pass key from all three call sites that invoke it. Preserve the existing count validation and exception behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java`:
- Around line 44-47: Handle the result returned by setAlias in the
alias-initialization flow around SecretServiceKeychainAccess, and log its
failure branch consistently with the existing readAlias failure handling. Ensure
setAlias failures are reported before the constructor continues.
- Around line 63-113: Use the imported Success and Failure type names
consistently across the switch handling the collection lookup result, replacing
the fully qualified DBusMessageHandler.DBusResult.Success/Failure labels while
leaving the existing unlock and item-creation cases unchanged.
- Around line 284-290: Update assertOnlyOneItem to accept the searched key,
include that key in the ambiguity exception message, and pass key from all three
call sites that invoke it. Preserve the existing count validation and exception
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 94e13621-3450-4b2d-bd89-e0d244bc2007
📒 Files selected for processing (2)
pom.xmlsrc/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
2 participants
The secret-service release 1.2.0 has breaking changes.
Secret-service release 1.2.0 has been tested intensively with latest Cryptomator and integrations-linux.