Improve signal handling for kdewallet #3

purejava · 2020-11-15T15:20:36Z

D-Bus signals are emitted on every change to a wallet, e.g. when it's opened asynchronously, closed etc.. Cryptomator checks for these signals and processes them.
Signal handling in ‪kdewallet does work well [1], but could be improved in some regards:

the SignalHandler#await method keeps the running application in a loop and waits for the signal to listen for until the signal was emitted or a timeout is reached
you cannot await a signal and execute the code that emits the signal afterwards
the SignalHandler does not support to catch signals asynchronously

kdewallet 1.2.0 solves this by providing a re-designed SignalHandler to handle signals asynchronously and make SignalHandler more robust against timed out D-Bus calls.

To make use of the new features, integrations-linux needs slight changes. And that’s the motivation for this PR.

Here is what happens in Cryptomator without and with this change:
SignalHandler in kdewallet 1.1.1:

06:38:39.286 [App Scheduled Executor 01] INFO  o.c.common.settings.SettingsProvider - Settings saved to /home/ralph/.config/Cryptomator/settings.json
06:38:41.821 [JavaFX Application Thread] INFO  o.f.dbus.handlers.SignalHandler - Await signal org.kde.KWallet$walletAsyncOpened(/modules/kwalletd5) within 120 seconds.
06:38:46.520 [DBus Worker Thread-4] INFO  o.f.dbus.handlers.SignalHandler - Received signal KWallet.walletOpened: kdewallet
06:38:46.521 [DBus Worker Thread-1] INFO  o.f.dbus.handlers.SignalHandler - Received signal KWallet.walletAsyncOpened: {TransactionID: 0, handle: 248503178}
06:38:53.330 [App Background Thread 005] INFO  org.cryptomator.common.vaults.Vault - Storing file name length limit of 220
06:38:54.331 [App Scheduled Executor 02] INFO  o.c.common.settings.SettingsProvider - Settings saved to /home/ralph/.config/Cryptomator/settings.json
06:38:55.621 [App Background Thread 005] INFO  o.c.ui.unlock.UnlockWorkflow - Unlock of 'Test' succeeded.
06:38:55.625 [DBus Worker Thread-1] INFO  o.f.dbus.handlers.SignalHandler - Received signal KWallet.folderUpdated: {wallet: kdewallet, folder: Cryptomator}

SignalHandler in kdewallet 1.2.0:

07:23:25.510 [App Scheduled Executor 01] INFO  o.c.common.settings.SettingsProvider - Settings saved to /home/ralph/.config/Cryptomator/settings.json
07:23:28.945 [DBus Worker Thread-3] INFO  o.c.l.k.KDEWalletKeychainAccess - Wallet successfully opened.
07:23:28.945 [DBus Worker Thread-2] INFO  o.f.dbus.handlers.SignalHandler - Received signal KWallet.walletOpened: kdewallet
07:23:28.945 [DBus Worker Thread-3] INFO  o.f.dbus.handlers.SignalHandler - Received signal KWallet.walletAsyncOpened: {TransactionID: 0, handle: 389267065}
07:23:48.265 [App Background Thread 004] INFO  org.cryptomator.common.vaults.Vault - Storing file name length limit of 220
07:23:49.266 [App Scheduled Executor 02] INFO  o.c.common.settings.SettingsProvider - Settings saved to /home/ralph/.config/Cryptomator/settings.json
07:23:50.589 [App Background Thread 004] INFO  o.c.ui.unlock.UnlockWorkflow - Unlock of 'Test' succeeded.
07:23:50.593 [DBus Worker Thread-3] INFO  o.f.dbus.handlers.SignalHandler - Received signal KWallet.folderUpdated: {wallet: kdewallet, folder: Cryptomator}

[1] the current kdewallet implementation within the integrations-api does work without this PR

infeo

Cool, i like the async approach. But...

the current keychain interface is not designed for async usage, its methods return "hard" values.

I made a small integration test with this PR and the results are as expected. You can store a vault password in KWallet, then lock the wallet and afterwards try to unlock the vault again. The result is the Unlock dialogue opens. And if you wait maybe 4 seconds, next to the unlock dialogue the KWallet password prompt pops up. But even if you unlock the wallet then, the stored password cannot be used in the current unlock dialogue.

Of course downstream the unlock dialogue can be redesigned to poll for availability, but currently are no plans.

I suggest to wait for an answer a short time period and if none comes, return the correct value.

infeo · 2020-12-09T16:50:43Z

src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java

@@ -11,9 +11,11 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

+import java.beans.PropertyChangeEvent;
+import java.beans.PropertyChangeListener;


Are there better alternatives? If at any point in the future the JPMS is introduced in this library, this leads to a dependency to the java.desktop module, which imho should not be present in this "system" library.

