swiesend / secret-service

A Java library for storing secrets under linux in the gnome-keyring over D-Bus. Like libsecret, but for Java.
MIT License
31 stars 9 forks source link

[feature-request] interface re-design to get more flexibility #17

Closed purejava closed 4 years ago

purejava commented 4 years ago

Dear Sebastian,

I would like to suggest a little re-design of the interfaces that are part of secret-service.

Right now, the interfaces in org.freedesktop.secret.interfaces are all public abstract classes. While this is no problem in having working D-Bus interfaces (and serecret-service is working very well), this has a flaw: sending these over D-Bus throws an exception.

Why is this a problem?

Generally, checking on D-Bus, if an interface is available, e.g. check for the corresponding daemon being installed and configured, is done via connection.getRemoteObject, which requires a "real" D-Bus interface instead of a public abstract class.

The advantage of having the change would be, that it would easily be possible to create a method to check whether gnome-keyring is installed and running, not more, without creating an instance of the SimpleCollectionand using the default-collection in one flush like it is now. This would make secret-service more flexible.

So, what do you think about a change like this one: https://github.com/purejava/secret-service/commit/ea272ba0b428356ce2cde1e994d12b8f89491ac1

The change passes all maven tests you have created. 😃

swiesend commented 4 years ago

Hey Ralph, that makes much sense as it puts emphasis on the composability and makes it possible to use the interfaces as intended.

I would have to check if I accidentally introduces more methods to the interface, as they actually provide. I would have to check again which are actually provided by the system. See https://github.com/swiesend/secret-service/blob/master/src/test/resources/org.freedesktop.Secret.Prompt.xml vs https://github.com/purejava/secret-service/blob/ea272ba0b428356ce2cde1e994d12b8f89491ac1/src/main/java/org/freedesktop/secret/interfaces/Prompt.java#L52

So you would like to see a factory-pattern, that checks for the service via connection.getRemoteObject (which I could introduce on the 2.0.0 branch) or do you just want to be able to use the cleaned interfaces externally?

I just checked out your branch and all seems fine. Be aware that there a many @Disabled tests, I usually test also on this one as end-to-end test: https://github.com/purejava/secret-service/blob/ea272ba0b428356ce2cde1e994d12b8f89491ac1/src/test/java/org/freedesktop/secret/simple/Example.java#L17

Thank you very much for your efforts! This library and the collaboration with Cryptomator wouldn't be where it is without your help and integration.

Cheers Sebastian

swiesend commented 4 years ago

Oh I just saw in the linked cryptomator issue, that you just want to add another static method. Yes, that should be an easy and non breaking change for the rest.

swiesend commented 4 years ago

I pulled it in https://github.com/swiesend/secret-service/pull/18 and added an isAvailable method, but to me it seems that connection.getRemoteObject never throws a DBusException even for wrong names or non existing interfaces.

https://github.com/swiesend/secret-service/blob/7c3d8a2ef8e4c6236b0fe421a8cf0afdf6a86a69/src/main/java/org/freedesktop/secret/simple/SimpleCollection.java#L127-L140

https://github.com/swiesend/secret-service/blob/7c3d8a2ef8e4c6236b0fe421a8cf0afdf6a86a69/src/test/java/org/freedesktop/secret/simple/SimpleCollectionTest.java#L205-L211

This is on https://github.com/swiesend/secret-service/tree/develop-1.0.0

swiesend commented 4 years ago

Instead of unlocking the collection I only check for the availability:

https://github.com/swiesend/secret-service/blob/7c3d8a2ef8e4c6236b0fe421a8cf0afdf6a86a69/src/main/java/org/freedesktop/secret/simple/SimpleCollection.java#L48-L59

instead of https://github.com/swiesend/secret-service/blob/e13b5397cbfef0eed8f5376df34e7532b227c41f/src/main/java/org/freedesktop/secret/simple/SimpleCollection.java#L47-L60