Skip to content

Bump secret-service to 1.2.0 #159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
purejava wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Bump secret-service to 1.2.0 #159

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7347609
Use nested records due to secret-service 1.2.0
purejava Aug 7, 2026
f4a8958
Fixes suggested by coderabbitai
purejava Aug 9, 2026
1ca3953
Use guarded pattern matching in switch
purejava Aug 10, 2026
28c3347
Check number of returned search items
purejava Aug 10, 2026
74a7c13
Add explaining comment for DBus prompt
purejava Aug 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
269 changes: 214 additions & 55 deletions src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@
import org.cryptomator.integrations.keychain.KeychainAccessProvider;
import org.freedesktop.dbus.DBusPath;
import org.purejava.secret.api.Collection;
import org.purejava.secret.api.DBusMessageHandler;
import org.purejava.secret.api.EncryptedSession;
import org.purejava.secret.api.Item;
import org.purejava.secret.api.Pair;
import org.purejava.secret.api.Static;
import org.purejava.secret.api.Util;
import org.slf4j.Logger;
Expand All @@ -19,6 +21,8 @@
import java.util.Map;
import java.util.Objects;

import static org.purejava.secret.api.DBusMessageHandler.DBusResult.*;

@Priority(1100)
@OperatingSystem(OperatingSystem.Value.LINUX)
@DisplayName("Secret Service")
Expand All @@ -36,9 +40,15 @@ public SecretServiceKeychainAccess() {
session.getService().addCollectionCreatedHandler(collection -> LOG.debug("Collection {} created", collection.getPath()));
session.getService().addCollectionDeletedHandler(collection -> LOG.debug("Collection {} deleted", 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));
switch (getAlias) {
case Success<DBusPath> success-> {
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));
}
}
case Failure<DBusPath> failure
-> LOG.warn("Getting the collection with the \"default\" alias failed with: {}", failure.error().getMessage());
}
collection.addItemChangedHandler(item -> LOG.debug("Item {} changed", item.getPath()));
collection.addItemCreatedHandler(item -> LOG.debug("Item {} created", item.getPath()));
Expand All @@ -50,98 +60,236 @@ public SecretServiceKeychainAccess() {
public void storePassphrase(String key, String displayName, CharSequence passphrase) throws KeychainAccessException {
try {
var call = collection.searchItems(withKey(key));
if (call.isSuccess()) {
if (call.value().isEmpty()) {
List<DBusPath> lockable = new ArrayList<>();
lockable.add(new DBusPath(collection.getDBusPath()));
var promptNeededToUnlock = session.getService().unlock(lockable);
if (promptNeededToUnlock.isSuccess() && !"/".equals(promptNeededToUnlock.value().b.getPath())) {
Util.promptAndGetResultAsArrayList(promptNeededToUnlock.value().b);
}
var itemProps = Item.createProperties(LABEL_FOR_SECRET_IN_KEYRING, withKeyAndName(key, displayName));
var secret = session.encrypt(passphrase);
var created = collection.createItem(itemProps, secret, false);
if (!created.isSuccess()) {
throw new KeychainAccessException("Storing password failed", created.error());

switch (call) {
case DBusMessageHandler.DBusResult.Success<List<DBusPath>> success -> {
if (success.value().isEmpty()) {

@overheadhunter overheadhunter Aug 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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()));

@overheadhunter overheadhunter Aug 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can this be an immutable List.of(...)?


var unlockResult = session.getService().unlock(lockable);

switch (unlockResult) {
case Success<Pair<List<DBusPath>, DBusPath>> unlockSuccess -> {
var prompt = unlockSuccess.value().b;

if (!"/".equals(prompt.getPath())) {
Util.promptAndGetResultAsArrayList(prompt);

@overheadhunter overheadhunter Aug 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Result gets discarded? What does this do? Method name doesn't explain the purpose to me, maybe add a comment inside this block?

@purejava purejava Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

}
}

case Failure<Pair<List<DBusPath>, DBusPath>> unlockFailure ->
LOG.warn(
"Failed to unlock collection {}",
collection.getDBusPath(),
unlockFailure.error()
);
}

var itemProps = Item.createProperties(
LABEL_FOR_SECRET_IN_KEYRING,
withKeyAndName(key, displayName)
);

var secret = session.encrypt(passphrase);
var created = collection.createItem(itemProps, secret, false);

switch (created) {
case Success<Pair<DBusPath, DBusPath>> successful ->
LOG.debug(
"Created item {} on collection {}",
successful.value().a.getPath(),
collection.getDBusPath()
);

case Failure<Pair<DBusPath, DBusPath>> failure ->
throw new KeychainAccessException(
"Storing password failed",
failure.error()
);
}
} else {
changePassphrase(key, displayName, passphrase);
}
} else {
changePassphrase(key, displayName, passphrase);
}
} else {
throw new KeychainAccessException("Storing password failed", call.error());

case DBusMessageHandler.DBusResult.Failure<List<DBusPath>> failure ->
throw new KeychainAccessException(
"Storing password failed",
failure.error()
);
}
} catch (Exception e) {
throw new KeychainAccessException("Storing password failed.", e);
throw new KeychainAccessException(
"Storing password failed for collection "
+ collection.getDBusPath(),
e
);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}

@Override
public char[] loadPassphrase(String key) throws KeychainAccessException {
try {
var call = collection.searchItems(withKey(key));
if (call.isSuccess()) {
if (!call.value().isEmpty()) {
var path = call.value().getFirst();

switch (call) {
case Success<List<DBusPath>> success -> {
if (success.value().isEmpty()) {

@overheadhunter overheadhunter Aug 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Again case ... when ...isEmpty()

return null;
}

var path = success.value().getFirst();

@overheadhunter overheadhunter Aug 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of silently returning getFirst, shouldn't it be three different cases:

  1. empty → null
  2. exactly one item → success
  3. more than one items → ambiguous exception? Or at least log a warning?

@purejava purejava Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.


session.getService().ensureUnlocked(path);

var secret = new Item(path).getSecret(session.getSession());
return session.decrypt(secret);
} else {
return null;
}
} else {
throw new KeychainAccessException("Loading password failed", call.error());

case Failure<List<DBusPath>> failure ->
throw new KeychainAccessException(
"Loading password failed for collection "
+ collection.getDBusPath(),
failure.error()
);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
} catch (Exception e) {
throw new KeychainAccessException("Loading password failed.", e);
throw new KeychainAccessException(
"Loading password failed for collection "
+ collection.getDBusPath(),
e
);
}
}

@Override
public void deletePassphrase(String key) throws KeychainAccessException {
try {
var call = collection.searchItems(withKey(key));
if (call.isSuccess()) {
if (!call.value().isEmpty()) {
var path = call.value().getFirst();

switch (call) {
case Success<List<DBusPath>> success -> {
if (success.value().isEmpty()) {

@overheadhunter overheadhunter Aug 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Case when

LOG.debug(
"Deleting entry with {}={} failed: No such item found",
ID_KEY,
key
);
return;
}

var path = success.value().getFirst();

@overheadhunter overheadhunter Aug 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Again: handle >1?


session.getService().ensureUnlocked(path);

var item = new Item(path);
var deleted = item.delete();
if (!deleted.isSuccess()) {
throw new KeychainAccessException("Deleting password failed", deleted.error());

switch (item.delete()) {
case Success<DBusPath> _ ->
LOG.debug(
"Deleted item {} from collection {}",
path.getPath(),
collection.getDBusPath()
);

case Failure<DBusPath> failure -> {
LOG.warn(
"Failed to delete item {} from collection {}",
path.getPath(),
collection.getDBusPath(),
failure.error()
);

throw new KeychainAccessException(
"Deleting password failed for collection "
+ collection.getDBusPath(),
failure.error()
);
}
}
} else {
LOG.debug("Deleting entry with {}={} failed: No such item found", ID_KEY, key);
}
} else {
throw new KeychainAccessException("Deleting password failed", call.error());

case Failure<List<DBusPath>> failure ->
throw new KeychainAccessException(
"Deleting password failed for collection "
+ collection.getDBusPath(),
failure.error()
);
}
} catch (Exception e) {
throw new KeychainAccessException("Deleting password failed", e);
throw new KeychainAccessException(
"Deleting password failed for collection "
+ collection.getDBusPath(),
e
);
}
}

@Override
public void changePassphrase(String key, String displayName, CharSequence passphrase) throws KeychainAccessException {
public void changePassphrase(String key, String displayName, CharSequence passphrase)
throws KeychainAccessException {

try {
var call = collection.searchItems(withKey(key));
if (call.isSuccess()) {
if (!call.value().isEmpty()) {
session.getService().ensureUnlocked(call.value().getFirst());

switch (call) {
case Success<List<DBusPath>> success -> {
if (success.value().isEmpty()) {

@overheadhunter overheadhunter Aug 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Case when

var message = "Vault " + key + " not found, updating failed";
throw new KeychainAccessException(message);
}

var path = success.value().getFirst();

@overheadhunter overheadhunter Aug 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Handle >1?


session.getService().ensureUnlocked(path);

var secret = session.encrypt(passphrase);
var itemProps = Item.createProperties(LABEL_FOR_SECRET_IN_KEYRING, withKeyAndName(key, displayName));
var itemProps = Item.createProperties(
LABEL_FOR_SECRET_IN_KEYRING,
withKeyAndName(key, displayName)
);

var updated = collection.createItem(itemProps, secret, true);
if (!updated.isSuccess()) {
throw new KeychainAccessException("Updating password failed", updated.error());

switch (updated) {
case Success<Pair<DBusPath, DBusPath>> _ ->
LOG.debug(
"Updated item {} in collection {}",
path.getPath(),
collection.getDBusPath()
);

case Failure<Pair<DBusPath, DBusPath>> failure -> {
LOG.warn(
"Failed to update item {} in collection {}",
path.getPath(),
collection.getDBusPath(),
failure.error()
);

throw new KeychainAccessException(
"Updating password failed for collection "
+ collection.getDBusPath(),
failure.error()
);
}
}
} else {
var msg = "Vault " + key + " not found, updating failed";
throw new KeychainAccessException(msg);
}
} else {
throw new KeychainAccessException("Updating password failed", call.error());

case Failure<List<DBusPath>> failure ->
throw new KeychainAccessException(
"Updating password failed for collection "
+ collection.getDBusPath(),
failure.error()
);
}
} catch (Exception e) {
throw new KeychainAccessException("Updating password failed", e);
throw new KeychainAccessException(
"Updating password failed for collection "
+ collection.getDBusPath(),
e
);
}
}

Expand All @@ -158,8 +306,19 @@ public boolean isSupported() {

@Override
public boolean isLocked() {
var call = collection.isLocked();
return !call.isSuccess() || call.value();
return switch (collection.isLocked()) {
case Success<Boolean> success ->
success.value(); // yields the value

case Failure<Boolean> failure -> {
LOG.warn(
"Failed to determine lock state of collection {}",
collection.getDBusPath(),
failure.error()
);
yield true;
}
};
}

private Map<String, String> withKey(String key) {
Expand Down