I cannot get the interconnection between the JPMS and the PropertyChangeListener pattern I introduced here.
Can you elaborate you point further?

The pattern fine!

The classes from the java.beans package are part of the java.desktop module. So if this library will be migrated to the module system, it needs a dependeny to this module.

From my perspective, this is a "system" library and should not depend in java.desktop. But I'm also a little nitpicky here 😄

src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java

purejava · 2020-12-14T06:04:56Z

Cool, i like the async approach. But...

the current keychain interface is not designed for async usage, its methods return "hard" values.

I made a small integration test with this PR and the results are as expected. You can store a vault password in KWallet, then lock the wallet and afterwards try to unlock the vault again. The result is the Unlock dialogue opens. And if you wait maybe 4 seconds, next to the unlock dialogue the KWallet password prompt pops up. But even if you unlock the wallet then, the stored password cannot be used in the current unlock dialogue.

That's the design so far. The two components Cryptomator and the kwallet backend were designed to work independently.
A good solution would be to check if the Unlock dialogue is open when the wallet got opened and inject the password into the password field, close the Unlock dialogue and proceed with the unlock workflow.

I suggest to wait for an answer a short time period and if none comes, return the correct value.

Who should wait for what answer and who should return which value? 🙂

infeo · 2020-12-15T12:04:32Z

A good solution would be to check if the Unlock dialogue is open when the wallet got opened and inject the password into the password field, close the Unlock dialogue and proceed with the unlock workflow.

I second that, but it needs a change in the keychain API, which, as a consequence, leads to a bigger refactoring(main project, other integrations libs).

Who should wait for what answer and who should return which value?

The functions of the keychain API for answer of the implemented keychain. E.g. loadPassphrase() checks if the wallet is open, if not tries to open it and waits at most 30s for an answer. If it gets the passphrase or its not present, return the appropiate value, otherwise throw the appropiate Exception. Of course you can run then into the same problem as the secret service (#1).

purejava · 2021-01-31T06:43:24Z

I made a small integration test with this PR and the results are as expected. You can store a vault password in KWallet, then lock the wallet and afterwards try to unlock the vault again. The result is the Unlock dialogue opens. And if you wait maybe 4 seconds, next to the unlock dialogue the KWallet password prompt pops up. But even if you unlock the wallet then, the stored password cannot be used in the current unlock dialogue.

Could you please retest with a current Cryptomator?
I just tried Cryptomator 1.5.11 on Kubuntu 20.04 and 20.04.1 and couldn't trigger the described behaviour.

In case the wallet is locked, the pop-up to open it is displayed and Cryptomator waits for the desired 120 seconds. The enter password dialog from Cryptomator does not show up during that time.
This is the same as current secret-service works right now and should be ok.

overheadhunter · 2022-04-13T04:38:58Z

Can this be closed? Looking at the diff, I guess it is obsolete

purejava · 2022-04-17T13:47:45Z

Not really obsolete, as the code still uses the depracated await.

integrations-linux/src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java

Line 188 in 1c23d06

    
           wallet.getSignalHandler().await(KWallet.walletAsyncOpened.class, Static.ObjectPaths.KWALLETD5, () -> null);

# Conflicts: # pom.xml # src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java

overheadhunter · 2022-04-18T07:16:20Z

I merged develop into this branch, making the inner class implement the proposed PropertyChangeListener. Can you please check if this is correct?

purejava · 2022-04-22T15:26:48Z

From looking at the MR code it looks ok. I wanted to compile the code, but could not find any branch of integrations-linux that contains the MR.

overheadhunter · 2022-04-22T19:13:50Z

could not find any branch of integrations-linux that contains the MR

Wrong remote, I guess? It should be in your fork, according to the metadata shown here.

purejava · 2022-04-23T16:06:17Z

Wrong remote, I guess? It should be in your fork, according to the metadata shown here.

Yeah, it is. This sometimes confuses me: which branch of which fork of which repo.

I can't compile the change:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:compile (default-compile) on project integrations-linux: Compilation failure: Compilation failure: 
[ERROR] /home/ralph/IdeaProjects/integrations-linux/src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java:[13,8] org.cryptomator.linux.keychain.SecretServiceKeychainAccess is not abstract and does not override abstract method changePassphrase(java.lang.String,java.lang.String,java.lang.CharSequence) in org.cryptomator.integrations.keychain.KeychainAccessProvider
[ERROR] /home/ralph/IdeaProjects/integrations-linux/src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java:[16,8] org.cryptomator.linux.keychain.KDEWalletKeychainAccess is not abstract and does not override abstract method changePassphrase(java.lang.String,java.lang.String,java.lang.CharSequence) in org.cryptomator.integrations.keychain.KeychainAccessProvider
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
ralph@ubuntu:~/IdeaProjects/integrations-linux$

ralph@ubuntu:~/IdeaProjects/integrations-linux$ git diff
diff --git a/pom.xml b/pom.xml
index 02a3a81..1001220 100644
--- a/pom.xml
+++ b/pom.xml
@@ -5,7 +5,7 @@
        <modelVersion>4.0.0</modelVersion>
        <groupId>org.cryptomator</groupId>
        <artifactId>integrations-linux</artifactId>
-       <version>0.2.0-SNAPSHOT</version>
+       <version>1.1.0-SNAPSHOT</version>
 
        <name>integrations-linux</name>
        <description>Provides optional Linux services used by Cryptomator</description>
@@ -38,9 +38,9 @@
                <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
 
                <!-- runtime dependencies -->
-               <api.version>1.0.0-beta2</api.version>
+               <api.version>1.1.0-rc1</api.version>
                <secret-service.version>1.5.0</secret-service.version>
-               <kdewallet.version>1.1.1</kdewallet.version>
+               <kdewallet.version>1.2.6</kdewallet.version>
                <guava.version>30.0-jre</guava.version>
                <slf4j.version>1.7.30</slf4j.version>

overheadhunter · 2022-04-23T19:27:13Z

[ERROR] /home/ralph/IdeaProjects/integrations-linux/src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java:[13,8] org.cryptomator.linux.keychain.SecretServiceKeychainAccess is not abstract and does not override abstract method changePassphrase(java.lang.String,java.lang.String,java.lang.CharSequence) in org.cryptomator.integrations.keychain.KeychainAccessProvider
[ERROR] /home/ralph/IdeaProjects/integrations-linux/src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java:[16,8] org.cryptomator.linux.keychain.KDEWalletKeychainAccess is not abstract and does not override abstract method changePassphrase(java.lang.String,java.lang.String,java.lang.CharSequence) in org.cryptomator.integrations.keychain.KeychainAccessProvider

The method is implemented though. Lines 79 and 69 resp. 🤔

purejava added 4 commits November 11, 2020 07:32

Implement PropertyChangeListener interface

49a8d90

Add missing PropertyChangeListener

5fb077b

Log only when the wallet has been successfully opened

0a3070e

Bump kdewallet version

d4d7379

infeo self-assigned this Nov 26, 2020

infeo added the enhancement New feature or request label Nov 26, 2020

infeo added this to the 0.1.1 milestone Dec 8, 2020

infeo requested changes Dec 9, 2020

View reviewed changes

infeo removed their assignment Jan 11, 2021

infeo removed this from the 0.1.1 milestone Jan 27, 2021

infeo mentioned this pull request Feb 22, 2021

Widen integrations-api to check whether keychain is locked #10

Merged

infeo self-assigned this Feb 23, 2021

purejava mentioned this pull request Apr 16, 2021

Remove Workaround code "catch RuntimeException | ExceptionInInitializerError " #5

Closed

Merge branch 'develop' into feature/ImproveSignaling

13f3f4f

# Conflicts: # pom.xml # src/main/java/org/cryptomator/linux/keychain/KDEWalletKeychainAccess.java

This comment was marked as resolved.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve signal handling for kdewallet #3

Improve signal handling for kdewallet #3

purejava commented Nov 15, 2020

infeo left a comment

infeo Dec 9, 2020

purejava Dec 14, 2020

infeo Dec 15, 2020

purejava commented Dec 14, 2020

infeo commented Dec 15, 2020 •

edited

purejava commented Jan 31, 2021

overheadhunter commented Apr 13, 2022

purejava commented Apr 17, 2022

overheadhunter commented Apr 18, 2022

purejava commented Apr 22, 2022

overheadhunter commented Apr 22, 2022

purejava commented Apr 23, 2022

overheadhunter commented Apr 23, 2022 •

edited

This comment was marked as resolved.

Improve signal handling for kdewallet #3

Are you sure you want to change the base?

Improve signal handling for kdewallet #3

Conversation

purejava commented Nov 15, 2020

infeo left a comment

Choose a reason for hiding this comment

infeo Dec 9, 2020

Choose a reason for hiding this comment

purejava Dec 14, 2020

Choose a reason for hiding this comment

infeo Dec 15, 2020

Choose a reason for hiding this comment

purejava commented Dec 14, 2020

infeo commented Dec 15, 2020 • edited

purejava commented Jan 31, 2021

overheadhunter commented Apr 13, 2022

purejava commented Apr 17, 2022

overheadhunter commented Apr 18, 2022

purejava commented Apr 22, 2022

overheadhunter commented Apr 22, 2022

purejava commented Apr 23, 2022

overheadhunter commented Apr 23, 2022 • edited

This comment was marked as resolved.

infeo commented Dec 15, 2020 •

edited

overheadhunter commented Apr 23, 2022 •

edited